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 60D6D1FF0E5 for ; Wed, 15 Jul 2026 12:26:25 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id DCFEC21446; Wed, 15 Jul 2026 12:26:24 +0200 (CEST) Message-ID: <57cf0d8b-4fa7-4e00-b106-e8c997f102dc@proxmox.com> Date: Wed, 15 Jul 2026 12:26:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH storage v9 3/5] fix #7339: lvm: add discard action for removed volumes To: Lukas Sichert , pve-devel@lists.proxmox.com References: <20260713160008.121125-1-l.sichert@proxmox.com> <20260713160008.121125-4-l.sichert@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260713160008.121125-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: 1784111160419 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.253 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: ZUHCHHSXNKYH2Q62ODZ7M2L3EVIB6O3L X-Message-ID-Hash: ZUHCHHSXNKYH2Q62ODZ7M2L3EVIB6O3L 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 13.07.26 um 6:00 PM schrieb Lukas Sichert: > On LVM storages backed by thin-provisioned SAN LUNs, removing an LV does > not release the allocated space on the backing storage. Using LVM's > `issue_discards` can avoid that, but makes `lvremove` issue the discards > while holding the cluster-wide storage lock, which can hit the lock > timeout for large volumes. > > Add an `on-volume-remove` property with an initial `discard` action. > Reject the option if discard is not supported by the backing devices. > The cleanup worker issues the discard for the renamed LV before the > final remove, so the long-running operation happens outside the storage > lock. When combined with `saferemove`, the worker zeroes and discards > the LV range by range, avoiding allocation of the whole LV with zeroes > on thin-provisioned backing storage. > > Signed-off-by: Lukas Sichert > Link: bugzilla.proxmox.com/show_bug.cgi?id=7339 Maybe include https:// ? I'd also prefer Buglink rather than Link, but no big deal. Reviewed-by: Fiona Ebner With one more comment and one more question below: > @@ -411,8 +421,8 @@ my sub free_lvm_volumes_locked { > > if ($zeroout_variant eq 'blkzeroout') { > eval { blockdev_ioctl_range($fh, BLKZEROOUT, $offset, $stepsize); }; > - if ($@) { > - die "blkzeroout for $stepsize bytes at offset $offset failed: $@"; > + if (my $err = $@) { > + die "blkzeroout for $stepsize bytes at offset $offset failed: $err"; > } > $written = $stepsize; > } elsif ($zeroout_variant eq 'syswrite') { Should be squashed into the previous patch. > +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_+./=-]+)$!) { The information comes from LVM and is only used as the argument for lsblk, so should we untaint with .* ? Or is there a specific reason for this exact regex? > + $dev_path = $1; # untaint