[PATCH 0/7] Misc volume patch set part2

[PATCH 0/7] Misc volume patch set part2

[Anand Jain]

As the last set if the pull wasn't integrated due to its pending
review corrections. Here, I have done them (not much except for adding
comments and function renames) and so sending it again. Also
this set clubs other independent patches which are in the mailing list.
I am explaining the changes of the individual patches in its change list.
And most of it still remains at v1.

This can also be pulled from:

  git@github.com:asj/btrfs-devel.git misc-next-for-kdave-16jul

Anand Jain (7):
  btrfs: drop uuid_mutex in btrfs_free_extra_devids()
  btrfs: fix race between free_stale_devices and close_fs_devices
  btrfs: do device clone using the btrfs_scan_one_device
  btrfs: use the assigned fs_devices instead of the dereference
  btrfs: warn for num_devices below 0
  btrfs: add helper btrfs_num_devices() to deduce num_devices
  btrfs: add helper function check device delete able

 fs/btrfs/volumes.c | 106 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 62 insertions(+), 44 deletions(-)

-- 
2.7.0

[PATCH v4 1/7] btrfs: drop uuid_mutex in btrfs_free_extra_devids()

[Anand Jain]

btrfs_free_extra_devids() is called only in the mount context which
traverses through the fs_devices::devices and frees the orphan devices
devices in the given %fs_devices if any. As the search for the orphan
device is limited to fs_devices::devices so we don't need the global
uuid_mutex.

There can't be any mount-point based ioctl threads in this context as
the mount thread is not yet returned. But there can be the btrfs-control
based scan ioctls thread which calls device_list_add().

Here in the mount thread the fs_devices::opened is incremented way before
btrfs_free_extra_devids() is called and in the scan context the fs_devices
which are already opened neither be freed or alloc-able at
device_list_add().

But lets say you change the device-path and call the scan again, then scan
would update the new device path and this operation could race against the
btrfs_free_extra_devids() thread, which might be in the process of
free-ing the same device. So synchronize it by using the
device_list_mutex.

This scenario is a very corner case, and practically the scan and mount
are anyway serialized by the usage so unless the race is instrumented its
very difficult to achieve.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.com>
---
v3->v4: As we traverse through the seed device, fs_device gets updated with
	the child seed fs_devices, so make sure we use the same fs_devices
	pointer for the mutex_unlock as used for the mutex_lock.
v2->v3: Update change log.
	(Currently device_list_add() is very lean on its device_list_mutex usage,
	a cleanup and fix is wip. Given the practicality of the above race
	condition this patch is good to merge).
v1->v2: replace uuid_mutex with device_list_mutex instead of delete.
	change log updated.

 fs/btrfs/volumes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 88d37bfa99c8..870c9f69a6a4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -934,8 +934,9 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 {
 	struct btrfs_device *device, *next;
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_fs_devices *parent_fs_devices = fs_devices;
 
-	mutex_lock(&uuid_mutex);
+	mutex_lock(&parent_fs_devices->device_list_mutex);
 again:
 	/* This is the initialized path, it is safe to release the devices. */
 	list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
@@ -989,8 +990,7 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 	}
 
 	fs_devices->latest_bdev = latest_dev->bdev;
-
-	mutex_unlock(&uuid_mutex);
+	mutex_unlock(&parent_fs_devices->device_list_mutex);
 }
 
 static void free_device_rcu(struct rcu_head *head)
-- 
2.7.0

[PATCH v2 2/7] btrfs: fix race between free_stale_devices and close_fs_devices

[Anand Jain]

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

%fs_devices can be free-ed by btrfs_free_stale_devices() when the
close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices
tries to access the %fs_devices again without the device_list_mutex.

Fix this by bringing the %fs_devices access with in the device_list_mutex.

Stack trace as below.

WARNING: CPU: 1 PID: 4499 at fs/btrfs/volumes.c:1071 close_fs_devices+0xbc7/0xfa0 fs/btrfs/volumes.c:1071
Kernel panic - not syncing: panic_on_warn set ...
::
RIP: 0010:close_fs_devices+0xbc7/0xfa0 fs/btrfs/volumes.c:1071
::
 btrfs_close_devices+0x29/0x150 fs/btrfs/volumes.c:1085
 open_ctree+0x589/0x7898 fs/btrfs/disk-io.c:3358
 btrfs_fill_super fs/btrfs/super.c:1202 [inline]
 btrfs_mount_root+0x16df/0x1e70 fs/btrfs/super.c:1593
 mount_fs+0xae/0x328 fs/super.c:1277
 vfs_kern_mount.part.34+0xd4/0x4d0 fs/namespace.c:1037
 vfs_kern_mount+0x40/0x60 fs/namespace.c:1027
 btrfs_mount+0x4a1/0x213e fs/btrfs/super.c:1661
 mount_fs+0xae/0x328 fs/super.c:1277

Reported-by: syzbot+ceb2606025ec1cc3479c@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: Commit log update. Satisfy checkpatch.pl. Remove HEAD..

 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 870c9f69a6a4..8450bcfed4cb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1062,13 +1062,13 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
 		btrfs_close_one_device(device);
 	}
-	mutex_unlock(&fs_devices->device_list_mutex);
-
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
 	fs_devices->opened = 0;
 	fs_devices->seeding = 0;
 
+	mutex_unlock(&fs_devices->device_list_mutex);
+
 	return 0;
 }
 
-- 
2.7.0

[PATCH 3/7] btrfs: do device clone using the btrfs_scan_one_device

[Anand Jain]

When we add a device to the RO mounted seed device, it becomes a
RW sprout FS. The following steps are used to hold the seed and
sprout fs_devices.
 (first two steps are not mandatory for the sprouting, they are there
  to ensure the seed device remains in the scanned state)
  . Clone the (mounted) fs_devices, lets call it as old_devices
  . Now add old_devices to fs_uuids (yeah, there is duplicate fsid in the
    list, as we are under uuid_mutex so its fine).

  . Alloc a new fs_devices, lets call it as seed_devices
  . Copy fs_devices into the seed_devices
  . Move fs_devices::devices into seed_devices::devices
  . Bring seed_devices to under fs_devices::seed
    (fs_devices->seed = seed_devices)
  . Assign a new FSID to the fs_devices and add the new writable device
    to the fs_devices.

This patch makes the following changes..
As we clone fs_devices to make sure the device remains scanned after the
sprouting. So use the btrfs_scan_one_device() code instead. And do it
at the end of the sprouting.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 8450bcfed4cb..c6f3f0dfbabe 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2179,7 +2179,7 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
 static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
-	struct btrfs_fs_devices *old_devices;
+	struct btrfs_fs_devices *old_fs_devices;
 	struct btrfs_fs_devices *seed_devices;
 	struct btrfs_super_block *disk_super = fs_info->super_copy;
 	struct btrfs_device *device;
@@ -2193,14 +2193,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	if (IS_ERR(seed_devices))
 		return PTR_ERR(seed_devices);
 
-	old_devices = clone_fs_devices(fs_devices);
-	if (IS_ERR(old_devices)) {
-		kfree(seed_devices);
-		return PTR_ERR(old_devices);
-	}
-
-	list_add(&old_devices->fs_list, &fs_uuids);
-
 	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
 	seed_devices->opened = 1;
 	INIT_LIST_HEAD(&seed_devices->devices);
@@ -2233,6 +2225,17 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 		      ~BTRFS_SUPER_FLAG_SEEDING;
 	btrfs_set_super_flags(disk_super, super_flags);
 
+	/*
+	 * As the above code hijacked the original seed fs_devices, now
+	 * create a new one for the original seed FSID.
+	 */
+	list_for_each_entry(device, &fs_devices->seed->devices, dev_list) {
+		if (!device->name)
+			continue;
+		btrfs_scan_one_device(device->name->str, FMODE_READ,
+				      fs_info->bdev_holder, &old_fs_devices);
+	}
+
 	return 0;
 }
 
-- 
2.7.0

[PATCH 4/7] btrfs: use the assigned fs_devices instead of the dereference

[Anand Jain]

We have assigned the %fs_info->fs_devices in %fs_devices as its not
modified just use it for the mutex_lock().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
I don't see this in the ML at all, I remember sending this. Anyway
I am marking this as V1.

 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c6f3f0dfbabe..7f4973fc2b52 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2199,7 +2199,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	INIT_LIST_HEAD(&seed_devices->alloc_list);
 	mutex_init(&seed_devices->device_list_mutex);
 
-	mutex_lock(&fs_info->fs_devices->device_list_mutex);
+	mutex_lock(&fs_devices->device_list_mutex);
 	list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices,
 			      synchronize_rcu);
 	list_for_each_entry(device, &seed_devices->devices, dev_list)
@@ -2219,7 +2219,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	generate_random_uuid(fs_devices->fsid);
 	memcpy(fs_info->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
 	memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
-	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
+	mutex_unlock(&fs_devices->device_list_mutex);
 
 	super_flags = btrfs_super_flags(disk_super) &
 		      ~BTRFS_SUPER_FLAG_SEEDING;
-- 
2.7.0

[PATCH 5/7] btrfs: warn for num_devices below 0

[Anand Jain]

In preparation to de-duplicate a section of code where we deduce the
num_devices, use warn instead of bug.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7f4973fc2b52..0f4c512aa6b4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3726,7 +3726,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	num_devices = fs_info->fs_devices->num_devices;
 	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
 	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
+		WARN_ON(num_devices < 1);
 		num_devices--;
 	}
 	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
-- 
2.7.0

[PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

When the replace is running the fs_devices::num_devices also includes
the replace device, however in some operations like device delete and
balance it needs the actual num_devices without the repalce devices, so
now the function btrfs_num_devices() just provides that.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: add comments. Thanks Nikolay.

 fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0f4c512aa6b4..1c0b56374992 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
 		fs_info->fs_devices->latest_bdev = next_device->bdev;
 }
 
+/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
+static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
+{
+	u64 num_devices = fs_info->fs_devices->num_devices;
+
+	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
+		WARN_ON(num_devices < 1);
+		num_devices--;
+	}
+	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+
+	return num_devices;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		WARN_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
 
 	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
 	if (ret)
@@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	num_devices = fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		WARN_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
+
 	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
 	if (num_devices > 1)
 		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
-- 
2.7.0

[PATCH 7/7] btrfs: add helper function check device delete able

[Anand Jain]

Move the section of the code which performs the check if the device is
indelible, move that into a helper function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: Rename function to btrfs_get_device_for_delete(), thanks
Nikolay.

 fs/btrfs/volumes.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1c0b56374992..0cefc24b028c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1859,6 +1859,33 @@ static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
 	return num_devices;
 }
 
+static struct btrfs_device *btrfs_get_device_for_delete(
+				struct btrfs_fs_info *fs_info,
+				const char *device_path, u64 devid)
+{
+	int ret;
+	struct btrfs_device *device;
+
+	ret = btrfs_check_raid_min_devices(fs_info,
+					   btrfs_num_devices(fs_info) - 1);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
+					   &device);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
+		return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);
+
+	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
+	    fs_info->fs_devices->rw_devices == 1)
+		return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
+
+	return device;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1872,25 +1899,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = btrfs_num_devices(fs_info);
-
-	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
-	if (ret)
-		goto out;
-
-	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
-					   &device);
-	if (ret)
-		goto out;
-
-	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
-		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
-		goto out;
-	}
-
-	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
-	    fs_info->fs_devices->rw_devices == 1) {
-		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
+	device = btrfs_get_device_for_delete(fs_info, device_path, devid);
+	if (IS_ERR(device)) {
+		ret = PTR_ERR(device);
 		goto out;
 	}
 
-- 
2.7.0

Re: [PATCH 0/7] Misc volume patch set part2

[Anand Jain]

On 07/16/2018 10:58 PM, Anand Jain wrote:
> As the last set if the pull wasn't integrated due to its pending
> review corrections. Here, I have done them (not much except for adding
> comments and function renames) and so sending it again. Also
> this set clubs other independent patches which are in the mailing list.
> I am explaining the changes of the individual patches in its change list.
> And most of it still remains at v1.
> 
> This can also be pulled from:
> 
>    git@github.com:asj/btrfs-devel.git misc-next-for-kdave-16jul

Sorry please pull from use the branch 'misc-next-for-kdave-17jul' 
instead. I notice that there were some changes since my last
fast-forward. And also here, I have added the following patch
which is non function change patch.
    3696c3aaf81b btrfs: rename btrfs_parse_early_options

Thanks, Anand

> Anand Jain (7):
>    btrfs: drop uuid_mutex in btrfs_free_extra_devids()
>    btrfs: fix race between free_stale_devices and close_fs_devices
>    btrfs: do device clone using the btrfs_scan_one_device
>    btrfs: use the assigned fs_devices instead of the dereference
>    btrfs: warn for num_devices below 0
>    btrfs: add helper btrfs_num_devices() to deduce num_devices
>    btrfs: add helper function check device delete able
> 
>   fs/btrfs/volumes.c | 106 +++++++++++++++++++++++++++++++----------------------
>   1 file changed, 62 insertions(+), 44 deletions(-)
> 

Re: [PATCH 7/7] btrfs: add helper function check device delete able

[David Sterba]

On Mon, Jul 16, 2018 at 10:58:12PM +0800, Anand Jain wrote:
> Move the section of the code which performs the check if the device is
> indelible, move that into a helper function.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v1->v2: Rename function to btrfs_get_device_for_delete(), thanks
> Nikolay.
> 
>  fs/btrfs/volumes.c | 49 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1c0b56374992..0cefc24b028c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1859,6 +1859,33 @@ static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>  	return num_devices;
>  }
>  
> +static struct btrfs_device *btrfs_get_device_for_delete(
> +				struct btrfs_fs_info *fs_info,
> +				const char *device_path, u64 devid)
> +{
> +	int ret;
> +	struct btrfs_device *device;
> +
> +	ret = btrfs_check_raid_min_devices(fs_info,
> +					   btrfs_num_devices(fs_info) - 1);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
> +					   &device);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
> +		return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);

This is wrong, the BTRFS_ERROR valueas are >= 1, but the IS_ERR, ERR_PTR
work for errno values -4095..0 .

Thouth ERR_PTR would cast the integer into pointer, the callers of
btrfs_get_device_for_delete will not detect the error and continue.

> +
> +	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
> +	    fs_info->fs_devices->rw_devices == 1)
> +		return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
> +
> +	return device;
> +}
> +
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		u64 devid)
>  {
> @@ -1872,25 +1899,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  
>  	mutex_lock(&uuid_mutex);
>  
> -	num_devices = btrfs_num_devices(fs_info);
> -
> -	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
> -	if (ret)
> -		goto out;
> -
> -	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
> -					   &device);
> -	if (ret)
> -		goto out;
> -
> -	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
> -		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
> -		goto out;
> -	}
> -
> -	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
> -	    fs_info->fs_devices->rw_devices == 1) {
> -		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
> +	device = btrfs_get_device_for_delete(fs_info, device_path, devid);
> +	if (IS_ERR(device)) {

BTRFS_ERROR_DEV_ONLY_WRITABLE won't work here.

> +		ret = PTR_ERR(device);
>  		goto out;
>  	}
>  
> -- 
> 2.7.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 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices

[David Sterba]

On Mon, Jul 16, 2018 at 10:58:11PM +0800, Anand Jain wrote:
> When the replace is running the fs_devices::num_devices also includes
> the replace device, however in some operations like device delete and
> balance it needs the actual num_devices without the repalce devices, so
> now the function btrfs_num_devices() just provides that.

We can't run any two from device delete, device replace or balance at
the same time.

> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: add comments. Thanks Nikolay.
> 
>  fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0f4c512aa6b4..1c0b56374992 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
>  		fs_info->fs_devices->latest_bdev = next_device->bdev;
>  }
>  
> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> +{
> +	u64 num_devices = fs_info->fs_devices->num_devices;
> +
> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> +		WARN_ON(num_devices < 1);
> +		num_devices--;
> +	}
> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);

This does not make sense, besides that btrfs_dev_replace_is_ongoing is
always going to be false here, the locking would need to cover the whole
range where we want the num_devices to remain unchanged by other
operatons.

> +
> +	return num_devices;
> +}
> +
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		u64 devid)
>  {
> @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  
>  	mutex_lock(&uuid_mutex);
>  
> -	num_devices = fs_devices->num_devices;
> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		WARN_ON(num_devices < 1);
> -		num_devices--;
> -	}
> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +	num_devices = btrfs_num_devices(fs_info);
>  
>  	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>  	if (ret)
> @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		}
>  	}
>  
> -	num_devices = fs_info->fs_devices->num_devices;
> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		WARN_ON(num_devices < 1);
> -		num_devices--;
> -	}
> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +	num_devices = btrfs_num_devices(fs_info);
> +
>  	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>  	if (num_devices > 1)
>  		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
> -- 
> 2.7.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 4/7] btrfs: use the assigned fs_devices instead of the dereference

[David Sterba]

On Mon, Jul 16, 2018 at 10:58:09PM +0800, Anand Jain wrote:
> We have assigned the %fs_info->fs_devices in %fs_devices as its not
> modified just use it for the mutex_lock().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Applied, as it's independent of the rest.

Re: [PATCH 3/7] btrfs: do device clone using the btrfs_scan_one_device

[David Sterba]

On Mon, Jul 16, 2018 at 10:58:08PM +0800, Anand Jain wrote:
> When we add a device to the RO mounted seed device, it becomes a
> RW sprout FS. The following steps are used to hold the seed and
> sprout fs_devices.
>  (first two steps are not mandatory for the sprouting, they are there
>   to ensure the seed device remains in the scanned state)
>   . Clone the (mounted) fs_devices, lets call it as old_devices
>   . Now add old_devices to fs_uuids (yeah, there is duplicate fsid in the
>     list, as we are under uuid_mutex so its fine).
>   . Alloc a new fs_devices, lets call it as seed_devices
>   . Copy fs_devices into the seed_devices
>   . Move fs_devices::devices into seed_devices::devices
>   . Bring seed_devices to under fs_devices::seed
>     (fs_devices->seed = seed_devices)
>   . Assign a new FSID to the fs_devices and add the new writable device
>     to the fs_devices.

> This patch makes the following changes..
> As we clone fs_devices to make sure the device remains scanned after the
> sprouting. So use the btrfs_scan_one_device() code instead. And do it
> at the end of the sprouting.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 8450bcfed4cb..c6f3f0dfbabe 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2179,7 +2179,7 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
>  static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
> -	struct btrfs_fs_devices *old_devices;
> +	struct btrfs_fs_devices *old_fs_devices;
>  	struct btrfs_fs_devices *seed_devices;
>  	struct btrfs_super_block *disk_super = fs_info->super_copy;
>  	struct btrfs_device *device;
> @@ -2193,14 +2193,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>  	if (IS_ERR(seed_devices))
>  		return PTR_ERR(seed_devices);
>  
> -	old_devices = clone_fs_devices(fs_devices);
> -	if (IS_ERR(old_devices)) {
> -		kfree(seed_devices);
> -		return PTR_ERR(old_devices);
> -	}
> -
> -	list_add(&old_devices->fs_list, &fs_uuids);
> -
>  	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
>  	seed_devices->opened = 1;
>  	INIT_LIST_HEAD(&seed_devices->devices);
> @@ -2233,6 +2225,17 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>  		      ~BTRFS_SUPER_FLAG_SEEDING;
>  	btrfs_set_super_flags(disk_super, super_flags);
>  
> +	/*
> +	 * As the above code hijacked the original seed fs_devices, now
> +	 * create a new one for the original seed FSID.
> +	 */
> +	list_for_each_entry(device, &fs_devices->seed->devices, dev_list) {
> +		if (!device->name)
> +			continue;
> +		btrfs_scan_one_device(device->name->str, FMODE_READ,
> +				      fs_info->bdev_holder, &old_fs_devices);

So this is clone_fs_devices vs btrfs_scan_one_device approach. The clone
only copies the devices from memory, while scan reads the superblock
from the block device.

The changelog describes how the new fsdevices is built but does not
explain why the new method is better than the current one. The end
result is possibly the same and the current code does the necessary work
to add the new fsdevices, while scanning will add the new fsid.

The rest of code adding the new device in device_list_add and inside the
loop in clone_fs_devices is equivalent.

Re: [PATCH 7/7] btrfs: add helper function check device delete able

[Anand Jain]

On 07/19/2018 07:45 PM, David Sterba wrote:
> On Mon, Jul 16, 2018 at 10:58:12PM +0800, Anand Jain wrote:
>> Move the section of the code which performs the check if the device is
>> indelible, move that into a helper function.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v1->v2: Rename function to btrfs_get_device_for_delete(), thanks
>> Nikolay.
>>
>>   fs/btrfs/volumes.c | 49 ++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 1c0b56374992..0cefc24b028c 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1859,6 +1859,33 @@ static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>   	return num_devices;
>>   }
>>   
>> +static struct btrfs_device *btrfs_get_device_for_delete(
>> +				struct btrfs_fs_info *fs_info,
>> +				const char *device_path, u64 devid)
>> +{
>> +	int ret;
>> +	struct btrfs_device *device;
>> +
>> +	ret = btrfs_check_raid_min_devices(fs_info,
>> +					   btrfs_num_devices(fs_info) - 1);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
>> +					   &device);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
>> +		return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);
> 
> This is wrong, the BTRFS_ERROR valueas are >= 1, but the IS_ERR, ERR_PTR
> work for errno values -4095..0 .
> 
> Thouth ERR_PTR would cast the integer into pointer, the callers of
> btrfs_get_device_for_delete will not detect the error and continue.

  Argh. Will fix.

Thanks, Anand

>> +
>> +	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>> +	    fs_info->fs_devices->rw_devices == 1)
>> +		return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
>> +
>> +	return device;
>> +}
>> +
>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		u64 devid)
>>   {
>> @@ -1872,25 +1899,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   
>>   	mutex_lock(&uuid_mutex);
>>   
>> -	num_devices = btrfs_num_devices(fs_info);
>> -
>> -	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>> -	if (ret)
>> -		goto out;
>> -
>> -	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
>> -					   &device);
>> -	if (ret)
>> -		goto out;
>> -
>> -	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
>> -		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
>> -		goto out;
>> -	}
>> -
>> -	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>> -	    fs_info->fs_devices->rw_devices == 1) {
>> -		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
>> +	device = btrfs_get_device_for_delete(fs_info, device_path, devid);
>> +	if (IS_ERR(device)) {
> 
> BTRFS_ERROR_DEV_ONLY_WRITABLE won't work here.
> 
>> +		ret = PTR_ERR(device);
>>   		goto out;
>>   	}
>>   
>> -- 
>> 2.7.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
> --
> 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 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 07/19/2018 07:53 PM, David Sterba wrote:
> On Mon, Jul 16, 2018 at 10:58:11PM +0800, Anand Jain wrote:
>> When the replace is running the fs_devices::num_devices also includes
>> the replace device, however in some operations like device delete and
>> balance it needs the actual num_devices without the repalce devices, so
>> now the function btrfs_num_devices() just provides that.
> 
> We can't run any two from device delete, device replace or balance at
> the same time.

  You are right. Will fix it in a separate patch. As, here in this patch
  my intention was to de-duplicate a section of the code.

>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2: add comments. Thanks Nikolay.
>>
>>   fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
>>   1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 0f4c512aa6b4..1c0b56374992 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
>>   		fs_info->fs_devices->latest_bdev = next_device->bdev;
>>   }
>>   
>> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>> +{
>> +	u64 num_devices = fs_info->fs_devices->num_devices;
>> +
>> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> +		WARN_ON(num_devices < 1);
>> +		num_devices--;
>> +	}
>> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> 
> This does not make sense,

  That's in the original code I did not add it.

> besides that btrfs_dev_replace_is_ongoing is
> always going to be false here,

  Right. Will fix in a separate patch.

> the locking would need to cover the whole
> range where we want the num_devices to remain unchanged by other
> operatons.

  Will review this part.

  Thanks, Anand

>> +
>> +	return num_devices;
>> +}
>> +
>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		u64 devid)
>>   {
>> @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   
>>   	mutex_lock(&uuid_mutex);
>>   
>> -	num_devices = fs_devices->num_devices;
>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		WARN_ON(num_devices < 1);
>> -		num_devices--;
>> -	}
>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +	num_devices = btrfs_num_devices(fs_info);
>>   
>>   	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>   	if (ret)
>> @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   		}
>>   	}
>>   
>> -	num_devices = fs_info->fs_devices->num_devices;
>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		WARN_ON(num_devices < 1);
>> -		num_devices--;
>> -	}
>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +	num_devices = btrfs_num_devices(fs_info);
>> +
>>   	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>>   	if (num_devices > 1)
>>   		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
>> -- 
>> 2.7.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
> --
> 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/7] btrfs: do device clone using the btrfs_scan_one_device

[Anand Jain]

On 07/19/2018 08:31 PM, David Sterba wrote:
> On Mon, Jul 16, 2018 at 10:58:08PM +0800, Anand Jain wrote:
>> When we add a device to the RO mounted seed device, it becomes a
>> RW sprout FS. The following steps are used to hold the seed and
>> sprout fs_devices.
>>   (first two steps are not mandatory for the sprouting, they are there
>>    to ensure the seed device remains in the scanned state)
>>    . Clone the (mounted) fs_devices, lets call it as old_devices
>>    . Now add old_devices to fs_uuids (yeah, there is duplicate fsid in the
>>      list, as we are under uuid_mutex so its fine).
>>    . Alloc a new fs_devices, lets call it as seed_devices
>>    . Copy fs_devices into the seed_devices
>>    . Move fs_devices::devices into seed_devices::devices
>>    . Bring seed_devices to under fs_devices::seed
>>      (fs_devices->seed = seed_devices)
>>    . Assign a new FSID to the fs_devices and add the new writable device
>>      to the fs_devices.
> 
>> This patch makes the following changes..
>> As we clone fs_devices to make sure the device remains scanned after the
>> sprouting. So use the btrfs_scan_one_device() code instead. And do it
>> at the end of the sprouting.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 21 ++++++++++++---------
>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 8450bcfed4cb..c6f3f0dfbabe 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2179,7 +2179,7 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
>>   static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>   {
>>   	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>> -	struct btrfs_fs_devices *old_devices;
>> +	struct btrfs_fs_devices *old_fs_devices;
>>   	struct btrfs_fs_devices *seed_devices;
>>   	struct btrfs_super_block *disk_super = fs_info->super_copy;
>>   	struct btrfs_device *device;
>> @@ -2193,14 +2193,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>   	if (IS_ERR(seed_devices))
>>   		return PTR_ERR(seed_devices);
>>   
>> -	old_devices = clone_fs_devices(fs_devices);
>> -	if (IS_ERR(old_devices)) {
>> -		kfree(seed_devices);
>> -		return PTR_ERR(old_devices);
>> -	}
>> -
>> -	list_add(&old_devices->fs_list, &fs_uuids);
>> -
>>   	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
>>   	seed_devices->opened = 1;
>>   	INIT_LIST_HEAD(&seed_devices->devices);
>> @@ -2233,6 +2225,17 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>   		      ~BTRFS_SUPER_FLAG_SEEDING;
>>   	btrfs_set_super_flags(disk_super, super_flags);
>>   
>> +	/*
>> +	 * As the above code hijacked the original seed fs_devices, now
>> +	 * create a new one for the original seed FSID.
>> +	 */
>> +	list_for_each_entry(device, &fs_devices->seed->devices, dev_list) {
>> +		if (!device->name)
>> +			continue;
>> +		btrfs_scan_one_device(device->name->str, FMODE_READ,
>> +				      fs_info->bdev_holder, &old_fs_devices);
> 
> So this is clone_fs_devices vs btrfs_scan_one_device approach. The clone
> only copies the devices from memory, while scan reads the superblock
> from the block device.

  Right.

> The changelog describes how the new fsdevices is built but does not
> explain why the new method is better than the current one. The end
> result is possibly the same and the current code does the necessary work
> to add the new fsdevices, while scanning will add the new fsid.
> 
> The rest of code adding the new device in device_list_add and inside the
> loop in clone_fs_devices is equivalent.

  Its just a code consolidate patch.

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
> 

Re: [PATCH 3/7] btrfs: do device clone using the btrfs_scan_one_device

[Anand Jain]

On 07/20/2018 02:35 PM, Anand Jain wrote:
> 
> 
> On 07/19/2018 08:31 PM, David Sterba wrote:
>> On Mon, Jul 16, 2018 at 10:58:08PM +0800, Anand Jain wrote:
>>> When we add a device to the RO mounted seed device, it becomes a
>>> RW sprout FS. The following steps are used to hold the seed and
>>> sprout fs_devices.
>>>   (first two steps are not mandatory for the sprouting, they are there
>>>    to ensure the seed device remains in the scanned state)
>>>    . Clone the (mounted) fs_devices, lets call it as old_devices
>>>    . Now add old_devices to fs_uuids (yeah, there is duplicate fsid 
>>> in the
>>>      list, as we are under uuid_mutex so its fine).
>>>    . Alloc a new fs_devices, lets call it as seed_devices
>>>    . Copy fs_devices into the seed_devices
>>>    . Move fs_devices::devices into seed_devices::devices
>>>    . Bring seed_devices to under fs_devices::seed
>>>      (fs_devices->seed = seed_devices)
>>>    . Assign a new FSID to the fs_devices and add the new writable device
>>>      to the fs_devices.
>>
>>> This patch makes the following changes..
>>> As we clone fs_devices to make sure the device remains scanned after the
>>> sprouting. So use the btrfs_scan_one_device() code instead. And do it
>>> at the end of the sprouting.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/volumes.c | 21 ++++++++++++---------
>>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 8450bcfed4cb..c6f3f0dfbabe 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -2179,7 +2179,7 @@ int btrfs_find_device_by_devspec(struct 
>>> btrfs_fs_info *fs_info, u64 devid,
>>>   static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>>   {
>>>       struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>>> -    struct btrfs_fs_devices *old_devices;
>>> +    struct btrfs_fs_devices *old_fs_devices;
>>>       struct btrfs_fs_devices *seed_devices;
>>>       struct btrfs_super_block *disk_super = fs_info->super_copy;
>>>       struct btrfs_device *device;
>>> @@ -2193,14 +2193,6 @@ static int btrfs_prepare_sprout(struct 
>>> btrfs_fs_info *fs_info)
>>>       if (IS_ERR(seed_devices))
>>>           return PTR_ERR(seed_devices);
>>> -    old_devices = clone_fs_devices(fs_devices);
>>> -    if (IS_ERR(old_devices)) {
>>> -        kfree(seed_devices);
>>> -        return PTR_ERR(old_devices);
>>> -    }
>>> -
>>> -    list_add(&old_devices->fs_list, &fs_uuids);
>>> -
>>>       memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
>>>       seed_devices->opened = 1;
>>>       INIT_LIST_HEAD(&seed_devices->devices);
>>> @@ -2233,6 +2225,17 @@ static int btrfs_prepare_sprout(struct 
>>> btrfs_fs_info *fs_info)
>>>                 ~BTRFS_SUPER_FLAG_SEEDING;
>>>       btrfs_set_super_flags(disk_super, super_flags);
>>> +    /*
>>> +     * As the above code hijacked the original seed fs_devices, now
>>> +     * create a new one for the original seed FSID.
>>> +     */
>>> +    list_for_each_entry(device, &fs_devices->seed->devices, dev_list) {
>>> +        if (!device->name)
>>> +            continue;
>>> +        btrfs_scan_one_device(device->name->str, FMODE_READ,
>>> +                      fs_info->bdev_holder, &old_fs_devices);
>>
>> So this is clone_fs_devices vs btrfs_scan_one_device approach. The clone
>> only copies the devices from memory, while scan reads the superblock
>> from the block device.
> 
>   Right.
> 
>> The changelog describes how the new fsdevices is built but does not
>> explain why the new method is better than the current one. The end
>> result is possibly the same and the current code does the necessary work
>> to add the new fsdevices, while scanning will add the new fsid.
>>
>> The rest of code adding the new device in device_list_add and inside the
>> loop in clone_fs_devices is equivalent.
> 
>   Its just a code consolidate patch.

  I am dropping this patch in the next upcoming patchset revision, as
  there isn't considerable benefit in using the btrfs_scan_one_device()
  VS clone_fs_devices().

Thanks, Anand

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

Re: [PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 07/19/2018 07:53 PM, David Sterba wrote:
> On Mon, Jul 16, 2018 at 10:58:11PM +0800, Anand Jain wrote:
>> When the replace is running the fs_devices::num_devices also includes
>> the replace device, however in some operations like device delete and
>> balance it needs the actual num_devices without the repalce devices, so
>> now the function btrfs_num_devices() just provides that.
> 
> We can't run any two from device delete, device replace or balance at
> the same time.
> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2: add comments. Thanks Nikolay.
>>
>>   fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
>>   1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 0f4c512aa6b4..1c0b56374992 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
>>   		fs_info->fs_devices->latest_bdev = next_device->bdev;
>>   }
>>   
>> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>> +{
>> +	u64 num_devices = fs_info->fs_devices->num_devices;
>> +
>> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> +		WARN_ON(num_devices < 1);
>> +		num_devices--;
>> +	}
>> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);



> This does not make sense, besides that > btrfs_dev_replace_is_ongoing is
> always going to be false here,

  No. There is a way how balance and replace could co-exists.
  (theoretically, I didn't experiment it yet)
  . Start balance and pause it
  . Now start the replace
  . power-fail
  . The open_ctree() first starts the balance so it must check
  for the replace device otherwise our num_devices calculation will
  be wrong. IMO its not a good idea to remove the replace check here.

  For now a consolidation as in this patch is better.

Thanks, Anand


> the locking would need to cover the whole
> range where we want the num_devices to remain unchanged by other
> operatons.
> 
>> +
>> +	return num_devices;
>> +}
>> +
>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		u64 devid)
>>   {
>> @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   
>>   	mutex_lock(&uuid_mutex);
>>   
>> -	num_devices = fs_devices->num_devices;
>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		WARN_ON(num_devices < 1);
>> -		num_devices--;
>> -	}
>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +	num_devices = btrfs_num_devices(fs_info);
>>   
>>   	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>   	if (ret)
>> @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   		}
>>   	}
>>   
>> -	num_devices = fs_info->fs_devices->num_devices;
>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		WARN_ON(num_devices < 1);
>> -		num_devices--;
>> -	}
>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +	num_devices = btrfs_num_devices(fs_info);
>> +
>>   	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>>   	if (num_devices > 1)
>>   		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
>> -- 
>> 2.7.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
> --
> 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 7/7] btrfs: add helper function check device delete able

[Anand Jain]

On 07/20/2018 09:34 AM, Anand Jain wrote:
> 
> 
> On 07/19/2018 07:45 PM, David Sterba wrote:
>> On Mon, Jul 16, 2018 at 10:58:12PM +0800, Anand Jain wrote:
>>> Move the section of the code which performs the check if the device is
>>> indelible, move that into a helper function.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> v1->v2: Rename function to btrfs_get_device_for_delete(), thanks
>>> Nikolay.
>>>
>>>   fs/btrfs/volumes.c | 49 
>>> ++++++++++++++++++++++++++++++-------------------
>>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 1c0b56374992..0cefc24b028c 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1859,6 +1859,33 @@ static inline u64 btrfs_num_devices(struct 
>>> btrfs_fs_info *fs_info)
>>>       return num_devices;
>>>   }
>>> +static struct btrfs_device *btrfs_get_device_for_delete(
>>> +                struct btrfs_fs_info *fs_info,
>>> +                const char *device_path, u64 devid)
>>> +{
>>> +    int ret;
>>> +    struct btrfs_device *device;
>>> +
>>> +    ret = btrfs_check_raid_min_devices(fs_info,
>>> +                       btrfs_num_devices(fs_info) - 1);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>> +    ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
>>> +                       &device);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>> +    if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
>>> +        return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);
>>
>> This is wrong, the BTRFS_ERROR valueas are >= 1, but the IS_ERR, ERR_PTR
>> work for errno values -4095..0 .
>>
>> Thouth ERR_PTR would cast the integer into pointer, the callers of
>> btrfs_get_device_for_delete will not detect the error and continue.
> 
>   Argh. Will fix.

  Pls ignore this patch.

> Thanks, Anand
> 
>>> +
>>> +    if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>>> +        fs_info->fs_devices->rw_devices == 1)
>>> +        return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
>>> +
>>> +    return device;
>>> +}
>>> +
>>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char 
>>> *device_path,
>>>           u64 devid)
>>>   {
>>> @@ -1872,25 +1899,9 @@ int btrfs_rm_device(struct btrfs_fs_info 
>>> *fs_info, const char *device_path,
>>>       mutex_lock(&uuid_mutex);
>>> -    num_devices = btrfs_num_devices(fs_info);
>>> -
>>> -    ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>> -    if (ret)
>>> -        goto out;
>>> -
>>> -    ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
>>> -                       &device);
>>> -    if (ret)
>>> -        goto out;
>>> -
>>> -    if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
>>> -        ret = BTRFS_ERROR_DEV_TGT_REPLACE;
>>> -        goto out;
>>> -    }
>>> -
>>> -    if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>>> -        fs_info->fs_devices->rw_devices == 1) {
>>> -        ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
>>> +    device = btrfs_get_device_for_delete(fs_info, device_path, devid);
>>> +    if (IS_ERR(device)) {
>>
>> BTRFS_ERROR_DEV_ONLY_WRITABLE won't work here.
>>
>>> +        ret = PTR_ERR(device);
>>>           goto out;
>>>       }
>>> -- 
>>> 2.7.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
>> -- 
>> 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 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices

[David Sterba]

On Fri, Jul 20, 2018 at 07:18:54PM +0800, Anand Jain wrote:
> 
> 
> On 07/19/2018 07:53 PM, David Sterba wrote:
> > On Mon, Jul 16, 2018 at 10:58:11PM +0800, Anand Jain wrote:
> >> When the replace is running the fs_devices::num_devices also includes
> >> the replace device, however in some operations like device delete and
> >> balance it needs the actual num_devices without the repalce devices, so
> >> now the function btrfs_num_devices() just provides that.
> > 
> > We can't run any two from device delete, device replace or balance at
> > the same time.
> > 
> >>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >> v2: add comments. Thanks Nikolay.
> >>
> >>   fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
> >>   1 file changed, 18 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index 0f4c512aa6b4..1c0b56374992 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
> >>   		fs_info->fs_devices->latest_bdev = next_device->bdev;
> >>   }
> >>   
> >> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
> >> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> >> +{
> >> +	u64 num_devices = fs_info->fs_devices->num_devices;
> >> +
> >> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> >> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> >> +		WARN_ON(num_devices < 1);
> >> +		num_devices--;
> >> +	}
> >> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> 
> 
> 
> > This does not make sense, besides that > btrfs_dev_replace_is_ongoing is
> > always going to be false here,
> 
>   No. There is a way how balance and replace could co-exists.
>   (theoretically, I didn't experiment it yet)
>   . Start balance and pause it
>   . Now start the replace
>   . power-fail
>   . The open_ctree() first starts the balance so it must check
>   for the replace device otherwise our num_devices calculation will
>   be wrong. IMO its not a good idea to remove the replace check here.

I see, the paused states can lead to balance that sees device replace
ongoing as true. This would be good to add to the function comment as
it's not quite obvious why the helper is needed.

>   For now a consolidation as in this patch is better.

Yeah, for this context it would be good.  The function name could be
more descriptive what devices it actually counts.

Re: [PATCH 5/7] btrfs: warn for num_devices below 0

[David Sterba]

On Mon, Jul 16, 2018 at 10:58:10PM +0800, Anand Jain wrote:
> In preparation to de-duplicate a section of code where we deduce the
> num_devices, use warn instead of bug.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7f4973fc2b52..0f4c512aa6b4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3726,7 +3726,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  	num_devices = fs_info->fs_devices->num_devices;
>  	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>  	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		BUG_ON(num_devices < 1);
> +		WARN_ON(num_devices < 1);

I wonder if there any valid cases when there are 0 devices when balance
is started, ie. before num_devices gets decremented.

The WARN_ON is either redundant or should be turned to a proper sanity
check.

>  		num_devices--;

Re: [PATCH 5/7] btrfs: warn for num_devices below 0

[Anand Jain]

On 07/23/2018 10:01 PM, David Sterba wrote:
> On Mon, Jul 16, 2018 at 10:58:10PM +0800, Anand Jain wrote:
>> In preparation to de-duplicate a section of code where we deduce the
>> num_devices, use warn instead of bug.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 7f4973fc2b52..0f4c512aa6b4 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3726,7 +3726,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   	num_devices = fs_info->fs_devices->num_devices;
>>   	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>   	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		BUG_ON(num_devices < 1);
>> +		WARN_ON(num_devices < 1);
> 
> I wonder if there any valid cases when there are 0 devices when balance
> is started, ie. before num_devices gets decremented.

  num_devices counts the in-memory devices of a fsid.
  On a mounted FS num_devices > 0 always.

> The WARN_ON is either redundant or should be turned to a proper sanity
> check.

   Yes is redundant. I suggest to delete it.

Thanks, Anand

>>   		num_devices--;
> --
> 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 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 07/23/2018 09:57 PM, David Sterba wrote:
> On Fri, Jul 20, 2018 at 07:18:54PM +0800, Anand Jain wrote:
>>
>>
>> On 07/19/2018 07:53 PM, David Sterba wrote:
>>> On Mon, Jul 16, 2018 at 10:58:11PM +0800, Anand Jain wrote:
>>>> When the replace is running the fs_devices::num_devices also includes
>>>> the replace device, however in some operations like device delete and
>>>> balance it needs the actual num_devices without the repalce devices, so
>>>> now the function btrfs_num_devices() just provides that.
>>>
>>> We can't run any two from device delete, device replace or balance at
>>> the same time.
>>>
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>> v2: add comments. Thanks Nikolay.
>>>>
>>>>    fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
>>>>    1 file changed, 18 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 0f4c512aa6b4..1c0b56374992 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
>>>>    		fs_info->fs_devices->latest_bdev = next_device->bdev;
>>>>    }
>>>>    
>>>> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
>>>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>>> +{
>>>> +	u64 num_devices = fs_info->fs_devices->num_devices;
>>>> +
>>>> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>>> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>>> +		WARN_ON(num_devices < 1);
>>>> +		num_devices--;
>>>> +	}
>>>> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>
>>
>>
>>> This does not make sense, besides that > btrfs_dev_replace_is_ongoing is
>>> always going to be false here,
>>
>>    No. There is a way how balance and replace could co-exists.
>>    (theoretically, I didn't experiment it yet)
>>    . Start balance and pause it
>>    . Now start the replace
>>    . power-fail
>>    . The open_ctree() first starts the balance so it must check
>>    for the replace device otherwise our num_devices calculation will
>>    be wrong. IMO its not a good idea to remove the replace check here.
> 
> I see, the paused states can lead to balance that sees device replace
> ongoing as true. This would be good to add to the function comment as
> it's not quite obvious why the helper is needed.
> 
>>    For now a consolidation as in this patch is better.
> 
> Yeah, for this context it would be good.  The function name could be
> more descriptive what devices it actually counts.

Regarding function names its tough to convince in a short form.
As of now I have the following choices, if there is anything better
I don't mind using it though.
   btrfs_num_devices_minus_replace()
   btrfs_get_num_devices_raw()
   btrfs_num_devices_raw()

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
>