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.eb...@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.eb...@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.eb...@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?
> 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)
>
> 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 = {
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
