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

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

Am 13.07.26 um 6:00 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 syswrites
> to 10 MiB/s. Redefine saferemove_throughput as integer instead of string
> to catch configuration errors with the schema already.
> 
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>

With some more comments:

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

> +                } 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";
> +
> +                    while ($written < $stepsize) {
> +                        my $remaining = $stepsize - $written;
> +                        my $retried_write = syswrite($fh, $zeroes, $remaining)

Nit: while it doesn't matter since the buffer is all zeroes, you should
specify the fourth argument for syswrite() for the offset within the
buffer. Or add a quick code comment stating that it's not necessary here.

> +                            or die "syswrite failed: $!\n";
> +                        $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 seconds\n",

Note that render_duration() already includes the time unit, so there
should be no "seconds" here.

> +                        render_bytes($written_total),
> +                        render_bytes($size),
> +                        $percent_finished,
> +                        $zeroout_variant,
> +                        render_duration($curr_seconds),
> +                    );
> +                    $lastprint = $curr_time;
> +                }
> +
> @@ -494,8 +568,8 @@ sub properties {
>              type => 'integer',
>          },
>          saferemove_throughput => {
> -            description => "Wipe throughput (cstream -t parameter value).",
> -            type => 'string',
> +            description => "Wipe throughput in bytes.",
> +            type => 'integer',
>          },
Nit: I'd prefer this to be its own patch.