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 C9F501FF14C for ; Fri, 15 May 2026 16:28:26 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 04E6B10781; Fri, 15 May 2026 16:28:24 +0200 (CEST) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable In-Reply-To: <52bd6c1c-172f-436c-a6ff-453dd84814ce@proxmox.com> References: <20260513120240.81893-1-l.sichert@proxmox.com> <20260513120240.81893-2-l.sichert@proxmox.com> <52bd6c1c-172f-436c-a6ff-453dd84814ce@proxmox.com> Subject: Re: [PATCH storage v4 1/2] fix #7339: lvmthick: add worker to free space of to be deleted VMs From: Fabian =?utf-8?q?Gr=C3=BCnbichler?= To: Friedrich Weber , Lukas Sichert , pve-devel@lists.proxmox.com Date: Fri, 15 May 2026 16:28:09 +0200 Message-ID: <177885528916.1932366.10236780530533306479@yuna.proxmox.com> User-Agent: alot/0.0.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778855292225 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.053 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: YWAHRQTVZSQZ7MOUVIHVIHCJNSUAU5VK X-Message-ID-Hash: YWAHRQTVZSQZ7MOUVIHVIHCJNSUAU5VK X-MailFrom: f.gruenbichler@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: Quoting Friedrich Weber (2026-05-15 16:02:52) > Hi, thanks for the v4! Some comments inline. CC'ing Fabian because of > naming questions (see below). >=20 > 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] > >=20 > > 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] > >=20 > > 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. > >=20 > > [1] https://man7.org/linux/man-pages/man8/blkdiscard.8.html > >=20 > > Signed-off-by: Lukas Sichert > > --- > > src/PVE/Storage.pm | 8 +++---- > > src/PVE/Storage/LVMPlugin.pm | 42 ++++++++++++++++++++++++++++++------ > > 2 files changed, 40 insertions(+), 10 deletions(-) > >=20 > > 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 { > > =20 > > activate_storage($cfg, $storeid); > > =20 > > - my $cleanup_worker; > > + my $discard_worker; > > =20 > > # lock shared storage > > $plugin->cluster_lock_storage( > > @@ -1206,16 +1206,16 @@ sub vdisk_free { > > =20 > > my (undef, undef, undef, undef, undef, $isBase, $format) = =3D > > $plugin->parse_volname($volname); > > - $cleanup_worker =3D $plugin->free_image($storeid, $scfg, $= volname, $isBase, $format); > > + $discard_worker =3D $plugin->free_image($storeid, $scfg, $= volname, $isBase, $format); > > }, > > ); > > =20 > > - return if !$cleanup_worker; > > + return if !$discard_worker; > > =20 > > my $rpcenv =3D PVE::RPCEnvironment::get(); > > my $authuser =3D $rpcenv->get_user(); > > =20 > > - $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker); > > + $rpcenv->fork_worker('imgdel', undef, $authuser, $discard_worker); > > } > > =20 >=20 > 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'). agreed. >=20 > > 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 tri= m); > > =20 > > use PVE::Storage::Common; > > use PVE::Storage::Plugin; > > +use PVE::SafeSyslog; > > =20 > > use base qw(PVE::Storage::Plugin); > > =20 > > @@ -351,11 +352,20 @@ my sub free_lvm_volumes_locked { > > }; > > =20 > > # we need to zero out LVM data for security reasons > > - # and to allow thin provisioning > > - my $zero_out_worker =3D sub { > > + # and discard images to free storage space to allow > > + # thin provisioning > > + my $discard_worker =3D sub { > > + > > for my $name (@$volnames) { > > my $lvmpath =3D "/dev/$vg/del-$name"; > > - print "zero-out data on image $name ($lvmpath)\n"; > > + > > + my $discard_action =3D > > + $scfg->{saferemove} > > + && $scfg->{'issue-blkdiscard'} ? 'zero-out data and di= scard (TRIM)' > > + : $scfg->{saferemove} ? 'zero-out data on' > > + : $scfg->{'issue-blkdiscard'} ? 'discard (TRIM)' > > + : undef; > > + print "$discard_action image $name ($lvmpath)\n"; >=20 > 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). would say so else well. also the undef branch is dead code - should that be enforced/handled? >=20 > > =20 > > my $cmd_activate =3D ['/sbin/lvchange', '-aly', $lvmpath]; > > run_command( > > @@ -367,8 +377,18 @@ my sub free_lvm_volumes_locked { > > $cmd_activate, > > errmsg =3D> "can't refresh LV '$lvmpath' to zero-out i= ts data", > > ); > > + syslog('info', "starting to $discard_action $name ($lvmpat= h)") > > + if defined($discard_action); > > =20 > > - $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 $@; > > + } > > =20 > > $class->cluster_lock_storage( > > $storeid, > > @@ -383,13 +403,13 @@ my sub free_lvm_volumes_locked { > > } > > }; > > =20 > > - if ($scfg->{saferemove}) { > > + if ($scfg->{saferemove} || $scfg->{'issue-blkdiscard'}) { > > for my $name (@$volnames) { > > # avoid long running task, so we only rename here > > my $cmd =3D ['/sbin/lvrename', $vg, $name, "del-$name"]; > > run_command($cmd, errmsg =3D> "lvrename '$vg/$name' error"= ); > > } > > - return $zero_out_worker; > > + return $discard_worker; > > } else { > > for my $name (@$volnames) { > > my $cmd =3D ['/sbin/lvremove', '-f', "$vg/$name"]; > > @@ -428,6 +448,15 @@ sub properties { > > description =3D> "Zero-out data when removing LVs.", > > type =3D> 'boolean', > > }, > > + 'issue-blkdiscard' =3D> { >=20 > 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? discard-on-remove sounds okay to me as well, but what about setting this up= for a unification with PVE 10 by doing: on-volume-remove =3D discard=3D1 and then migrating the saferemove parameters to this new property string? on-volume-remove =3D discard=3Dbool,zero=3Dbool,zero_stepsize=3D..,zero_thr= oughput=3D.. ? >=20 > > + description =3D> "Issue discard (TRIM) requests for LVs be= fore removing them.", > > + type =3D> 'boolean', > > + verbose_description =3D> > > + "If enabled, blkdiscard is issued for the LV before re= moving it." > > + . " This sends discard requests for the LV's block ran= ge, allowing" >=20 > I'd mention TRIM here too, see my v3 comment [1]. >=20 > > + . " thin-provisioned storage to reclaim previously all= ocated physical" > > + . " space, provided the storage supports discard.", > > + }, > > 'saferemove-stepsize' =3D> { > > description =3D> "Wipe step size in MiB." > > . " It will be capped to the maximum supported by the = storage.", > > @@ -453,6 +482,7 @@ sub options { > > shared =3D> { optional =3D> 1 }, > > disable =3D> { optional =3D> 1 }, > > saferemove =3D> { optional =3D> 1 }, > > + 'issue-blkdiscard' =3D> { optional =3D> 1 }, > > 'saferemove-stepsize' =3D> { optional =3D> 1 }, > > saferemove_throughput =3D> { optional =3D> 1 }, > > content =3D> { optional =3D> 1 }, >=20 > [1] > https://lore.proxmox.com/all/24d217bf-5b9f-48d2-8754-9614bbbc5484@proxmox= .com/