From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 5B82B1FF0AF for ; Fri, 06 Nov 2026 15:49:13 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id BFCE221695; Fri, 06 Nov 2026 15:49:08 +0100 (CET) Message-ID: <21889e70-4dd4-4470-b9d7-48a80dbfc5fa@proxmox.com> Date: Fri, 6 Nov 2026 15:49:00 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH manager 12/19] vzdump: allow jobs to run alongside each other To: Thomas Lamprecht , pve-devel@lists.proxmox.com References: <20261101225849.884587-1-t.lamprecht@proxmox.com> <20261101225849.884587-13-t.lamprecht@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261101225849.884587-13-t.lamprecht@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793976541090 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.290 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: OOQ5OAHE4YXWKGACD6N2QZJYWVOGYUGV X-Message-ID-Hash: OOQ5OAHE4YXWKGACD6N2QZJYWVOGYUGV X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Am 02.11.26 um 12:01 AM schrieb Thomas Lamprecht: > diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm > index 73291189f..3e31505f3 100644 > --- a/PVE/API2/VZDump.pm > +++ b/PVE/API2/VZDump.pm > @@ -137,7 +137,7 @@ __PACKAGE__->register_method({ > my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist); > > my $LOCK_FH = eval { > - my $lock_fh = $vzdump->getlock($upid); # only one process allowed > + my $lock_fh = $vzdump->getlock($upid); > $vzdump->init_job(); > $lock_fh; > }; > diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm > index 33fe8793e..f597f7ed7 100644 > --- a/PVE/VZDump.pm > +++ b/PVE/VZDump.pm > @@ -46,6 +46,7 @@ PVE::JSONSchema::register_standard_option( > my @posix_filesystems = qw(ext3 ext4 nfs nfs4 reiserfs xfs); > > my $lockfile = '/var/run/vzdump.lock'; > +my $lockdir = '/run/vzdump'; > my $logdir = '/var/log/vzdump'; > > my @plugins = qw(); > @@ -682,6 +683,14 @@ sub new { > > $self->{'default-prune-backups'} = $defaults->{'prune-backups'}; > > + # external providers are not known to cope with other backups running at the same time > + my $uses_provider = $opts->{storage} && eval { > + PVE::Storage::storage_has_feature( > + PVE::Storage::config(), $opts->{storage}, 'backup-provider', > + ); > + }; Should log a message/warning if something goes wrong in the eval to help debug issues. > + $self->{exclusive} = !$opts->{overlap} || $uses_provider; > + > return $self; > } > > @@ -781,14 +790,32 @@ sub getlock { > my $maxwait = $self->{opts}->{lockwait} || $self->{lockwait}; Pre-existing, but it seems like $self->{lockwait} is never set. > > die "missing UPID" if !$upid; # should not happen > + $self->{upid} = $upid; > + > + mkdir $lockdir; Might be nicer to add it to the package's tmpfiles config, like /run/pve already is. > > my $SERVER_FLCK; > if (!open($SERVER_FLCK, '>>', "$lockfile")) { > debugmsg('err', "can't open lock on file '$lockfile' - $!", undef, 1); > die "can't open lock on file '$lockfile' - $!"; > } > + open(my $gate, '>>', "$lockdir/gate.lock") > + or die "can't open lock on file '$lockdir/gate.lock' - $!\n"; > > - if (!flock($SERVER_FLCK, LOCK_EX | LOCK_NB)) { > + # Jobs that may overlap share the global lock. A job waiting to get it exclusively holds the > + # gate meanwhile, so overlapping jobs starting later cannot keep it waiting forever. > + my $mode = $self->{exclusive} ? LOCK_EX : LOCK_SH; > + my $acquire = sub { > + my ($flags) = @_; > + > + flock($gate, $mode | $flags) or return 0; > + my $locked = flock($SERVER_FLCK, $mode | $flags); > + flock($gate, LOCK_UN); > + > + return $locked; > + }; > + > + if (!$acquire->(LOCK_NB)) { > if (!$maxwait) { > debugmsg('err', "can't acquire lock '$lockfile' (wait = 0)", undef, 1); > die "can't acquire lock '$lockfile' (wait = 0)"; > @@ -800,7 +827,7 @@ sub getlock { > > local $SIG{ALRM} = sub { alarm(0); die "got timeout\n"; }; > > - if (!flock($SERVER_FLCK, LOCK_EX)) { > + if (!$acquire->(0)) { > my $err = $!; This might not be the error code from the failure of flock($SERVER_FLCK, ...), which we are interested in, since there is an flock($gate, ...) call after it. > close($SERVER_FLCK); > alarm(0); > @@ -1635,12 +1662,145 @@ my sub finish_guest_backup { > } > } > > +my sub try_lock_file { > + my ($path) = @_; > + > + open(my $fh, '>>', $path) or die "unable to open lock file '$path' - $!\n"; > + return $fh if flock($fh, LOCK_EX | LOCK_NB); > + > + close($fh); > + return undef; > +} > + > +my sub try_guest_lock { > + my ($self, $vmid) = @_; > + > + my $fh = try_lock_file("$lockdir/guest-$vmid.lock") or return undef; > + truncate($fh, 0); > + print $fh "$self->{upid}\n"; > + $fh->flush(); > + > + return $fh; > +} > + > +# Slots above a lowered limit stay in use until their backups end, no new one starts before. > +my sub try_slot { > + my ($prefix, $limit) = @_; > + > + for my $path (glob("$prefix-*.lock")) { > + my ($slot) = $path =~ m/^\Q$prefix\E-(\d+)\.lock$/ or next; > + next if $slot <= $limit; > + my $fh = try_lock_file("$prefix-$slot.lock") or return undef; We could start re-using lower slots already, even if there is a remaining slot in-use above the limit. We just need to make sure that the count of in-use slots stays below the limit. Otherwise, in a situation with a single long-running task with a slot above the limit, we have to wait until that single high-slot task is finished, which seems unfortunate. > + close($fh); > + } > + for my $slot (1 .. $limit) { > + my $fh = try_lock_file("$prefix-$slot.lock"); > + return $fh if $fh; > + } > + return undef; > +} > + > +my sub try_node_slot { > + return try_slot("$lockdir/slot", node_backup_concurrency()); > +} > + > +my sub log_wait { > + my ($self, $reason) = @_; > + > + return if ($self->{'waiting-for'} // '') eq $reason; > + $self->{'waiting-for'} = $reason; > + debugmsg('info', "waiting for $reason"); > +} > + > +my sub guest_moved_to { > + my ($vmid) = @_; > + > + PVE::Cluster::cfs_update(); > + my $node = PVE::Cluster::get_vmlist()->{ids}->{$vmid}->{node}; > + > + return defined($node) && $node ne PVE::INotify::nodename() ? $node : undef; > +} > + > +my sub claim_next_guest { > + my ($self, $queue, $deferred) = @_; > + > + my $maxwait = ($self->{opts}->{lockwait} || 0) * 60; The default according to the schema would be 3 hours. But getlock() already relies on the value being initialized by new(), so we could do the same here. > + > + for (my $i = 0; $i < scalar($queue->@*); $i++) { > + my $task = $queue->[$i]; > + my $vmid = $task->{vmid}; > + > + my $guest_lock = try_guest_lock($self, $vmid); > + if (!$guest_lock) { > + my $holder = PVE::Tools::file_read_firstline("$lockdir/guest-$vmid.lock") // ''; Nit: could use PVE::File > + if (!defined($deferred->{$vmid})) { > + $deferred->{$vmid} = time(); > + debugmsg('info', "VM $vmid is being backed up by '$holder', deferring"); > + } elsif (time() - $deferred->{$vmid} >= $maxwait) { > + splice($queue->@*, $i--, 1); > + skip_task($task, "still being backed up by '$holder'"); > + } > + next; > + } > +