* [pve-devel] [PATCH storage 3/4] lvm plugin: volume import: lock allocation and removal sections
2025-11-03 16:23 [pve-devel] [PATCH-SERIES storage 0/4] lvm plugin: error handling and locking fixes Fiona Ebner
2025-11-03 16:23 ` [pve-devel] [PATCH storage 1/4] lvm plugin: snapshot delete: propagate previously captured error Fiona Ebner
2025-11-03 16:23 ` [pve-devel] [PATCH storage 2/4] lvm plugin: fix error handling in volume_snapshot_rollback() Fiona Ebner
@ 2025-11-03 16:23 ` Fiona Ebner
2025-11-03 16:23 ` [pve-devel] [PATCH storage 4/4] lvm plugin: snapshot-as-volume-chain: use locking for snapshot operations Fiona Ebner
3 siblings, 0 replies; 5+ messages in thread
From: Fiona Ebner @ 2025-11-03 16:23 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH storage 4/4] lvm plugin: snapshot-as-volume-chain: use locking for snapshot operations
2025-11-03 16:23 [pve-devel] [PATCH-SERIES storage 0/4] lvm plugin: error handling and locking fixes Fiona Ebner
` (2 preceding siblings ...)
2025-11-03 16:23 ` [pve-devel] [PATCH storage 3/4] lvm plugin: volume import: lock allocation and removal sections Fiona Ebner
@ 2025-11-03 16:23 ` Fiona Ebner
3 siblings, 0 replies; 5+ messages in thread
From: Fiona Ebner @ 2025-11-03 16:23 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread