From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6F4401FF187 for ; Mon, 3 Nov 2025 17:23:07 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9722C2252A; Mon, 3 Nov 2025 17:23:38 +0100 (CET) From: Fiona Ebner To: pve-devel@lists.proxmox.com Date: Mon, 3 Nov 2025 17:23:15 +0100 Message-ID: <20251103162330.112603-5-f.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251103162330.112603-1-f.ebner@proxmox.com> References: <20251103162330.112603-1-f.ebner@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762186997487 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.021 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [PATCH storage 4/4] lvm plugin: snapshot-as-volume-chain: use locking for snapshot operations X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "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 --- 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