Re: [pve-devel] [PATCH v2 pve-storage 1/2] lvmplugin: use blkdiscard when supported instead cstream to saferemove drive

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

It's v4 not v2.

Am 21.10.25 um 4:01 PM schrieb Alexandre Derumier via pve-devel:
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 0416c9e..1c633a3 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -3,10 +3,11 @@ package PVE::Storage::LVMPlugin;
>  use strict;
>  use warnings;
>  
> +use Cwd 'abs_path';
>  use File::Basename;
>  use IO::File;
>  
> -use PVE::Tools qw(run_command trim);
> +use PVE::Tools qw(run_command file_read_firstline trim);
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  
> @@ -284,23 +285,40 @@ my sub free_lvm_volumes {
>  
>      my $vg = $scfg->{vgname};
>  
> -    # we need to zero out LVM data for security reasons
> -    # and to allow thin provisioning
> -    my $zero_out_worker = sub {
> -        # wipe throughput up to 10MB/s by default; may be overwritten with saferemove_throughput
> -        my $throughput = '-10485760';
> -        if ($scfg->{saferemove_throughput}) {
> -            $throughput = $scfg->{saferemove_throughput};
> +    my $secure_delete_cmd = sub {
> +        my ($lvmpath) = @_;
> +
> +        my $stepsize = $scfg->{'saferemove-stepsize'} // 32;
> +        $stepsize = $stepsize * 1024 * 1024;
> +
> +        my $bdev = abs_path($lvmpath);
> +
> +        my $sysdir = undef;
> +        if ($bdev && $bdev =~ m|^/dev/(dm-\d+)|) {
> +            $sysdir = "/sys/block/$1";
> +        } else {
> +            warn "skip secure delete. lvm volume don't seem to be activated\n";
> +            return;
>          }
> -        for my $name (@$volnames) {
> -            print "zero-out data on image $name (/dev/$vg/del-$name)\n";
> +
> +        my $write_zeroes_max_bytes =
> +            file_read_firstline("$sysdir/queue/write_zeroes_max_bytes") // 0;
> +        ($write_zeroes_max_bytes) = $write_zeroes_max_bytes =~ m/^(\d+)$/; #untaint
> +
> +        if ($write_zeroes_max_bytes == 0) {

What if a storage supports discard, but this value is zero? Then we'd
fall back to the slow method even if we don't need to.

> +            # if storage don't support write zeroes, we fallback to cstream
> +            # wipe throughput up to 10MB/s by default; may be overwritten with saferemove_throughput
> +            my $throughput = '-10485760';
> +            if ($scfg->{saferemove_throughput}) {
> +                $throughput = $scfg->{saferemove_throughput};
> +            }
>  
>              my $cmd = [
>                  '/usr/bin/cstream',
>                  '-i',
>                  '/dev/zero',
>                  '-o',
> -                "/dev/$vg/del-$name",
> +                $lvmpath,
>                  '-T',
>                  '10',
>                  '-v',
> @@ -318,6 +336,47 @@ my sub free_lvm_volumes {
>              };
>              warn $@ if $@;
>  
> +        } else {
> +
> +            # if the storage support write_zeroes but stepsize is too big,
> +            # reduce the stepsize to the max possible
> +            if ($write_zeroes_max_bytes > 0 && $stepsize > $write_zeroes_max_bytes) {
> +                warn "reduce stepsize to the maximum supported by the storage:"
> +                    . "$write_zeroes_max_bytes bytes\n";
> +
> +                $stepsize = $write_zeroes_max_bytes;

Similar here, we'd also reduce the step size even if later using discard.

> +            }
> +
> +            my $discard_enabled = undef;
> +
> +            if ($scfg->{'saferemove-discard'}) {
> +                my $discard_zeroes_data =
> +                    file_read_firstline("$sysdir/queue/discard_zeroes_data") // 0;

Are you sure this works? See:
https://www.kernel.org/doc/html/v6.17/admin-guide/abi-stable.html#abi-sys-block-disk-queue-discard-zeroes-data

"[RO] Will always return 0. Don’t rely on any specific behavior for
discards, and don’t read this file."

Isn't discard_max_hw_bytes the correct one, which also can be used to
determine the step size:
https://www.kernel.org/doc/html/v6.17/admin-guide/abi-stable.html#abi-sys-block-disk-queue-discard-max-hw-bytes

"[RO] Devices that support discard functionality may have internal
limits on the number of bytes that can be trimmed or unmapped in a
single operation. The discard_max_hw_bytes parameter is set by the
device driver to the maximum number of bytes that can be discarded in a
single operation. Discard requests issued to the device must not exceed
this limit. A discard_max_hw_bytes value of 0 means that the device does
not support discard functionality."

And I'm not sure a limit of 32 MiB makes sense then. If the hardware
supports much more, it should be fine to use that, or? Do we even want
to consider saferemove-stepsize if discard is supported? Of course
depending on what we decide on the description in the schema needs to be
adapted.

> +
> +                if ($discard_zeroes_data == 0) {
> +                    warn "Discard zeroes not supported. Fallback to zeroing.\n";
> +                } else {
> +                    $discard_enabled = 1;
> +                }
> +            }
> +
> +            my $cmd = ['/usr/sbin/blkdiscard', $lvmpath, '-v', '--step', "${stepsize}"];
> +            push @$cmd, '--zeroout' if !$discard_enabled;
> +
> +            eval { run_command($cmd); };
> +            warn $@ if $@;
> +        }
> +    };
> +
> +    # we need to zero out LVM data for security reasons
> +    # and to allow thin provisioning
> +    my $zero_out_worker = sub {
> +        for my $name (@$volnames) {
> +            my $lvmpath = "/dev/$vg/del-$name";
> +            print "zero-out data on image $name ($lvmpath)\n";
> +
> +            $secure_delete_cmd->($lvmpath);
> +
>              $class->cluster_lock_storage(
>                  $storeid,
>                  $scfg->{shared},
> @@ -376,6 +435,17 @@ sub properties {
>              description => "Zero-out data when removing LVs.",
>              type => 'boolean',
>          },
> +        'saferemove-discard' => {
> +            description => "Wipe with discard instead of zeroing.",
> +            type => 'boolean',
> +            default => 0,
> +        },
> +        'saferemove-stepsize' => {
> +            description => "Wipe step size (default 32MB)."

I'd put "Wipe step size in MiB." in the description and put a
"default => 32" via the dedicated key for defaults in the schema.

> +                . "It will be capped to the maximum storage support.",
> +            enum => [qw(1 2 4 8 16 32)],
> +            type => 'integer',
> +        },
>          saferemove_throughput => {
>              description => "Wipe throughput (cstream -t parameter value).",
>              type => 'string',
> @@ -394,6 +464,8 @@ sub options {
>          shared => { optional => 1 },
>          disable => { optional => 1 },
>          saferemove => { optional => 1 },
> +        'saferemove-discard' => { optional => 1 },
> +        'saferemove-stepsize' => { optional => 1 },
>          saferemove_throughput => { optional => 1 },
>          content => { optional => 1 },
>          base => { fixed => 1, optional => 1 },
> -- 
> 2.47.3
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel