Re: [PATCH storage v4 1/2] fix #7339: lvmthick: add worker to free space of to be deleted VMs

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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).
> 
> 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 <l.sichert@proxmox.com>
> > ---
> >  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').

agreed.

> 
> >  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).

would say so else well. also the undef branch is dead code - should that be
enforced/handled?

> 
> >  
> >              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?

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 = discard=1

and then migrating the saferemove parameters to this new property string?

on-volume-remove = discard=bool,zero=bool,zero_stepsize=..,zero_throughput=..

?

> 
> > +            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/