From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 2B2601FF13A for ; Wed, 22 Jul 2026 15:48:51 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id F2627214E6; Wed, 22 Jul 2026 15:48:50 +0200 (CEST) Message-ID: <909c90f7-3f00-4f77-b572-0fe2abe8c539@proxmox.com> Date: Wed, 22 Jul 2026 15:48:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH storage v10 2/6] lvm: saferemove: zero out volumes range by range To: Lukas Sichert , pve-devel@lists.proxmox.com References: <20260721123724.45395-1-l.sichert@proxmox.com> <20260721123724.45395-3-l.sichert@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: <20260721123724.45395-3-l.sichert@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784728066170 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.106 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NA7YT4RSUWSPLTV4IPO7KEQ3X2KDADCT X-Message-ID-Hash: NA7YT4RSUWSPLTV4IPO7KEQ3X2KDADCT X-MailFrom: d.riley@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > Reviewed-by: Fiona Ebner > --- > 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 => {