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

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

[Tomohiro Misono]

[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(-)

-- 
2.14.3

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

[Tomohiro Misono]

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 fs/btrfs/sysfs.c | 11 ++++++-----
 fs/btrfs/sysfs.h |  4 ++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index fa6c8c88b250..217d401fe8ae 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -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;
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index b567560d9aa9..c6ee600aff89 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -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,
-- 
2.14.3

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

[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 <misono.tomohiro@jp.fujitsu.com>
---
 fs/btrfs/sysfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 217d401fe8ae..35b3ac567899 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -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);
 }
-- 
2.14.3

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

[David Sterba]

On Wed, May 16, 2018 at 05:09:27PM +0900, Tomohiro Misono wrote:
> 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.

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.

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

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

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
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(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 217d401fe8ae..ae6003b34c76 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -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);
-- 
2.14.3

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

[David Sterba]

On Thu, May 17, 2018 at 02:24:51PM +0900, Misono Tomohiro wrote:
> 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 <misono.tomohiro@jp.fujitsu.com>
> ---
> Hi David,
> 
> Sorry for the misunderstanding.

No problem.

> 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 <dsterba@suse.com>