From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id CF2F81FF187 for ; Mon, 22 Sep 2025 16:06:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5D3BA1DC7D; Mon, 22 Sep 2025 16:07:14 +0200 (CEST) Message-ID: Date: Mon, 22 Sep 2025 16:06:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion References: Content-Language: en-US From: Fiona Ebner In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1758549988113 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.024 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH V3 pve-storage] lvm: use blkdiscard instead cstream to saferemove drive X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 18.08.25 um 2:09 PM schrieb Alexandre Derumier via pve-devel: > Current cstream implementation is pretty slow as hell, even without throttling. I'd use either "pretty slow" or "slow as hell" > use blkdiscard --zeroout instead, which is lot of magnetude faster should be: "a few magnitudes" Seems like nice-to-have options given the improvement :) Mostly nits, but also a question about REQ_OP_WRITE_ZEROES below: > Signed-off-by: Alexandre Derumier > --- > src/PVE/Storage/LVMPlugin.pm | 74 +++++++++++++++++++++++------------- > 1 file changed, 48 insertions(+), 26 deletions(-) > > diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm > index 0416c9e..c3a864e 100644 > --- a/src/PVE/Storage/LVMPlugin.pm > +++ b/src/PVE/Storage/LVMPlugin.pm > @@ -6,7 +6,7 @@ use warnings; > use File::Basename; > use IO::File; > > -use PVE::Tools qw(run_command trim); > +use PVE::Tools qw(run_command file_read_firstline trim); > use PVE::Storage::Plugin; > use PVE::JSONSchema qw(get_standard_option); > > @@ -287,35 +287,46 @@ my sub free_lvm_volumes { > # we need to zero out LVM data for security reasons > # and to allow thin provisioning > my $zero_out_worker = sub { > - # 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}; > - } > for my $name (@$volnames) { > - print "zero-out data on image $name (/dev/$vg/del-$name)\n"; > + my $lvmpath = "/dev/$vg/del-$name"; > + print "zero-out data on image $name ($lvmpath)\n"; > + > + warn > + "The 'saferemove_throughput' option set for '${storeid}' is deprecated and will be removed." > + if $scfg->{saferemove_throughput}; Please split the string up (too long right now) and add a newline at the end. It should be an if block rather than post if, since it's multiple lines. I'd also go for "and has no effect anymore." rather than "and will be removed". Would be good to add a reminder comment in the code to remove the option with PVE 10 (and then, for PVE 10, we can warn/error in the pve9to10 script if the setting is still present). > + > + my $stepsize = $scfg->{saferemove_stepsize} // 32; > + $stepsize = $stepsize * 1048576; Minor nit, but "$stepsize * 1024 * 1024" would be more readable. > + > + my $bdev = readlink($lvmpath); I'd prefer abs_path() like we use in other places. > + if ($bdev && $bdev =~ m|^../(dm-(\d+))|) { You don't need to group/capture the \d+ > + my $sysdir = "/sys/block/$1"; > + > + my $write_zeroes_max_bytes = > + file_read_firstline("$sysdir/queue/write_zeroes_max_bytes") // 0; According to the docs: "The value 0 means that REQ_OP_WRITE_ZEROES is not supported.", so it makes sense as a fallback too. But do we need to handle that somehow below, i.e. can --zeroout still be used if the reported value is 0? Or do we need to fall back to the old way of doing it? > + ($write_zeroes_max_bytes) = $write_zeroes_max_bytes =~ m/^(\d+)$/; #untaint > + > + #if the storage support write_zeroes but stepsize is too big, reduce the stepsize to the max possible > + if ($write_zeroes_max_bytes > 0 && $stepsize > $write_zeroes_max_bytes) { > + warn > + "reduce stepsize to the maximum supported by the storage : $write_zeroes_max_bytes bytes\n"; Style nit: line too long > + $stepsize = $write_zeroes_max_bytes; > + } > + > + my $discard_zeroes_data = > + file_read_firstline("$sysdir/queue/discard_zeroes_data") // 0; > + if ($scfg->{saferemove_discard} && $discard_zeroes_data == 0) { > + warn "disabling saferemove_discard as the storage don't support it\n"; > + $scfg->{saferemove_discard} = undef; This should be done with a local variable instead of changing the $scfg hash. > + } > + } > > my $cmd = [ > - '/usr/bin/cstream', > - '-i', > - '/dev/zero', > - '-o', > - "/dev/$vg/del-$name", > - '-T', > - '10', > - '-v', > - '1', > - '-b', > - '1048576', > - '-t', > - "$throughput", > + '/usr/sbin/blkdiscard', $lvmpath, '-v', '--step', "${stepsize}", > ]; Style nit: could now be one line > - eval { > - run_command( > - $cmd, > - errmsg => "zero out finished (note: 'No space left on device' is ok here)", > - ); > - }; > + push @$cmd, '--zeroout' if !$scfg->{saferemove_discard}; > + > + eval { run_command($cmd); }; > warn $@ if $@; > > $class->cluster_lock_storage( > @@ -376,6 +387,15 @@ sub properties { > description => "Zero-out data when removing LVs.", > type => 'boolean', > }, > + saferemove_discard => { > + description => "Wipe with discard instead zeroing.", Should be: "instead of zeroing" > + type => 'boolean', > + default => 0, > + }, > + saferemove_stepsize => { > + description => "Wipe step size (default 32MB).", Could mention that it will be capped to what the storage supports > + enum => [qw(1 2 4 8 16 32)], Missing type > + }, > saferemove_throughput => { > description => "Wipe throughput (cstream -t parameter value).", > type => 'string', Please adapt the description to mention that it is deprecated and a reminder comment in the code to remove it with PVE 10. > @@ -394,6 +414,8 @@ sub options { > shared => { optional => 1 }, > disable => { optional => 1 }, > saferemove => { optional => 1 }, > + saferemove_discard => { optional => 1 }, > + saferemove_stepsize => { optional => 1 }, For new options, we try to use kebab-case rather than snake_case > saferemove_throughput => { optional => 1 }, > content => { optional => 1 }, > base => { fixed => 1, optional => 1 }, I wonder if we should turn saferemove into a property string with enabled=BOOLEAN,mode=(zeroout|discard),stepsize=(1|2|4|8|16|32) and enabled being the default key for backwards compat. But I'm also fine with having the two properties. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel