Re: [PATCH storage v7 2/4] fix #7339: lvm: add discard action for removed volumes

[Fiona Ebner <f.ebner@proxmox.com>]

Am 16.06.26 um 12:13 PM schrieb Lukas Sichert:
> On LVM storages backed by thin-provisioned SAN LUNs, removing an LV does
> not release the allocated space on the backing storage. Using LVM's
> `issue_discards` can avoid that, but makes `lvremove` issue the discards
> while holding the cluster-wide storage lock, which can hit the lock
> timeout for large volumes.
> 
> Add an `on-volume-remove` property with an initial `discard` action. The
> cleanup worker issues the discard for the renamed LV before the final
> remove, so the long-running operation happens outside the storage lock.
> When combined with `saferemove`, the worker zeroes and discards the LV
> range by range, avoiding allocation of the whole LV with zeroes on
> thin-provisioned backing storage.

If a storage has
on-volume-remove discard=1
saferemove 0
then zero-out is still done. I would expect only discard to be done then.

> 
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> Link: bugzilla.proxmox.com/show_bug.cgi?id=7339
> ---
>  src/PVE/Storage/LVMPlugin.pm | 95 ++++++++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index f0a7a80..ee07543 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;

Same nit as in patch 1

>  use PVE::RESTEnvironment qw(log_warn);
>  
>  use base qw(PVE::Storage::Plugin);
> @@ -20,6 +21,7 @@ use base qw(PVE::Storage::Plugin);
>  # lvm helper functions
>  
>  use constant {
> +    BLKDISCARD => 0x1277,
>      BLKZEROOUT => 0x127f,
>  };
>  
> @@ -296,6 +298,12 @@ my sub free_lvm_volumes_locked {
>  
>      my $vg = $scfg->{vgname};
>  
> +    my $on_remove_opts;
> +    if ($scfg->{'on-volume-remove'}) {
> +        $on_remove_opts =
> +            PVE::JSONSchema::parse_property_string('on-volume-remove', $scfg->{'on-volume-remove'});
> +    }
> +
>      my $secure_delete_cmd = sub {
>          my ($lvmpath) = @_;
>  
> @@ -373,6 +381,14 @@ my sub free_lvm_volumes_locked {
>                          die "short syswrite: wrote $written of $stepsize bytes\n";
>                      }
>                  }
> +                if ($on_remove_opts->{discard}) {
> +                    eval {
> +                        blockdev_ioctl_range($fh, 'BLKDISCARD', BLKDISCARD, $offset, $stepsize);
> +                    };
> +                    if ($@) {
> +                        log_warn("blkdiscard failed: $@");

Similar to the last patch, I would log the offset and length for
completeness. I also wonder if we should

> +                    }
> +                }
>                  $written_total += $stepsize;
>  
>                  my $curr_time = time();