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 5644D1FF141 for ; Tue, 30 Jun 2026 12:56:06 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id E908A21412; Tue, 30 Jun 2026 12:56:02 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 30 Jun 2026 12:55:57 +0200 Message-Id: To: "Lukas Sichert" , Subject: Re: [PATCH storage v7 2/4] fix #7339: lvm: add discard action for removed volumes From: =?utf-8?q?Michael_K=C3=B6ppl?= X-Mailer: aerc 0.21.0 References: <20260616101323.24981-1-l.sichert@proxmox.com> <20260616101323.24981-3-l.sichert@proxmox.com> In-Reply-To: <20260616101323.24981-3-l.sichert@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782816945794 X-SPAM-LEVEL: Spam detection results: 0 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: GOOEH7SI4AGQJZQ4U3J4ZGYUJ6BKN4XB X-Message-ID-Hash: GOOEH7SI4AGQJZQ4U3J4ZGYUJ6BKN4XB X-MailFrom: m.koeppl@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: On Tue Jun 16, 2026 at 12:13 PM CEST, Lukas Sichert wrote: [snip] > + # also don't remove the volume if blkdiscard fails, because = otherwise the > + # storage on a thinly-backed storage can't be reclaimed with= out removing the > + # complete pool > + if ($on_remove_opts->{discard} && !$scfg->{saferemove}) { > + eval { run_command(['/sbin/blkdiscard', $lvmpath]); }; > + $err =3D $@ if $@; I'm not quite sure regarding this one. I understand why it's there, but I think the case where discard is simply not supported should maybe be covered separately. Consider the following scenario: - Block device that does not support discard (/sys/block//queue/discard_max_bytes is 0) - Create LVM storage on it (discard on, saferemove off) - pvcreate /dev/ - vgcreate vgtest - pvesm add lvm test --vgname vgtest --content images - pvesm set test --on-volume-remove discard=3D1 - Allocate a disk image (pvesm alloc test 9999 vm-disk-0 1G) If you now call `pvesm free test:vm-disk-0`, the task will show an "Operation not supported" warning for blkdiscard. dev-vm-disk-0 remains, the space is not cleared, which is expected. But if I now try to `alloc` again and `pvesm free` again, this will always fail due to a naming collision with the leftover `del-vm-disk-0`. One could argue that enabling discard for a device that does not support it is a user error, but on the other hand the "fallout" from this mistake is IMO at least annoying and it's relatively easy to prevent by e.g. explicitly checking /sys/block//queue/discard_max_bytes and warning the user that the operation is not supported, but carrying on with the regular removal of the volume? Because if the block device does not support discard, there's nothing to be done anyway to reclaim the space later, no? If the device does not support discard today, it won't support it tomorrow. Although the best UX would probably be to perform this check when the storage is created to prevent the discard option from being set if it's not supported? > + } > =20 > if (!$err) { > $class->cluster_lock_storage( > @@ -449,13 +486,13 @@ my sub free_lvm_volumes_locked { > } > }; [snip]