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

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

noticed one more thing here.

On Mon Mar 23, 2026 at 11:14 AM CET, Lukas Sichert wrote:
>      # lock shared storage
>      $plugin->cluster_lock_storage(
> @@ -1206,16 +1206,24 @@ 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);
> +    if ($scfg->{saferemove} && $scfg->{issue_blkdiscard}) {
> +        $rpcenv->fork_worker('diskzerodiscard', undef, $authuser, $discard_worker);
> +    } elsif ($scfg->{saferemove}) {
> +        $rpcenv->fork_worker('diskzero', undef, $authuser, $discard_worker); 
> +    } elsif ($scfg->{issue_blkdiscard}) {
> +        $rpcenv->fork_worker('diskdiscard', undef, $authuser, $discard_worker); 
> +    } else {
> +      die "config must have changed during execution. Disk can't be deleted safely";

This changes the current behavior in a way that affects other storage
plugins. vdisk_free is used for various storage types and this part kind
of assumes specifics of the LVM plugin (as in, for the LVM plugin it is
true that if the discard_workers is set, at least one of the fields
would have to be set) and will simply not fork a worker for any storage
plugin where this is not the case. From what I can see right now, this
discard worker is not returned for any of the other plugins (?), but
that might not always be the case. I think it'd make sense to still run
the 'imgdel' worker from before in the else case since the
discard_worker was obviously defined.

I'm also not sure about the error message. The storage config is only
read once at the beginning of the function. Even if it did change in the
meantime, you would not detect it here.

> +    }
>  }
>  
>  sub vdisk_list {
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 32a8339..b0b94a6 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -352,12 +352,12 @@ my sub free_lvm_volumes {
>  
>      # we need to zero out LVM data for security reasons
>      # and to allow thin provisioning