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 387E01FF146 for ; Tue, 07 Jul 2026 16:33:10 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AB19D2148A; Tue, 07 Jul 2026 16:33:04 +0200 (CEST) From: Lukas Sichert To: pve-devel@lists.proxmox.com Subject: [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes Date: Tue, 7 Jul 2026 16:32:46 +0200 Message-ID: <20260707143252.101757-4-l.sichert@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260707143252.101757-1-l.sichert@proxmox.com> References: <20260707143252.101757-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: 1783434772426 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.000 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) 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: AMGBWTRXS7ZB6S5DJ4JS2YM2J4CG2N5M X-Message-ID-Hash: AMGBWTRXS7ZB6S5DJ4JS2YM2J4CG2N5M 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 | 186 +++++++++++++++++++++++++++++++++-- 1 file changed, 177 insertions(+), 9 deletions(-) diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm index a3adeef..8653331 100644 --- a/src/PVE/Storage/LVMPlugin.pm +++ b/src/PVE/Storage/LVMPlugin.pm @@ -12,8 +12,10 @@ use Fcntl qw(SEEK_SET); use PVE::JSONSchema qw(get_standard_option); use PVE::Tools qw(run_command file_read_firstline trim); +use PVE::RESTEnvironment qw(log_warn); use PVE::SafeSyslog; use PVE::Format qw(render_bytes render_duration); +use PVE::Exception qw(raise_param_exc); use PVE::Storage::Common; use PVE::Storage::Plugin; @@ -23,6 +25,7 @@ use base qw(PVE::Storage::Plugin); # lvm helper functions use constant { + BLKDISCARD => 0x1277, BLKZEROOUT => 0x127f, }; @@ -299,6 +302,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) = @_; @@ -362,6 +371,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) { @@ -389,6 +400,18 @@ my sub free_lvm_volumes_locked { } } + if ($on_remove_opts->{discard}) { + $discard_attempts++; + + eval { blockdev_ioctl_range($fh, BLKDISCARD, $offset, $written); }; + if ($@) { + if ($discard_failures == 0) { + log_warn( + "blkdiscard for $written bytes at offset $offset failed: $@"); + } + $discard_failures += 1; + } + } $written_total += $written; my $curr_time = time(); @@ -416,7 +439,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 = $@; @@ -426,24 +451,44 @@ my sub free_lvm_volumes_locked { } }; # we need to zero out LVM data for security reasons - # and to allow thin provisioning - my $zero_out_worker = sub { + # and discard images to free storage space to allow + # thin provisioning + my $cleanup_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 { $secure_delete_cmd->($lvmpath); }; + eval { + if ($scfg->{saferemove}) { + $secure_delete_cmd->($lvmpath); + } + + if ($on_remove_opts->{discard} && !$scfg->{saferemove}) { + run_command(['/sbin/blkdiscard', $lvmpath]); + } + }; if (my $cleanup_err = $@) { my $failed_name; $class->cluster_lock_storage( @@ -491,13 +536,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"]; @@ -520,6 +565,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 => { @@ -536,6 +611,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.", @@ -561,6 +643,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 }, @@ -583,6 +666,88 @@ 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 = [ + '/bin/lsblk', + '--json', + '--bytes', + '--discard', + '--nodeps', + '--output', + 'PATH,DISC-MAX', + $dev_path, + ]; + + eval { + run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; }); + }; + if ($@) { + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but lsblk could not " + . "query discard support for the backing device '$dev_path'", + }); + } + + my $parsed = eval { decode_json($output) }; + if ($@ || !$parsed->{blockdevices} || !$parsed->{blockdevices}->[0]) { + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but lsblk could not " + . "parse discard support for the backing device '$dev_path'", + }); + + } + + return $parsed->{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_+./=-]+)$!) { + raise_param_exc({ + 'on-volume-remove' => "discard on remove is enabled, but discard support " + . "cannot be resolved for the backing device '$dev_path'", + }); + } else { + $dev_path = $1; # untaint + } + + 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) = @_; @@ -604,6 +769,8 @@ sub on_add_hook { lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared}); } + assert_discard_supported($scfg->{vgname}, $scfg->{'on-volume-remove'}); + return; } @@ -624,6 +791,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