Re: [PATCH storage v9 3/5] fix #7339: lvm: add discard action for removed volumes

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

Am 13.07.26 um 6:00 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.
> Reject the option if discard is not supported by the backing devices.
> 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.
> 
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> Link: bugzilla.proxmox.com/show_bug.cgi?id=7339

Maybe include https:// ? I'd also prefer Buglink rather than Link, but
no big deal.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

With one more comment and one more question below:

> @@ -411,8 +421,8 @@ my sub free_lvm_volumes_locked {
>  
>                  if ($zeroout_variant eq 'blkzeroout') {
>                      eval { blockdev_ioctl_range($fh, BLKZEROOUT, $offset, $stepsize); };
> -                    if ($@) {
> -                        die "blkzeroout for $stepsize bytes at offset $offset failed: $@";
> +                    if (my $err = $@) {
> +                        die "blkzeroout for $stepsize bytes at offset $offset failed: $err";
>                      }
>                      $written = $stepsize;
>                  } elsif ($zeroout_variant eq 'syswrite') {

Should be squashed into the previous patch.

> +sub assert_discard_supported {
> +    my ($vgname, $on_remove) = @_;
> +
> +    return if !defined($on_remove);
> +
> +    my $on_remove_opts = PVE::JSONSchema::parse_property_string('on-volume-remove', $on_remove);
> +    if ($on_remove_opts->{discard}) {
> +
> +        my $vgs = lvm_vgs(1);
> +        my $vg = $vgs->{$vgname};
> +        die "no such volume group '$vgname'\n" if !$vg;
> +
> +        my $pvs = $vg->{pvs};
> +        die "volume group '$vgname' has no physical volumes\n"
> +            if !defined($pvs) || scalar($pvs->@*) == 0;
> +
> +        # check if all the block devices configured to the volume group support discard
> +        for my $pv ($vg->{pvs}->@*) {
> +
> +            my $dev_path = abs_path($pv->{name}) // $pv->{name};
> +
> +            if ($dev_path =~ m!^(/dev/[A-Za-z0-9_+./=-]+)$!) {

The information comes from LVM and is only used as the argument for
lsblk, so should we untaint with .* ? Or is there a specific reason for
this exact regex?

> +                $dev_path = $1; # untaint