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 39D8B1FF14C for ; Fri, 15 May 2026 16:03:35 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E584CFC32; Fri, 15 May 2026 16:03:29 +0200 (CEST) Message-ID: <52bd6c1c-172f-436c-a6ff-453dd84814ce@proxmox.com> Date: Fri, 15 May 2026 16:02:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Friedrich Weber Subject: Re: [PATCH storage v4 1/2] fix #7339: lvmthick: add worker to free space of to be deleted VMs To: Lukas Sichert , pve-devel@lists.proxmox.com References: <20260513120240.81893-1-l.sichert@proxmox.com> <20260513120240.81893-2-l.sichert@proxmox.com> Content-Language: en-US In-Reply-To: <20260513120240.81893-2-l.sichert@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778853766689 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.012 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 KAM_SHORT 0.001 Use of a URL Shortener for very short URL 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: LOMCTMDK64HFIM57IYLYSPPFNQJ5E4TI X-Message-ID-Hash: LOMCTMDK64HFIM57IYLYSPPFNQJ5E4TI X-MailFrom: f.weber@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi, thanks for the v4! Some comments inline. CC'ing Fabian because of naming questions (see below). On 13/05/2026 14:03, Lukas Sichert wrote: > Currently when deleting a VM whose disk is stored on a > thinly-provisioned LUN there is no way to also free the storage space > used by the VM. This is because the current implementation only calls > 'lvremove'. This command deletes the LVM meta-data for the disk, but it > does not send discards to the SAN. 'lvmremove' can also be used with > 'issue_discards', but since LVM meta-data is changed, it needs to be > done under a cluster-wide lock, which can lead to timeouts. There is > already an option to enable 'saferemove', which executes 'blkdiscard > --zeroout' to override the whole storage space allocated to the disk > with zeros. However it does not free the storage space.[1] > > To add the functionality that frees the storage space, adjust the worker > in the code that is already there for zeroing out. In the worker parse > the storage config and if 'issue-blkdiscard' is enabled execute > 'blkdiscard'. This can also be executed in combination with 'blkdiscard > --zeroout' to first zero out the disk and then free the storage > space.[1] > > To add an option to set 'issue-blkdiscard' in the frontend, add a > description, so that the variable will be included in the json-Schema. > > [1] https://man7.org/linux/man-pages/man8/blkdiscard.8.html > > Signed-off-by: Lukas Sichert > --- > src/PVE/Storage.pm | 8 +++---- > src/PVE/Storage/LVMPlugin.pm | 42 ++++++++++++++++++++++++++++++------ > 2 files changed, 40 insertions(+), 10 deletions(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 020fa03..fa0c0bc 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -1192,7 +1192,7 @@ sub vdisk_free { > > activate_storage($cfg, $storeid); > > - my $cleanup_worker; > + my $discard_worker; > > # lock shared storage > $plugin->cluster_lock_storage( > @@ -1206,16 +1206,16 @@ sub vdisk_free { > > my (undef, undef, undef, undef, undef, $isBase, $format) = > $plugin->parse_volname($volname); > - $cleanup_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format); > + $discard_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format); > }, > ); > > - return if !$cleanup_worker; > + return if !$discard_worker; > > my $rpcenv = PVE::RPCEnvironment::get(); > my $authuser = $rpcenv->get_user(); > > - $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker); > + $rpcenv->fork_worker('imgdel', undef, $authuser, $discard_worker); > } > Do we have to rename the variable here? If we don't have to, I don't think we should -- IMO $cleanup_worker is a better name than $discard_worker ('cleanup' is more general than 'discard'). > sub vdisk_list { > diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm > index 3a35e38..03455c6 100644 > --- a/src/PVE/Storage/LVMPlugin.pm > +++ b/src/PVE/Storage/LVMPlugin.pm > @@ -13,6 +13,7 @@ use PVE::Tools qw(run_command file_read_firstline trim); > > use PVE::Storage::Common; > use PVE::Storage::Plugin; > +use PVE::SafeSyslog; > > use base qw(PVE::Storage::Plugin); > > @@ -351,11 +352,20 @@ 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 $discard_worker = sub { > + > for my $name (@$volnames) { > my $lvmpath = "/dev/$vg/del-$name"; > - print "zero-out data on image $name ($lvmpath)\n"; > + > + my $discard_action = > + $scfg->{saferemove} > + && $scfg->{'issue-blkdiscard'} ? 'zero-out data and discard (TRIM)' > + : $scfg->{saferemove} ? 'zero-out data on' > + : $scfg->{'issue-blkdiscard'} ? 'discard (TRIM)' > + : undef; > + print "$discard_action image $name ($lvmpath)\n"; Could be just me, but I find nested ?: expressions quite hard to read. I'd slightly favor a big if/else here (though that is admittedly verbose). > > my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath]; > run_command( > @@ -367,8 +377,18 @@ my sub free_lvm_volumes_locked { > $cmd_activate, > errmsg => "can't refresh LV '$lvmpath' to zero-out its data", > ); > + syslog('info', "starting to $discard_action $name ($lvmpath)") > + if defined($discard_action); > > - $secure_delete_cmd->($lvmpath); > + if ($scfg->{saferemove}) { > + print "zero-out data on image $name ($lvmpath)\n"; > + $secure_delete_cmd->($lvmpath); > + } > + if ($scfg->{'issue-blkdiscard'}) { > + print "discard image $name ($lvmpath)\n"; > + eval { run_command(['/sbin/blkdiscard', $lvmpath]); }; > + warn $@ if $@; > + } > > $class->cluster_lock_storage( > $storeid, > @@ -383,13 +403,13 @@ my sub free_lvm_volumes_locked { > } > }; > > - if ($scfg->{saferemove}) { > + if ($scfg->{saferemove} || $scfg->{'issue-blkdiscard'}) { > 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 $discard_worker; > } else { > for my $name (@$volnames) { > my $cmd = ['/sbin/lvremove', '-f', "$vg/$name"]; > @@ -428,6 +448,15 @@ sub properties { > description => "Zero-out data when removing LVs.", > type => 'boolean', > }, > + 'issue-blkdiscard' => { I'm still not so sure about the name -- in my v3 comment [1] I was in favor of 'discard' over 'blkdiscard' (and I still am), but in addition, 'issue-discard' (or 'issue-blkdiscard' for that matter) may be a bit too general, because this is only relevant for removal, right? What about 'discard-on-remove'? @Fabian what do you think? > + description => "Issue discard (TRIM) requests for LVs before removing them.", > + type => 'boolean', > + verbose_description => > + "If enabled, blkdiscard is issued for the LV before removing it." > + . " This sends discard requests for the LV's block range, allowing" I'd mention TRIM here too, see my v3 comment [1]. > + . " thin-provisioned storage to reclaim previously allocated physical" > + . " space, provided the storage supports discard.", > + }, > 'saferemove-stepsize' => { > description => "Wipe step size in MiB." > . " It will be capped to the maximum supported by the storage.", > @@ -453,6 +482,7 @@ sub options { > shared => { optional => 1 }, > disable => { optional => 1 }, > saferemove => { optional => 1 }, > + 'issue-blkdiscard' => { optional => 1 }, > 'saferemove-stepsize' => { optional => 1 }, > saferemove_throughput => { optional => 1 }, > content => { optional => 1 }, [1] https://lore.proxmox.com/all/24d217bf-5b9f-48d2-8754-9614bbbc5484@proxmox.com/