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 <[email protected]> --- 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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
