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

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

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.

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.

Thanks,
Qu

>
>
>
>>   static struct attribute *btrfs_supported_static_feature_attrs[] = {
>>       BTRFS_ATTR_PTR(static_feature, rmdir_subvol),
>>       BTRFS_ATTR_PTR(static_feature, supported_checksums),
>>       BTRFS_ATTR_PTR(static_feature, send_stream_version),
>>       BTRFS_ATTR_PTR(static_feature, supported_rescue_options),
>> +    BTRFS_ATTR_PTR(static_feature, supported_ro_sectorsize),
>> +    BTRFS_ATTR_PTR(static_feature, supported_rw_sectorsize),
>>       NULL
>>   };
>>
>