From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id B0B831FF0E1 for ; Mon, 13 Jul 2026 18:01:01 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A6FCD214C1; Mon, 13 Jul 2026 18:00:55 +0200 (CEST) From: Lukas Sichert To: pve-devel@lists.proxmox.com Subject: [PATCH storage v9 3/5] fix #7339: lvm: add discard action for removed volumes Date: Mon, 13 Jul 2026 18:00:04 +0200 Message-ID: <20260713160008.121125-4-l.sichert@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260713160008.121125-1-l.sichert@proxmox.com> References: <20260713160008.121125-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: 1783958404959 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.242 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: MPGDKJB3OVXOFRMYC6HWQWXBUM6YS73U X-Message-ID-Hash: MPGDKJB3OVXOFRMYC6HWQWXBUM6YS73U 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: On LVM storages backed by thin-provisioned SAN LUNs, removing an LV does not release the allocated space on the backing storage. Using LVM's `issue_discards` can avoid that, but makes `lvremove` issue the discards while holding the cluster-wide storage lock, which can hit the lock timeout for large volumes. Add an `on-volume-remove` property with an initial `discard` action. Reject the option if discard is not supported by the backing devices. The cleanup worker issues the discard for the renamed LV before the final remove, so the long-running operation happens outside the storage lock. When combined with `saferemove`, the worker zeroes and discards the LV range by range, avoiding allocation of the whole LV with zeroes on thin-provisioned backing storage. Signed-off-by: Lukas Sichert Link: bugzilla.proxmox.com/show_bug.cgi?id=7339 --- src/PVE/Storage/LVMPlugin.pm | 195 +++++++++++++++++++++++++++++++++-- 1 file changed, 185 insertions(+), 10 deletions(-) diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm index d8b5c8c..4d50c4b 100644 --- a/src/PVE/Storage/LVMPlugin.pm +++ b/src/PVE/Storage/LVMPlugin.pm @@ -10,6 +10,7 @@ use IO::File; use JSON; use List::Util qw(max); +use PVE::Exception qw(raise_param_exc); use PVE::Format qw(render_bytes render_duration); use PVE::JSONSchema qw(get_standard_option); use PVE::RESTEnvironment qw(log_warn); @@ -24,6 +25,7 @@ use base qw(PVE::Storage::Plugin); # lvm helper functions use constant { + BLKDISCARD => 0x1277, BLKZEROOUT => 0x127f, }; @@ -338,6 +340,12 @@ my sub free_lvm_volumes_locked { my $vg = $scfg->{vgname}; + my $on_remove_opts = {}; + if ($scfg->{'on-volume-remove'}) { + $on_remove_opts = + PVE::JSONSchema::parse_property_string('on-volume-remove', $scfg->{'on-volume-remove'}); + } + my $secure_delete_cmd = sub { my ($lvmpath) = @_; @@ -402,6 +410,8 @@ my sub free_lvm_volumes_locked { my $written_total = 0; my $lastprint = -1; my $written; + my $discard_attempts = 0; + my $discard_failures = 0; for (my $offset = 0; $offset < $size; $offset += $written) { @@ -411,8 +421,8 @@ my sub free_lvm_volumes_locked { if ($zeroout_variant eq 'blkzeroout') { eval { blockdev_ioctl_range($fh, BLKZEROOUT, $offset, $stepsize); }; - if ($@) { - die "blkzeroout for $stepsize bytes at offset $offset failed: $@"; + if (my $err = $@) { + die "blkzeroout for $stepsize bytes at offset $offset failed: $err"; } $written = $stepsize; } elsif ($zeroout_variant eq 'syswrite') { @@ -432,6 +442,18 @@ my sub free_lvm_volumes_locked { } } + if ($on_remove_opts->{discard}) { + $discard_attempts++; + + eval { blockdev_ioctl_range($fh, BLKDISCARD, $offset, $written); }; + if (my $err = $@) { + if ($discard_failures == 0) { + log_warn( + "blkdiscard for $written bytes at offset $offset failed: $err"); + } + $discard_failures += 1; + } + } $written_total += $written; my $curr_time = time(); @@ -459,6 +481,9 @@ my sub free_lvm_volumes_locked { } } } + if ($discard_failures != 0) { + die "$discard_failures out of $discard_attempts blkdiscards failed\n"; + } }; # close filehandle before throwing an error my $err = $@; @@ -467,25 +492,47 @@ my sub free_lvm_volumes_locked { die "$err"; } }; + # we need to zero out LVM data for security reasons + # and discard images to free storage space to allow + # thin provisioning + my $cleanup_worker = sub { - # we need to zero out LVM data for security reasons and to allow thin provisioning - my $zero_out_worker = sub { for my $name (@$volnames) { my $lvmpath = "/dev/$vg/del-$name"; - print "zero-out data on image $name ($lvmpath)\n"; + + my $discard_action; + if ($scfg->{saferemove} && $on_remove_opts->{discard}) { + $discard_action = 'zero-out and discard (TRIM)'; + } elsif ($scfg->{saferemove}) { + $discard_action = 'zero-out'; + } elsif ($on_remove_opts->{discard}) { + $discard_action = 'discard (TRIM)'; + } + print "$discard_action 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", + errmsg => "can't activate LV '$lvmpath' to $discard_action its data", ); $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath]; run_command( $cmd_activate, - errmsg => "can't refresh LV '$lvmpath' to zero-out its data", + errmsg => "can't refresh LV '$lvmpath' to $discard_action its data", ); + syslog('info', "starting to $discard_action $name ($lvmpath)"); + + eval { + if ($scfg->{saferemove}) { + $secure_delete_cmd->($lvmpath); - eval { $secure_delete_cmd->($lvmpath); }; + } elsif ($on_remove_opts->{discard}) { + run_command( + ['/sbin/blkdiscard', $lvmpath], + errmsg => "blkdiscard '$lvmpath' error", + ); + } + }; if (my $cleanup_err = $@) { my $failed_name = eval { cleanup_failed_lvm_volume($class, $scfg, $storeid, $vg, $name) }; @@ -515,13 +562,13 @@ my sub free_lvm_volumes_locked { } }; - if ($scfg->{saferemove}) { + if ($scfg->{saferemove} || $on_remove_opts->{discard}) { for my $name (@$volnames) { # avoid long running task, so we only rename here my $cmd = ['/sbin/lvrename', $vg, $name, "del-$name"]; run_command($cmd, errmsg => "lvrename '$vg/$name' error"); } - return $zero_out_worker; + return $cleanup_worker; } else { for my $name (@$volnames) { my $cmd = ['/sbin/lvremove', '-f', "$vg/$name"]; @@ -544,6 +591,36 @@ sub plugindata { }; } +my $on_volume_remove_format = { + discard => { + description => "Issue discard (TRIM) requests for LVs before removing them.", + type => 'boolean', + optional => 1, + verbose_description => "If enabled, blkdiscard is issued for the LV before removing it." + . " This sends discard (TRIM) requests for the LV's block range, allowing" + . " thin-provisioned storage to reclaim previously allocated physical" + . " space, provided the storage supports discard.", + }, +}; + +sub verify_on_volume_remove { + my ($value, $noerr) = @_; + + return undef if !defined($value); + + if (!keys %$value) { + return undef if $noerr; + die "at least one on-volume-remove option must be specified if the property is set\n"; + } + return $value; +} + +PVE::JSONSchema::register_format( + 'on-volume-remove', + $on_volume_remove_format, + \&verify_on_volume_remove, +); + sub properties { return { vgname => { @@ -560,6 +637,13 @@ sub properties { description => "Zero-out data when removing LVs.", type => 'boolean', }, + 'on-volume-remove' => { + description => "Optional actions when removing LVs.", + type => 'string', + format => 'on-volume-remove', + verbose_description => "Configure actions performed before removing an LV." + . " Use 'discard=1' to issue discard (TRIM) requests before removal.", + }, 'saferemove-stepsize' => { description => "Wipe step size in MiB." . " It will be capped to the maximum supported by the storage.", @@ -585,6 +669,7 @@ sub options { shared => { optional => 1 }, disable => { optional => 1 }, saferemove => { optional => 1 }, + 'on-volume-remove' => { optional => 1 }, 'saferemove-stepsize' => { optional => 1 }, saferemove_throughput => { optional => 1 }, content => { optional => 1 }, @@ -607,6 +692,93 @@ sub get_formats { return { default => 'raw', valid => { 'raw' => 1 } }; } +my sub get_discard_max { + my ($dev_path) = @_; + + my $output = ''; + # use lsblk as it resolves discard support in setups with nested partitions or lv/vgs + my $cmd = [ + 'lsblk', '--json', '--bytes', '--discard', '--nodeps', '--output', 'DISC-MAX', + $dev_path, + ]; + + eval { + run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; }); + }; + if (my $err = $@) { + chomp $err; + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but lsblk could not " + . "query discard support for the backing device '$dev_path': $err", + }); + } + + my $parsed = eval { decode_json($output) }; + my $err = $@; + my $blockdevices = $parsed->{blockdevices}; + if ($err || !$blockdevices || scalar($blockdevices->@*) == 0) { + chomp $err if $err; + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but lsblk could not " + . "parse discard support for the backing device '$dev_path'" + . ($err ? ": $err" : ""), + }); + + } + + if (scalar($blockdevices->@*) > 1) { + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but lsblk returned " + . "ambiguous discard support for the backing device '$dev_path'", + }); + } + + return $blockdevices->[0]->{'disc-max'} // 0; +} + +sub assert_discard_supported { + my ($vgname, $on_remove) = @_; + + return if !defined($on_remove); + + my $on_remove_opts = PVE::JSONSchema::parse_property_string('on-volume-remove', $on_remove); + if ($on_remove_opts->{discard}) { + + my $vgs = lvm_vgs(1); + my $vg = $vgs->{$vgname}; + die "no such volume group '$vgname'\n" if !$vg; + + my $pvs = $vg->{pvs}; + die "volume group '$vgname' has no physical volumes\n" + if !defined($pvs) || scalar($pvs->@*) == 0; + + # check if all the block devices configured to the volume group support discard + for my $pv ($vg->{pvs}->@*) { + + my $dev_path = abs_path($pv->{name}) // $pv->{name}; + + if ($dev_path =~ m!^(/dev/[A-Za-z0-9_+./=-]+)$!) { + $dev_path = $1; # untaint + } else { + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but discard support " + . "cannot be resolved for the backing device '$dev_path'", + }); + } + + my $discard_max_bytes = get_discard_max($dev_path); + + # use discard_max_bytes as indicator if discard is supported + if (!$discard_max_bytes) { + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but discard is not " + . "supported by the backing device '$dev_path'", + }); + } + } + } +} + sub on_add_hook { my ($class, $storeid, $scfg, %param) = @_; @@ -628,6 +800,8 @@ sub on_add_hook { lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared}); } + assert_discard_supported($scfg->{vgname}, $scfg->{'on-volume-remove'}); + return; } @@ -648,6 +822,7 @@ sub on_update_hook_full { die "$storeid - cannot disable 'snapshot-as-volume-chain' while a qcow2 image exists\n" if grep { $_->{format} eq 'qcow2' } $images->@*; } + assert_discard_supported($scfg->{vgname}, $update->{'on-volume-remove'}); } sub parse_volname { -- 2.47.3