[PATCH 0/4] update sysfs per btrfs device operations

[PATCH 0/4] update sysfs per btrfs device operations

[Anand Jain]

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

[PATCH 1/4] btrfs: dev delete should remove sysfs entry

[Anand Jain]

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

[PATCH 2/4] btrfs: dev add should add its sysfs entry

[Anand Jain]

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

[PATCH 3/4] btrfs: dev replace should replace the sysfs entry

[Anand Jain]

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

[PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well

[Anand Jain]

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

[PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices

[Anand Jain]

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

Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well

[David Sterba]

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.

Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry

[David Sterba]

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;

Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry

[David Sterba]

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.

Re: [PATCH 2/4] btrfs: dev add should add its sysfs entry

[David Sterba]

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.

Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry

[Anand Jain]

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
>

Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry

[Anand Jain]

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
>

Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry

[David Sterba]

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.

Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well

[Anand Jain]

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
>

Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well

[David Sterba]

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.

Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well

[Anand Jain]

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
>

Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry

[Anand Jain]

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

Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry

[David Sterba]

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.