Re: [PATCH] btrfs: drop unused memparse() parameter

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/12/7 22:45, David Sterba wrote:
> On Thu, Dec 07, 2023 at 01:01:50PM +1030, Qu Wenruo wrote:
>>
>>
>> On 2023/12/5 21:43, David Disseldorp wrote:
>>> The @retptr parameter for memparse() is optional.
>>> btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
>>> validation, so the parameter can be dropped.
>>
>> To me, I believe it's better to completely get rid of memparse().
>>
>> As you already found out, some suffix, especially "e|E" can screw up the
>> result.
>> E.g. "25e" would be interpreted as 25 with "e" as suffix, which is fine
>> according to the rule. (without prefix, the base is 10, so only "25" is
>> valid. Then the remaining part is interpreted as suffix).
>>
>> And since btrfs is not going to do pretty size output for sysfs (as most
>> sysfs is not directly for end users, and we need accurate output), to be
>> consistent there isn't much need for suffix handling either.
>>
>> So can't we just replace memparse() with kstrtoull()?
>
> The value that can be read from the sysfs file is in bytes and it's so
> that applications do not need to interpret it, like multiplying with
> 1024. We'll probably never return the pretty values with suffixes in
> sysfs files.
>
> However, on the input side the suffixes are a convenience, setting to
> limit the throughput as '32m' is better than typing '32000000' and
> counting zeros or $((32*1024*1024)) or 33554432.
>
> This is why memparse is there and kstrtoull does not do that.

That suffix is causing confusion already, just check my "25e" case.
(It does not only lead to huge number, but also lead to incorrect value
even if we treat "e" as a suffix)

Furthermore, the convenience argument is not that strong, you won't
expect end users to do the change for a fs every time.
Thus it's mostly managed by a small script or some other tool.

In that case I don't think doing extra bash calculation is a big deal
anyway.

Thanks,
Qu