From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 7D2B21FF0E7 for ; Fri, 10 Jul 2026 12:20:10 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A5DDF2141C; Fri, 10 Jul 2026 12:20:09 +0200 (CEST) Message-ID: <160d121b-f7db-466d-bfff-edf9d50f4ef5@proxmox.com> Date: Fri, 10 Jul 2026 12:20:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range To: Lukas Sichert , pve-devel@lists.proxmox.com References: <20260707143252.101757-1-l.sichert@proxmox.com> <20260707143252.101757-3-l.sichert@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260707143252.101757-3-l.sichert@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783678793636 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.026 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) 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: GHNITZWPJKUIDZZ7FBM75JMBOZ7EFKPN X-Message-ID-Hash: GHNITZWPJKUIDZZ7FBM75JMBOZ7EFKPN X-MailFrom: f.ebner@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: 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 > --- > 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 {