[pve-devel] [PATCH storage 3/4] lvm plugin: volume import: lock allocation and removal sections

[Fiona Ebner]

With a shared LVM storage, parallel imports, which might be done in
the context of remote migration, could lead to metadata corruption
with unlucky timing, because of missing locking. Add locking around
allocation and removal, which are the sections that modify LVM
metadata. Note that other plugins suffer from missing locking here as
well, but only regarding naming conflicts. Adding locking around the
full call to volume_import() would mean locking for much too long.
Other plugins could follow the approach here, or there could be a
reservation approach like proposed in [0].

[0]: https://lore.proxmox.com/pve-devel/20240403150712.262773-1-h.duerr@proxmox.com/

Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
---

Better viewed with "-w" or
"-w --word-diff=color --word-diff-regex='\w+'".

 src/PVE/Storage/LVMPlugin.pm | 37 +++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index f5a2008..c5f71a2 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1300,19 +1300,27 @@ sub volume_import {
     die "cannot import format $format into a file of format $file_format\n"
         if $file_format ne 'raw';
 
-    my $vg = $scfg->{vgname};
-    my $lvs = lvm_list_volumes($vg);
-    if ($lvs->{$vg}->{$volname}) {
-        die "volume $vg/$volname already exists\n" if !$allow_rename;
-        warn "volume $vg/$volname already exists - importing with a different name\n";
-        $name = undef;
-    }
+    my $allocname = $class->cluster_lock_storage(
+        $storeid,
+        $scfg->{shared},
+        undef,
+        sub {
+            my $vg = $scfg->{vgname};
+            my $lvs = lvm_list_volumes($vg);
+            if ($lvs->{$vg}->{$volname}) {
+                die "volume $vg/$volname already exists\n" if !$allow_rename;
+                warn "volume $vg/$volname already exists - importing with a different name\n";
+                $name = undef;
+            }
 
-    my ($size) = PVE::Storage::Plugin::read_common_header($fh);
-    $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024;
+            my ($size) = PVE::Storage::Plugin::read_common_header($fh);
+            $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024;
+
+            return $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size);
+        },
+    );
 
     eval {
-        my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size);
         my $oldname = $volname;
         $volname = $allocname;
         if (defined($name) && $allocname ne $oldname) {
@@ -1324,7 +1332,14 @@ sub volume_import {
         $class->volume_import_write($fh, $file);
     };
     if (my $err = $@) {
-        my $cleanup_worker = eval { $class->free_image($storeid, $scfg, $volname, 0) };
+        my $cleanup_worker = eval {
+            return $class->cluster_lock_storage(
+                $storeid,
+                $scfg->{shared},
+                undef,
+                sub { return $class->free_image($storeid, $scfg, $volname, 0); },
+            );
+        };
         warn $@ if $@;
         fork_cleanup_worker($cleanup_worker);
         die $err;
-- 
2.47.3