Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize

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

On 2021/3/16 上午2:44, David Sterba wrote:
> On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/3/15 下午7:59, Anand Jain wrote:
>>> On 10/03/2021 17:08, Qu Wenruo wrote:
>>>> Add extra sysfs interface features/supported_ro_sectorsize and
>>>> features/supported_rw_sectorsize to indicate subpage support.
>>>>
>>>> Currently for supported_rw_sectorsize all architectures only have their
>>>> PAGE_SIZE listed.
>>>>
>>>> While for supported_ro_sectorsize, for systems with 64K page size, 4K
>>>> sectorsize is also supported.
>>>>
>>>> This new sysfs interface would help mkfs.btrfs to do more accurate
>>>> warning.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>
>>> Changes looks good. Nit below...
>>> And maybe it is a good idea to wait for other comments before reroll.
>>>
>>>>    fs/btrfs/sysfs.c | 34 ++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>>>> index 6eb1c50fa98c..3ef419899472 100644
>>>> --- a/fs/btrfs/sysfs.c
>>>> +++ b/fs/btrfs/sysfs.c
>>>> @@ -360,11 +360,45 @@ static ssize_t
>>>> supported_rescue_options_show(struct kobject *kobj,
>>>>    BTRFS_ATTR(static_feature, supported_rescue_options,
>>>>           supported_rescue_options_show);
>>>> +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj,
>>>> +                        struct kobj_attribute *a,
>>>> +                        char *buf)
>>>> +{
>>>> +    ssize_t ret = 0;
>>>> +    int i = 0;
>>>
>>>    Drop variable i, as ret can be used instead of 'i'.
>>>
>>>> +
>>>> +    /* For 64K page size, 4K sector size is supported */
>>>> +    if (PAGE_SIZE == SZ_64K) {
>>>> +        ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K);
>>>> +        i++;
>>>> +    }
>>>> +    /* Other than above subpage, only support PAGE_SIZE as sectorsize
>>>> yet */
>>>> +    ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n",
>>>
>>>> +             (i ? " " : ""), PAGE_SIZE);
>>>                             ^ret
>>>
>>>> +    return ret;
>>>> +}
>>>> +BTRFS_ATTR(static_feature, supported_ro_sectorsize,
>>>> +       supported_ro_sectorsize_show);
>>>> +
>>>> +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj,
>>>> +                        struct kobj_attribute *a,
>>>> +                        char *buf)
>>>> +{
>>>> +    ssize_t ret = 0;
>>>> +
>>>> +    /* Only PAGE_SIZE as sectorsize is supported */
>>>> +    ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE);
>>>> +    return ret;
>>>> +}
>>>> +BTRFS_ATTR(static_feature, supported_rw_sectorsize,
>>>> +       supported_rw_sectorsize_show);
>>>
>>>    Why not merge supported_ro_sectorsize and supported_rw_sectorsize
>>>    and show both in two lines...
>>>    For example:
>>>      cat supported_sectorsizes
>>>      ro: 4096 65536
>>>      rw: 65536
>>
>> If merged, btrfs-progs needs to do line number check before doing string
>> matching.
>
> The sysfs files should do one value per file.
>
>> Although I doubt the usefulness for supported_ro_sectorsize, as the
>> window for RO support without RW support should not be that large.
>> (Current RW passes most generic test cases, and the remaining failures
>> are very limited)
>>
>> Thus I can merged them into supported_sectorsize, and only report
>> sectorsize we can do RW as supported.
>
> In that case one file with the list of supported values is a better
> option. The main point is to have full RW support, until then it's
> interesting only for developers and they know what to expect.
>

Indeed only full RW support makes sense.

BTW, any comment on the file name? If no problem I would just use
"supported_sectorsize" in next update.

Although I hope the sysfs interface can be merged separately early, so
that I can add the proper support in btrfs-progs.

Thanks,
Qu