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

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

Am 07.07.26 um 4:33 PM schrieb Lukas Sichert:
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index a3adeef..8653331 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -12,8 +12,10 @@ use Fcntl qw(SEEK_SET);
>  
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Tools qw(run_command file_read_firstline trim);
> +use PVE::RESTEnvironment qw(log_warn);
>  use PVE::SafeSyslog;
>  use PVE::Format qw(render_bytes render_duration);
> +use PVE::Exception qw(raise_param_exc);

Nit: not ordered alphabetically

>  
>  use PVE::Storage::Common;
>  use PVE::Storage::Plugin;
> @@ -23,6 +25,7 @@ use base qw(PVE::Storage::Plugin);
>  # lvm helper functions
>  
>  use constant {
> +    BLKDISCARD => 0x1277,
>      BLKZEROOUT => 0x127f,
>  };
>  
> @@ -299,6 +302,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) = @_;
>  
> @@ -362,6 +371,8 @@ my sub free_lvm_volumes_locked {
>              my $written_total = 0;
>              my $lastprint = -1;
>              my $written;
> +            my $discard_attempts = 0;
> +            my $discard_failures = 0;
>  
>              for (my $offset = 0; $offset < $size; $offset += $written) {
>  
> @@ -389,6 +400,18 @@ my sub free_lvm_volumes_locked {
>                      }
>  
>                  }
> +                if ($on_remove_opts->{discard}) {
> +                    $discard_attempts++;
> +
> +                    eval { blockdev_ioctl_range($fh, BLKDISCARD, $offset, $written); };
> +                    if ($@) {

Style nit: usually, we assign immediately to a local variable in such
cases, i.e. if (my $err = $@) {

> +                        if ($discard_failures == 0) {
> +                            log_warn(
> +                                "blkdiscard for $written bytes at offset $offset failed: $@");
> +                        }
> +                        $discard_failures += 1;
> +                    }
> +                }
>                  $written_total += $written;
>  
>                  my $curr_time = time();
> @@ -416,7 +439,9 @@ my sub free_lvm_volumes_locked {
>                      }
>                  }
>              }
> -
> +            if ($discard_failures != 0) {
> +                die "$discard_failures out of $discard_attempts blkdiscards failed\n";
> +            }
>          };
>          # close filehandle before throwing an error
>          my $err = $@;
> @@ -426,24 +451,44 @@ 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 $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}) {
> +                $discard_action = 'zero-out and discard (TRIM)';
> +            } elsif ($scfg->{saferemove}) {
> +                $discard_action = 'zero-out';
> +            } elsif ($on_remove_opts->{discard}) {
> +                $discard_action = 'discard (TRIM)';
> +            }
> +            print "$discard_action data on image $name ($lvmpath)\n";
>  
>              my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
>              run_command(
>                  $cmd_activate,
> -                errmsg => "can't activate LV '$lvmpath' to zero-out its data",
> +                errmsg => "can't activate LV '$lvmpath' to $discard_action its data",
>              );
>              $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath];
>              run_command(
>                  $cmd_activate,
> -                errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
> +                errmsg => "can't refresh LV '$lvmpath' to $discard_action its data",
>              );
> +            syslog('info', "starting to $discard_action $name ($lvmpath)");
>  
> -            eval { $secure_delete_cmd->($lvmpath); };
> +            eval {
> +                if ($scfg->{saferemove}) {
> +                    $secure_delete_cmd->($lvmpath);
> +                }
> +
> +                if ($on_remove_opts->{discard} && !$scfg->{saferemove}) {

Style nit: I would put it into an elsif

> +                    run_command(['/sbin/blkdiscard', $lvmpath]);

Nit: should we put an errmsg here?

---snip 8<---

> @@ -583,6 +666,88 @@ sub get_formats {
>      return { default => 'raw', valid => { 'raw' => 1 } };
>  }
>  
> +my sub get_discard_max {
> +    my ($dev_path) = @_;
> +
> +    my $output = '';
> +    # use lsblk as it resolves discard support in setups with
> +    # nested partitions or lv/vgs

Style nit: comment fits in one line

> +    my $cmd = [
> +        '/bin/lsblk',

Nit: use just 'lsblk'

> +        '--json',
> +        '--bytes',
> +        '--discard',
> +        '--nodeps',
> +        '--output',
> +        'PATH,DISC-MAX',

Nit: The path is not used later

> +        $dev_path,
> +    ];
> +
> +    eval {
> +        run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; });
> +    };
> +    if ($@) {
> +        raise_param_exc({
> +            'on-volume-remove' => "discard on remove is enabled, but lsblk could not "
> +                . "query discard support for the backing device '$dev_path'",

Please include the error $@ for completeness

> +        });
> +    }
> +
> +    my $parsed = eval { decode_json($output) };
> +    if ($@ || !$parsed->{blockdevices} || !$parsed->{blockdevices}->[0]) {
> +        raise_param_exc({
> +            'on-volume-remove' => "discard on remove is enabled, but lsblk could not "
> +                . "parse discard support for the backing device '$dev_path'",
> +        });
> +
> +    }
> +
> +    return $parsed->{blockdevices}->[0]->{'disc-max'} // 0;

Nit: We could warn if there is more than one result or are there cases
where that is expected and non-problematic?

> +}
> +
> +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_+./=-]+)$!) {
> +                raise_param_exc({
> +                    'on-volume-remove' => "discard on remove is enabled, but discard support "
> +                        . "cannot be resolved for the backing device '$dev_path'",
> +                });
> +            } else {
> +                $dev_path = $1; # untaint

Style nit: I'd prefer using =~ and switching the branches so the $1 is
closer to where it's obtained

> +            }
> +
> +            my $discard_max_bytes = get_discard_max($dev_path);
> +
> +            # use discard_max_bytes as indicator if discard is supported
> +            if (!$discard_max_bytes) {
> +                raise_param_exc({
> +                    'on-volume-remove' => "discard on remove is enabled, but discard is not "
> +                        . "supported by the backing device '$dev_path'",
> +                });
> +            }
> +        }
> +    }
> +}
> +
>  sub on_add_hook {
>      my ($class, $storeid, $scfg, %param) = @_;
>  
> @@ -604,6 +769,8 @@ sub on_add_hook {
>          lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared});
>      }
>  
> +    assert_discard_supported($scfg->{vgname}, $scfg->{'on-volume-remove'});
> +
>      return;
>  }
>  
> @@ -624,6 +791,7 @@ sub on_update_hook_full {
>          die "$storeid - cannot disable 'snapshot-as-volume-chain' while a qcow2 image exists\n"
>              if grep { $_->{format} eq 'qcow2' } $images->@*;
>      }
> +    assert_discard_supported($scfg->{vgname}, $update->{'on-volume-remove'});
>  }
>  
>  sub parse_volname {