Re: [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range

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

Am 07.07.26 um 4:33 PM schrieb Lukas Sichert:
> 'saferemove' currently uses different full-volume zero-out paths:
> `blkdiscard --zeroout` for devices with write-zeroes support and
> `cstream` otherwise. This makes consistent progress reporting and
> throttling difficult and prevents interleaving future discard cleanup
> with zeroing. On thin-provisioned backing storage, zeroing the whole LV
> first can force unnecessary allocation.
> 
> Move zeroing into an explicit range loop. Use BLKZEROOUT when supported,
> cap to the device limit, and fall back to manually writing zeroes via
> syswrite otherwise. Add progress reporting in the shared loop, and apply
> a configured saferemove throughput limit there as well. Without an
> explicit limit, keep BLKZEROOUT unthrottled and throttle only manual
> writes to 10 MiB/s.

Nit: s/manual writes/syswrites/

> 
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> ---
>  src/PVE/Storage/LVMPlugin.pm | 146 +++++++++++++++++++++++++++--------
>  1 file changed, 112 insertions(+), 34 deletions(-)
> 
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index a0feb5c..a3adeef 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -8,9 +8,12 @@ use File::Basename;
>  use IO::File;
>  use JSON;
>  use List::Util qw(max);
> +use Fcntl qw(SEEK_SET);

Nit: Not ordered alphabetically

>  
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Tools qw(run_command file_read_firstline trim);
> +use PVE::SafeSyslog;
> +use PVE::Format qw(render_bytes render_duration);

Nit: Not ordered alphabetically

>  
>  use PVE::Storage::Common;
>  use PVE::Storage::Plugin;
> @@ -19,6 +22,10 @@ use base qw(PVE::Storage::Plugin);
>  
>  # lvm helper functions
>  
> +use constant {
> +    BLKZEROOUT => 0x127f,
> +};
> +
>  my $ignore_no_medium_warnings = sub {
>      my $line = shift;
>      # ignore those, most of the time they're from (virtual) IPMI/iKVM devices
> @@ -280,6 +287,13 @@ sub lvm_list_volumes {
>      return $lvs;
>  }
>  
> +my sub blockdev_ioctl_range {
> +    my ($fh, $ioctl, $offset, $length) = @_;
> +
> +    my $range = pack('QQ', $offset, $length);
> +    ioctl($fh, $ioctl, $range) or die "$!\n";
> +}
> +
>  my sub free_lvm_volumes_locked {
>      my ($class, $scfg, $storeid, $volnames) = @_;
>  
> @@ -305,48 +319,112 @@ my sub free_lvm_volumes_locked {
>              file_read_firstline("$sysdir/queue/write_zeroes_max_bytes") // 0;
>          ($write_zeroes_max_bytes) = $write_zeroes_max_bytes =~ m/^(\d+)$/; #untaint
>  
> +        my $size = file_read_firstline("$sysdir/size") or die "size of $sysdir cannot be read";

Missing newline at the end of error message
s/size of/size from/

> +        ($size) = $size =~ m/^(\d+)$/; # untaint
> +        $size *= 512; # sysfs size is in 512-byte sectors
> +
> +        my $zeroout_variant = 'blkzeroout';
> +        my $throughput = -1;

Nit: I'd prefer having it be undef rather than some magic value,
especially since -1 is valid for saferemove_throughput. And unrelated to
the series, saferemove_throughput is type => string, I guess we should
change it to a number/integer. cstream just ignores it if it cannot
parse it, so a bad value is silently ignored right now, but since you
use it for calculations, it will cause errors/warnings, so let's catch
it with the schema already. An already present invalid value would then
cause warnings and be dropped on the next storage.cfg write.

> +        if ($scfg->{saferemove_throughput}) {
> +            # use abs as legacy cstream accepted negative values
> +            $throughput = abs($scfg->{saferemove_throughput});
> +            my $throughput_in_mibs = render_bytes($throughput);
> +            print "using saferemove throughput limit: $throughput_in_mibs/s\n";

Nit: it's not necessarily in mibs, render_bytes() might choose a
different unit. The 's' seems also a bit off, because otherwise it
wouldn't be mibs/s. Maybe just call it $rendered(_throughput)?

> +        }
> +
> +        # If the storage does not support write_zeroes fall back to writing zeroes manually using
> +        # syswrite. Otherwise if the storage supports write_zeroes but stepsize is too big,
> +        # reduce the stepsize to the maximum supported by the storage.
> +        my $zeroes;
>          if ($write_zeroes_max_bytes == 0) {
> -            # If the storage does not 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};
> +            print
> +                "WRITE_ZEROES operation not supported, falling back to syswrite to zero-out '$lvmpath'\n";

Style nit: line too long

> +            $zeroout_variant = 'syswrite';
> +            $stepsize = 1024 * 1024; # 1 MiB
> +            print "reduce stepsize to 1 MiB for syswrite\n";
> +            $zeroes = "\0" x $stepsize;
> +            # limit throughput to 10MiB/s for syswrite, if throughput was not set
> +            if ($throughput <= 0) {
> +                # FIXME: increase to 100 MiB/s with 10.0

Please use the standard format:
# FIXME: MAJOR VERSION: increase ...

> +                $throughput = 10485760;
> +                print "using default syswrite-saferemove throughput limit: 10 MiB/s\n";
>              }
> +        } elsif ($stepsize > $write_zeroes_max_bytes) {
> +            print "reduce stepsize to the maximum supported by the storage:"
> +                . " $write_zeroes_max_bytes bytes\n";
> +            $stepsize = $write_zeroes_max_bytes;
> +        }
> +        open(my $fh, '+<', $lvmpath) or die "can't open '$lvmpath' - $!\n";
>  
> -            my $cmd = [
> -                '/usr/bin/cstream',
> -                '-i',
> -                '/dev/zero',
> -                '-o',
> -                $lvmpath,
> -                '-T',
> -                '10',
> -                '-v',
> -                '1',
> -                '-b',
> -                '1048576',
> -                '-t',
> -                "$throughput",
> -            ];
> -            run_command(
> -                $cmd,
> -                errmsg => "zero out finished (note: 'No space left on device' is ok here)",
> -            );
> -        } else {
> -            # If the storage supports write_zeroes but stepsize is too big, reduce the stepsize to
> -            # the maximum supported by the storage.
> -            if ($write_zeroes_max_bytes > 0 && $stepsize > $write_zeroes_max_bytes) {
> -                print "reduce stepsize to the maximum supported by the storage:"
> -                    . " $write_zeroes_max_bytes bytes\n";
> +        # eval block, so filehandle is closed even if something fails below
> +        eval {
> +            my $start = time();
> +            my $written_total = 0;
> +            my $lastprint = -1;
> +            my $written;
> +
> +            for (my $offset = 0; $offset < $size; $offset += $written) {
>  
> -                $stepsize = $write_zeroes_max_bytes;
> +                if ($offset + $stepsize > $size) {
> +                    $stepsize = $size - $offset;
> +                }
> +
> +                if ($zeroout_variant eq 'blkzeroout') {
> +                    eval { blockdev_ioctl_range($fh, BLKZEROOUT, $offset, $stepsize); };
> +                    if ($@) {
> +                        die "blkzeroout for $stepsize bytes at offset $offset failed: $@";
> +                    }
> +                    $written = $stepsize;
> +                } elsif ($zeroout_variant eq 'syswrite') {
> +                    # if the $offset is 0, sysseek can return 0, therefore use // to only
> +                    # throw an error, if it returns undef
> +                    sysseek($fh, $offset, SEEK_SET) // die "sysseek failed: $!\n";
> +
> +                    # use or as we also want to die if no progress was made, i.e. if $written is 0
> +                    $written = syswrite($fh, $zeroes, $stepsize)
> +                        or die "syswrite failed: $!\n";
> +
> +                    if ($written != $stepsize) {
> +                        warn "short syswrite: wrote $written of $stepsize bytes\n";
> +                    }

I don't think a warning is warranted here. You can just re-issue the
rest of the write. I would do that with an inner loop to keep the
alignment for subsequent non-short writes in-line with the stepsize.

> +
> +                }
> +                $written_total += $written;
> +
> +                my $curr_time = time();
> +                if (($curr_time - $lastprint) >= 3) {
> +                    my $percent_finished = 100 * $written_total / $size;
> +                    my $curr_seconds = $curr_time - $start;
> +
> +                    printf(
> +                        "zeroed out %s of %s (%.2f%%) using %s in %s seconds\n",
> +                        render_bytes($written_total),
> +                        render_bytes($size),
> +                        $percent_finished,
> +                        $zeroout_variant,
> +                        render_duration($curr_seconds),
> +                    );
> +                    $lastprint = $curr_time;
> +                }
> +
> +                if ($throughput > 0) {
> +                    my $expected_elapsed = $written_total / $throughput;
> +                    my $actual_elapsed = $curr_time - $start;
> +                    my $delay = $expected_elapsed - $actual_elapsed;
> +                    if ($delay > 0) {
> +                        sleep($delay);
> +                    }
> +                }
>              }
>  
> -            my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
> -            run_command($cmd);
> +        };
> +        # close filehandle before throwing an error
> +        my $err = $@;
> +        close($fh);
> +        if ($err) {
> +            die "$err";
>          }
>      };
> -

Style nit: I'd keep the blank here for better separation

>      # we need to zero out LVM data for security reasons
>      # and to allow thin provisioning
>      my $zero_out_worker = sub {