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

[Anand Jain <anand.jain@oracle.com>]

On 16/03/2021 08:05, Qu Wenruo wrote:
> 
> 
> 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.
> 
  Makes sense to me.

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

  supported_sectorsizes (plural) is better IMO.

Thanks, Anand

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