[PATCH] btrfs-progs: fix discard support check

[PATCH] btrfs-progs: fix discard support check

[Wang Yugui]

[BUG]
mkfs.btrfs(v5.15) output a message even if the disk is a HDD without
TRIM/DISCARD support.
  Performing full device TRIM /dev/sdc2 (326.03GiB) ...

[CAUSE]
mkfs.btrfs check TRIM/DISCARD support through the content of
queue/discard_granularity, but compare it against a wrong value.

When hdd without TRIM/DISCARD support, the content of
queue/discard_granularity is '0' '\n' '\0', rather than '0' '\0'.

[FIX]
compare it against the right value.

Fixes: c50c448518bb ("btrfs-progs: do sysfs detection of device discard capability")
Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
---
 common/device-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/device-utils.c b/common/device-utils.c
index 74a25879..76d5c584 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -64,7 +64,7 @@ static int discard_supported(const char *device)
 		pr_verbose(3, "cannot read discard_granularity for %s\n", device);
 		return 0;
 	} else {
-		if (buf[0] == '0' && buf[1] == 0) {
+		if (buf[0] == '0' && buf[1] == '\n') {
 			pr_verbose(3, "%s: discard_granularity %s\n", device, buf);
 			return 0;
 		}
-- 
2.32.0

Re: [PATCH] btrfs-progs: fix discard support check

[Qu Wenruo]

On 2021/11/13 11:14, Wang Yugui wrote:
> [BUG]
> mkfs.btrfs(v5.15) output a message even if the disk is a HDD without
> TRIM/DISCARD support.
>    Performing full device TRIM /dev/sdc2 (326.03GiB) ...
>
> [CAUSE]
> mkfs.btrfs check TRIM/DISCARD support through the content of
> queue/discard_granularity, but compare it against a wrong value.
>
> When hdd without TRIM/DISCARD support, the content of
> queue/discard_granularity is '0' '\n' '\0', rather than '0' '\0'.

Can we get rid of such bad comparison and just go strtoll() and compare
the value?

Thanks,
Qu

>
> [FIX]
> compare it against the right value.
>
> Fixes: c50c448518bb ("btrfs-progs: do sysfs detection of device discard capability")
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
> ---
>   common/device-utils.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/device-utils.c b/common/device-utils.c
> index 74a25879..76d5c584 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -64,7 +64,7 @@ static int discard_supported(const char *device)
>   		pr_verbose(3, "cannot read discard_granularity for %s\n", device);
>   		return 0;
>   	} else {
> -		if (buf[0] == '0' && buf[1] == 0) {
> +		if (buf[0] == '0' && buf[1] == '\n') {
>   			pr_verbose(3, "%s: discard_granularity %s\n", device, buf);
>   			return 0;
>   		}
>

Re: [PATCH] btrfs-progs: fix discard support check

[Wang Yugui]

Hi,

> On 2021/11/13 11:14, Wang Yugui wrote:
> > [BUG]
> > mkfs.btrfs(v5.15) output a message even if the disk is a HDD without
> > TRIM/DISCARD support.
> >    Performing full device TRIM /dev/sdc2 (326.03GiB) ...
> >
> > [CAUSE]
> > mkfs.btrfs check TRIM/DISCARD support through the content of
> > queue/discard_granularity, but compare it against a wrong value.
> >
> > When hdd without TRIM/DISCARD support, the content of
> > queue/discard_granularity is '0' '\n' '\0', rather than '0' '\0'.
> 
> Can we get rid of such bad comparison and just go strtoll() and compare
> the value?

strtoll() or strcmp() is a good choice for refact.  but now just a
direct fix.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/11/13


> 
> >
> > [FIX]
> > compare it against the right value.
> >
> > Fixes: c50c448518bb ("btrfs-progs: do sysfs detection of device discard capability")
> > Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
> > ---
> >   common/device-utils.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/common/device-utils.c b/common/device-utils.c
> > index 74a25879..76d5c584 100644
> > --- a/common/device-utils.c
> > +++ b/common/device-utils.c
> > @@ -64,7 +64,7 @@ static int discard_supported(const char *device)
> >   		pr_verbose(3, "cannot read discard_granularity for %s\n", device);
> >   		return 0;
> >   	} else {
> > -		if (buf[0] == '0' && buf[1] == 0) {
> > +		if (buf[0] == '0' && buf[1] == '\n') {
> >   			pr_verbose(3, "%s: discard_granularity %s\n", device, buf);
> >   			return 0;
> >   		}
> >

Re: [PATCH] btrfs-progs: fix discard support check

[Nikolay Borisov]

On 13.11.21 г. 9:04, Wang Yugui wrote:
> Hi,
> 
>> On 2021/11/13 11:14, Wang Yugui wrote:
>>> [BUG]
>>> mkfs.btrfs(v5.15) output a message even if the disk is a HDD without
>>> TRIM/DISCARD support.
>>>    Performing full device TRIM /dev/sdc2 (326.03GiB) ...
>>>
>>> [CAUSE]
>>> mkfs.btrfs check TRIM/DISCARD support through the content of
>>> queue/discard_granularity, but compare it against a wrong value.
>>>
>>> When hdd without TRIM/DISCARD support, the content of
>>> queue/discard_granularity is '0' '\n' '\0', rather than '0' '\0'.
>>
>> Can we get rid of such bad comparison and just go strtoll() and compare
>> the value?
> 
> strtoll() or strcmp() is a good choice for refact.  but now just a
> direct fix.

NACK,

Use one of the conversion functions to convert the string to a number
and do a != 0 comparison. atoi() would be sufficient and deals with
whitespace/trailing \n character gracefully. I think it's high time we
ditch the "let's do a quick and dirty fix" mentality which leads to
unmaintainable gunk in the long-term as it tends to rarely get fixed
afterwards.

<snip>