Re: [PATCH storage v7 2/4] fix #7339: lvm: add discard action for removed volumes

["Michael Köppl" <m.koeppl@proxmox.com>]

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 without removing the
> +            # complete pool
> +            if ($on_remove_opts->{discard} && !$scfg->{saferemove}) {
> +                eval { run_command(['/sbin/blkdiscard', $lvmpath]); };
> +                $err = $@ 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/<device>/queue/discard_max_bytes is 0)
- Create LVM storage on it (discard on, saferemove off)
    - pvcreate /dev/<device>
    - vgcreate vgtest <device>
    - pvesm add lvm test --vgname vgtest --content images
    - pvesm set test --on-volume-remove discard=1
- 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/<dev>/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?

> +            }
>  
>              if (!$err) {
>                  $class->cluster_lock_storage(
> @@ -449,13 +486,13 @@ my sub free_lvm_volumes_locked {
>          }
>      };

[snip]