| Message ID | ef0efdacd9bd53a55a02c6419b9ff0d51edf5408.1673412612.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: keep sysfs features in tandem with runtime features change | expand |
On 2023/1/11 13:40, Anand Jain wrote: > When we change runtime features, the sysfs under > /sys/fs/btrfs/<uuid>/features > render stale. > > For example: (before) > > $ btrfs filesystem df /btrfs > Data, single: total=8.00MiB, used=0.00B > System, DUP: total=8.00MiB, used=16.00KiB > Metadata, DUP: total=51.19MiB, used=128.00KiB > global reserve, single: total=3.50MiB, used=0.00B > > $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ > extended_iref free_space_tree no_holes skinny_metadata > > Use balance to convert from single/dup to RAID5 profile. > > $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs > > Still, sysfs is unaware of raid5. > > $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ > extended_iref free_space_tree no_holes skinny_metadata > > Which doesn't match superblock > > $ btrfs in dump-super /dev/loop0 > > incompat_flags 0x3e1 > ( MIXED_BACKREF | > BIG_METADATA | > EXTENDED_IREF | > RAID56 | > SKINNY_METADATA | > NO_HOLES ) > > Require mount-recycle as a workaround. > > Fix this by laying out all attributes on the sysfs at mount time. However, > return 0 or 1 when read, for used or unused, respectively. > > For example: (after) > > $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ > block_group_tree compress_zstd extended_iref free_space_tree mixed_groups raid1c34 skinny_metadata zoned > compress_lzo default_subvol extent_tree_v2 metadata_uuid no_holes raid56 verity > > $ cat /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 > 0 > > $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs > > $ cat /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 > 1 Oh, I found this very confusing. Previously features/ directory just shows what we have (either in kernel support or the specified fs), which is very straightforward. Changing it to 0/1 is way less easy to understand, and can be considered as big behavior change. Is it really no way to change the fs' features? Thanks, Qu > > A fstests test case will follow. > > The source code changes involve removing the visible function pointer for > the btrfs_feature_attr_group, as it is an optional feature. And the > store/show part for the same is already implemented. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/sysfs.c | 23 ----------------------- > 1 file changed, 23 deletions(-) > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index 45615ce36498..fa3354f8213f 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -256,28 +256,6 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj, > return count; > } > > -static umode_t btrfs_feature_visible(struct kobject *kobj, > - struct attribute *attr, int unused) > -{ > - struct btrfs_fs_info *fs_info = to_fs_info(kobj); > - umode_t mode = attr->mode; > - > - if (fs_info) { > - struct btrfs_feature_attr *fa; > - u64 features; > - > - fa = attr_to_btrfs_feature_attr(attr); > - features = get_features(fs_info, fa->feature_set); > - > - if (can_modify_feature(fa)) > - mode |= S_IWUSR; > - else if (!(features & fa->feature_bit)) > - mode = 0; > - } > - > - return mode; > -} > - > BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL); > BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS); > BTRFS_FEAT_ATTR_INCOMPAT(compress_lzo, COMPRESS_LZO); > @@ -335,7 +313,6 @@ static struct attribute *btrfs_supported_feature_attrs[] = { > > static const struct attribute_group btrfs_feature_attr_group = { > .name = "features", > - .is_visible = btrfs_feature_visible, > .attrs = btrfs_supported_feature_attrs, > }; >
On 2023/1/11 15:05, Qu Wenruo wrote: > > > On 2023/1/11 13:40, Anand Jain wrote: >> When we change runtime features, the sysfs under >> /sys/fs/btrfs/<uuid>/features >> render stale. >> >> For example: (before) >> >> $ btrfs filesystem df /btrfs >> Data, single: total=8.00MiB, used=0.00B >> System, DUP: total=8.00MiB, used=16.00KiB >> Metadata, DUP: total=51.19MiB, used=128.00KiB >> global reserve, single: total=3.50MiB, used=0.00B >> >> $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ >> extended_iref free_space_tree no_holes skinny_metadata >> >> Use balance to convert from single/dup to RAID5 profile. >> >> $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs >> >> Still, sysfs is unaware of raid5. >> >> $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ >> extended_iref free_space_tree no_holes skinny_metadata >> >> Which doesn't match superblock >> >> $ btrfs in dump-super /dev/loop0 >> >> incompat_flags 0x3e1 >> ( MIXED_BACKREF | >> BIG_METADATA | >> EXTENDED_IREF | >> RAID56 | >> SKINNY_METADATA | >> NO_HOLES ) >> >> Require mount-recycle as a workaround. >> >> Fix this by laying out all attributes on the sysfs at mount time. >> However, >> return 0 or 1 when read, for used or unused, respectively. >> >> For example: (after) >> >> $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ >> block_group_tree compress_zstd extended_iref free_space_tree >> mixed_groups raid1c34 skinny_metadata zoned >> compress_lzo default_subvol extent_tree_v2 metadata_uuid no_holes >> raid56 verity >> >> $ cat >> /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 >> 0 >> >> $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs >> >> $ cat >> /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 >> 1 > > Oh, I found this very confusing. > > Previously features/ directory just shows what we have (either in kernel > support or the specified fs), which is very straightforward. > > Changing it to 0/1 is way less easy to understand, and can be considered > as big behavior change. > > Is it really no way to change the fs' features? Furthermore, we have something left already in the sysfs.c, btrfs_sysfs_feature_update() to do the work. I'm working on a patch to revive the behavior, which is working fine so far in my environment. I'll address all the concerns (mostly related to the context) to make it work as expected. Thanks, Qu > > Thanks, > Qu >> >> A fstests test case will follow. >> >> The source code changes involve removing the visible function pointer for >> the btrfs_feature_attr_group, as it is an optional feature. And the >> store/show part for the same is already implemented. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/sysfs.c | 23 ----------------------- >> 1 file changed, 23 deletions(-) >> >> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c >> index 45615ce36498..fa3354f8213f 100644 >> --- a/fs/btrfs/sysfs.c >> +++ b/fs/btrfs/sysfs.c >> @@ -256,28 +256,6 @@ static ssize_t btrfs_feature_attr_store(struct >> kobject *kobj, >> return count; >> } >> -static umode_t btrfs_feature_visible(struct kobject *kobj, >> - struct attribute *attr, int unused) >> -{ >> - struct btrfs_fs_info *fs_info = to_fs_info(kobj); >> - umode_t mode = attr->mode; >> - >> - if (fs_info) { >> - struct btrfs_feature_attr *fa; >> - u64 features; >> - >> - fa = attr_to_btrfs_feature_attr(attr); >> - features = get_features(fs_info, fa->feature_set); >> - >> - if (can_modify_feature(fa)) >> - mode |= S_IWUSR; >> - else if (!(features & fa->feature_bit)) >> - mode = 0; >> - } >> - >> - return mode; >> -} >> - >> BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL); >> BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS); >> BTRFS_FEAT_ATTR_INCOMPAT(compress_lzo, COMPRESS_LZO); >> @@ -335,7 +313,6 @@ static struct attribute >> *btrfs_supported_feature_attrs[] = { >> static const struct attribute_group btrfs_feature_attr_group = { >> .name = "features", >> - .is_visible = btrfs_feature_visible, >> .attrs = btrfs_supported_feature_attrs, >> };
On 12/01/2023 10:35, Qu Wenruo wrote: > > > On 2023/1/11 15:05, Qu Wenruo wrote: >> >> >> On 2023/1/11 13:40, Anand Jain wrote: >>> When we change runtime features, the sysfs under >>> /sys/fs/btrfs/<uuid>/features >>> render stale. >>> >>> For example: (before) >>> >>> $ btrfs filesystem df /btrfs >>> Data, single: total=8.00MiB, used=0.00B >>> System, DUP: total=8.00MiB, used=16.00KiB >>> Metadata, DUP: total=51.19MiB, used=128.00KiB >>> global reserve, single: total=3.50MiB, used=0.00B >>> >>> $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ >>> extended_iref free_space_tree no_holes skinny_metadata >>> >>> Use balance to convert from single/dup to RAID5 profile. >>> >>> $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs >>> >>> Still, sysfs is unaware of raid5. >>> >>> $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ >>> extended_iref free_space_tree no_holes skinny_metadata >>> >>> Which doesn't match superblock >>> >>> $ btrfs in dump-super /dev/loop0 >>> >>> incompat_flags 0x3e1 >>> ( MIXED_BACKREF | >>> BIG_METADATA | >>> EXTENDED_IREF | >>> RAID56 | >>> SKINNY_METADATA | >>> NO_HOLES ) >>> >>> Require mount-recycle as a workaround. >>> >>> Fix this by laying out all attributes on the sysfs at mount time. >>> However, >>> return 0 or 1 when read, for used or unused, respectively. >>> >>> For example: (after) >>> >>> $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ >>> block_group_tree compress_zstd extended_iref free_space_tree >>> mixed_groups raid1c34 skinny_metadata zoned >>> compress_lzo default_subvol extent_tree_v2 metadata_uuid no_holes >>> raid56 verity >>> >>> $ cat >>> /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 >>> 0 >>> >>> $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs >>> >>> $ cat >>> /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 >>> 1 >> >> Oh, I found this very confusing. >> >> Previously features/ directory just shows what we have (either in >> kernel support or the specified fs), which is very straightforward. >> >> Changing it to 0/1 is way less easy to understand, and can be >> considered as big behavior change. >> >> Is it really no way to change the fs' features? Sysfs files (attributes) design doesn't support dynamically altering their presence though it is intuitive. Another option could be to deprecate the /sys/fs/btrfs/uuid/features directory (kobject) and create /sys/fs/btrfs/uuid/running_features as a file (attribute) to show all features in plain text. > Furthermore, we have something left already in the sysfs.c, > btrfs_sysfs_feature_update() to do the work. > > I'm working on a patch to revive the behavior, which is working fine so > far in my environment. > > I'll address all the concerns (mostly related to the context) to make it > work as expected. > Hmm. It should be ok, but I am afraid it would be too pervasive, as we have many dynamic features. I am happy to review your patch when ready. Otherwise, I have a patch to clean the unused btrfs_sysfs_feature_update() function ;-). I will hold it for now.
On 2023/1/12 14:08, Anand Jain wrote: > On 12/01/2023 10:35, Qu Wenruo wrote: [...] >>> Oh, I found this very confusing. >>> >>> Previously features/ directory just shows what we have (either in >>> kernel support or the specified fs), which is very straightforward. >>> >>> Changing it to 0/1 is way less easy to understand, and can be >>> considered as big behavior change. >>> >>> Is it really no way to change the fs' features? > > Sysfs files (attributes) design doesn't support dynamically altering > their presence though it is intuitive. > > Another option could be to deprecate the /sys/fs/btrfs/uuid/features > directory (kobject) and create /sys/fs/btrfs/uuid/running_features > as a file (attribute) to show all features in plain text. Nope, the attribute group for mounted fs "features" has the .is_visible() hook to determine which files are visible (aka, created and deleted). The call sysfs_update_group() has explicit comments for it: * Furthermore, * if the visibility of the files has changed through the is_visible() * callback, it will update the permissions and add or remove the * relevant files. Changing a group's name (subdirectory name under * kobj's directory in sysfs) is not allowed. Thus it's a perfect match for our usage. Thanks, Qu > >> Furthermore, we have something left already in the sysfs.c, >> btrfs_sysfs_feature_update() to do the work. >> >> I'm working on a patch to revive the behavior, which is working fine >> so far in my environment. >> >> I'll address all the concerns (mostly related to the context) to make >> it work as expected. >> > > Hmm. It should be ok, but I am afraid it would be too pervasive, as we > have many dynamic features. I am happy to review your patch when ready. > > Otherwise, I have a patch to clean the unused > btrfs_sysfs_feature_update() function ;-). I will hold it for now.
On Wed, Jan 11, 2023 at 03:05:45PM +0800, Qu Wenruo wrote: > On 2023/1/11 13:40, Anand Jain wrote: > > When we change runtime features, the sysfs under > > /sys/fs/btrfs/<uuid>/features > > render stale. > > > > For example: (before) > > > > $ btrfs filesystem df /btrfs > > Data, single: total=8.00MiB, used=0.00B > > System, DUP: total=8.00MiB, used=16.00KiB > > Metadata, DUP: total=51.19MiB, used=128.00KiB > > global reserve, single: total=3.50MiB, used=0.00B > > > > $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ > > extended_iref free_space_tree no_holes skinny_metadata > > > > Use balance to convert from single/dup to RAID5 profile. > > > > $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs > > > > Still, sysfs is unaware of raid5. > > > > $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ > > extended_iref free_space_tree no_holes skinny_metadata > > > > Which doesn't match superblock > > > > $ btrfs in dump-super /dev/loop0 > > > > incompat_flags 0x3e1 > > ( MIXED_BACKREF | > > BIG_METADATA | > > EXTENDED_IREF | > > RAID56 | > > SKINNY_METADATA | > > NO_HOLES ) > > > > Require mount-recycle as a workaround. > > > > Fix this by laying out all attributes on the sysfs at mount time. However, > > return 0 or 1 when read, for used or unused, respectively. > > > > For example: (after) > > > > $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ > > block_group_tree compress_zstd extended_iref free_space_tree mixed_groups raid1c34 skinny_metadata zoned > > compress_lzo default_subvol extent_tree_v2 metadata_uuid no_holes raid56 verity > > > > $ cat /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 > > 0 > > > > $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs > > > > $ cat /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 > > 1 > > Oh, I found this very confusing. > > Previously features/ directory just shows what we have (either in kernel > support or the specified fs), which is very straightforward. > > Changing it to 0/1 is way less easy to understand, and can be considered > as big behavior change. Yes, the sysfs files are considered an ABI though we can do changes that are not backward compatible. The semantics of the features file is that if it exists then it's also in the filesystem and tests depend on that.
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 45615ce36498..fa3354f8213f 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -256,28 +256,6 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj, return count; } -static umode_t btrfs_feature_visible(struct kobject *kobj, - struct attribute *attr, int unused) -{ - struct btrfs_fs_info *fs_info = to_fs_info(kobj); - umode_t mode = attr->mode; - - if (fs_info) { - struct btrfs_feature_attr *fa; - u64 features; - - fa = attr_to_btrfs_feature_attr(attr); - features = get_features(fs_info, fa->feature_set); - - if (can_modify_feature(fa)) - mode |= S_IWUSR; - else if (!(features & fa->feature_bit)) - mode = 0; - } - - return mode; -} - BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL); BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS); BTRFS_FEAT_ATTR_INCOMPAT(compress_lzo, COMPRESS_LZO); @@ -335,7 +313,6 @@ static struct attribute *btrfs_supported_feature_attrs[] = { static const struct attribute_group btrfs_feature_attr_group = { .name = "features", - .is_visible = btrfs_feature_visible, .attrs = btrfs_supported_feature_attrs, };
When we change runtime features, the sysfs under /sys/fs/btrfs/<uuid>/features render stale. For example: (before) $ btrfs filesystem df /btrfs Data, single: total=8.00MiB, used=0.00B System, DUP: total=8.00MiB, used=16.00KiB Metadata, DUP: total=51.19MiB, used=128.00KiB global reserve, single: total=3.50MiB, used=0.00B $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ extended_iref free_space_tree no_holes skinny_metadata Use balance to convert from single/dup to RAID5 profile. $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs Still, sysfs is unaware of raid5. $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ extended_iref free_space_tree no_holes skinny_metadata Which doesn't match superblock $ btrfs in dump-super /dev/loop0 incompat_flags 0x3e1 ( MIXED_BACKREF | BIG_METADATA | EXTENDED_IREF | RAID56 | SKINNY_METADATA | NO_HOLES ) Require mount-recycle as a workaround. Fix this by laying out all attributes on the sysfs at mount time. However, return 0 or 1 when read, for used or unused, respectively. For example: (after) $ ls /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/ block_group_tree compress_zstd extended_iref free_space_tree mixed_groups raid1c34 skinny_metadata zoned compress_lzo default_subvol extent_tree_v2 metadata_uuid no_holes raid56 verity $ cat /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 0 $ btrfs balance start -f -dconvert=raid5 -mconvert=raid5 /btrfs $ cat /sys/fs/btrfs/d5ccbf34-bb40-4b4c-af62-8c6c8226f1b7/features/raid56 1 A fstests test case will follow. The source code changes involve removing the visible function pointer for the btrfs_feature_attr_group, as it is an optional feature. And the store/show part for the same is already implemented. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/sysfs.c | 23 ----------------------- 1 file changed, 23 deletions(-)