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 E42AD1FF0E7 for ; Fri, 10 Jul 2026 13:50:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1AF622142F; Fri, 10 Jul 2026 13:50:15 +0200 (CEST) Message-ID: Date: Fri, 10 Jul 2026 13:49:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes To: Lukas Sichert , pve-devel@lists.proxmox.com References: <20260707143252.101757-1-l.sichert@proxmox.com> <20260707143252.101757-4-l.sichert@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260707143252.101757-4-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: 1783684168087 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.025 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: JOSEN6XHLHPJHKOOGDGUOEJEF6RRDB7A X-Message-ID-Hash: JOSEN6XHLHPJHKOOGDGUOEJEF6RRDB7A 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: > diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm > index a3adeef..8653331 100644 > --- a/src/PVE/Storage/LVMPlugin.pm > +++ b/src/PVE/Storage/LVMPlugin.pm > @@ -12,8 +12,10 @@ use Fcntl qw(SEEK_SET); > > use PVE::JSONSchema qw(get_standard_option); > use PVE::Tools qw(run_command file_read_firstline trim); > +use PVE::RESTEnvironment qw(log_warn); > use PVE::SafeSyslog; > use PVE::Format qw(render_bytes render_duration); > +use PVE::Exception qw(raise_param_exc); Nit: not ordered alphabetically > > use PVE::Storage::Common; > use PVE::Storage::Plugin; > @@ -23,6 +25,7 @@ use base qw(PVE::Storage::Plugin); > # lvm helper functions > > use constant { > + BLKDISCARD => 0x1277, > BLKZEROOUT => 0x127f, > }; > > @@ -299,6 +302,12 @@ my sub free_lvm_volumes_locked { > > my $vg = $scfg->{vgname}; > > + my $on_remove_opts = {}; > + if ($scfg->{'on-volume-remove'}) { > + $on_remove_opts = > + PVE::JSONSchema::parse_property_string('on-volume-remove', $scfg->{'on-volume-remove'}); > + } > + > my $secure_delete_cmd = sub { > my ($lvmpath) = @_; > > @@ -362,6 +371,8 @@ my sub free_lvm_volumes_locked { > my $written_total = 0; > my $lastprint = -1; > my $written; > + my $discard_attempts = 0; > + my $discard_failures = 0; > > for (my $offset = 0; $offset < $size; $offset += $written) { > > @@ -389,6 +400,18 @@ my sub free_lvm_volumes_locked { > } > > } > + if ($on_remove_opts->{discard}) { > + $discard_attempts++; > + > + eval { blockdev_ioctl_range($fh, BLKDISCARD, $offset, $written); }; > + if ($@) { Style nit: usually, we assign immediately to a local variable in such cases, i.e. if (my $err = $@) { > + if ($discard_failures == 0) { > + log_warn( > + "blkdiscard for $written bytes at offset $offset failed: $@"); > + } > + $discard_failures += 1; > + } > + } > $written_total += $written; > > my $curr_time = time(); > @@ -416,7 +439,9 @@ my sub free_lvm_volumes_locked { > } > } > } > - > + if ($discard_failures != 0) { > + die "$discard_failures out of $discard_attempts blkdiscards failed\n"; > + } > }; > # close filehandle before throwing an error > my $err = $@; > @@ -426,24 +451,44 @@ my sub free_lvm_volumes_locked { > } > }; > # we need to zero out LVM data for security reasons > - # and to allow thin provisioning > - my $zero_out_worker = sub { > + # and discard images to free storage space to allow > + # thin provisioning > + my $cleanup_worker = sub { > + > for my $name (@$volnames) { > my $lvmpath = "/dev/$vg/del-$name"; > - print "zero-out data on image $name ($lvmpath)\n"; > + > + my $discard_action; > + if ($scfg->{saferemove} && $on_remove_opts->{discard}) { > + $discard_action = 'zero-out and discard (TRIM)'; > + } elsif ($scfg->{saferemove}) { > + $discard_action = 'zero-out'; > + } elsif ($on_remove_opts->{discard}) { > + $discard_action = 'discard (TRIM)'; > + } > + print "$discard_action data on image $name ($lvmpath)\n"; > > my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath]; > run_command( > $cmd_activate, > - errmsg => "can't activate LV '$lvmpath' to zero-out its data", > + errmsg => "can't activate LV '$lvmpath' to $discard_action its data", > ); > $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath]; > run_command( > $cmd_activate, > - errmsg => "can't refresh LV '$lvmpath' to zero-out its data", > + errmsg => "can't refresh LV '$lvmpath' to $discard_action its data", > ); > + syslog('info', "starting to $discard_action $name ($lvmpath)"); > > - eval { $secure_delete_cmd->($lvmpath); }; > + eval { > + if ($scfg->{saferemove}) { > + $secure_delete_cmd->($lvmpath); > + } > + > + if ($on_remove_opts->{discard} && !$scfg->{saferemove}) { Style nit: I would put it into an elsif > + run_command(['/sbin/blkdiscard', $lvmpath]); Nit: should we put an errmsg here? ---snip 8<--- > @@ -583,6 +666,88 @@ sub get_formats { > return { default => 'raw', valid => { 'raw' => 1 } }; > } > > +my sub get_discard_max { > + my ($dev_path) = @_; > + > + my $output = ''; > + # use lsblk as it resolves discard support in setups with > + # nested partitions or lv/vgs Style nit: comment fits in one line > + my $cmd = [ > + '/bin/lsblk', Nit: use just 'lsblk' > + '--json', > + '--bytes', > + '--discard', > + '--nodeps', > + '--output', > + 'PATH,DISC-MAX', Nit: The path is not used later > + $dev_path, > + ]; > + > + eval { > + run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; }); > + }; > + if ($@) { > + raise_param_exc({ > + 'on-volume-remove' => "discard on remove is enabled, but lsblk could not " > + . "query discard support for the backing device '$dev_path'", Please include the error $@ for completeness > + }); > + } > + > + my $parsed = eval { decode_json($output) }; > + if ($@ || !$parsed->{blockdevices} || !$parsed->{blockdevices}->[0]) { > + raise_param_exc({ > + 'on-volume-remove' => "discard on remove is enabled, but lsblk could not " > + . "parse discard support for the backing device '$dev_path'", > + }); > + > + } > + > + return $parsed->{blockdevices}->[0]->{'disc-max'} // 0; Nit: We could warn if there is more than one result or are there cases where that is expected and non-problematic? > +} > + > +sub assert_discard_supported { > + my ($vgname, $on_remove) = @_; > + > + return if !defined($on_remove); > + > + my $on_remove_opts = PVE::JSONSchema::parse_property_string('on-volume-remove', $on_remove); > + if ($on_remove_opts->{discard}) { > + > + my $vgs = lvm_vgs(1); > + my $vg = $vgs->{$vgname}; > + die "no such volume group '$vgname'\n" if !$vg; > + > + my $pvs = $vg->{pvs}; > + die "volume group '$vgname' has no physical volumes\n" > + if !defined($pvs) || scalar($pvs->@*) == 0; > + > + # check if all the block devices configured to the volume group support discard > + for my $pv ($vg->{pvs}->@*) { > + > + my $dev_path = abs_path($pv->{name}) // $pv->{name}; > + > + if ($dev_path !~ m!^(/dev/[A-Za-z0-9_+./=-]+)$!) { > + raise_param_exc({ > + 'on-volume-remove' => "discard on remove is enabled, but discard support " > + . "cannot be resolved for the backing device '$dev_path'", > + }); > + } else { > + $dev_path = $1; # untaint Style nit: I'd prefer using =~ and switching the branches so the $1 is closer to where it's obtained > + } > + > + my $discard_max_bytes = get_discard_max($dev_path); > + > + # use discard_max_bytes as indicator if discard is supported > + if (!$discard_max_bytes) { > + raise_param_exc({ > + 'on-volume-remove' => "discard on remove is enabled, but discard is not " > + . "supported by the backing device '$dev_path'", > + }); > + } > + } > + } > +} > + > sub on_add_hook { > my ($class, $storeid, $scfg, %param) = @_; > > @@ -604,6 +769,8 @@ sub on_add_hook { > lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared}); > } > > + assert_discard_supported($scfg->{vgname}, $scfg->{'on-volume-remove'}); > + > return; > } > > @@ -624,6 +791,7 @@ sub on_update_hook_full { > die "$storeid - cannot disable 'snapshot-as-volume-chain' while a qcow2 image exists\n" > if grep { $_->{format} eq 'qcow2' } $images->@*; > } > + assert_discard_supported($scfg->{vgname}, $update->{'on-volume-remove'}); > } > > sub parse_volname {