Re: [PATCH v2] btrfs-progs: check discard_max_bytes in discard_supported()

[Boris Burkov <boris@bur.io>]

On Wed, Aug 06, 2025 at 01:14:19AM +0800, Anand Jain wrote:
> Some devices may advertise discard support but have discard_max_bytes=0,
> effectively disabling it. Add a check to read discard_max_bytes and
> treat zero as no discard support.
> 
> Example:
> $ cat /sys/block/sda/queue/discard_granularity
> 512
> 
> $ ./mkfs.btrfs -vvv -f /dev/sda
> ...
> Performing full device TRIM /dev/sda (3.00GiB) ...
> discard_range ret -1 errno Operation not supported
> ...
> 
> Fix is to also check discard_max_bytes for a non-zero value.
> 
> $ cat /sys/block/sda/queue/discard_max_bytes
> 0
> 
> Helps avoid false positives in discard capability detection.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v1: https://lore.kernel.org/linux-btrfs/2f9687740a9f9d60bdea8d24f215c6c0e2a9657b.1753713395.git.anand.jain@oracle.com/
> 
> v2: Checks for discard_max_bytes().
> 
>  common/device-utils.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index 783d79555446..d110292fe718 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -76,6 +76,17 @@ static int discard_supported(const char *device)
>  		}
>  	}
>  
> +	ret = device_get_queue_param(device, "discard_max_bytes", buf, sizeof(buf));
> +	if (ret == 0) {

Looks good overall, one small thing I noticed:

I was a little surprised by this check so I read
device_get_queue_param() and saw that it does return 0 on every error
condition, except for the final read() call. So if that read() fails, it
will return -1 and then this logic won't work. That applies equally to
the existing code for granularity, so it's not a new bug in your patch.

Unless I'm missing something in that analysis, I would either:
make both of these checks for <= 0
or
fix the return at read() in device_get_queue_param()

Thanks,
Boris

> +		pr_verbose(3, "cannot read discard_max_bytes for %s\n", device);
> +		return 0;
> +	} else {
> +		if (atoi(buf) == 0) {
> +			pr_verbose(3, "%s: discard_max_bytes %s", device, buf);
> +			return 0;
> +		}
> +	}
> +
>  	return 1;
>  }
>  
> -- 
> 2.50.1
>