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

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

[Anand Jain]

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) {
+		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

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

[Boris Burkov]

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
> 

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

[Anand Jain]

On 6/8/25 02:04, Boris Burkov wrote:
> 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()
> 

I hadn’t looked into device_get_queue_param() earlier — now I have.

One way we could get a return value < 0 from read(2) is if len or count
is zero, theoretically. But in practice, we aren't passing zero, so that
part is fine.

Next, in the queue sysfs show() functions, the values are read directly
from the settings like this:

--------
/**
  * blk_set_stacking_limits - set default limits for stacking devices
  * @lim:  the queue_limits structure to reset
  *
  * Prepare queue limits for applying limits from underlying devices using
  * blk_stack_limits().
  */
void blk_set_stacking_limits(struct queue_limits *lim)
{
::
         lim->discard_granularity = SECTOR_SIZE;
::
         lim->max_hw_zone_append_sectors = UINT_MAX;
         lim->max_user_discard_sectors = UINT_MAX;
::
--------


         lim->max_discard_sectors =
                 min(lim->max_hw_discard_sectors, 
lim->max_user_discard_sectors);

---------


/*
  * Set the default limits for a newly allocated queue.  @lim contains the
  * initial limits set by the driver, which could be no limit in which case
  * all fields are cleared to zero.
  */
int blk_set_default_limits(struct queue_limits *lim)
{
         /*
          * Most defaults are set by capping the bounds in 
blk_validate_limits,
          * but max_user_discard_sectors is special and needs an explicit
          * initialization to the max value here.
          */
         lim->max_user_discard_sectors = UINT_MAX;
         return blk_validate_limits(lim);
}

----------------------

For discard_granularity and max_discard_sectors, the limit is set to > 0
by default, so that part looks fine for now.

We still need to check other parameters in device_get_queue_param().
IMO we can handle that in a separate patch.?

Thanks, Anand


> 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
>>

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

[Qu Wenruo]

在 2025/8/6 02:44, Anand Jain 写道:
> 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

Where does the error come from?

In device_discard_blocks() it just calls discard_range() in steps, nor 
discard_range() itself would output error message.

> ...
> 
> 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.

Since there is no error message and the error code is either ignored (in 
btrfs_prepare_device()) or properly handled (in btrfs_reset_zones).

So I didn't see how the false positives are even possible.

Thanks,
Qu

> 
> 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) {
> +		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;
>   }
>   

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

[Anand Jain]

On 6/8/25 13:06, Qu Wenruo wrote:
> 
> 
> 在 2025/8/6 02:44, Anand Jain 写道:
>> 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
> 
> Where does the error come from?
> 
> In device_discard_blocks() it just calls discard_range() in steps, nor 
> discard_range() itself would output error message.
> 

Its from the my own added debug message at

         if (ioctl(fd, BLKDISCARD, &range) < 0)

>> ...
>>
>> 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.
> 
> Since there is no error message and the error code is either ignored (in 
> btrfs_prepare_device()) or properly handled (in btrfs_reset_zones).
> 
> So I didn't see how the false positives are even possible.
> 

discard_granularity suggests discard is supported, but it's actually not
discard_max_bytes is zero. So the discard_granularity check gives a
false positive.
----
$ cat /sys/block/sda/queue/discard_granularity
512

$ cat /sys/block/sda/queue/discard_max_bytes
0
----

If the line is confusing, I’ll remove it.

Thanks, Anand

> Thanks,
> Qu
> 
>>
>> 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) {
>> +        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;
>>   }
> 

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

[Qu Wenruo]

在 2025/8/6 15:31, Anand Jain 写道:
> 
> 
> On 6/8/25 13:06, Qu Wenruo wrote:
>>
>>
>> 在 2025/8/6 02:44, Anand Jain 写道:
>>> 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
>>
>> Where does the error come from?
>>
>> In device_discard_blocks() it just calls discard_range() in steps, nor 
>> discard_range() itself would output error message.
>>
> 
> Its from the my own added debug message at
> 
>          if (ioctl(fd, BLKDISCARD, &range) < 0)

So you're only fixing a message caused by a patch not in upstream progs?
> 
>>> ...
>>>
>>> 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.
>>
>> Since there is no error message and the error code is either ignored 
>> (in btrfs_prepare_device()) or properly handled (in btrfs_reset_zones).
>>
>> So I didn't see how the false positives are even possible.
>>
> 
> discard_granularity suggests discard is supported, but it's actually not
> discard_max_bytes is zero. So the discard_granularity check gives a
> false positive.
> ----
> $ cat /sys/block/sda/queue/discard_granularity
> 512
> 
> $ cat /sys/block/sda/queue/discard_max_bytes
> 0
> ----
> 
> If the line is confusing, I’ll remove it.

I understand the sysfs problem, but since there is no such error message 
in the first place, it won't cause any confusion, thus I see nothing to 
fix in progs.

Thanks,
Qu

> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
>>
>>>
>>> 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) {
>>> +        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;
>>>   }
>>
> 

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

[Anand Jain]

On 6/8/25 14:15, Qu Wenruo wrote:
> 
> 
> 在 2025/8/6 15:31, Anand Jain 写道:
>>
>>
>> On 6/8/25 13:06, Qu Wenruo wrote:
>>>
>>>
>>> 在 2025/8/6 02:44, Anand Jain 写道:
>>>> 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
>>>
>>> Where does the error come from?
>>>
>>> In device_discard_blocks() it just calls discard_range() in steps, 
>>> nor discard_range() itself would output error message.
>>>
>>
>> Its from the my own added debug message at
>>
>>          if (ioctl(fd, BLKDISCARD, &range) < 0)
> 
> So you're only fixing a message caused by a patch not in upstream progs?
>>
>>>> ...
>>>>
>>>> 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.
>>>
>>> Since there is no error message and the error code is either ignored 
>>> (in btrfs_prepare_device()) or properly handled (in btrfs_reset_zones).
>>>
>>> So I didn't see how the false positives are even possible.
>>>
>>
>> discard_granularity suggests discard is supported, but it's actually not
>> discard_max_bytes is zero. So the discard_granularity check gives a
>> false positive.
>> ----
>> $ cat /sys/block/sda/queue/discard_granularity
>> 512
>>
>> $ cat /sys/block/sda/queue/discard_max_bytes
>> 0
>> ----
>>
>> If the line is confusing, I’ll remove it.
> 
> I understand the sysfs problem, but since there is no such error message 
> in the first place, it won't cause any confusion, thus I see nothing to 
> fix in progs.
> 

Please!
there are two messages...

1.
Performing full device TRIM /dev/sda (3.00GiB) ...

2.
discard_range ret -1 errno Operation not supported

#1 is in upstream
and
#2 is my own debug.

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

[Qu Wenruo]

在 2025/8/6 15:59, Anand Jain 写道:
> 
> 
> On 6/8/25 14:15, Qu Wenruo wrote:
>>
>>
>> 在 2025/8/6 15:31, Anand Jain 写道:
>>>
>>>
>>> On 6/8/25 13:06, Qu Wenruo wrote:
>>>>
>>>>
>>>> 在 2025/8/6 02:44, Anand Jain 写道:
>>>>> 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
>>>>
>>>> Where does the error come from?
>>>>
>>>> In device_discard_blocks() it just calls discard_range() in steps, 
>>>> nor discard_range() itself would output error message.
>>>>
>>>
>>> Its from the my own added debug message at
>>>
>>>          if (ioctl(fd, BLKDISCARD, &range) < 0)
>>
>> So you're only fixing a message caused by a patch not in upstream progs?
>>>
>>>>> ...
>>>>>
>>>>> 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.
>>>>
>>>> Since there is no error message and the error code is either ignored 
>>>> (in btrfs_prepare_device()) or properly handled (in btrfs_reset_zones).
>>>>
>>>> So I didn't see how the false positives are even possible.
>>>>
>>>
>>> discard_granularity suggests discard is supported, but it's actually not
>>> discard_max_bytes is zero. So the discard_granularity check gives a
>>> false positive.
>>> ----
>>> $ cat /sys/block/sda/queue/discard_granularity
>>> 512
>>>
>>> $ cat /sys/block/sda/queue/discard_max_bytes
>>> 0
>>> ----
>>>
>>> If the line is confusing, I’ll remove it.
>>
>> I understand the sysfs problem, but since there is no such error 
>> message in the first place, it won't cause any confusion, thus I see 
>> nothing to fix in progs.
>>
> 
> Please!
> there are two messages...
> 
> 1.
> Performing full device TRIM /dev/sda (3.00GiB) ...
> 
> 2.
> discard_range ret -1 errno Operation not supported

You didn't get the point.

With or without this patch, there will be no difference to the end users.

And I didn't even mention that, since there is no error message related 
to discard, we do not even need the helper discard_supported() at all.


> 
> #1 is in upstream
> and
> #2 is my own debug.
> 
> 

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

[Anand Jain]

> With or without this patch, there will be no difference to the end users.

How?

> And I didn't even mention that, since there is no error message related 
> to discard, we do not even need the helper discard_supported() at all.

We currently check for discard support before issuing the ioctl().
So that we can info ..

   Performing full device TRIM %s...

And then issue the ioctl().

Are you suggesting we skip that check, issue the ioctl() directly?
and handle 'Operation not supported' if it fails?