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

["Michael Köppl" <m.koeppl@proxmox.com>]

Left some comments inline.

On Fri May 22, 2026 at 3:04 PM CEST, Lukas Sichert wrote:

[snip]

> -
> +    my $on_remove_opts;
> +    if ($scfg->{'on-volume-remove'}) {
> +        $on_remove_opts =
> +            PVE::JSONSchema::parse_property_string('on-volume-remove', $scfg->{'on-volume-remove'});
> +    }
>      # 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'}) {

nit: can just be $on_remove_opts->{discard}

> +                $discard_action = 'zero-out data and discard (TRIM)';
> +            } elsif ($scfg->{saferemove}) {
> +                $discard_action = 'zero-out data on';
> +            } elsif ($on_remove_opts->{'discard'}) {

nit: same as above

> +                $discard_action = 'discard (TRIM)';
> +            }
> +            print "$discard_action image $name ($lvmpath)\n";
>  
>              my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
>              run_command(
> @@ -367,8 +383,18 @@ my sub free_lvm_volumes_locked {
>                  $cmd_activate,
>                  errmsg => "can't refresh LV '$lvmpath' to zero-out its data",

This error message and the one a few lines above it (can't active ...)
should also be updated because they're otherwise a bit misleading. Since
the cleanup worker is now also called when using just `discard` without
`saferemove`, data is not always zeroed-out.

>              );
> +            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";

nit: not sure if the prints inside these if-blocks are necessary.
Because there's already the $discard_action being printed above and then
you'd have output like

    zero-out data and discard (TRIM) image vm-100-disk-0 (/dev/.../del-vm-100-disk-0)
    zero-out data on image vm-100-disk-0 (/dev/.../del-vm-100-disk-0)
    discard image vm-100-disk-0 (/dev/.../del-vm-100-disk-0)

> +                $secure_delete_cmd->($lvmpath);
> +            }
> +            if ($on_remove_opts->{'discard'}) {

nit: can also just be $on_remove_opts->{discard}

> +                print "discard image $name ($lvmpath)\n";
> +                eval { run_command(['/sbin/blkdiscard', $lvmpath]); };

If blkdiscard fails for some reason, I think it'd make sense to
explicitly inform the user in the warning message that the disk space
was not reclaimed. Otherwise they'd "just" get a reason why it failed,
while the consequences of this might not be clear.

> +                warn $@ if $@;
> +            }
>  
>              $class->cluster_lock_storage(
>                  $storeid,
> @@ -383,13 +409,13 @@ my sub free_lvm_volumes_locked {
>          }

[snip]