[pve-devel] [PATCH v3 pve-zsync 1/2] Refactor locking
Fabian Ebner
f.ebner at proxmox.com
Tue Oct 8 13:05:31 CEST 2019
On 10/8/19 11:09 AM, Thomas Lamprecht wrote:
> On 10/8/19 11:00 AM, Thomas Lamprecht wrote:
>> On 10/7/19 11:16 AM, Fabian Ebner wrote:
>>> This introduces a new locked() mechanism allowing to enclose locked
>>> sections in a cleaner way. There's only two types of locks namely one
>>> for state and cron (they are always read together and almost always
>>> written together) and one for sync.
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>> ---
>>>
>>> Changes from v2:
>>> * Split into two patches
>>>
>>> Changes from v1:
>>> * Refactored locking as Thomas and Fabian suggested
>>>
>>> pve-zsync | 239 +++++++++++++++++++++++++++---------------------------
>>> 1 file changed, 119 insertions(+), 120 deletions(-)
>>>
>> so below a "git show -w" to ignore indentation changes, makes it easier
>> to see what you change - as IMO not all changes are related to the lock
>> refactoring only - which should be just a cleanup but no semantic code
>> changes. I'll reply to this with the places I'm unsure (makes it easier
>> to see for others, if done in quotes, IMO)
>>
>> ----8<----
>> commit 68ecaed42b6778c3f21729468ce6e1a71ad81a7f
>> Author: Fabian Ebner <f.ebner at proxmox.com>
>> Date: Mon Oct 7 11:16:10 2019 +0200
>>
>> Refactor locking
>>
>> This introduces a new locked() mechanism allowing to enclose locked
>> sections in a cleaner way. There's only two types of locks namely one
>> for state and cron (they are always read together and almost always
>> written together) and one for sync.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>
>> diff --git a/pve-zsync b/pve-zsync
>> index 425ffa2..f7bf5bd 100755
>> --- a/pve-zsync
>> +++ b/pve-zsync
>> @@ -18,7 +18,6 @@ my $PATH = "/usr/sbin";
>> my $PVE_DIR = "/etc/pve/local";
>> my $QEMU_CONF = "${PVE_DIR}/qemu-server";
>> my $LXC_CONF = "${PVE_DIR}/lxc";
>> -my $LOCKFILE = "$CONFIG_PATH/${PROGNAME}.lock";
>> my $PROG_PATH = "$PATH/${PROGNAME}";
>> my $INTERVAL = 15;
>> my $DEBUG;
>> @@ -110,14 +109,20 @@ sub cut_target_width {
>> return "$head/" . $path . "/$tail";
>> }
>>
>> -sub lock {
>> - my ($fh) = @_;
>> - flock($fh, LOCK_EX) || die "Can't lock config - $!\n";
>> -}
>> -
>> -sub unlock {
>> - my ($fh) = @_;
>> - flock($fh, LOCK_UN) || die "Can't unlock config- $!\n";
>> +sub locked {
>> + my ($lock_fn, $code) = @_;
>> +
>> + my $lock_fh = IO::File->new("> $lock_fn");
>> +
>> + flock($lock_fh, LOCK_EX) || die "Couldn't acquire lock - $!\n";
>> + my $res = eval { $code->() };
>> + my $err = $@;
>> +
>> + flock($lock_fh, LOCK_UN) || warn "Error unlocking - $!\n";
>> + die "$err" if $err;
>> +
>> + close($lock_fh);
>> + return $res;
>> }
>>
>> sub get_status {
>> @@ -338,13 +343,11 @@ sub update_state {
>> my $text;
>> my $in_fh;
>>
>> - eval {
>> -
>> + if (-e $STATE) {
> why is this change mixed into this?
>
>> $in_fh = IO::File->new("< $STATE");
>> die "Could not open file $STATE: $!\n" if !$in_fh;
>> - lock($in_fh);
>> $text = <$in_fh>;
>> - };
>> + }
>>
>> my $out_fh = IO::File->new("> $STATE.new");
>> die "Could not open file ${STATE}.new: $!\n" if !$out_fh;
>> @@ -376,9 +379,7 @@ sub update_state {
>>
>> close($out_fh);
>> rename "$STATE.new", $STATE;
>> - eval {
> here too, is this required to be done for the refactoring?
No it's not. And I shouldn't have changed it. I was wrongly thinking
"since update_state is
always called inside the lock's eval I can remove the evals here". Thank
you for pointing this out.
>> close($in_fh);
>> - };
>>
>> return $states;
>> }
>> @@ -395,7 +396,6 @@ sub update_cron {
>>
>> my $fh = IO::File->new("< $CRONJOBS");
>> die "Could not open file $CRONJOBS: $!\n" if !$fh;
>> - lock($fh);
>>
>> my @test = <$fh>;
>>
>> @@ -502,6 +502,7 @@ sub vm_exists {
>> sub init {
>> my ($param) = @_;
>>
>> + locked("$CONFIG_PATH/cron_and_state.lock", sub {
>> my $cfg = read_cron();
>>
>> my $job = param_to_job($param);
>> @@ -539,6 +540,7 @@ sub init {
>>
>> update_cron($job);
>> update_state($job);
>> + }); #cron and state lock
>>
>> eval {
>> sync($param) if !$param->{skip};
>> @@ -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.
>>
>> if ($job && defined($job->{state}) && $job->{state} eq "syncing") {
>> die "Job --source $param->{source} --name $param->{name} is syncing at the moment";
>> }
>>
>> - my $dest = parse_target($param->{dest});
>> - my $source = parse_target($param->{source});
>> + $dest = parse_target($param->{dest});
>> + $source = parse_target($param->{source});
>> +
>> + $vm_type = vm_exists($source, $param->{source_user});
>> + $source->{vm_type} = $vm_type;
>> +
>> + if ($job) {
>> + $job->{state} = "syncing";
>> + $job->{vm_type} = $vm_type if !$job->{vm_type};
>> + update_state($job);
>> + }
>> + }); #cron and state lock
>>
>> my $sync_path = sub {
>> my ($source, $dest, $job, $param, $date) = @_;
>> -
>> ($source->{old_snap}, $source->{last_snap}) = snapshot_get($source, $dest, $param->{maxsnap}, $param->{name}, $param->{source_user});
>> -
>> snapshot_add($source, $dest, $param->{name}, $date, $param->{source_user}, $param->{dest_user});
>> -
>> send_image($source, $dest, $param);
>> -
>> snapshot_destroy($source, $dest, $param->{method}, $source->{old_snap}, $param->{source_user}, $param->{dest_user}) if ($source->{destroy} && $source->{old_snap});
>> -
>> };
>>
>> - my $vm_type = vm_exists($source, $param->{source_user});
>> - $source->{vm_type} = $vm_type;
>> -
>> - if ($job) {
>> - $job->{state} = "syncing";
>> - $job->{vm_type} = $vm_type if !$job->{vm_type};
>> - update_state($job);
>> - }
>> -
>> eval{
>> if ($source->{vmid}) {
>> die "VM $source->{vmid} doesn't exist\n" if !$vm_type;
>> @@ -642,9 +641,7 @@ sub sync {
>> if (my $err = $@) {
>> if ($job) {
>> $job->{state} = "error";
>> - update_state($job);
>> - unlock($lock_fh);
>> - close($lock_fh);
>> + locked("$CONFIG_PATH/cron_and_state.lock", sub { update_state($job); });
>> print "Job --source $param->{source} --name $param->{name} got an ERROR!!!\nERROR Message:\n";
>> }
>> die "$err\n";
>> @@ -653,11 +650,9 @@ sub sync {
>> if ($job) {
>> $job->{state} = "ok";
>> $job->{lsync} = $date;
>> - update_state($job);
>> + locked("$CONFIG_PATH/cron_and_state.lock", sub { update_state($job); });
>> }
>> -
>> - unlock($lock_fh);
>> - close($lock_fh);
>> + }); #sync lock
>> }
>>
>> sub snapshot_get{
>> @@ -1031,19 +1026,23 @@ sub status {
>> sub enable_job {
>> my ($param) = @_;
>>
>> + locked("$CONFIG_PATH/cron_and_state.lock", sub {
>> my $job = get_job($param);
>> $job->{state} = "ok";
>> update_state($job);
>> update_cron($job);
>> + });
>> }
>>
>> sub disable_job {
>> my ($param) = @_;
>>
>> + locked("$CONFIG_PATH/cron_and_state.lock", sub {
>> my $job = get_job($param);
>> $job->{state} = "stopped";
>> update_state($job);
>> update_cron($job);
>> + });
>> }
>>
>> my $cmd_help = {
>>
>
More information about the pve-devel
mailing list