linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] update sysfs per btrfs device operations
@ 2014-05-26  9:30 Anand Jain
  2014-05-26  9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Anand Jain @ 2014-05-26  9:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: jeffm, dsterba, clm

This patch set fixes the bugs which Jeff patch is fixing,
which is to update sysfs when device is added and removed.

Further, this patch set also address the following.
  - Update sysfs path when device is replaced
  - Update sysfs path when sprout is created

Also mainly this patch makes the code more modular
which will support the enhancement like in the RFC
sent before.
   [PATCH RFC] btrfs: revamp /sys/fs/btrfs/<fsid>/devices

Thanks.


Anand Jain (4):
  btrfs: dev delete should remove sysfs entry
  btrfs: dev add should add its sysfs entry
  btrfs: dev replace should replace the sysfs entry
  btrfs: create sprout should rename fsid on the sysfs as well

 fs/btrfs/dev-replace.c |    5 +++++
 fs/btrfs/sysfs.c       |   32 +++++++++++++++++++++++++++++---
 fs/btrfs/sysfs.h       |    4 ++++
 fs/btrfs/volumes.c     |   17 +++++++++++++++++
 4 files changed, 55 insertions(+), 3 deletions(-)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/4] btrfs: dev delete should remove sysfs entry
  2014-05-26  9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
@ 2014-05-26  9:30 ` Anand Jain
  2014-05-29 13:04   ` David Sterba
  2014-05-26  9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26  9:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: jeffm, dsterba, clm

From: Anand Jain <Anand.Jain@oracle.com>

when we delete the device from the mounted btrfs,
we would need its corresponding sysfs enty to
be removed as well.

Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
 fs/btrfs/sysfs.c   |   20 ++++++++++++++++++++
 fs/btrfs/sysfs.h   |    2 ++
 fs/btrfs/volumes.c |    4 ++++
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 522d023..4fe3f0b 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -572,6 +572,26 @@ static void init_feature_attrs(void)
 	}
 }
 
+int rm_device_membership(struct btrfs_fs_info *fs_info,
+		struct btrfs_device *one_device)
+{
+	struct hd_struct *disk;
+	struct kobject *disk_kobj;
+
+	if (!fs_info->device_dir_kobj)
+		return -EINVAL;
+
+	if (one_device) {
+		disk = one_device->bdev->bd_part;
+		disk_kobj = &part_to_dev(disk)->kobj;
+
+		sysfs_remove_link(fs_info->device_dir_kobj,
+						disk_kobj->name);
+	}
+
+	return 0;
+}
+
 static int add_device_membership(struct btrfs_fs_info *fs_info)
 {
 	int error = 0;
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index 9ab5763..eaed810 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -66,4 +66,6 @@ char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
 extern const char * const btrfs_feature_set_names[3];
 extern struct kobj_type space_info_ktype;
 extern struct kobj_type btrfs_raid_ktype;
+int rm_device_membership(struct btrfs_fs_info *fs_info,
+                struct btrfs_device *one_device);
 #endif /* _BTRFS_SYSFS_H_ */
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 15a8934..3ceb28c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -40,6 +40,7 @@
 #include "rcu-string.h"
 #include "math.h"
 #include "dev-replace.h"
+#include "sysfs.h"
 
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -1651,6 +1652,9 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	if (device->bdev)
 		device->fs_devices->open_devices--;
 
+	/* remove sysfs entry */
+	rm_device_membership(root->fs_info, device);
+
 	call_rcu(&device->rcu, free_device);
 
 	num_devices = btrfs_super_num_devices(root->fs_info->super_copy) - 1;
-- 
1.7.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 2/4] btrfs: dev add should add its sysfs entry
  2014-05-26  9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
  2014-05-26  9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
@ 2014-05-26  9:30 ` Anand Jain
  2014-05-29 14:49   ` David Sterba
  2014-05-26  9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26  9:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: jeffm, dsterba, clm

From: Anand Jain <Anand.Jain@oracle.com>

we would need the device links to be created,
when device is added.

Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
 fs/btrfs/sysfs.c   |   12 +++++++++---
 fs/btrfs/sysfs.h   |    2 ++
 fs/btrfs/volumes.c |    5 +++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4fe3f0b..0f2ca33 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -592,14 +592,17 @@ int rm_device_membership(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static int add_device_membership(struct btrfs_fs_info *fs_info)
+int add_device_membership(struct btrfs_fs_info *fs_info,
+		struct btrfs_device *one_device)
 {
 	int error = 0;
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	struct btrfs_device *dev;
 
-	fs_info->device_dir_kobj = kobject_create_and_add("devices",
+	if (!fs_info->device_dir_kobj)
+		fs_info->device_dir_kobj = kobject_create_and_add("devices",
 						&fs_info->super_kobj);
+
 	if (!fs_info->device_dir_kobj)
 		return -ENOMEM;
 
@@ -610,6 +613,9 @@ static int add_device_membership(struct btrfs_fs_info *fs_info)
 		if (!dev->bdev)
 			continue;
 
+		if (one_device && one_device != dev)
+			continue;
+
 		disk = dev->bdev->bd_part;
 		disk_kobj = &part_to_dev(disk)->kobj;
 
@@ -653,7 +659,7 @@ int btrfs_sysfs_add_one(struct btrfs_fs_info *fs_info)
 	if (error)
 		goto failure;
 
-	error = add_device_membership(fs_info);
+	error = add_device_membership(fs_info, NULL);
 	if (error)
 		goto failure;
 
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index eaed810..2de1314 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -66,6 +66,8 @@ char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
 extern const char * const btrfs_feature_set_names[3];
 extern struct kobj_type space_info_ktype;
 extern struct kobj_type btrfs_raid_ktype;
+int add_device_membership(struct btrfs_fs_info *fs_info,
+		struct btrfs_device *one_device);
 int rm_device_membership(struct btrfs_fs_info *fs_info,
                 struct btrfs_device *one_device);
 #endif /* _BTRFS_SYSFS_H_ */
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3ceb28c..5a577ab 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2077,6 +2077,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 	total_bytes = btrfs_super_num_devices(root->fs_info->super_copy);
 	btrfs_set_super_num_devices(root->fs_info->super_copy,
 				    total_bytes + 1);
+
+	/* add sysfs device entry */
+	add_device_membership(root->fs_info, device);
+
 	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
 
 	if (seeding_dev) {
@@ -2137,6 +2141,7 @@ error_trans:
 	unlock_chunks(root);
 	btrfs_end_transaction(trans, root);
 	rcu_string_free(device->name);
+	rm_device_membership(root->fs_info, device);
 	kfree(device);
 error:
 	blkdev_put(bdev, FMODE_EXCL);
-- 
1.7.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
  2014-05-26  9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
  2014-05-26  9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
  2014-05-26  9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
@ 2014-05-26  9:30 ` Anand Jain
  2014-05-29 13:29   ` David Sterba
  2014-05-26  9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
  2014-05-28  8:30 ` [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices Anand Jain
  4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26  9:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: jeffm, dsterba, clm

when we replace the device its corresponding sysfs
entry has to be replaced as well

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 9f22905..f4f8728 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -36,6 +36,7 @@
 #include "check-integrity.h"
 #include "rcu-string.h"
 #include "dev-replace.h"
+#include "sysfs.h"
 
 static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 				       int scrub_ret);
@@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
 	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
 
+	/* replace the sysfs entry */
+	rm_device_membership(fs_info, src_device);
+	add_device_membership(fs_info, tgt_device);
+
 	btrfs_rm_dev_replace_blocked(fs_info);
 
 	btrfs_rm_dev_replace_srcdev(fs_info, src_device);
-- 
1.7.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
  2014-05-26  9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
                   ` (2 preceding siblings ...)
  2014-05-26  9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
@ 2014-05-26  9:30 ` Anand Jain
  2014-05-29 12:54   ` David Sterba
  2014-05-28  8:30 ` [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices Anand Jain
  4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26  9:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: jeffm, dsterba, clm

Creating sprout will change the fsid of the mounted root.
do the same on the sysfs as well.

reproducer:
 mount /dev/sdb /btrfs (seed disk)
 btrfs dev add /dev/sdc /btrfs
 mount -o rw,remount /btrfs
 btrfs dev del /dev/sdb /btrfs
 mount /dev/sdb /btrfs

Error:
kobject_add_internal failed for fe350492-dc28-4051-a601-e017b17e6145 with -EEXIST, don't try to register things with the same name in the same directory.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5a577ab..e6cf3a7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
 
 	if (seeding_dev) {
+		char fsid_buf[37];
 		ret = init_first_rw_device(trans, root, device);
 		if (ret) {
 			btrfs_abort_transaction(trans, root, ret);
@@ -2094,6 +2095,13 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 			btrfs_abort_transaction(trans, root, ret);
 			goto error_trans;
 		}
+
+		/* Sprouting would change fsid of the mounted root,
+		 * so rename the fsid on the sysfs
+		 */
+		sprintf(fsid_buf, "%pU", root->fs_info->fsid);
+		if (kobject_rename(&root->fs_info->super_kobj, fsid_buf))
+			goto error_trans;
 	} else {
 		ret = btrfs_add_device(trans, root, device);
 		if (ret) {
-- 
1.7.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices
  2014-05-26  9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
                   ` (3 preceding siblings ...)
  2014-05-26  9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
@ 2014-05-28  8:30 ` Anand Jain
  4 siblings, 0 replies; 18+ messages in thread
From: Anand Jain @ 2014-05-28  8:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: jeffm, dsterba, clm

As of now with out this patch the sysfs interface under dir
/sys/fs/btrfs/<fsid>/devices is just link to the block devs.

Moving forward we would need the above btrfs sysfs path to contain more
info about the btrfs devices. So this patch provides a framework for
the same.

And as of now a probe interface is added, which can be used to notify
btrfs for any change in the device status.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 V2: On the 2nd thought I kept the device link, but under the device name.
      eg: /sys/fs/btrfs/<fsid>/devices/<sdx>/sdx-> link to blk device
          /sys/fs/btrfs/<fsid>/devices/<sdx>/probe
      and commit update accordingly


 fs/btrfs/sysfs.c   |   73 +++++++++++++++++++++++++++++++++++++++++++++++----
 fs/btrfs/volumes.h |    2 +
 2 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 0f2ca33..a8d367c 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -31,8 +31,11 @@
 #include "transaction.h"
 #include "sysfs.h"
 #include "volumes.h"
+#include "rcu-string.h"
 
 static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj);
+int rm_device_membership(struct btrfs_fs_info *fs_info,
+		struct btrfs_device *one_device);
 
 static u64 get_features(struct btrfs_fs_info *fs_info,
 			enum btrfs_feature_set set)
@@ -490,8 +493,13 @@ void btrfs_sysfs_remove_one(struct btrfs_fs_info *fs_info)
 		kobject_del(fs_info->space_info_kobj);
 		kobject_put(fs_info->space_info_kobj);
 	}
-	kobject_del(fs_info->device_dir_kobj);
-	kobject_put(fs_info->device_dir_kobj);
+
+	if (fs_info->device_dir_kobj) {
+		rm_device_membership(fs_info, NULL);
+		kobject_del(fs_info->device_dir_kobj);
+		kobject_put(fs_info->device_dir_kobj);
+	}
+
 	addrm_unknown_feature_attrs(fs_info, false);
 	sysfs_remove_group(&fs_info->super_kobj, &btrfs_feature_attr_group);
 	__btrfs_sysfs_remove_one(fs_info);
@@ -577,21 +585,68 @@ int rm_device_membership(struct btrfs_fs_info *fs_info,
 {
 	struct hd_struct *disk;
 	struct kobject *disk_kobj;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_device *dev;
 
 	if (!fs_info->device_dir_kobj)
 		return -EINVAL;
 
 	if (one_device) {
+		if (!one_device->device_kobj.parent)
+			return -EINVAL;
+
 		disk = one_device->bdev->bd_part;
 		disk_kobj = &part_to_dev(disk)->kobj;
 
-		sysfs_remove_link(fs_info->device_dir_kobj,
+		sysfs_remove_link(&one_device->device_kobj,
 						disk_kobj->name);
+		kobject_del(&one_device->device_kobj);
+		kobject_put(&one_device->device_kobj);
+		return 0;
 	}
 
+	list_for_each_entry(dev, &fs_devices->devices, dev_list) {
+		if (!dev->device_kobj.parent)
+			continue;
+
+		disk = dev->bdev->bd_part;
+		disk_kobj = &part_to_dev(disk)->kobj;
+
+		sysfs_remove_link(&dev->device_kobj, disk_kobj->name);
+		kobject_del(&dev->device_kobj);
+		kobject_put(&dev->device_kobj);
+	}
 	return 0;
 }
 
+#define to_btrfs_device(_kobj) container_of(_kobj, struct btrfs_device, device_kobj)
+
+static ssize_t device_kobj_probe_store(struct kobject *dev_kobj,
+			struct kobj_attribute *a, const char *buf, size_t len)
+{
+	/* Fixme: Call appropriate device check status handler */
+
+        return len;
+}
+
+BTRFS_ATTR_RW(probe, 0200, NULL, device_kobj_probe_store);
+
+static struct attribute *device_kobj_attrs[] = {
+	BTRFS_ATTR_PTR(probe),
+	NULL,
+};
+
+static void device_kobj_release(struct kobject *dev_kobj)
+{
+	/* nothing to free as of now */
+}
+
+struct kobj_type device_ktype = {
+	.sysfs_ops	= &kobj_sysfs_ops,
+	.release	= device_kobj_release,
+	.default_attrs	= device_kobj_attrs,
+};
+
 int add_device_membership(struct btrfs_fs_info *fs_info,
 		struct btrfs_device *one_device)
 {
@@ -610,19 +665,25 @@ int add_device_membership(struct btrfs_fs_info *fs_info,
 		struct hd_struct *disk;
 		struct kobject *disk_kobj;
 
-		if (!dev->bdev)
+		if (!dev->bdev || dev->missing || dev->device_kobj.parent)
 			continue;
 
 		if (one_device && one_device != dev)
 			continue;
 
+		error = kobject_init_and_add(&dev->device_kobj, &device_ktype,
+				fs_info->device_dir_kobj, "%s",
+				strrchr(rcu_str_deref(dev->name), '/') + 1);
+		if (error)
+			break;
+
 		disk = dev->bdev->bd_part;
 		disk_kobj = &part_to_dev(disk)->kobj;
-
-		error = sysfs_create_link(fs_info->device_dir_kobj,
+		error = sysfs_create_link(&dev->device_kobj,
 					  disk_kobj, disk_kobj->name);
 		if (error)
 			break;
+
 	}
 
 	return error;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 25f0505..d0c9c32 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -113,6 +113,8 @@ struct btrfs_device {
 	int dev_stats_valid;
 	int dev_stats_dirty; /* counters need to be written to disk */
 	atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
+
+	struct kobject device_kobj;
 };
 
 struct btrfs_fs_devices {
-- 
1.7.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
  2014-05-26  9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
@ 2014-05-29 12:54   ` David Sterba
  2014-06-02  8:22     ` Anand Jain
  0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-05-29 12:54 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm

On Mon, May 26, 2014 at 05:30:26PM +0800, Anand Jain wrote:
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>  	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
>  
>  	if (seeding_dev) {
> +		char fsid_buf[37];

Is there a symbolic constant available? We have one in userspace, but I
can't find one for kernel, only a few locally defined.

>  		ret = init_first_rw_device(trans, root, device);
>  		if (ret) {
>  			btrfs_abort_transaction(trans, root, ret);
> @@ -2094,6 +2095,13 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>  			btrfs_abort_transaction(trans, root, ret);
>  			goto error_trans;
>  		}
> +
> +		/* Sprouting would change fsid of the mounted root,
> +		 * so rename the fsid on the sysfs
> +		 */
> +		sprintf(fsid_buf, "%pU", root->fs_info->fsid);

Would be better do use snprintf explicitly.

> +		if (kobject_rename(&root->fs_info->super_kobj, fsid_buf))
> +			goto error_trans;
>  	} else {

Otherwise ok.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry
  2014-05-26  9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
@ 2014-05-29 13:04   ` David Sterba
  2014-05-30  6:10     ` Anand Jain
  0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-05-29 13:04 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm

On Mon, May 26, 2014 at 05:30:23PM +0800, Anand Jain wrote:
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -572,6 +572,26 @@ static void init_feature_attrs(void)
>  	}
>  }
>  
> +int rm_device_membership(struct btrfs_fs_info *fs_info,
> +		struct btrfs_device *one_device)

The name is too generic for a non-static function, so it would be good
to add at least the btrfs_ prefix. Same applies to the change of
'add_device_membership' in the next patch.

As this is only a trivial change, consider this
Reviewed-by: David Sterba <dsterba@suse.cz>

> +{
> +	struct hd_struct *disk;
> +	struct kobject *disk_kobj;
> +
> +	if (!fs_info->device_dir_kobj)
> +		return -EINVAL;
> +
> +	if (one_device) {
> +		disk = one_device->bdev->bd_part;
> +		disk_kobj = &part_to_dev(disk)->kobj;
> +
> +		sysfs_remove_link(fs_info->device_dir_kobj,
> +						disk_kobj->name);
> +	}
> +
> +	return 0;
> +}
> +
>  static int add_device_membership(struct btrfs_fs_info *fs_info)
>  {
>  	int error = 0;

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
  2014-05-26  9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
@ 2014-05-29 13:29   ` David Sterba
  2014-05-30  7:40     ` Anand Jain
  0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-05-29 13:29 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm

On Mon, May 26, 2014 at 05:30:25PM +0800, Anand Jain wrote:
> when we replace the device its corresponding sysfs
> entry has to be replaced as well
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 9f22905..f4f8728 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -36,6 +36,7 @@
>  #include "check-integrity.h"
>  #include "rcu-string.h"
>  #include "dev-replace.h"
> +#include "sysfs.h"
>  
>  static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  				       int scrub_ret);
> @@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>  	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>  
> +	/* replace the sysfs entry */
> +	rm_device_membership(fs_info, src_device);
> +	add_device_membership(fs_info, tgt_device);
> +
>  	btrfs_rm_dev_replace_blocked(fs_info);
>  
>  	btrfs_rm_dev_replace_srcdev(fs_info, src_device);

569         btrfs_rm_dev_replace_unblocked(fs_info);
570

The comment that follows says

571         /*
572          * this is again a consistent state where no dev_replace procedure
573          * is running, the target device is part of the filesystem, the
574          * source device is not part of the filesystem anymore and its 1st
575          * superblock is scratched out so that it is no longer marked to
576          * belong to this filesystem.
577          */

and I think this is the right place to put the sysfs updates, because the
srcdev is processed.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] btrfs: dev add should add its sysfs entry
  2014-05-26  9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
@ 2014-05-29 14:49   ` David Sterba
  0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-05-29 14:49 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm

On Mon, May 26, 2014 at 05:30:24PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
> 
> we would need the device links to be created,
> when device is added.

Looks good (plus the btrfs_ prefix mentioned before).

Reviewed-by: David Sterba <dsterba@suse.cz>

I was comparing it to Jeff's patch and I think Anand's version is
cleaner because it uses helpers that conveniently wrap the
bdev/part_to_dev trickery.

There's one difference in btrfs_init_new_device, where the
add_membership is called, but both locations look correct to me.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry
  2014-05-29 13:04   ` David Sterba
@ 2014-05-30  6:10     ` Anand Jain
  2014-05-30 14:10       ` David Sterba
  0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-30  6:10 UTC (permalink / raw)
  To: dsterba, linux-btrfs, jeffm, clm


Thanks for the review David.

On 29/05/14 21:04, David Sterba wrote:
> On Mon, May 26, 2014 at 05:30:23PM +0800, Anand Jain wrote:
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -572,6 +572,26 @@ static void init_feature_attrs(void)
>>   	}
>>   }
>>
>> +int rm_device_membership(struct btrfs_fs_info *fs_info,
>> +		struct btrfs_device *one_device)
>
> The name is too generic for a non-static function, so it would be good
> to add at least the btrfs_ prefix. Same applies to the change of
> 'add_device_membership' in the next patch.

  Yes indeed. I wanted it to change as well. But the diff became
  unfriendly to read.  I shall do that in a separate patch.


> As this is only a trivial change, consider this
> Reviewed-by: David Sterba <dsterba@suse.cz>
>
>> +{
>> +	struct hd_struct *disk;
>> +	struct kobject *disk_kobj;
>> +
>> +	if (!fs_info->device_dir_kobj)
>> +		return -EINVAL;
>> +
>> +	if (one_device) {
>> +		disk = one_device->bdev->bd_part;
>> +		disk_kobj = &part_to_dev(disk)->kobj;
>> +
>> +		sysfs_remove_link(fs_info->device_dir_kobj,
>> +						disk_kobj->name);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int add_device_membership(struct btrfs_fs_info *fs_info)
>>   {
>>   	int error = 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
  2014-05-29 13:29   ` David Sterba
@ 2014-05-30  7:40     ` Anand Jain
  2014-06-03  3:47       ` Anand Jain
  0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-30  7:40 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs, jeffm, clm




On 29/05/14 21:29, David Sterba wrote:
> On Mon, May 26, 2014 at 05:30:25PM +0800, Anand Jain wrote:
>> when we replace the device its corresponding sysfs
>> entry has to be replaced as well
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/dev-replace.c |    5 +++++
>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 9f22905..f4f8728 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -36,6 +36,7 @@
>>   #include "check-integrity.h"
>>   #include "rcu-string.h"
>>   #include "dev-replace.h"
>> +#include "sysfs.h"
>>
>>   static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>   				       int scrub_ret);
>> @@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>   		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>>   	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>>
>> +	/* replace the sysfs entry */
>> +	rm_device_membership(fs_info, src_device);
>> +	add_device_membership(fs_info, tgt_device);
>> +
>>   	btrfs_rm_dev_replace_blocked(fs_info);
>>
>>   	btrfs_rm_dev_replace_srcdev(fs_info, src_device);
>
> 569         btrfs_rm_dev_replace_unblocked(fs_info);
> 570
>
> The comment that follows says
>
> 571         /*
> 572          * this is again a consistent state where no dev_replace procedure
> 573          * is running, the target device is part of the filesystem, the
> 574          * source device is not part of the filesystem anymore and its 1st
> 575          * superblock is scratched out so that it is no longer marked to
> 576          * belong to this filesystem.
> 577          */
>
> and I think this is the right place to put the sysfs updates, because the
> srcdev is processed.

Looking into this, will update. Thanks for the review.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry
  2014-05-30  6:10     ` Anand Jain
@ 2014-05-30 14:10       ` David Sterba
  0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-05-30 14:10 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs, jeffm, clm

On Fri, May 30, 2014 at 02:10:28PM +0800, Anand Jain wrote:
> >>@@ -572,6 +572,26 @@ static void init_feature_attrs(void)
> >>  	}
> >>  }
> >>
> >>+int rm_device_membership(struct btrfs_fs_info *fs_info,
> >>+		struct btrfs_device *one_device)
> >
> >The name is too generic for a non-static function, so it would be good
> >to add at least the btrfs_ prefix. Same applies to the change of
> >'add_device_membership' in the next patch.
> 
>  Yes indeed. I wanted it to change as well. But the diff became
>  unfriendly to read.  I shall do that in a separate patch.

No problem with doing the rename changes in a separate patch, but here
you introduce a new function, that should get the right name from the
beginning. If this would look inconsitent with the add_ counterpart,
then do the rename of add_ in advance.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
  2014-05-29 12:54   ` David Sterba
@ 2014-06-02  8:22     ` Anand Jain
  2014-06-02 15:39       ` David Sterba
  0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-06-02  8:22 UTC (permalink / raw)
  To: dsterba, linux-btrfs, jeffm, clm





On 29/05/14 20:54, David Sterba wrote:
> On Mon, May 26, 2014 at 05:30:26PM +0800, Anand Jain wrote:
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>>   	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
>>
>>   	if (seeding_dev) {
>> +		char fsid_buf[37];
>
> Is there a symbolic constant available? We have one in userspace, but I
> can't find one for kernel, only a few locally defined.

  now defined the same (as in progs) in kernel as well.

>>   		ret = init_first_rw_device(trans, root, device);
>>   		if (ret) {
>>   			btrfs_abort_transaction(trans, root, ret);
>> @@ -2094,6 +2095,13 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>>   			btrfs_abort_transaction(trans, root, ret);
>>   			goto error_trans;
>>   		}
>> +
>> +		/* Sprouting would change fsid of the mounted root,
>> +		 * so rename the fsid on the sysfs
>> +		 */
>> +		sprintf(fsid_buf, "%pU", root->fs_info->fsid);
>
> Would be better do use snprintf explicitly.

  added. Thanks for commenting.

Anand




>> +		if (kobject_rename(&root->fs_info->super_kobj, fsid_buf))
>> +			goto error_trans;
>>   	} else {
>
> Otherwise ok.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
  2014-06-02  8:22     ` Anand Jain
@ 2014-06-02 15:39       ` David Sterba
  2014-06-03  0:27         ` Anand Jain
  0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-06-02 15:39 UTC (permalink / raw)
  To: Anand Jain; +Cc: dsterba, linux-btrfs, jeffm, clm

On Mon, Jun 02, 2014 at 04:22:20PM +0800, Anand Jain wrote:
> >>--- a/fs/btrfs/volumes.c
> >>+++ b/fs/btrfs/volumes.c
> >>@@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
> >>  	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
> >>
> >>  	if (seeding_dev) {
> >>+		char fsid_buf[37];
> >
> >Is there a symbolic constant available? We have one in userspace, but I
> >can't find one for kernel, only a few locally defined.
> 
>  now defined the same (as in progs) in kernel as well.

In progs it's in utils.h

 40 #define BTRFS_UUID_UNPARSED_SIZE        37

but I don't see where it's defined in kernel. Can you please give me a
pointer?

On the other hand, progs have a local define, we can do the same, I
don't see any other potential users.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
  2014-06-02 15:39       ` David Sterba
@ 2014-06-03  0:27         ` Anand Jain
  0 siblings, 0 replies; 18+ messages in thread
From: Anand Jain @ 2014-06-03  0:27 UTC (permalink / raw)
  To: dsterba, linux-btrfs, jeffm, clm



On 02/06/2014 23:39, David Sterba wrote:
> On Mon, Jun 02, 2014 at 04:22:20PM +0800, Anand Jain wrote:
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>>>>   	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
>>>>
>>>>   	if (seeding_dev) {
>>>> +		char fsid_buf[37];
>>>
>>> Is there a symbolic constant available? We have one in userspace, but I
>>> can't find one for kernel, only a few locally defined.
>>
>>   now defined the same (as in progs) in kernel as well.
>
> In progs it's in utils.h
>
>   40 #define BTRFS_UUID_UNPARSED_SIZE        37
>
> but I don't see where it's defined in kernel. Can you please give me a
> pointer?
>
> On the other hand, progs have a local define, we can do the same, I
> don't see any other potential users.

  David,
  What I have as of now (in my workspace) is a local define in volume.h.

  but in the 2nd thought I am thinking if it is better to be at

./include/uapi/linux/btrfs.h
::
#define BTRFS_FSID_SIZE 16
#define BTRFS_UUID_SIZE 16
#define BTRFS_UUID_UNPARSED_SIZE 37 <--

  Eventually in the long run, when we clean up btrfs-progs it could
  just include ./include/uapi/linux/btrfs.h

  Also there is this driver, who has defined it but its local

#define MAXUUIDLEN  37
./drivers/staging/tidspbridge/include/dspbridge/uuidutil.h

  How do you like the idea of define at include/uapi/linux/btrfs.h
  let me know.

Thxs, Anand

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
  2014-05-30  7:40     ` Anand Jain
@ 2014-06-03  3:47       ` Anand Jain
  2014-06-03 13:39         ` David Sterba
  0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-06-03  3:47 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs, jeffm, clm

inline below.


On 30/05/2014 15:40, Anand Jain wrote:
>
>
>
> On 29/05/14 21:29, David Sterba wrote:
>> On Mon, May 26, 2014 at 05:30:25PM +0800, Anand Jain wrote:
>>> when we replace the device its corresponding sysfs
>>> entry has to be replaced as well
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/dev-replace.c |    5 +++++
>>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>> index 9f22905..f4f8728 100644
>>> --- a/fs/btrfs/dev-replace.c
>>> +++ b/fs/btrfs/dev-replace.c
>>> @@ -36,6 +36,7 @@
>>>   #include "check-integrity.h"
>>>   #include "rcu-string.h"
>>>   #include "dev-replace.h"
>>> +#include "sysfs.h"
>>>
>>>   static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>>                          int scrub_ret);
>>> @@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct
>>> btrfs_fs_info *fs_info,
>>>           fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>>>       list_add(&tgt_device->dev_alloc_list,
>>> &fs_info->fs_devices->alloc_list);
>>>
>>> +    /* replace the sysfs entry */
>>> +    rm_device_membership(fs_info, src_device);
>>> +    add_device_membership(fs_info, tgt_device);
>>> +
>>>       btrfs_rm_dev_replace_blocked(fs_info);
>>>
>>>       btrfs_rm_dev_replace_srcdev(fs_info, src_device);
>>
>> 569         btrfs_rm_dev_replace_unblocked(fs_info);
>> 570
>>
>> The comment that follows says
>>
>> 571         /*
>> 572          * this is again a consistent state where no dev_replace
>> procedure
>> 573          * is running, the target device is part of the
>> filesystem, the
>> 574          * source device is not part of the filesystem anymore and
>> its 1st
>> 575          * superblock is scratched out so that it is no longer
>> marked to
>> 576          * belong to this filesystem.
>> 577          */
>>
>> and I think this is the right place to put the sysfs updates, because the
>> srcdev is processed.
>
> Looking into this, will update. Thanks for the review.

  btrfs_rm_dev_replace_srcdev()  would destroy the btrfs_device of
  src_device, and I am removing its sys/fs entry before we destroy
  btrfs_device of src_device. Which is logically correct.

  Further, RFC like 6/6 would depend on the btrfs_device struct,
  so we have to call btrfs_kobj_rm_device() before
  btrfs_rm_dev_replace_srcdev()

  Also I did some extra tests and code walk I don't see any case
  which it would fail by calling btrfs_rm_dev_replace_srcdev()
  before btrfs_rm_dev_replace_srcdev().

  V2 for this patch-set has been sent out.

Thanks, Anand

>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
  2014-06-03  3:47       ` Anand Jain
@ 2014-06-03 13:39         ` David Sterba
  0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-06-03 13:39 UTC (permalink / raw)
  To: Anand Jain; +Cc: dsterba, linux-btrfs, jeffm, clm

On Tue, Jun 03, 2014 at 11:47:42AM +0800, Anand Jain wrote:
> >>>+    /* replace the sysfs entry */
> >>>+    rm_device_membership(fs_info, src_device);
> >>>+    add_device_membership(fs_info, tgt_device);
> >>>+
> >>>      btrfs_rm_dev_replace_blocked(fs_info);
> >>>
> >>>      btrfs_rm_dev_replace_srcdev(fs_info, src_device);
> >>
> >>569         btrfs_rm_dev_replace_unblocked(fs_info);
> >>570
> >>
> >>The comment that follows says
> >>
> >>571         /*
> >>572          * this is again a consistent state where no dev_replace
> >>procedure
> >>573          * is running, the target device is part of the
> >>filesystem, the
> >>574          * source device is not part of the filesystem anymore and
> >>its 1st
> >>575          * superblock is scratched out so that it is no longer
> >>marked to
> >>576          * belong to this filesystem.
> >>577          */
> >>
> >>and I think this is the right place to put the sysfs updates, because the
> >>srcdev is processed.
> >
> >Looking into this, will update. Thanks for the review.
> 
>  btrfs_rm_dev_replace_srcdev()  would destroy the btrfs_device of
>  src_device, and I am removing its sys/fs entry before we destroy
>  btrfs_device of src_device. Which is logically correct.

I agree, my analysis was wrong.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2014-06-03 13:39 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-26  9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
2014-05-26  9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
2014-05-29 13:04   ` David Sterba
2014-05-30  6:10     ` Anand Jain
2014-05-30 14:10       ` David Sterba
2014-05-26  9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
2014-05-29 14:49   ` David Sterba
2014-05-26  9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
2014-05-29 13:29   ` David Sterba
2014-05-30  7:40     ` Anand Jain
2014-06-03  3:47       ` Anand Jain
2014-06-03 13:39         ` David Sterba
2014-05-26  9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
2014-05-29 12:54   ` David Sterba
2014-06-02  8:22     ` Anand Jain
2014-06-02 15:39       ` David Sterba
2014-06-03  0:27         ` Anand Jain
2014-05-28  8:30 ` [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices Anand Jain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).