| Message ID | 20220624080123.1521917-2-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup 2 sysfs flags | expand |
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),
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.
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
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.
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
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.
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
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.
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
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),
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(-)