[pve-devel] [PATCH-SERIES storage 0/4] lvm plugin: error handling and locking fixes

[pve-devel] [PATCH-SERIES storage 0/4] lvm plugin: error handling and locking fixes

[Fiona Ebner]

Patch 1/4 is improving an error message by including the previously
captured error.

Patch 2/4 fixes error handling in volume_snapshot_rollback().
Previously, allocation would be ignored when 'saferemove' was enabled
on the storage.

Patches 3/4 and 4/4 fix the locking for volume import and snapshot
operations. Better viewed with "-w" or
"-w --word-diff=color --word-diff-regex='\w+'".

I noticed that the default timeout of 10 seconds for locking
essentially means only having 10 tries to acquire the lock [0], which
in a busy cluster might not be enough to get the lock with high
confidence. This is not fully orthogonal to the series here, since we
might want to either change that [0] or to use a higher timeout for
(some of) the callers here.

[0]: https://git.proxmox.com/?p=pve-cluster.git;a=blob;f=src/PVE/Cluster.pm;h=e829687a74224408106f74ee6873a44095f0ff4f;hb=HEAD#l601

pve-storage:

Fiona Ebner (4):
  lvm plugin: snapshot delete: propagate previously captured error
  lvm plugin: fix error handling in volume_snapshot_rollback()
  lvm plugin: volume import: lock allocation and removal sections
  lvm plugin: snapshot-as-volume-chain: use locking for snapshot
    operations

 src/PVE/Storage/LVMPlugin.pm | 128 ++++++++++++++++++++++++++---------
 1 file changed, 96 insertions(+), 32 deletions(-)


Summary over all repositories:
  1 files changed, 96 insertions(+), 32 deletions(-)

-- 
Generated by git-murpp 0.5.0


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage 1/4] lvm plugin: snapshot delete: propagate previously captured error

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 275e84a..24f2534 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1190,9 +1190,7 @@ sub volume_snapshot_delete {
         }
         #delete the snapshot
         my $cleanup_worker = eval { free_snap_image($class, $storeid, $scfg, $volname, $snap); };
-        if ($@) {
-            die "error deleting old snapshot volume $snapvolname\n";
-        }
+        die "error deleting old snapshot volume $snapvolname: $@\n" if $@;
         fork_cleanup_worker($cleanup_worker);
     }
 
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage 2/4] lvm plugin: fix error handling in volume_snapshot_rollback()

[Fiona Ebner]

In case a cleanup worker is spawned, the error from the eval block
for allocation was lost. Save it in a variable for checking later.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 24f2534..f5a2008 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1097,11 +1097,12 @@ sub volume_snapshot_rollback {
     die "error deleting snapshot $snap $@\n" if $@;
 
     eval { alloc_snap_image($class, $storeid, $scfg, $volname, $snap) };
+    my $alloc_err = $@;
 
     fork_cleanup_worker($cleanup_worker);
 
-    if ($@) {
-        die "can't allocate new volume $volname: $@\n";
+    if ($alloc_err) {
+        die "can't allocate new volume $volname: $alloc_err\n";
     }
 
     return undef;
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[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@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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage 4/4] lvm plugin: snapshot-as-volume-chain: use locking for snapshot operations

[Fiona Ebner]

As reported by a user in the enterprise support in a ticket handled by
Friedrich, concurrent snapshot operations could lead to metadata
corruption of the volume group with unlucky timing. Add the missing
locking for operations modifying the metadata, i.e. allocation, rename
and removal. Since volume_snapshot() and volume_snapshot_rollback()
only do those, use a wrapper for the whole function. Since
volume_snapshot_delete() can do longer-running commit or rebase
operations, only lock the necessary sections there.

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

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

 src/PVE/Storage/LVMPlugin.pm | 82 +++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 16 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index c5f71a2..3badfef 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1029,7 +1029,7 @@ sub volume_size_info {
     return wantarray ? ($size, 'raw', 0, undef) : $size;
 }
 
-sub volume_snapshot {
+my sub volume_snapshot_locked {
     my ($class, $scfg, $storeid, $volname, $snap) = @_;
 
     my ($vmid, $format) = ($class->parse_volname($volname))[2, 6];
@@ -1050,6 +1050,17 @@ sub volume_snapshot {
     }
 }
 
+sub volume_snapshot {
+    my ($class, $scfg, $storeid, $volname, $snap) = @_;
+
+    return $class->cluster_lock_storage(
+        $storeid,
+        $scfg->{shared},
+        undef,
+        sub { return volume_snapshot_locked($class, $scfg, $storeid, $volname, $snap); },
+    );
+}
+
 # Asserts that a rollback to $snap on $volname is possible.
 # If certain snapshots are preventing the rollback and $blockers is an array
 # reference, the snapshot names can be pushed onto $blockers prior to dying.
@@ -1086,7 +1097,7 @@ sub volume_rollback_is_possible {
     return 1;
 }
 
-sub volume_snapshot_rollback {
+my sub volume_snapshot_rollback_locked {
     my ($class, $scfg, $storeid, $volname, $snap) = @_;
 
     my $format = ($class->parse_volname($volname))[6];
@@ -1108,6 +1119,19 @@ sub volume_snapshot_rollback {
     return undef;
 }
 
+sub volume_snapshot_rollback {
+    my ($class, $scfg, $storeid, $volname, $snap) = @_;
+
+    return $class->cluster_lock_storage(
+        $storeid,
+        $scfg->{shared},
+        undef,
+        sub {
+            return volume_snapshot_rollback_locked($class, $scfg, $storeid, $volname, $snap);
+        },
+    );
+}
+
 sub volume_snapshot_delete {
     my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
 
@@ -1117,7 +1141,14 @@ sub volume_snapshot_delete {
     die "can't delete snapshot for '$format' volume\n" if $format ne 'qcow2';
 
     if ($running) {
-        my $cleanup_worker = eval { free_snap_image($class, $storeid, $scfg, $volname, $snap); };
+        my $cleanup_worker = eval {
+            return $class->cluster_lock_storage(
+                $storeid,
+                $scfg->{shared},
+                undef,
+                sub { return free_snap_image($class, $storeid, $scfg, $volname, $snap); },
+            );
+        };
         die "error deleting snapshot $snap $@\n" if $@;
         fork_cleanup_worker($cleanup_worker);
         return;
@@ -1152,19 +1183,31 @@ sub volume_snapshot_delete {
                 "The state of $snap is now invalid. Don't try to clone or rollback it. You can only try to delete it again later\n";
             die "error commiting $childsnap to $snap; $@\n";
         }
-        print "delete $childvolname\n";
-        my $cleanup_worker =
-            eval { free_snap_image($class, $storeid, $scfg, $volname, $childsnap) };
-        if ($@) {
-            die "error delete old snapshot volume $childvolname: $@\n";
-        }
 
-        print "rename $snapvolname to $childvolname\n";
-        eval { lvrename($scfg, $snapvolname, $childvolname) };
-        if ($@) {
-            warn $@;
-            $err = "error renaming snapshot: $@\n";
-        }
+        print "delete $childvolname\n";
+        my $cleanup_worker = eval {
+            return $class->cluster_lock_storage(
+                $storeid,
+                $scfg->{shared},
+                undef,
+                sub {
+                    my $cleanup_worker_sub =
+                        eval { free_snap_image($class, $storeid, $scfg, $volname, $childsnap) };
+                    if ($@) {
+                        die "error delete old snapshot volume $childvolname: $@\n";
+                    }
+
+                    print "rename $snapvolname to $childvolname\n";
+                    eval { lvrename($scfg, $snapvolname, $childvolname) };
+                    if ($@) {
+                        warn $@;
+                        $err = "error renaming snapshot: $@\n";
+                    }
+
+                    return $cleanup_worker_sub;
+                },
+            );
+        };
         fork_cleanup_worker($cleanup_worker);
 
     } else {
@@ -1190,7 +1233,14 @@ sub volume_snapshot_delete {
             die "error rebase $childsnap from $parentsnap; $@\n";
         }
         #delete the snapshot
-        my $cleanup_worker = eval { free_snap_image($class, $storeid, $scfg, $volname, $snap); };
+        my $cleanup_worker = eval {
+            return $class->cluster_lock_storage(
+                $storeid,
+                $scfg->{shared},
+                undef,
+                sub { return free_snap_image($class, $storeid, $scfg, $volname, $snap); },
+            );
+        };
         die "error deleting old snapshot volume $snapvolname: $@\n" if $@;
         fork_cleanup_worker($cleanup_worker);
     }
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH-SERIES storage 0/4] lvm plugin: error handling and locking fixes

[Friedrich Weber]

On 03/11/2025 17:23, Fiona Ebner wrote:
> Patch 1/4 is improving an error message by including the previously
> captured error.
> 
> Patch 2/4 fixes error handling in volume_snapshot_rollback().
> Previously, allocation would be ignored when 'saferemove' was enabled
> on the storage.
> 
> Patches 3/4 and 4/4 fix the locking for volume import and snapshot
> operations. Better viewed with "-w" or
> "-w --word-diff=color --word-diff-regex='\w+'".

Thanks for the patch!

I tested the series in my 2-node cluster with shared iSCSI + LVM +
Snapshot-as-volume-chain. My previous reproducer for the metadata
corruption (creating and deleting snapshots of two VMs on the two nodes
in a loop) does not trigger a metadata corruption anymore, so:

Tested-by: Friedrich Weber <f.weber@proxmox.com>

Note I have not tested the parallel import scenario sketched in patch 3/4.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH-SERIES storage 0/4] lvm plugin: error handling and locking fixes

[Thomas Lamprecht]

On Mon, 03 Nov 2025 17:23:11 +0100, Fiona Ebner wrote:
> Patch 1/4 is improving an error message by including the previously
> captured error.
> 
> Patch 2/4 fixes error handling in volume_snapshot_rollback().
> Previously, allocation would be ignored when 'saferemove' was enabled
> on the storage.
> 
> [...]

Applied, thanks!

[1/4] lvm plugin: snapshot delete: propagate previously captured error
      commit: f0f90549264982158414761ae01f96b0ca0099aa
[2/4] lvm plugin: fix error handling in volume_snapshot_rollback()
      commit: 0864fda2fd11f811a138f31477ce6e09ad4da186
[3/4] lvm plugin: volume import: lock allocation and removal sections
      commit: 5988ac02504b45eed5df1c98439695a5d561cee2
[4/4] lvm plugin: snapshot-as-volume-chain: use locking for snapshot operations
      commit: 8eabcc70114146fe95f1c977c404261cea366f15


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel