[PATCH 0/2] Cleanup 2 sysfs flags

[PATCH 0/2] Cleanup 2 sysfs flags

[Nikolay Borisov]

This series removes MIXED_BACKREF/BIG_METADATA sysfs files. While discussing
flags with David it transpired that those 2 have been merged for quite some time
and practically all modern (created in the last 9 years) filesystem will have
them set. They are not opt-in so let's use this fact to simplify our list of
sysfs flags.

Nikolay Borisov (2):
  btrfs: remove MIXED_BACKREF sysfs file
  btrfs: remove BIG_METADATA syfs file

 fs/btrfs/sysfs.c | 4 ----
 1 file changed, 4 deletions(-)

--
2.25.1

[PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Nikolay Borisov]

This feature has been the default for about 13 year. At this point it's
safe to consider it an indispensable feature of BTRFS as such there's
no need to advertise it in sysfs. Simply remove the sysfs file.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/sysfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index d4502f0e4fca..715d1e725f2a 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -276,7 +276,6 @@ static umode_t btrfs_feature_visible(struct kobject *kobj,
 	return mode;
 }
 
-BTRFS_FEAT_ATTR_INCOMPAT(mixed_backref, MIXED_BACKREF);
 BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL);
 BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS);
 BTRFS_FEAT_ATTR_INCOMPAT(compress_lzo, COMPRESS_LZO);
@@ -308,7 +307,6 @@ BTRFS_FEAT_ATTR_COMPAT_RO(verity, VERITY);
  *                               can be changed on a mounted filesystem.
  */
 static struct attribute *btrfs_supported_feature_attrs[] = {
-	BTRFS_FEAT_ATTR_PTR(mixed_backref),
 	BTRFS_FEAT_ATTR_PTR(default_subvol),
 	BTRFS_FEAT_ATTR_PTR(mixed_groups),
 	BTRFS_FEAT_ATTR_PTR(compress_lzo),
-- 
2.25.1

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Qu Wenruo]

On 2022/6/24 16:01, Nikolay Borisov wrote:
> This feature has been the default for about 13 year. At this point it's
> safe to consider it an indispensable feature of BTRFS as such there's
> no need to advertise it in sysfs. Simply remove the sysfs file.

I don't think that's the correct way to go.

In fact, I think sysfs should have everything, no matter how long
supported it is.

Thanks,
Qu
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/sysfs.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index d4502f0e4fca..715d1e725f2a 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -276,7 +276,6 @@ static umode_t btrfs_feature_visible(struct kobject *kobj,
>   	return mode;
>   }
>
> -BTRFS_FEAT_ATTR_INCOMPAT(mixed_backref, MIXED_BACKREF);
>   BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL);
>   BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS);
>   BTRFS_FEAT_ATTR_INCOMPAT(compress_lzo, COMPRESS_LZO);
> @@ -308,7 +307,6 @@ BTRFS_FEAT_ATTR_COMPAT_RO(verity, VERITY);
>    *                               can be changed on a mounted filesystem.
>    */
>   static struct attribute *btrfs_supported_feature_attrs[] = {
> -	BTRFS_FEAT_ATTR_PTR(mixed_backref),
>   	BTRFS_FEAT_ATTR_PTR(default_subvol),
>   	BTRFS_FEAT_ATTR_PTR(mixed_groups),
>   	BTRFS_FEAT_ATTR_PTR(compress_lzo),

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Nikolay Borisov]

On 24.06.22 г. 11:13 ч., Qu Wenruo wrote:
> 
> I don't think that's the correct way to go.
> 
> In fact, I think sysfs should have everything, no matter how long
> supported it is.


I disagree, for things which are considered stand alone features - yes. 
Like free space tree 2, but for something like backrefs, heck I think 
we've even removed code which predates mixed backrefs so I'm not 
entirely use the filesystem can function with that feature turned off, 
actually it's not possible to create a non-mixedbackref file system 
since this behavior is hard-coded in btrfs-progs. Also the commit for 
the backrefs states:


This commit introduces a new kind of back reference for btrfs metadata.
Once a filesystem has been mounted with this commit, IT WILL NO LONGER
BE MOUNTABLE BY OLDER KERNELS.

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Qu Wenruo]

On 2022/6/24 19:32, Nikolay Borisov wrote:
>
>
> On 24.06.22 г. 11:13 ч., Qu Wenruo wrote:
>>
>> I don't think that's the correct way to go.
>>
>> In fact, I think sysfs should have everything, no matter how long
>> supported it is.
>
>
> I disagree, for things which are considered stand alone features - yes.
> Like free space tree 2, but for something like backrefs, heck I think
> we've even removed code which predates mixed backrefs so I'm not
> entirely use the filesystem can function with that feature turned off,
> actually it's not possible to create a non-mixedbackref file system
> since this behavior is hard-coded in btrfs-progs. Also the commit for
> the backrefs states:
>
>
> This commit introduces a new kind of back reference for btrfs metadata.
> Once a filesystem has been mounted with this commit, IT WILL NO LONGER
> BE MOUNTABLE BY OLDER KERNELS.
>

That means we're hiding incompat features from the user.

Even if it's not tunable and should always be enabled, we still need to
add that.

History can not be forgotten.

Thanks,
Qu

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[David Sterba]

On Fri, Jun 24, 2022 at 07:46:12PM +0800, Qu Wenruo wrote:
> On 2022/6/24 19:32, Nikolay Borisov wrote:
> > On 24.06.22 г. 11:13 ч., Qu Wenruo wrote:
> >>
> >> I don't think that's the correct way to go.
> >>
> >> In fact, I think sysfs should have everything, no matter how long
> >> supported it is.
> >
> > I disagree, for things which are considered stand alone features - yes.
> > Like free space tree 2, but for something like backrefs, heck I think
> > we've even removed code which predates mixed backrefs so I'm not
> > entirely use the filesystem can function with that feature turned off,
> > actually it's not possible to create a non-mixedbackref file system
> > since this behavior is hard-coded in btrfs-progs. Also the commit for
> > the backrefs states:
> >
> >
> > This commit introduces a new kind of back reference for btrfs metadata.
> > Once a filesystem has been mounted with this commit, IT WILL NO LONGER
> > BE MOUNTABLE BY OLDER KERNELS.
> 
> That means we're hiding incompat features from the user.
> 
> Even if it's not tunable and should always be enabled, we still need to
> add that.

I think the mixed_backref is an exception because it's been part of the
default format for so long that we don't even remember there was
something else. For users it does not mean anything today, moreover it
could be confused with mixed block groups.

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Qu Wenruo]

On 2022/6/24 21:47, David Sterba wrote:
> On Fri, Jun 24, 2022 at 07:46:12PM +0800, Qu Wenruo wrote:
>> On 2022/6/24 19:32, Nikolay Borisov wrote:
>>> On 24.06.22 г. 11:13 ч., Qu Wenruo wrote:
>>>>
>>>> I don't think that's the correct way to go.
>>>>
>>>> In fact, I think sysfs should have everything, no matter how long
>>>> supported it is.
>>>
>>> I disagree, for things which are considered stand alone features - yes.
>>> Like free space tree 2, but for something like backrefs, heck I think
>>> we've even removed code which predates mixed backrefs so I'm not
>>> entirely use the filesystem can function with that feature turned off,
>>> actually it's not possible to create a non-mixedbackref file system
>>> since this behavior is hard-coded in btrfs-progs. Also the commit for
>>> the backrefs states:
>>>
>>>
>>> This commit introduces a new kind of back reference for btrfs metadata.
>>> Once a filesystem has been mounted with this commit, IT WILL NO LONGER
>>> BE MOUNTABLE BY OLDER KERNELS.
>>
>> That means we're hiding incompat features from the user.
>>
>> Even if it's not tunable and should always be enabled, we still need to
>> add that.
> 
> I think the mixed_backref is an exception because it's been part of the
> default format for so long that we don't even remember there was
> something else. For users it does not mean anything today, moreover it
> could be confused with mixed block groups.

Then after some time, there will be some "smart" users find that we have 
one incompat bit without any explanation.

Thanks,
Qu

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[David Sterba]

On Fri, Jun 24, 2022 at 10:02:43PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/6/24 21:47, David Sterba wrote:
> > On Fri, Jun 24, 2022 at 07:46:12PM +0800, Qu Wenruo wrote:
> >> On 2022/6/24 19:32, Nikolay Borisov wrote:
> >>> On 24.06.22 г. 11:13 ч., Qu Wenruo wrote:
> >>>>
> >>>> I don't think that's the correct way to go.
> >>>>
> >>>> In fact, I think sysfs should have everything, no matter how long
> >>>> supported it is.
> >>>
> >>> I disagree, for things which are considered stand alone features - yes.
> >>> Like free space tree 2, but for something like backrefs, heck I think
> >>> we've even removed code which predates mixed backrefs so I'm not
> >>> entirely use the filesystem can function with that feature turned off,
> >>> actually it's not possible to create a non-mixedbackref file system
> >>> since this behavior is hard-coded in btrfs-progs. Also the commit for
> >>> the backrefs states:
> >>>
> >>>
> >>> This commit introduces a new kind of back reference for btrfs metadata.
> >>> Once a filesystem has been mounted with this commit, IT WILL NO LONGER
> >>> BE MOUNTABLE BY OLDER KERNELS.
> >>
> >> That means we're hiding incompat features from the user.
> >>
> >> Even if it's not tunable and should always be enabled, we still need to
> >> add that.
> > 
> > I think the mixed_backref is an exception because it's been part of the
> > default format for so long that we don't even remember there was
> > something else. For users it does not mean anything today, moreover it
> > could be confused with mixed block groups.
> 
> Then after some time, there will be some "smart" users find that we have 
> one incompat bit without any explanation.

Removed functionality is documented, the sysfs feature files are in
manual pages and we can add a notice in which version it was removed.

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Qu Wenruo]

On 2022/6/24 23:44, David Sterba wrote:
> On Fri, Jun 24, 2022 at 10:02:43PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2022/6/24 21:47, David Sterba wrote:
>>> On Fri, Jun 24, 2022 at 07:46:12PM +0800, Qu Wenruo wrote:
>>>> On 2022/6/24 19:32, Nikolay Borisov wrote:
>>>>> On 24.06.22 г. 11:13 ч., Qu Wenruo wrote:
>>>>>>
>>>>>> I don't think that's the correct way to go.
>>>>>>
>>>>>> In fact, I think sysfs should have everything, no matter how long
>>>>>> supported it is.
>>>>>
>>>>> I disagree, for things which are considered stand alone features - yes.
>>>>> Like free space tree 2, but for something like backrefs, heck I think
>>>>> we've even removed code which predates mixed backrefs so I'm not
>>>>> entirely use the filesystem can function with that feature turned off,
>>>>> actually it's not possible to create a non-mixedbackref file system
>>>>> since this behavior is hard-coded in btrfs-progs. Also the commit for
>>>>> the backrefs states:
>>>>>
>>>>>
>>>>> This commit introduces a new kind of back reference for btrfs metadata.
>>>>> Once a filesystem has been mounted with this commit, IT WILL NO LONGER
>>>>> BE MOUNTABLE BY OLDER KERNELS.
>>>>
>>>> That means we're hiding incompat features from the user.
>>>>
>>>> Even if it's not tunable and should always be enabled, we still need to
>>>> add that.
>>>
>>> I think the mixed_backref is an exception because it's been part of the
>>> default format for so long that we don't even remember there was
>>> something else. For users it does not mean anything today, moreover it
>>> could be confused with mixed block groups.
>>
>> Then after some time, there will be some "smart" users find that we have
>> one incompat bit without any explanation.
>
> Removed functionality is documented, the sysfs feature files are in
> manual pages and we can add a notice in which version it was removed.

Then have all the old features get removed one by one, until one day we
have a dozen of bits set, but only one or two still show in sysfs features?

No, this definitely doesn't look sane to me.

It's just trying to hide some bad behaviors which we didn't get it right
in the first place.
It's fine to didn't get those things done correct in the first place,
but not fine to hide them.

Especially those sysfs is already hidden to most users, way less
invasive than the dmesg output/mkfs features/etc, why we still want to
remove them?

Thanks,
Qu

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[David Sterba]

On Sat, Jun 25, 2022 at 01:53:47PM +0800, Qu Wenruo wrote:
> > Removed functionality is documented, the sysfs feature files are in
> > manual pages and we can add a notice in which version it was removed.
> 
> Then have all the old features get removed one by one, until one day we
> have a dozen of bits set, but only one or two still show in sysfs features?
> 
> No, this definitely doesn't look sane to me.

And nobody is suggesting that either, the big data and mixed backrefs
are something that's exceptional in this regard. Removing other features
would take some significant time to remove and a check if it would still
not break some testing setups.

> It's just trying to hide some bad behaviors which we didn't get it right
> in the first place.
> It's fine to didn't get those things done correct in the first place,
> but not fine to hide them.
>
> Especially those sysfs is already hidden to most users, way less
> invasive than the dmesg output/mkfs features/etc, why we still want to
> remove them?

To unclutter the namespace a bit, in case of the mixed_backrefs it's a
bit confusing with the mixed block groups but I think I've mentioned
that already.

Re: [PATCH 1/2] btrfs: remove MIXED_BACKREF sysfs file

[Qu Wenruo]

On 2022/7/11 23:23, David Sterba wrote:
> On Sat, Jun 25, 2022 at 01:53:47PM +0800, Qu Wenruo wrote:
>>> Removed functionality is documented, the sysfs feature files are in
>>> manual pages and we can add a notice in which version it was removed.
>>
>> Then have all the old features get removed one by one, until one day we
>> have a dozen of bits set, but only one or two still show in sysfs features?
>>
>> No, this definitely doesn't look sane to me.
>
> And nobody is suggesting that either,

Doesn't the patch just remove the sysfs file for both global and per-fs
features?
Or did I miss something?

> the big data and mixed backrefs
> are something that's exceptional in this regard. Removing other features
> would take some significant time to remove and a check if it would still
> not break some testing setups.
>
>> It's just trying to hide some bad behaviors which we didn't get it right
>> in the first place.
>> It's fine to didn't get those things done correct in the first place,
>> but not fine to hide them.
>>
>> Especially those sysfs is already hidden to most users, way less
>> invasive than the dmesg output/mkfs features/etc, why we still want to
>> remove them?
>
> To unclutter the namespace a bit, in case of the mixed_backrefs it's a
> bit confusing with the mixed block groups but I think I've mentioned
> that already.

If you're just to avoid the namespace conflicts/confusion, then just
rename them with some prefix like "__always_on_" or something similar.

Thanks,
Qu

[PATCH 2/2] btrfs: remove BIG_METADATA syfs file

[Nikolay Borisov]

This flag has been merged in 3.10 and is effectively always-on. As
such it can be considered an indispensable feature of the filesystem.
Simply remove it from sysfs.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/sysfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 715d1e725f2a..e13792214325 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -280,7 +280,6 @@ BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL);
 BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS);
 BTRFS_FEAT_ATTR_INCOMPAT(compress_lzo, COMPRESS_LZO);
 BTRFS_FEAT_ATTR_INCOMPAT(compress_zstd, COMPRESS_ZSTD);
-BTRFS_FEAT_ATTR_INCOMPAT(big_metadata, BIG_METADATA);
 BTRFS_FEAT_ATTR_INCOMPAT(extended_iref, EXTENDED_IREF);
 BTRFS_FEAT_ATTR_INCOMPAT(raid56, RAID56);
 BTRFS_FEAT_ATTR_INCOMPAT(skinny_metadata, SKINNY_METADATA);
@@ -311,7 +310,6 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
 	BTRFS_FEAT_ATTR_PTR(mixed_groups),
 	BTRFS_FEAT_ATTR_PTR(compress_lzo),
 	BTRFS_FEAT_ATTR_PTR(compress_zstd),
-	BTRFS_FEAT_ATTR_PTR(big_metadata),
 	BTRFS_FEAT_ATTR_PTR(extended_iref),
 	BTRFS_FEAT_ATTR_PTR(raid56),
 	BTRFS_FEAT_ATTR_PTR(skinny_metadata),
-- 
2.25.1