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