btrfs: sysfs: Add entry which shows rmdir(2) can work for subvolume

PATCH Superseded Tomohiro Misono 2 participants 6 messages May 16, 2018 – May 17, 2018 Btrfs file system

lore

Share & tools

lore

Tomohiro Misono

author

2018-05-16 08:06 UTC #0 lore raw

v2 cover btrfs: sysfs: Add entry which shows rmdir(2) can work for subvolume

[based on current misc-next]

-----
changelog
v1 -> v2
- Explicitly set start of enum btrfs_feature_set as 0 in 1st patch
- Create new sysfs group which shows static feature (i.e. features which only depends on kernel version). See 2nd path.
-----

This adds new sysfs entry
/sys/fs/btrfs/static_features/rmdir_subvol
to indicate that the kernel can delete a subvolume by rmdir(2), which is allowed by: https://www.spinics.net/lists/linux-btrfs/msg76938.html

The aim of adding a entry is to enable a user program (e.g. xfstest) to detect a feature.

The first patch is a cleanup and the second one is a main part.

Tomohiro Misono (2):
btrfs: sysfs: Use enum/define value intead of magic number
btrfs: sysfs: Add entry which shows rmdir(2) can work for subvolume

 fs/btrfs/sysfs.c | 40 +++++++++++++++++++++++++++++++++++-----
 fs/btrfs/sysfs.h |  4 ++--
 2 files changed, 37 insertions(+), 7 deletions(-)

Signature · 1 line

2.14.3

Tomohiro Misono

author

2018-05-16 08:06 UTC #1 lore raw

PATCH v2 1/2 btrfs: sysfs: Use enum/define value intead of magic number

Signed-off-by Tomohiro Misono <[email protected]>

---
 fs/btrfs/sysfs.c | 11 ++++++-----
 fs/btrfs/sysfs.h |  4 ++--
 2 files changed, 8 insertions(+), 7 deletions(-)

fs/btrfs/sysfs.c +6 -5

index fa6c8c88b250..217d401fe8ae 100644
@@ -514,10 +514,11 @@ static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj)
 }
 
 #define NUM_FEATURE_BITS 64
-static char btrfs_unknown_feature_names[3][NUM_FEATURE_BITS][13];
-static struct btrfs_feature_attr btrfs_feature_attrs[3][NUM_FEATURE_BITS];
+#define BTRFS_FEATURE_NAME_MAX 13
+static char btrfs_unknown_feature_names[FEAT_MAX][NUM_FEATURE_BITS][BTRFS_FEATURE_NAME_MAX];
+static struct btrfs_feature_attr btrfs_feature_attrs[FEAT_MAX][NUM_FEATURE_BITS];
 
-static const u64 supported_feature_masks[3] = {
+static const u64 supported_feature_masks[FEAT_MAX] = {
 	[FEAT_COMPAT]    = BTRFS_FEATURE_COMPAT_SUPP,
 	[FEAT_COMPAT_RO] = BTRFS_FEATURE_COMPAT_RO_SUPP,
 	[FEAT_INCOMPAT]  = BTRFS_FEATURE_INCOMPAT_SUPP,

@@ -609,7 +610,7 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 	btrfs_sysfs_rm_device_link(fs_info->fs_devices, NULL);
 }
 
-const char * const btrfs_feature_set_names[3] = {
+const char * const btrfs_feature_set_names[FEAT_MAX] = {
 	[FEAT_COMPAT]	 = "compat",
 	[FEAT_COMPAT_RO] = "compat_ro",
 	[FEAT_INCOMPAT]	 = "incompat",

@@ -673,7 +674,7 @@ static void init_feature_attrs(void)
 			if (fa->kobj_attr.attr.name)
 				continue;
 
-			snprintf(name, 13, "%s:%u",
+			snprintf(name, BTRFS_FEATURE_NAME_MAX, "%s:%u",
 				 btrfs_feature_set_names[set], i);
 
 			fa->kobj_attr.attr.name = name;

fs/btrfs/sysfs.h +2 -2

index b567560d9aa9..c6ee600aff89 100644
@@ -9,7 +9,7 @@
 extern u64 btrfs_debugfs_test;
 
 enum btrfs_feature_set {
-	FEAT_COMPAT,
+	FEAT_COMPAT = 0,
 	FEAT_COMPAT_RO,
 	FEAT_INCOMPAT,
 	FEAT_MAX

@@ -77,7 +77,7 @@ attr_to_btrfs_feature_attr(struct attribute *attr)
 }
 
 char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
-extern const char * const btrfs_feature_set_names[3];
+extern const char * const btrfs_feature_set_names[FEAT_MAX];
 extern struct kobj_type space_info_ktype;
 extern struct kobj_type btrfs_raid_ktype;
 int btrfs_sysfs_add_device_link(struct btrfs_fs_devices *fs_devices,

Signature · 1 line

2.14.3

Tomohiro Misono

author

2018-05-16 08:06 UTC #2 lore raw

PATCH v2 2/2 btrfs: sysfs: Add entry which shows rmdir(2) can work for subvolume

In reply to Tomohiro Misono

Deletion of a subvolume by rmdir(2) has become allowed by the 'commit cd2decf640b1 ("btrfs: Allow rmdir(2) to delete an empty subvolume")'.

It is a kind of new feature and this commits add a sysfs entry
/sys/fs/btrfs/static_features/rmdir_subvol
to indicate the availability of feature so that a user program (e.g. xfstests) can detect it.

Note that new sysfs directory "static_features" is created since a entry in /sys/fs/btrfs/features depends on feature bits of superblock (in other words, they may be different between each fs) and is not suitable to hold the features which only depend on kernel version. New attribute_group "btrfs_static_feature_attr_group" is created for this purpose.

Signed-off-by Tomohiro Misono <[email protected]>

---
 fs/btrfs/sysfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

fs/btrfs/sysfs.c +29 -0

index 217d401fe8ae..35b3ac567899 100644
@@ -210,12 +210,32 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
 	NULL
 };
 
+/* Features which depend on feature bits and may differ between each fs */
 static const struct attribute_group btrfs_feature_attr_group = {
 	.name = "features",
 	.is_visible = btrfs_feature_visible,
 	.attrs = btrfs_supported_feature_attrs,
 };
 
+static ssize_t rmdir_subvol_show(struct kobject *kobj,
+				 struct kobj_attribute *ka, char *buf)
+{
+	/* No meaning for the value */
+	return snprintf(buf, PAGE_SIZE, "0\n");
+}
+BTRFS_ATTR(static_feature, rmdir_subvol, rmdir_subvol_show);
+
+static struct attribute *btrfs_supported_static_feature_attrs[] = {
+	BTRFS_ATTR_PTR(static_feature, rmdir_subvol),
+	NULL
+};
+
+/* Features which only depend on kernel version */
+static const struct attribute_group btrfs_static_feature_attr_group = {
+	.name = "static_features",
+	.attrs = btrfs_supported_static_feature_attrs,
+};
+
 static ssize_t btrfs_show_u64(u64 *value_ptr, spinlock_t *lock, char *buf)
 {
 	u64 val;

@@ -901,8 +921,15 @@ int __init btrfs_init_sysfs(void)
 	ret = sysfs_create_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
 	if (ret)
 		goto out2;
+	ret = sysfs_create_group(&btrfs_kset->kobj,
+				 &btrfs_static_feature_attr_group);
+	if (ret)
+		goto out3;
 
 	return 0;
+
+out3:
+	sysfs_remove_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
 out2:
 	debugfs_remove_recursive(btrfs_debugfs_root_dentry);
 out1:

@@ -914,6 +941,8 @@ int __init btrfs_init_sysfs(void)
 void __cold btrfs_exit_sysfs(void)
 {
 	sysfs_remove_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
+	sysfs_remove_group(&btrfs_kset->kobj,
+			   &btrfs_static_feature_attr_group);
 	kset_unregister(btrfs_kset);
 	debugfs_remove_recursive(btrfs_debugfs_root_dentry);
 }

Signature · 1 line

2.14.3

David Sterba

2018-05-16 15:21 UTC #3 lore raw

Quoted 7 lines · Tomohiro Misono

Deletion of a subvolume by rmdir(2) has become allowed by the 'commit cd2decf640b1 ("btrfs: Allow rmdir(2) to delete an empty subvolume")'.
It is a kind of new feature and this commits add a sysfs entry
/sys/fs/btrfs/static_features/rmdir_subvol

Quoted 8 lines

to indicate the availability of feature so that a user program (e.g. xfstests) can detect it.
Note that new sysfs directory "static_features" is created since a entry in /sys/fs/btrfs/features depends on feature bits of superblock (in other words, they may be different between each fs) and is not suitable to hold the features which only depend on kernel version. New attribute_group "btrfs_static_feature_attr_group" is created for this purpose.

No, we don't want to add another directory for that, please use '/sys/fs/btrfs/features'. Listing in this directory reflects capabilities of the kernel module, the filesystem specific features are in the /sys/fs/btrfs/UUID/features directory.

Misono Tomohiro

author

2018-05-17 05:27 UTC #4 lore raw

PATCH v3 2/2 btrfs: sysfs: Add entry which shows rmdir(2) can work for subvolume

Deletion of a subvolume by rmdir(2) has become allowed by the 'commit cd2decf640b1 ("btrfs: Allow rmdir(2) to delete an empty subvolume")'.

It is a kind of new feature and this commits add a sysfs entry
/sys/fs/btrfs/features/rmdir_subvol
to indicate the availability of the feature so that a user program (e.g. xfstests) can detect it.

Prior to this commit, all entry in /sys/fs/btrfs/features is a feature which depends on feature bits of superblock (i.e. each feature affects on-disk format) and managed by attribute_group "btrfs_feature_attr_group". For each fs, entries in /sys/fs/btrfs/UUID/features indicate which features are enabled (or can be changed online) for the fs.

However, rmdir_subvol feature only depends on kernel modules. Therefore new attribute_group "btrfs_static_feature_attr_group" is introduced and sysfs_merge_group() is used to share /sys/fs/btrfs/features directory. Features in "btrfs_static_feature_attr_group" won't be listed in each /sys/fs/btrfs/UUID/features.

Signed-off-by Tomohiro Misono <[email protected]>

---
Hi David,

Sorry for the misunderstanding.
How about this version which uses /sys/fs/btrfs/features while using different attribute_group for static features.

Thanks,

 fs/btrfs/sysfs.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

fs/btrfs/sysfs.c +37 -0

index 217d401fe8ae..ae6003b34c76 100644
@@ -210,12 +210,40 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
 	NULL
 };
 
+/*
+ * Features which depend on feature bits and may differ between each fs.
+ * /sys/fs/btrfs/features lists all capable features of this kernel
+ * while /sys/fs/btrfs/UUID/features shows features of the fs which are
+ * enabled or can be changed online
+ */
 static const struct attribute_group btrfs_feature_attr_group = {
 	.name = "features",
 	.is_visible = btrfs_feature_visible,
 	.attrs = btrfs_supported_feature_attrs,
 };
 
+static ssize_t rmdir_subvol_show(struct kobject *kobj,
+				 struct kobj_attribute *ka, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0\n");
+}
+BTRFS_ATTR(static_feature, rmdir_subvol, rmdir_subvol_show);
+
+static struct attribute *btrfs_supported_static_feature_attrs[] = {
+	BTRFS_ATTR_PTR(static_feature, rmdir_subvol),
+	NULL
+};
+
+/*
+ * Features which only depend on a kernel version.
+ * These are listed in /sys/fs/btrfs/features along with
+ * btrfs_feature_attr_group
+ */
+static const struct attribute_group btrfs_static_feature_attr_group = {
+	.name = "features",
+	.attrs = btrfs_supported_static_feature_attrs,
+};
+
 static ssize_t btrfs_show_u64(u64 *value_ptr, spinlock_t *lock, char *buf)
 {
 	u64 val;

@@ -901,8 +929,15 @@ int __init btrfs_init_sysfs(void)
 	ret = sysfs_create_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
 	if (ret)
 		goto out2;
+	ret = sysfs_merge_group(&btrfs_kset->kobj,
+				&btrfs_static_feature_attr_group);
+	if (ret)
+		goto out3;
 
 	return 0;
+
+out3:
+	sysfs_remove_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
 out2:
 	debugfs_remove_recursive(btrfs_debugfs_root_dentry);
 out1:

@@ -913,6 +948,8 @@ int __init btrfs_init_sysfs(void)
 
 void __cold btrfs_exit_sysfs(void)
 {
+	sysfs_unmerge_group(&btrfs_kset->kobj,
+			    &btrfs_static_feature_attr_group);
 	sysfs_remove_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
 	kset_unregister(btrfs_kset);
 	debugfs_remove_recursive(btrfs_debugfs_root_dentry);

Signature · 1 line

2.14.3

David Sterba

Reviewed-by on v3 2/2

2018-05-17 16:52 UTC #5 lore raw

Quoted 21 lines · Misono Tomohiro

Deletion of a subvolume by rmdir(2) has become allowed by the 'commit cd2decf640b1 ("btrfs: Allow rmdir(2) to delete an empty subvolume")'.
It is a kind of new feature and this commits add a sysfs entry
/sys/fs/btrfs/features/rmdir_subvol
to indicate the availability of the feature so that a user program (e.g. xfstests) can detect it.
Prior to this commit, all entry in /sys/fs/btrfs/features is a feature which depends on feature bits of superblock (i.e. each feature affects on-disk format) and managed by attribute_group "btrfs_feature_attr_group". For each fs, entries in /sys/fs/btrfs/UUID/features indicate which features are enabled (or can be changed online) for the fs.
However, rmdir_subvol feature only depends on kernel modules. Therefore new attribute_group "btrfs_static_feature_attr_group" is introduced and sysfs_merge_group() is used to share /sys/fs/btrfs/features directory. Features in "btrfs_static_feature_attr_group" won't be listed in each /sys/fs/btrfs/UUID/features.

Quoted 5 lines

Signed-off-by Tomohiro Misono <[email protected]>
---
Hi David,
Sorry for the misunderstanding.

No problem.

Quoted 2 lines

How about this version which uses /sys/fs/btrfs/features while using different attribute_group for static features.

Perfect, that's what I had in mind, added to misc-next, thanks.

Reviewed-by David Sterba <[email protected]>