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

[Friedrich Weber <f.weber@proxmox.com>]

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

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