Re: [PATCH storage v10 2/6] lvm: saferemove: zero out volumes range by range

[David Riley <d.riley@proxmox.com>]

Thanks for tackling this issue.
one comment inline.

On 7/21/26 2:37 PM, Lukas Sichert wrote:
> 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 syswrites
> to 10 MiB/s.
>
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   src/PVE/Storage/LVMPlugin.pm | 160 +++++++++++++++++++++++++----------
>   1 file changed, 117 insertions(+), 43 deletions(-)
>
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 4734e11..ee54aef 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -4,11 +4,13 @@ use strict;
>   use warnings;
>   
>   use Cwd qw(abs_path);
> +use Fcntl qw(SEEK_SET);
>   use File::Basename;
>   use IO::File;
>   use JSON;
>   use List::Util qw(max);
>   
> +use PVE::Format qw(render_bytes render_duration);
>   use PVE::JSONSchema qw(get_standard_option);
>   use PVE::RESTEnvironment qw(log_warn);
>   use PVE::Tools qw(run_command file_read_firstline trim);
> @@ -20,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
> @@ -324,6 +330,13 @@ my sub rename_after_failed_cleanup {
>       }
>   }
>   
> +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) = @_;
>   
> @@ -349,56 +362,117 @@ 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 from $sysdir cannot be read\n";
> +        ($size) = $size =~ m/^(\d+)$/; # untaint
> +        $size *= 512; # sysfs size is in 512-byte sectors
> +
> +        my $zeroout_variant = 'blkzeroout';
> +        my $throughput = undef;
> +        if ($scfg->{saferemove_throughput}) {
> +            # use abs as legacy cstream accepted negative values
> +            $throughput = abs($scfg->{saferemove_throughput});
> +            my $rendered_throughput = render_bytes($throughput);
> +            print "using saferemove throughput limit: $rendered_throughput/s\n";
> +        }
> +
> +        # 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";
> +            $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 (!defined($throughput)) {
> +                # FIXME: MAJOR VERSION: increase to 100 MiB/s
> +                $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",
> -            ];
> -            # FIXME: handle cstream's expected ENOSPC failure explicitly and let other
> -            # errors propagate. For now, preserve the old behavior where cstream can
> -            # fail successfully with ENOSPC after writing until the device is full.
> -            eval {
> -                run_command(
> -                    $cmd,
> -                    errmsg => "zero out finished (note: 'No space left on device' is ok here)",
> -                );
> -            };
> -            warn $@ if $@;
> -        } 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;
>   
> -                $stepsize = $write_zeroes_max_bytes;
> -            }
> +            for (my $offset = 0; $offset < $size; $offset += $written) {
> +
> +                if ($offset + $stepsize > $size) {
> +                    $stepsize = $size - $offset;
> +                }
> +
> +                if ($zeroout_variant eq 'blkzeroout') {
> +                    eval { blockdev_ioctl_range($fh, BLKZEROOUT, $offset, $stepsize); };
> +                    if (my $err = $@) {
> +                        die "blkzeroout for $stepsize bytes at offset $offset failed: $err";
> +                    }
> +                    $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, 0)
> +                        or die "syswrite failed: $!\n";


nit: if syswrite returns 0 here, $! won't be populated with an error message [0]. This
means the die statement will either print empty ("syswrite failed: ") or print a stale
error message from a previous failure.

[0] https://perldoc.perl.org/functions/syswrite

> +
> +                    while ($written < $stepsize) {
> +                        my $remaining = $stepsize - $written;
> +                        my $retried_write = syswrite($fh, $zeroes, $remaining, $written)
> +                            or die "syswrite failed: $!\n";


same here.

> +                        $written += $retried_write;
> +                    }
> +
> +                }
> +                $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\n",
> +                        render_bytes($written_total),
> +                        render_bytes($size),
> +                        $percent_finished,
> +                        $zeroout_variant,
> +                        render_duration($curr_seconds),
> +                    );
> +                    $lastprint = $curr_time;
> +                }
>   
> -            my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
> -            run_command($cmd);
> +                if (defined($throughput)) {
> +                    my $expected_elapsed = $written_total / $throughput;
> +                    my $actual_elapsed = $curr_time - $start;
> +                    my $delay = $expected_elapsed - $actual_elapsed;
> +                    if ($delay > 0) {
> +                        sleep($delay);

nit: the built in sleep only accepts integers, so if the delay is 0.45 for example
it would call sleep 0 and therefore would not throttle [0]. The alternative
would be to use Time::HiRes [1].

[0] https://perldoc.perl.org/functions/sleep
[1] https://perldoc.perl.org/Time::HiRes


> +                    }
> +                }
> +            }
> +        };
> +        # close filehandle before throwing an error
> +        my $err = $@;
> +        close($fh);
> +        if ($err) {
> +            die "$err";
>           }
>       };
>   
> -    # we need to zero out LVM data for security reasons
> -    # and to allow thin provisioning
> +    # we need to zero out LVM data for security reasons and to allow thin provisioning
>       my $zero_out_worker = sub {
>   
>           my $total_cleanup_errors = 0;
> @@ -507,7 +581,7 @@ sub properties {
>               type => 'integer',
>           },
>           saferemove_throughput => {
> -            description => "Wipe throughput (cstream -t parameter value).",
> +            description => "Wipe throughput in bytes.",
>               type => 'string',
>           },
>           tagged_only => {