On 19/08/2021 19:16, Anand Jain wrote:

On 19/08/2021 17:14, Nikolay Borisov wrote:

quoted

On 19.08.21 г. 12:08, Nikolay Borisov wrote:

quoted

On 19.08.21 г. 2:48, David Sterba wrote:

quoted

On Tue, Aug 10, 2021 at 08:30:51AM +0800, Anand Jain wrote:

quoted

   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

quoted

which of the two sets to use when defining a feature is whether it can

...be enabled per fsid.
::

quoted

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.

quoted

If so then it needs to only be
defined as a static feature?

  No. pls.

quoted

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.