From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id A04F91FF138 for ; Tue, 21 Jul 2026 14:38:05 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A75BF21475; Tue, 21 Jul 2026 14:38:04 +0200 (CEST) From: Lukas Sichert To: pve-devel@lists.proxmox.com Subject: [PATCH storage v10 1/6] lvm: saferemove: keep LVs where zero-out failed for manual zero-out Date: Tue, 21 Jul 2026 14:37:16 +0200 Message-ID: <20260721123724.45395-2-l.sichert@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260721123724.45395-1-l.sichert@proxmox.com> References: <20260721123724.45395-1-l.sichert@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784637423762 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.131 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: H3VSUSXDMBN7STSHN524VHQ3PCZUBC63 X-Message-ID-Hash: H3VSUSXDMBN7STSHN524VHQ3PCZUBC63 X-MailFrom: l.sichert@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Lukas Sichert X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Currently even if 'zeroout' fails, the LV is removed and can't be zeroed out manually later. Let zeroing errors propagate from the secure delete command, and rename failed removals to a 'failed--del-*' LV name instead of immediately removing them. Signed-off-by: Lukas Sichert --- src/PVE/Storage/LVMPlugin.pm | 119 ++++++++++++++++++++++++++++------- 1 file changed, 95 insertions(+), 24 deletions(-) diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm index a313ecc..4734e11 100644 --- a/src/PVE/Storage/LVMPlugin.pm +++ b/src/PVE/Storage/LVMPlugin.pm @@ -7,8 +7,10 @@ use Cwd qw(abs_path); use File::Basename; use IO::File; use JSON; +use List::Util qw(max); use PVE::JSONSchema qw(get_standard_option); +use PVE::RESTEnvironment qw(log_warn); use PVE::Tools qw(run_command file_read_firstline trim); use PVE::Storage::Common; @@ -279,6 +281,49 @@ sub lvm_list_volumes { return $lvs; } +my sub rename_after_failed_cleanup { + my ($class, $scfg, $storeid, $vg, $name) = @_; + + eval { + my $failed_name; + $class->cluster_lock_storage( + $storeid, + $scfg->{shared}, + undef, + sub { + my $vgs = lvm_vgs(); + die "volume group '$vg' not found\n" + if !defined($vgs->{$vg}); + + my $lvs = lvm_list_volumes($vg); + my $existing = $lvs->{$vg} // {}; + + my $prefix = 'failed-'; + my $suffix = "-del-$name"; + + my $last_fail = max( + -1, + map { + /^\Q$prefix\E(\d+)\Q$suffix\E$/ ? $1 : () + } keys %$existing, + ); + + $failed_name = 'failed-' . ($last_fail + 1) . $suffix; + + my $cmd = ['/sbin/lvrename', $vg, "del-$name", $failed_name]; + run_command( + $cmd, + errmsg => "lvrename '$vg/del-$name' to '$vg/$failed_name' error", + ); + print "renamed '$vg/del-$name' to '$vg/$failed_name'\n"; + }, + ); + }; + if (my $rename_err = $@) { + print STDERR "ERROR: unable to rename '$vg/del-$name': $rename_err"; + } +} + my sub free_lvm_volumes_locked { my ($class, $scfg, $storeid, $volnames) = @_; @@ -327,6 +372,9 @@ my sub free_lvm_volumes_locked { '-t', "$throughput", ]; + # FIXME: handle cstream's expected ENOSPC failure explicitly and let other + # errors propagate. For now, preserve the old behavior where cstream can + # fail successfully with ENOSPC after writing until the device is full. eval { run_command( $cmd, @@ -345,41 +393,64 @@ my sub free_lvm_volumes_locked { } my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"]; - eval { run_command($cmd); }; - warn $@ if $@; + run_command($cmd); } }; # we need to zero out LVM data for security reasons # and to allow thin provisioning my $zero_out_worker = sub { + + my $total_cleanup_errors = 0; for my $name (@$volnames) { my $lvmpath = "/dev/$vg/del-$name"; print "zero-out data on image $name ($lvmpath)\n"; - my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath]; - run_command( - $cmd_activate, - errmsg => "can't activate LV '$lvmpath' to zero-out its data", - ); - $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath]; - run_command( - $cmd_activate, - errmsg => "can't refresh LV '$lvmpath' to zero-out its data", - ); - - $secure_delete_cmd->($lvmpath); + eval { + # pass an errfunc here so that debug information is not by lvm to stderr, + # but by the print STDERR below with additional information + my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath]; + run_command( + $cmd_activate, + errmsg => "can't activate LV '$lvmpath' to zero-out its data", + errfunc => sub { }, + ); + $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath]; + run_command( + $cmd_activate, + errmsg => "can't refresh LV '$lvmpath' to zero-out its data", + errfunc => sub { }, + ); + }; + if (my $activation_err = $@) { + print STDERR "ERROR: $activation_err"; + eval { rename_after_failed_cleanup($class, $scfg, $storeid, $vg, $name) }; + $total_cleanup_errors += 1; + next; + } - $class->cluster_lock_storage( - $storeid, - $scfg->{shared}, - undef, - sub { - my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$name"]; - run_command($cmd, errmsg => "lvremove '$vg/del-$name' error"); - }, - ); - print "successfully removed volume $name ($vg/del-$name)\n"; + eval { $secure_delete_cmd->($lvmpath); }; + if (my $cleanup_err = $@) { + print STDERR "ERROR: cleanup failed for lv $name: $cleanup_err"; + eval { rename_after_failed_cleanup($class, $scfg, $storeid, $vg, $name) }; + $total_cleanup_errors += 1; + next; + } else { + $class->cluster_lock_storage( + $storeid, + $scfg->{shared}, + undef, + sub { + my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$name"]; + run_command($cmd, errmsg => "lvremove '$vg/del-$name' error"); + }, + ); + print "successfully removed volume $name ($vg/del-$name)\n"; + } + } + if ($total_cleanup_errors != 0) { + my $number_of_vols = scalar @$volnames; + die "cleanup failed for $total_cleanup_errors out of $number_of_vols volumes\n"; } }; -- 2.47.3