[PATCH common/storage 0/3] fix #8286: do not hold inherited locks in volume cleanup workers

[PATCH common/storage 0/3] fix #8286: do not hold inherited locks in volume cleanup workers

[Michael Köppl]

Destroying a VM through the API with a disk on an LVM storage with
saferemove=1 leaves its VMID locked until the wipe of the disk is done.
The imgdel worker is forked while the qmdestroy worker holds the
config lock and inherits the locked file handle. The worker keeps the
lock held after the qmdestroy worker exited. Re-creating a VM with the
same VMID fails with a lock timeout in the meantime.

This series tries to keep the fix targeted, since closing all inherited
file descriptors in fork_worker() has a lot of regression
potential [0]. An opt-in helper is added in pve-common to close the
lock handles a forked child inherited from its ancestors. This call is
added to vdisk_free before running the cleanup workers of the
respective plugin. At the moment, this affects only the LVM plugin,
which only operates on volumes that are not referenced anymore and
takes the storage lock on its own where needed. Thus, this also
expects external plugins with cleanup workers to not rely on the
cleanup worker holding the caller's lock.

The last patch does the same for the cleanup workers the LVM plugin
forks itself for snapshot rollback and deletion and for failed
imports. It is not needed to fix the bug, but these workers inherit
their caller's locks too. For example, restoring a VM over an existing
one with qcow2 snapshots on LVM deletes the snapshots of kept disks
under the config lock. Since the rollback cleanup worker then no
longer holds the guest migration lock, the patch also adds a rollback
check that the volume replaced by a previous rollback is not still
being wiped. Otherwise, the rollback would fail after the guest was
already locked for it.

Tested on a single node by destroying a VM with a disk on an LVM
storage with saferemove through the API and re-creating it right after
the destroy task finished, while the wipe was still running.

The last patch was tested by restoring a backup through the API over
an existing VM with a qcow2 snapshot on an LVM storage with saferemove
and a disk excluded from the backup. The config of the VM could be
changed while the kept disk was still being wiped. Both cleanup workers
finished successfully.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=8286


common:

Michael Köppl (1):
  tools: lock file: add helper to close inherited lock handles

 src/PVE/Tools.pm  | 23 +++++++++++++++-----
 test/lock_file.pl | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 5 deletions(-)


storage:

Michael Köppl (2):
  fix #8286: vdisk free: do not hold inherited locks in cleanup worker
  lvm plugin: do not hold inherited locks in cleanup workers

 src/PVE/Storage.pm           | 10 +++++++++-
 src/PVE/Storage/LVMPlugin.pm | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)


Summary over all repositories:
  4 files changed, 96 insertions(+), 7 deletions(-)

-- 
Generated by murpp 0.12.1

[PATCH common 1/3] tools: lock file: add helper to close inherited lock handles

[Michael Köppl]

A child forked while its parent holds a lock from lock_file_full()
inherits the locked file handle. The lock then is held until the child
exits too, even after the parent released it.

Keep the locked handle instead of a flag in the scalar that
$lock_handles weakly references. Add a helper that lets a child close
its copies of the handles locked by its ancestors. The ancestor keeps
holding the lock, since its own handle stays open.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/Tools.pm  | 23 +++++++++++++++-----
 test/lock_file.pl | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 9176b358..2f2123c1 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -198,7 +198,8 @@ sub run_with_timeout {
 # Since this is the only way we lock now and we don't have the old
 # 'lock(); code(); unlock();' pattern anymore we do not actually need to
 # count how deep we're nesting. Therefore this hash now stores a weak reference
-# to a boolean telling us whether we already have a lock.
+# to a scalar holding the locked file handle. If the lock has not been acquired
+# yet, it holds a false value.
 
 my $lock_handles = {};
 
@@ -243,7 +244,7 @@ sub lock_file_full {
     if (!$checkptr || !$$checkptr) {
         # We cannot create a weak reference in a single atomic step, so we first
         # create a false-value, then create a reference to it, then weaken it,
-        # and after successfully locking the file we change the boolean value.
+        # and after successfully locking the file we store the handle in it.
         #
         # The reason for this is that if an outer SIGALRM throws an exception
         # between creating the reference and weakening it, a subsequent call to
@@ -253,8 +254,8 @@ sub lock_file_full {
         #
         # An externally triggered exception elsewhere in the code will cause the
         # weak reference to become 'undef', and since the file handle is only
-        # stored in the local scope in $local_fh, the file will be closed by
-        # perl's cleanup routines as well.
+        # stored in the local scope in $local_fh and $check, the file will be
+        # closed by perl's cleanup routines as well.
         #
         # This still assumes that an IO::File handle can properly deal with such
         # exceptions thrown during its own destruction, but that's up to perls
@@ -266,7 +267,7 @@ sub lock_file_full {
             $@ = "can't lock file '$filename' - $@";
             return undef;
         }
-        $check = 1;
+        $check = $local_fh;
     }
     $res = eval { &$code(@param); };
     return undef if $@;
@@ -279,6 +280,18 @@ sub lock_file {
     return lock_file_full($filename, $timeout, 0, $code, @param);
 }
 
+# Close this process's copies of lock handles inherited from its ancestors, so
+# it does not keep their locks held after they released them.
+sub close_inherited_lock_handles {
+    for my $pid (keys %$lock_handles) {
+        next if $pid == $$;
+        for my $checkptr (values $lock_handles->{$pid}->%*) {
+            close($$checkptr) if $checkptr && $$checkptr;
+        }
+        delete $lock_handles->{$pid};
+    }
+}
+
 # file_ helper for backward compat, prefer PVE::File directly for new usage.
 # TODO: remove with PVE 10 / Debian Forky releases.
 sub file_set_contents {
diff --git a/test/lock_file.pl b/test/lock_file.pl
index 83965c55..ffa2404a 100755
--- a/test/lock_file.pl
+++ b/test/lock_file.pl
@@ -5,6 +5,7 @@ use strict;
 use warnings;
 
 use Socket;
+use Fcntl qw(:flock);
 use POSIX (); # don't import assert()
 
 use PVE::Tools 'lock_file_full';
@@ -91,6 +92,54 @@ sub forktest1($) {
     close($fmain);
 }
 
+sub lock_is_free {
+    open(my $fh, '>>', $name) or die "open(): $!\n";
+    return flock($fh, LOCK_EX | LOCK_NB);
+}
+
+sub inherited_lock_test {
+    my ($close_in_child) = @_;
+
+    socketpair(my $fparent, my $fchild, AF_UNIX, SOCK_STREAM, PF_UNSPEC)
+        or die "socketpair(): $!\n";
+
+    my $pid;
+    lock_file_full(
+        $name,
+        10,
+        0,
+        sub {
+            $pid = fork() // die "fork(): $!\n";
+            if (!$pid) {
+                close($fparent);
+                PVE::Tools::close_inherited_lock_handles() if $close_in_child;
+                print {$fchild} "1\n";
+                $fchild->flush();
+                <$fchild>; # wait for parent to finish checks
+                POSIX::_exit(0);
+            }
+            close($fchild);
+            my $line = <$fparent>;
+            die "child failed to start\n" if !$line || $line ne "1\n";
+            die "lock released while the parent still holds it\n" if lock_is_free();
+        },
+    );
+
+    my $err = $@;
+    my $free = !$err && lock_is_free();
+    print {$fparent} "2\n";
+    $fparent->flush();
+    waitpid($pid, 0) if $pid;
+    close($fparent);
+    die $err if $err;
+
+    if ($close_in_child) {
+        die "child kept the inherited lock held\n" if !$free;
+    } else {
+        die "child did not inherit the lock\n" if $free;
+    }
+}
+
 eval {
     # Regular lock:
     new();
@@ -143,6 +192,11 @@ eval {
     # Can we get two simultaneous shared locks? (shared=1)
     forktest1(0);
     forktest1(1);
+
+    # Lock held by child after parent released it?
+    inherited_lock_test(0);
+    # Lock free once child closed inherited copies?
+    inherited_lock_test(1);
 };
 my $err = $@;
 system("rm $name*");
-- 
2.47.3

[PATCH storage 2/3] fix #8286: vdisk free: do not hold inherited locks in cleanup worker

[Michael Köppl]

vdisk_free() forks the cleanup worker of a volume (e.g. for wiping LVs
on LVM storages with saferemove), while the caller often still holds the
guest config lock. The worker inherits the locked handle and keeps the
lock held until the cleanup is done. For example, after destroying a VM
through the API, re-creating a VM with the same VMID fails with a lock
timeout until the wipe has finished.

The cleanup workers in the LVM plugin only operate on volumes that are
not referenced anymore. The plugin's implementation takes the lock on
its own where needed, so close the inherited lock handles when the
worker starts. This applies to the cleanup worker of any plugin, which
must not rely on the cleanup worker holding the caller's lock.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---

Notes:
    I'm not quite sure about this one. I understand that it breaks this
    behavior for existing external plugins, but I'm not sure it's something
    they should rely on in the first place. An alternative to this would be
    to put the cleanup call into the LVM plugin's cleanup worker
    specifically, which would also require every plugin to implement this
    in its cleanup worker. Could perhaps also be a sensible approach for
    now to have the call in the LVM plugin specifically and move it to
    vdisk_free for PVE 10. I mostly decided on this version for now because
    of Fabian's comment on the Bugzilla ticket.

 src/PVE/Storage.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index d660cfc4..4cec7c31 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1222,7 +1222,15 @@ sub vdisk_free {
     my $rpcenv = PVE::RPCEnvironment::get();
     my $authuser = $rpcenv->get_user();
 
-    $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
+    $rpcenv->fork_worker(
+        'imgdel',
+        undef,
+        $authuser,
+        sub {
+            PVE::Tools::close_inherited_lock_handles();
+            $cleanup_worker->(@_);
+        },
+    );
 }
 
 sub vdisk_list {
-- 
2.47.3

[PATCH storage 3/3] lvm plugin: do not hold inherited locks in cleanup workers

[Michael Köppl]

The LVM plugin forks its own cleanup workers when rolling back or
deleting snapshots of qcow2 volumes and after failed imports. Like the
worker forked by vdisk_free(), they inherit the locks of their caller
and keep them held until the cleanup is done. This can happen, e.g.,
when restoring a backup over an existing VM with a qcow2 snapshot on LVM
and a disk excluded from the backup, which deletes that disk's snapshot
while holding the config lock. After the restore has finished, changing
or starting the VM then fails with a lock timeout until the cleanup is
done.

Close the inherited lock handles at the start of these workers too.

Since the cleanup worker of a rollback no longer holds the guest
migration lock, another rollback of the same volume can start while the
previous volume is still being wiped. Renaming the current volume to
del-<name> then fails, but only after the guest was already locked for
the rollback, leaving it locked. Check in volume_rollback_is_possible()
that del-<name> does not exist, so such a rollback is refused before the
guest is locked.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index a6a5eb70..f9e6936f 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -50,7 +50,15 @@ my sub fork_cleanup_worker {
     return if !$cleanup_worker;
     my $rpcenv = PVE::RPCEnvironment::get();
     my $authuser = $rpcenv->get_user();
-    $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
+    $rpcenv->fork_worker(
+        'imgdel',
+        undef,
+        $authuser,
+        sub {
+            PVE::Tools::close_inherited_lock_handles();
+            $cleanup_worker->(@_);
+        },
+    );
 }
 
 sub lvm_pv_info {
@@ -1645,6 +1653,12 @@ sub volume_rollback_is_possible {
     die "can't rollback, '$snap' is not most recent snapshot on '$volname'\n"
         if scalar($blockers->@*) > 0;
 
+    my $vg = $scfg->{vgname};
+    my $name = ($class->parse_volname($volname))[1];
+    my $lvs = lvm_list_volumes($vg);
+    die "can't rollback, previous volume '$vg/del-$name' is still being removed\n"
+        if $lvs->{$vg}->{"del-$name"};
+
     return 1;
 }
 
-- 
2.47.3