[pve-devel] [PATCH v3 pve-zsync 1/2] Refactor locking

[Thomas Lamprecht]

On 10/8/19 1:05 PM, Fabian Ebner wrote:
>>> @@ -568,55 +570,52 @@ sub get_job {
>>>   sub destroy_job {
>>>       my ($param) = @_;
>>>   +    locked("$CONFIG_PATH/cron_and_state.lock", sub {
>>>       my $job = get_job($param);
>>>       $job->{state} = "del";
>>> -
>> nit pick: no need to delete above empty line
>>
>>>       update_cron($job);
>>>       update_state($job);
>>> +    });
>>>   }
>>>     sub sync {
>>>       my ($param) = @_;
>>>   -    my $lock_fh = IO::File->new("> $LOCKFILE");
>>> -    die "Can't open Lock File: $LOCKFILE $!\n" if !$lock_fh;
>>> -    lock($lock_fh);
>>> -
>>>       my $date = get_date();
>>>       my $job;
>>> -    eval {
>>> -    $job = get_job($param);
>>> -    };
>>> +    my $dest;
>>> +    my $source;
>>> +    my $vm_type;
>>> +
>>> +    locked("$CONFIG_PATH/sync.lock", sub {
>>> +    locked("$CONFIG_PATH/cron_and_state.lock", sub {
>>> +        eval { $job = get_job($param); };
>> but you added locking in get_job and here too? Are those nested locks?
>>
>> AFAICS, you reduce lock granularity. This can be OK and nice, but did
>> you ensure that you never broke a read-modify-write part up?
>>
>> As when the code flow was:
>>
>> 1. lock
>> 2. read conf
>> 3. do A with read conf
>> 4. do B with read conf
>> 5. write updated conf
>> 6. unlock
>>
>> you must keep the lock over all those steps, as when one only locks e.g., 3
>> and 4 separately another processes changes could be overwritten..
>> I did not looked to closesly, but it seems that you introduce such changes.
>> I can be wrong, so if you really checked then OK, but also then I would
>> like to have two separate patches, either:
>> 1. plain transform into locked
>> 2. change of granularity/lock scopes with rationale why it can/should be done
>>
>> (you can swap the order of doing those, but not mix them)
> The old locks were three independent ones inside 'update_cron', 'update_state', and 'sync'.
> The ones in 'update_state' and 'update_cron' did not respect read-modify-write enclosure, since the read
> of the state happens with 'get_job' and there are 'read_cron' calls outside of 'update_cron'.
> This patch introduces those missing read-modify-write enclosures by moving the locking from inside
> 'update_state' and 'update_cron' to the call sites. And it should preserve the old 'sync' lock scope.
> Since get_job triggers both the read of state and the read of cron and also cron and state are almost
> always written together, it seemed natural to merge the locks.
> 
> If you want me to, I can do a direct refactoring of the old approach and then expand the locks around the
> read-modify-write parts where they actually belong. But that would also mean a patch introducing a bunch
> of code that would be removed by the very next one again.
> 
> But I would prefer just sending a new version of this one, not doing the wrong 'eval' "cleanup", and not doing
> the newline changes. Also the reordering of the 'if ($job)' block and 'my $sync path =' below can be split
> into its own patch or left out entirely if you prefer.

Let's do that for now, maybe I just was confused by the unrelated
changes.. Need to look at this with a more clear mind.

thanks,
Thomas