Re: [PATCH] btrfs: sysfs: advertise zoned support among features

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

On 19/08/2021 19:16, Anand Jain wrote:
> 
> 
> On 19/08/2021 17:14, Nikolay Borisov wrote:
>>
>>
>> On 19.08.21 г. 12:08, Nikolay Borisov wrote:
>>>
>>>
>>> On 19.08.21 г. 2:48, David Sterba wrote:
>>>> On Tue, Aug 10, 2021 at 08:30:51AM +0800, Anand Jain wrote:
>>>>>    As in the proposed patch which adds a table to figure out the 
>>>>> correct
>>>>> table to add the attribute, adding to the
>>>>> btrfs_supported_static_feature_attrs attribute list will add only to
>>>>> /sys/fs/btrfs/features, however adding the attribute to
>>>>> btrfs_supported_feature_attrs adds to both /sys/fs/btrfs/features and
>>>>> /sys/fs/btrfs/uuid/features.
>>>>
>>>> I can't see it in the code right now, but that would mean that eg. 
>>>> zoned
>>>> would show up in static features once such filesystem is mounted. And
>>>> that does not happen or I'm missing something.
>>>>
>>>
>>> This happens because when initialising sysfs we create the
>>> btrfs_feature_attr_group which contains btrfs_supported_feature_attrs
>>> attributes, which have
>>> #ifdef CONFIG_BLK_DEV_ZONED
>>>
>>>           BTRFS_FEAT_ATTR_PTR(zoned),
>>>
>>> #endif
>>>
>>> Subsequently you define the static feature via
>>> BTRFS_ATTR(static_feature, zoned, zoned_show);
>>>
>>> and finally we call:
>>>
>>> ret = sysfs_merge_group(&btrfs_kset->kobj,
>>> &btrfs_static_feature_attr_group);
>>>
>>>
>>> Which merges the static feature to the feature group. That's why the
>>> message about duplication. So one of the 2 definitions needs to go.
>>>
>>
>> Looking at the maze that btrfs' sysfs code is it seems the decision
> 
> 
>> which of the two sets to use when defining a feature is whether it can
> 
> ...be enabled per fsid.
> ::
> 
>> I think for zoned block device we can't
>> really disable the support at runtime?
> 
>   Hmm. It is not like that. Suppose there are two btrfs filesystems on a
>   system fsid1 and fsid2.  fsid1 is zoned. fsid2 is a normal conventional
>   device. Then on fsid1 zoned incompatible feature/flag is enabled and,
>   on fsid2 it is not enabled.
> 
>   So IMO zoned should be added to btrfs_supported_feature_attrs.

   Provided /sys/fs/btrfs/feature/zoned will return nothing in specific 
values like sectorsize.

> 
>   By doing this,
>   on /sys/fs/btrfs/feature zoned is shown
>   on /sys/fs/btrfs/fsid1/feature zoned is shown
>   on /sys/fs/btrfs/faid2/feature zoned is NOT shown
> 
>   btrfs_feature_visible() and get_features() manages to do that
>   dynamically.
> 
> 
>> If so then it needs to only be
>> defined as a static feature?
> 
>   No. pls.
> 
>> Just because the kernel supports it doesn't
>> necessarily mean it's going to be used if the underlying device is not
>> zoned, right, it should depend on the runtime characteristics of the
>> underlying device?
> 
>   Right. As in the above example. Imagine runtime == mkfs time in this
>   context, so to say.
>