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

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

[David Sterba]

We've hidden the zoned support in sysfs under debug config for the first
releases but now the stability is reasonable, though not all features
have been implemented.

As this depends on a config option, the per-filesystem feature won't
exist as such filesystem can't be mounted. The static feature will print
1 when the support is built-in, 0 otherwise.

Signed-off-by: David Sterba <dsterba@suse.com>
---

The merge target is not set, depends if everybody thinks it's the time
even though there are still known bugs. We're also waiting for
util-linux support (blkid, wipefs), so that needs to be synced too.

 fs/btrfs/sysfs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index bfe5e27617b0..7ad8f802ab88 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
 BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
 BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
 BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
-/* Remove once support for zoned allocation is feature complete */
-#ifdef CONFIG_BTRFS_DEBUG
+#ifdef CONFIG_BLK_DEV_ZONED
 BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
 #endif
 #ifdef CONFIG_FS_VERITY
@@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
 	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
 	BTRFS_FEAT_ATTR_PTR(free_space_tree),
 	BTRFS_FEAT_ATTR_PTR(raid1c34),
-#ifdef CONFIG_BTRFS_DEBUG
+#ifdef CONFIG_BLK_DEV_ZONED
 	BTRFS_FEAT_ATTR_PTR(zoned),
 #endif
 #ifdef CONFIG_FS_VERITY
@@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
 BTRFS_ATTR(static_feature, supported_sectorsizes,
 	   supported_sectorsizes_show);
 
+static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
+}
+BTRFS_ATTR(static_feature, zoned, zoned_show);
+
 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_sectorsizes),
+	BTRFS_ATTR_PTR(static_feature, zoned),
 	NULL
 };
 
-- 
2.31.1

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

[Anand Jain]

On 29/07/2021 00:56, David Sterba wrote:
> We've hidden the zoned support in sysfs under debug config for the first
> releases but now the stability is reasonable, though not all features
> have been implemented.
> 
> As this depends on a config option, the per-filesystem feature won't
> exist as such filesystem can't be mounted. The static feature will print
> 1 when the support is built-in, 0 otherwise.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> The merge target is not set, depends if everybody thinks it's the time
> even though there are still known bugs. We're also waiting for
> util-linux support (blkid, wipefs), so that needs to be synced too.
> 
>   fs/btrfs/sysfs.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index bfe5e27617b0..7ad8f802ab88 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
>   BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
>   BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
>   BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
> -/* Remove once support for zoned allocation is feature complete */
> -#ifdef CONFIG_BTRFS_DEBUG
> +#ifdef CONFIG_BLK_DEV_ZONED
>   BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
>   #endif
>   #ifdef CONFIG_FS_VERITY
> @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
>   	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
>   	BTRFS_FEAT_ATTR_PTR(free_space_tree),
>   	BTRFS_FEAT_ATTR_PTR(raid1c34),
> -#ifdef CONFIG_BTRFS_DEBUG
> +#ifdef CONFIG_BLK_DEV_ZONED
>   	BTRFS_FEAT_ATTR_PTR(zoned),
>   #endif
>   #ifdef CONFIG_FS_VERITY


  Looks good until here.


> @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>   BTRFS_ATTR(static_feature, supported_sectorsizes,
>   	   supported_sectorsizes_show);
>   
> +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
> +}
> +BTRFS_ATTR(static_feature, zoned, zoned_show);
> +
>   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_sectorsizes),
> +	BTRFS_ATTR_PTR(static_feature, zoned),
>   	NULL
>   };

  We don't need this part. btrfs_supported_feature_attrs will
  take care of showing zoned if when enabled on the mounted btrfs.

  As of now, this patch fails with
     [ 44.464597] sysfs: cannot create duplicate filename 
'/fs/btrfs/features/zoned'

Thanks, Anand

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

[David Sterba]

On Thu, Aug 05, 2021 at 08:13:03AM +0800, Anand Jain wrote:
> On 29/07/2021 00:56, David Sterba wrote:
> > We've hidden the zoned support in sysfs under debug config for the first
> > releases but now the stability is reasonable, though not all features
> > have been implemented.
> > 
> > As this depends on a config option, the per-filesystem feature won't
> > exist as such filesystem can't be mounted. The static feature will print
> > 1 when the support is built-in, 0 otherwise.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> > 
> > The merge target is not set, depends if everybody thinks it's the time
> > even though there are still known bugs. We're also waiting for
> > util-linux support (blkid, wipefs), so that needs to be synced too.
> > 
> >   fs/btrfs/sysfs.c | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> > index bfe5e27617b0..7ad8f802ab88 100644
> > --- a/fs/btrfs/sysfs.c
> > +++ b/fs/btrfs/sysfs.c
> > @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
> >   BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
> >   BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
> >   BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
> > -/* Remove once support for zoned allocation is feature complete */
> > -#ifdef CONFIG_BTRFS_DEBUG
> > +#ifdef CONFIG_BLK_DEV_ZONED
> >   BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
> >   #endif
> >   #ifdef CONFIG_FS_VERITY
> > @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
> >   	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
> >   	BTRFS_FEAT_ATTR_PTR(free_space_tree),
> >   	BTRFS_FEAT_ATTR_PTR(raid1c34),
> > -#ifdef CONFIG_BTRFS_DEBUG
> > +#ifdef CONFIG_BLK_DEV_ZONED
> >   	BTRFS_FEAT_ATTR_PTR(zoned),
> >   #endif
> >   #ifdef CONFIG_FS_VERITY
> 
> 
>   Looks good until here.
> 
> 
> > @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
> >   BTRFS_ATTR(static_feature, supported_sectorsizes,
> >   	   supported_sectorsizes_show);
> >   
> > +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
> > +{
> > +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
> > +}
> > +BTRFS_ATTR(static_feature, zoned, zoned_show);
> > +
> >   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_sectorsizes),
> > +	BTRFS_ATTR_PTR(static_feature, zoned),
> >   	NULL
> >   };
> 
>   We don't need this part. btrfs_supported_feature_attrs will
>   take care of showing zoned if when enabled on the mounted btrfs.

The idea is to put zoned also to the static features so the zoned
support can be determined before mount.

> 
>   As of now, this patch fails with
>      [ 44.464597] sysfs: cannot create duplicate filename 
> '/fs/btrfs/features/zoned'

I'll have a look.

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

[Anand Jain]

On 09/08/2021 17:19, David Sterba wrote:
> On Thu, Aug 05, 2021 at 08:13:03AM +0800, Anand Jain wrote:
>> On 29/07/2021 00:56, David Sterba wrote:
>>> We've hidden the zoned support in sysfs under debug config for the first
>>> releases but now the stability is reasonable, though not all features
>>> have been implemented.
>>>
>>> As this depends on a config option, the per-filesystem feature won't
>>> exist as such filesystem can't be mounted. The static feature will print
>>> 1 when the support is built-in, 0 otherwise.
>>>
>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>> ---
>>>
>>> The merge target is not set, depends if everybody thinks it's the time
>>> even though there are still known bugs. We're also waiting for
>>> util-linux support (blkid, wipefs), so that needs to be synced too.
>>>
>>>    fs/btrfs/sysfs.c | 12 +++++++++---
>>>    1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>>> index bfe5e27617b0..7ad8f802ab88 100644
>>> --- a/fs/btrfs/sysfs.c
>>> +++ b/fs/btrfs/sysfs.c
>>> @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
>>>    BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
>>>    BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
>>>    BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
>>> -/* Remove once support for zoned allocation is feature complete */
>>> -#ifdef CONFIG_BTRFS_DEBUG
>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>>    BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
>>>    #endif
>>>    #ifdef CONFIG_FS_VERITY
>>> @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
>>>    	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
>>>    	BTRFS_FEAT_ATTR_PTR(free_space_tree),
>>>    	BTRFS_FEAT_ATTR_PTR(raid1c34),
>>> -#ifdef CONFIG_BTRFS_DEBUG
>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>>    	BTRFS_FEAT_ATTR_PTR(zoned),
>>>    #endif
>>>    #ifdef CONFIG_FS_VERITY
>>
>>
>>    Looks good until here.
>>
>>
>>> @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>>>    BTRFS_ATTR(static_feature, supported_sectorsizes,
>>>    	   supported_sectorsizes_show);
>>>    
>>> +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
>>> +{
>>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
>>> +}
>>> +BTRFS_ATTR(static_feature, zoned, zoned_show);
>>> +
>>>    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_sectorsizes),
>>> +	BTRFS_ATTR_PTR(static_feature, zoned),
>>>    	NULL
>>>    };
>>
>>    We don't need this part. btrfs_supported_feature_attrs will
>>    take care of showing zoned if when enabled on the mounted btrfs.
> 
> The idea is to put zoned also to the static features so the zoned
> support can be determined before mount.


  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.

-------------
  btrfs_supported_static_feature_attrs /sys/fs/btrfs/features
  btrfs_supported_feature_attrs /sys/fs/btrfs/features and
  /sys/fs/btrfs/uuid/features (if visible)
-------------



> 
>>
>>    As of now, this patch fails with
>>       [ 44.464597] sysfs: cannot create duplicate filename
>> '/fs/btrfs/features/zoned'
> 
> I'll have a look.
> 

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

[David Sterba]

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.

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

[Nikolay Borisov]

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.

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

[Nikolay Borisov]

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 or not at runtime. I think for zoned block device we can't
really disable the support at runtime? If so then it needs to only be
defined as a static feature? 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?

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

[Anand Jain]

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.

  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.

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

[Anand Jain]

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.
> 

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

[Nikolay Borisov]

On 28.07.21 г. 19:56, David Sterba wrote:
> We've hidden the zoned support in sysfs under debug config for the first
> releases but now the stability is reasonable, though not all features
> have been implemented.
> 
> As this depends on a config option, the per-filesystem feature won't
> exist as such filesystem can't be mounted. The static feature will print
> 1 when the support is built-in, 0 otherwise.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> The merge target is not set, depends if everybody thinks it's the time
> even though there are still known bugs. We're also waiting for
> util-linux support (blkid, wipefs), so that needs to be synced too.
> 
>  fs/btrfs/sysfs.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index bfe5e27617b0..7ad8f802ab88 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
>  BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
>  BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
>  BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
> -/* Remove once support for zoned allocation is feature complete */
> -#ifdef CONFIG_BTRFS_DEBUG
> +#ifdef CONFIG_BLK_DEV_ZONED
>  BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
>  #endif
>  #ifdef CONFIG_FS_VERITY
> @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
>  	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
>  	BTRFS_FEAT_ATTR_PTR(free_space_tree),
>  	BTRFS_FEAT_ATTR_PTR(raid1c34),
> -#ifdef CONFIG_BTRFS_DEBUG
> +#ifdef CONFIG_BLK_DEV_ZONED
>  	BTRFS_FEAT_ATTR_PTR(zoned),
>  #endif
>  #ifdef CONFIG_FS_VERITY
> @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>  BTRFS_ATTR(static_feature, supported_sectorsizes,
>  	   supported_sectorsizes_show);
>  
> +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
> +}
> +BTRFS_ATTR(static_feature, zoned, zoned_show);
> +
>  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_sectorsizes),
> +	BTRFS_ATTR_PTR(static_feature, zoned),
>  	NULL
>  };

Why isn't the above hunk predicated on CONFIG_BLK_DEV_ZONED the same as
the ATTR_INCOMPAT zoneed bit, but as explained in my earlier email one
of these should go and whichever remains must be predicated on
CONFIG_BLK_DEV_ZONED.
>  
> 

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

[Damien Le Moal]

On 2021/08/19 18:21, Nikolay Borisov wrote:
> 
> 
> On 28.07.21 г. 19:56, David Sterba wrote:
>> We've hidden the zoned support in sysfs under debug config for the first
>> releases but now the stability is reasonable, though not all features
>> have been implemented.
>>
>> As this depends on a config option, the per-filesystem feature won't
>> exist as such filesystem can't be mounted. The static feature will print
>> 1 when the support is built-in, 0 otherwise.
>>
>> Signed-off-by: David Sterba <dsterba@suse.com>
>> ---
>>
>> The merge target is not set, depends if everybody thinks it's the time
>> even though there are still known bugs. We're also waiting for
>> util-linux support (blkid, wipefs), so that needs to be synced too.
>>
>>  fs/btrfs/sysfs.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index bfe5e27617b0..7ad8f802ab88 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
>>  BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
>>  BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
>>  BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
>> -/* Remove once support for zoned allocation is feature complete */
>> -#ifdef CONFIG_BTRFS_DEBUG
>> +#ifdef CONFIG_BLK_DEV_ZONED
>>  BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
>>  #endif
>>  #ifdef CONFIG_FS_VERITY
>> @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
>>  	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
>>  	BTRFS_FEAT_ATTR_PTR(free_space_tree),
>>  	BTRFS_FEAT_ATTR_PTR(raid1c34),
>> -#ifdef CONFIG_BTRFS_DEBUG
>> +#ifdef CONFIG_BLK_DEV_ZONED
>>  	BTRFS_FEAT_ATTR_PTR(zoned),
>>  #endif
>>  #ifdef CONFIG_FS_VERITY
>> @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>>  BTRFS_ATTR(static_feature, supported_sectorsizes,
>>  	   supported_sectorsizes_show);
>>  
>> +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
>> +{
>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
>> +}
>> +BTRFS_ATTR(static_feature, zoned, zoned_show);
>> +
>>  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_sectorsizes),
>> +	BTRFS_ATTR_PTR(static_feature, zoned),
>>  	NULL
>>  };
> 
> Why isn't the above hunk predicated on CONFIG_BLK_DEV_ZONED the same as
> the ATTR_INCOMPAT zoneed bit, but as explained in my earlier email one
> of these should go and whichever remains must be predicated on
> CONFIG_BLK_DEV_ZONED.

zoned-btrfs can be used with regular devices too. In that case, zones are
emulated, all of them being conventional. So btrfs zoned feature should
definitely not be dependent on CONFIG_BLK_DEV_ZONED.

If CONFIG_BLK_DEV_ZONED is not defined, then zoned btrfs will be usable only on
regular devices. But since in that case zoned devices will not show up, it is
all consistent.


-- 
Damien Le Moal
Western Digital Research

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

[Nikolay Borisov]

On 19.08.21 г. 12:31, Damien Le Moal wrote:
> On 2021/08/19 18:21, Nikolay Borisov wrote:
>>
>>
>> On 28.07.21 г. 19:56, David Sterba wrote:
>>> We've hidden the zoned support in sysfs under debug config for the first
>>> releases but now the stability is reasonable, though not all features
>>> have been implemented.
>>>
>>> As this depends on a config option, the per-filesystem feature won't
>>> exist as such filesystem can't be mounted. The static feature will print
>>> 1 when the support is built-in, 0 otherwise.
>>>
>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>> ---
>>>
>>> The merge target is not set, depends if everybody thinks it's the time
>>> even though there are still known bugs. We're also waiting for
>>> util-linux support (blkid, wipefs), so that needs to be synced too.
>>>
>>>  fs/btrfs/sysfs.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>>> index bfe5e27617b0..7ad8f802ab88 100644
>>> --- a/fs/btrfs/sysfs.c
>>> +++ b/fs/btrfs/sysfs.c
>>> @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
>>>  BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
>>>  BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
>>>  BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
>>> -/* Remove once support for zoned allocation is feature complete */
>>> -#ifdef CONFIG_BTRFS_DEBUG
>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>>  BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
>>>  #endif
>>>  #ifdef CONFIG_FS_VERITY
>>> @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
>>>  	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
>>>  	BTRFS_FEAT_ATTR_PTR(free_space_tree),
>>>  	BTRFS_FEAT_ATTR_PTR(raid1c34),
>>> -#ifdef CONFIG_BTRFS_DEBUG
>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>>  	BTRFS_FEAT_ATTR_PTR(zoned),
>>>  #endif
>>>  #ifdef CONFIG_FS_VERITY
>>> @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>>>  BTRFS_ATTR(static_feature, supported_sectorsizes,
>>>  	   supported_sectorsizes_show);
>>>  
>>> +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
>>> +{
>>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
>>> +}
>>> +BTRFS_ATTR(static_feature, zoned, zoned_show);
>>> +
>>>  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_sectorsizes),
>>> +	BTRFS_ATTR_PTR(static_feature, zoned),
>>>  	NULL
>>>  };
>>
>> Why isn't the above hunk predicated on CONFIG_BLK_DEV_ZONED the same as
>> the ATTR_INCOMPAT zoneed bit, but as explained in my earlier email one
>> of these should go and whichever remains must be predicated on
>> CONFIG_BLK_DEV_ZONED.
> 
> zoned-btrfs can be used with regular devices too. In that case, zones are
> emulated, all of them being conventional. So btrfs zoned feature should
> definitely not be dependent on CONFIG_BLK_DEV_ZONED.
> 
> If CONFIG_BLK_DEV_ZONED is not defined, then zoned btrfs will be usable only on
> regular devices. But since in that case zoned devices will not show up, it is
> all consistent.

Then we should discuss what the semantics of the ZONED flag under
features should be? I.e do we need to explicitly distinguish between
"btrfs supports zoned AND the kernel is compiled with BLK_DEV_ZONED so
we say we support it" and "btrfs has support for zoned devices but you
have to figure on your own if BLK_DEV_ZONED is enabled" ? I.e by having
sys/fs/btrfs/zoned set to 1 what information do we want to convey to the
user?

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

[Damien Le Moal]

On 2021/08/19 18:37, Nikolay Borisov wrote:
> 
> 
> On 19.08.21 г. 12:31, Damien Le Moal wrote:
>> On 2021/08/19 18:21, Nikolay Borisov wrote:
>>>
>>>
>>> On 28.07.21 г. 19:56, David Sterba wrote:
>>>> We've hidden the zoned support in sysfs under debug config for the first
>>>> releases but now the stability is reasonable, though not all features
>>>> have been implemented.
>>>>
>>>> As this depends on a config option, the per-filesystem feature won't
>>>> exist as such filesystem can't be mounted. The static feature will print
>>>> 1 when the support is built-in, 0 otherwise.
>>>>
>>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>>> ---
>>>>
>>>> The merge target is not set, depends if everybody thinks it's the time
>>>> even though there are still known bugs. We're also waiting for
>>>> util-linux support (blkid, wipefs), so that needs to be synced too.
>>>>
>>>>  fs/btrfs/sysfs.c | 12 +++++++++---
>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>>>> index bfe5e27617b0..7ad8f802ab88 100644
>>>> --- a/fs/btrfs/sysfs.c
>>>> +++ b/fs/btrfs/sysfs.c
>>>> @@ -263,8 +263,7 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
>>>>  BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
>>>>  BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
>>>>  BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
>>>> -/* Remove once support for zoned allocation is feature complete */
>>>> -#ifdef CONFIG_BTRFS_DEBUG
>>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>>>  BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
>>>>  #endif
>>>>  #ifdef CONFIG_FS_VERITY
>>>> @@ -285,7 +284,7 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
>>>>  	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
>>>>  	BTRFS_FEAT_ATTR_PTR(free_space_tree),
>>>>  	BTRFS_FEAT_ATTR_PTR(raid1c34),
>>>> -#ifdef CONFIG_BTRFS_DEBUG
>>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>>>  	BTRFS_FEAT_ATTR_PTR(zoned),
>>>>  #endif
>>>>  #ifdef CONFIG_FS_VERITY
>>>> @@ -384,12 +383,19 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>>>>  BTRFS_ATTR(static_feature, supported_sectorsizes,
>>>>  	   supported_sectorsizes_show);
>>>>  
>>>> +static ssize_t zoned_show(struct kobject *kobj, struct kobj_attribute *a, char *buf)
>>>> +{
>>>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", IS_ENABLED(CONFIG_BLK_DEV_ZONED));
>>>> +}
>>>> +BTRFS_ATTR(static_feature, zoned, zoned_show);
>>>> +
>>>>  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_sectorsizes),
>>>> +	BTRFS_ATTR_PTR(static_feature, zoned),
>>>>  	NULL
>>>>  };
>>>
>>> Why isn't the above hunk predicated on CONFIG_BLK_DEV_ZONED the same as
>>> the ATTR_INCOMPAT zoneed bit, but as explained in my earlier email one
>>> of these should go and whichever remains must be predicated on
>>> CONFIG_BLK_DEV_ZONED.
>>
>> zoned-btrfs can be used with regular devices too. In that case, zones are
>> emulated, all of them being conventional. So btrfs zoned feature should
>> definitely not be dependent on CONFIG_BLK_DEV_ZONED.
>>
>> If CONFIG_BLK_DEV_ZONED is not defined, then zoned btrfs will be usable only on
>> regular devices. But since in that case zoned devices will not show up, it is
>> all consistent.
> 
> Then we should discuss what the semantics of the ZONED flag under
> features should be? I.e do we need to explicitly distinguish between
> "btrfs supports zoned AND the kernel is compiled with BLK_DEV_ZONED so
> we say we support it" and "btrfs has support for zoned devices but you
> have to figure on your own if BLK_DEV_ZONED is enabled" ? I.e by having
> sys/fs/btrfs/zoned set to 1 what information do we want to convey to the
> user?

That this version of btrfs supports the zoned mode. What device you can actually
apply it to then depends on CONFIG_BLK_DEV_ZONED. If that one is set, the zoned
btrfs feature applies to all devices, regular and zoned. If it is not set, then
the feature applies to regular devices only, since zoned devices will NOT be
present. The user will not see them :)

Rather all simple I think. But I am heavily biased with this since I have been
doing zoned stuff for years. So someone totally new to zoned storage needs to
chime in !


-- 
Damien Le Moal
Western Digital Research

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

[David Sterba]

We've hidden the zoned support in sysfs under debug config for the first
releases but now the stability is reasonable, though not all features
have been implemented.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/sysfs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index ebe76d7a4a64..3554c7b4204f 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -289,9 +289,10 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
 BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
 BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
 BTRFS_FEAT_ATTR_INCOMPAT(raid1c34, RAID1C34);
-#ifdef CONFIG_BTRFS_DEBUG
-/* Remove once support for zoned allocation is feature complete */
+#ifdef CONFIG_BLK_DEV_ZONED
 BTRFS_FEAT_ATTR_INCOMPAT(zoned, ZONED);
+#endif
+#ifdef CONFIG_BTRFS_DEBUG
 /* Remove once support for extent tree v2 is feature complete */
 BTRFS_FEAT_ATTR_INCOMPAT(extent_tree_v2, EXTENT_TREE_V2);
 #endif
@@ -320,8 +321,10 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
 	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
 	BTRFS_FEAT_ATTR_PTR(free_space_tree),
 	BTRFS_FEAT_ATTR_PTR(raid1c34),
-#ifdef CONFIG_BTRFS_DEBUG
+#ifdef CONFIG_BLK_DEV_ZONED
 	BTRFS_FEAT_ATTR_PTR(zoned),
+#endif
+#ifdef CONFIG_BTRFS_DEBUG
 	BTRFS_FEAT_ATTR_PTR(extent_tree_v2),
 #endif
 #ifdef CONFIG_FS_VERITY
-- 
2.36.1

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

[David Sterba]

On Mon, Jun 06, 2022 at 06:36:35PM +0200, David Sterba wrote:
> We've hidden the zoned support in sysfs under debug config for the first
> releases but now the stability is reasonable, though not all features
> have been implemented.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Added to misc-next.