[PATCH 0/5] device_list_add() peparation to add reappearing missing device

[PATCH 0/5] device_list_add() peparation to add reappearing missing device

[Anand Jain]

Cleanup of device_list_add(), mainly in preparation to handle
reappearing missing device which its next reroll will be sent
separately.

Anand Jain (5):
  btrfs: move pr_info into device_list_add
  btrfs: set the total_devices in device_list_add()
  btrfs: get device pointer from device_list_add()
  btrfs: drop devid as device_list_add() arg
  btrfs: move uuid_mutex closer to exclusivity

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

-- 
2.7.0

[PATCH 1/5] btrfs: move pr_info into device_list_add

[Anand Jain]

Commit 60999ca4b403 ("btrfs: make device scan less noisy")
adds return value 1 to device_list_add(), so that parent function can
call pr_info only when new device is added. Move the pr_info() part
into device_list_add() so that this function can be kept simple.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0d7f912f68ff..091eb452f3c8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -727,8 +727,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
  * Add new device to list of registered devices
  *
  * Returns:
- * 1   - first time device is seen
- * 0   - device already known
+ * 0   - device already known or newly added
  * < 0 - error
  */
 static noinline int device_list_add(const char *path,
@@ -738,7 +737,6 @@ static noinline int device_list_add(const char *path,
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices;
 	struct rcu_string *name;
-	int ret = 0;
 	u64 found_transid = btrfs_super_generation(disk_super);
 
 	fs_devices = find_fsid(disk_super->fsid);
@@ -778,9 +776,16 @@ static noinline int device_list_add(const char *path,
 		fs_devices->num_devices++;
 		mutex_unlock(&fs_devices->device_list_mutex);
 
-		ret = 1;
 		device->fs_devices = fs_devices;
 		btrfs_free_stale_device(path, device);
+
+		if (disk_super->label[0])
+			pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
+				disk_super->label, devid, found_transid, path);
+		else
+			pr_info("BTRFS: device fsid %pU devid %llu transid %llu %s\n",
+				disk_super->fsid, devid, found_transid, path);
+
 	} else if (!device->name || strcmp(device->name->str, path)) {
 		/*
 		 * When FS is already mounted.
@@ -841,7 +846,7 @@ static noinline int device_list_add(const char *path,
 
 	*fs_devices_ret = fs_devices;
 
-	return ret;
+	return 0;
 }
 
 static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
@@ -1180,7 +1185,6 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	struct page *page;
 	int ret;
 	u64 devid;
-	u64 transid;
 	u64 total_devices;
 	u64 bytenr;
 
@@ -1203,25 +1207,14 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	}
 
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
-	transid = btrfs_super_generation(disk_super);
 	total_devices = btrfs_super_num_devices(disk_super);
 
 	mutex_lock(&uuid_mutex);
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
-	if (ret >= 0 && fs_devices_ret)
+	if (!ret && fs_devices_ret)
 		(*fs_devices_ret)->total_devices = total_devices;
 	mutex_unlock(&uuid_mutex);
 
-	if (ret > 0) {
-		if (disk_super->label[0])
-			pr_info("BTRFS: device label %s ", disk_super->label);
-		else
-			pr_info("BTRFS: device fsid %pU ", disk_super->fsid);
-
-		pr_cont("devid %llu transid %llu %s\n", devid, transid, path);
-		ret = 0;
-	}
-
 	btrfs_release_disk_super(page);
 
 error_bdev_put:
-- 
2.7.0

[PATCH 2/5] btrfs: set the total_devices in device_list_add()

[Anand Jain]

There is no other parent for device_list_add() except for
btrfs_scan_one_device(), which would set btrfs_fs_devices::total_devices
if device_list_add is successful and this can be done with in
device_list_add() itself.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 091eb452f3c8..f9aaf65e27f5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -844,6 +844,8 @@ static noinline int device_list_add(const char *path,
 	if (!fs_devices->opened)
 		device->generation = found_transid;
 
+	fs_devices->total_devices = btrfs_super_num_devices(disk_super);
+
 	*fs_devices_ret = fs_devices;
 
 	return 0;
@@ -1185,7 +1187,6 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	struct page *page;
 	int ret;
 	u64 devid;
-	u64 total_devices;
 	u64 bytenr;
 
 	/*
@@ -1207,12 +1208,9 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	}
 
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
-	total_devices = btrfs_super_num_devices(disk_super);
 
 	mutex_lock(&uuid_mutex);
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
-	if (!ret && fs_devices_ret)
-		(*fs_devices_ret)->total_devices = total_devices;
 	mutex_unlock(&uuid_mutex);
 
 	btrfs_release_disk_super(page);
-- 
2.7.0

[PATCH 3/5] btrfs: get device pointer from device_list_add()

[Anand Jain]

Instead of pointer to btrfs_fs_devices from device_list_add() its
better to get pointer to btrfs_device, then we have both, pointer
to btrfs_device and btrfs_fs_devices. This is needed in preparation
to add handling reappearing missing device feature.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f9aaf65e27f5..2317ca1b3d83 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -732,7 +732,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
  */
 static noinline int device_list_add(const char *path,
 			   struct btrfs_super_block *disk_super,
-			   u64 devid, struct btrfs_fs_devices **fs_devices_ret)
+			   u64 devid, struct btrfs_device **device_ret)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices;
@@ -846,7 +846,7 @@ static noinline int device_list_add(const char *path,
 
 	fs_devices->total_devices = btrfs_super_num_devices(disk_super);
 
-	*fs_devices_ret = fs_devices;
+	*device_ret = device;
 
 	return 0;
 }
@@ -1183,6 +1183,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 			  struct btrfs_fs_devices **fs_devices_ret)
 {
 	struct btrfs_super_block *disk_super;
+	struct btrfs_device *device;
 	struct block_device *bdev;
 	struct page *page;
 	int ret;
@@ -1210,9 +1211,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
 
 	mutex_lock(&uuid_mutex);
-	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
+	ret = device_list_add(path, disk_super, devid, &device);
 	mutex_unlock(&uuid_mutex);
 
+	*fs_devices_ret = device->fs_devices;
+
 	btrfs_release_disk_super(page);
 
 error_bdev_put:
-- 
2.7.0

[PATCH 4/5] btrfs: drop devid as device_list_add() arg

[Anand Jain]

As struct btrfs_disk_super is being passed, so it can get devid
the same way its parent does.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2317ca1b3d83..f942e8193862 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -732,12 +732,13 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
  */
 static noinline int device_list_add(const char *path,
 			   struct btrfs_super_block *disk_super,
-			   u64 devid, struct btrfs_device **device_ret)
+			   struct btrfs_device **device_ret)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices;
 	struct rcu_string *name;
 	u64 found_transid = btrfs_super_generation(disk_super);
+	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
 
 	fs_devices = find_fsid(disk_super->fsid);
 	if (!fs_devices) {
@@ -1187,7 +1188,6 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	struct block_device *bdev;
 	struct page *page;
 	int ret;
-	u64 devid;
 	u64 bytenr;
 
 	/*
@@ -1208,10 +1208,8 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 		goto error_bdev_put;
 	}
 
-	devid = btrfs_stack_device_id(&disk_super->dev_item);
-
 	mutex_lock(&uuid_mutex);
-	ret = device_list_add(path, disk_super, devid, &device);
+	ret = device_list_add(path, disk_super, &device);
 	mutex_unlock(&uuid_mutex);
 
 	*fs_devices_ret = device->fs_devices;
-- 
2.7.0

[PATCH 5/5] btrfs: move uuid_mutex closer to exclusivity

[Anand Jain]

move uuid_mutex with in device_list_add().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f942e8193862..283417bf3b00 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -740,6 +740,7 @@ static noinline int device_list_add(const char *path,
 	u64 found_transid = btrfs_super_generation(disk_super);
 	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
 
+	mutex_lock(&uuid_mutex);
 	fs_devices = find_fsid(disk_super->fsid);
 	if (!fs_devices) {
 		fs_devices = alloc_fs_devices(disk_super->fsid);
@@ -847,6 +848,8 @@ static noinline int device_list_add(const char *path,
 
 	fs_devices->total_devices = btrfs_super_num_devices(disk_super);
 
+	mutex_unlock(&uuid_mutex);
+
 	*device_ret = device;
 
 	return 0;
@@ -1208,9 +1211,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 		goto error_bdev_put;
 	}
 
-	mutex_lock(&uuid_mutex);
 	ret = device_list_add(path, disk_super, &device);
-	mutex_unlock(&uuid_mutex);
 
 	*fs_devices_ret = device->fs_devices;
 
-- 
2.7.0

Re: [PATCH 1/5] btrfs: move pr_info into device_list_add

[Josef Bacik]

On Tue, Jan 09, 2018 at 10:46:21PM +0800, Anand Jain wrote:
> Commit 60999ca4b403 ("btrfs: make device scan less noisy")
> adds return value 1 to device_list_add(), so that parent function can
> call pr_info only when new device is added. Move the pr_info() part
> into device_list_add() so that this function can be kept simple.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Josef Bacik <jbacik@fb.com>

Thanks,

Josef

Re: [PATCH 2/5] btrfs: set the total_devices in device_list_add()

[Josef Bacik]

On Tue, Jan 09, 2018 at 10:46:22PM +0800, Anand Jain wrote:
> There is no other parent for device_list_add() except for
> btrfs_scan_one_device(), which would set btrfs_fs_devices::total_devices
> if device_list_add is successful and this can be done with in
> device_list_add() itself.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Josef Bacik <jbacik@fb.com>

Thanks,

Josef

Re: [PATCH 3/5] btrfs: get device pointer from device_list_add()

[Josef Bacik]

On Tue, Jan 09, 2018 at 10:46:23PM +0800, Anand Jain wrote:
> Instead of pointer to btrfs_fs_devices from device_list_add() its
> better to get pointer to btrfs_device, then we have both, pointer
> to btrfs_device and btrfs_fs_devices. This is needed in preparation
> to add handling reappearing missing device feature.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Can we just change device_list_add to return struct btrfs_device * instead, and
just do PTR_ERR() if there's a problem?  Thanks,

Josef

Re: [PATCH 4/5] btrfs: drop devid as device_list_add() arg

[Josef Bacik]

On Tue, Jan 09, 2018 at 10:46:24PM +0800, Anand Jain wrote:
> As struct btrfs_disk_super is being passed, so it can get devid
> the same way its parent does.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Josef Bacik <jbacik@fb.com>

Thanks,

Josef

Re: [PATCH 5/5] btrfs: move uuid_mutex closer to exclusivity

[Josef Bacik]

On Tue, Jan 09, 2018 at 10:46:25PM +0800, Anand Jain wrote:
> move uuid_mutex with in device_list_add().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

This isn't going to work, there's a bunch of places we return errors so this
just deadlocks the box.  Leave it like it is, it's not hurting anybody leaving
it like this.  Thanks,

Josef

Re: [PATCH 3/5] btrfs: get device pointer from device_list_add()

[Anand Jain]

On 01/10/2018 12:13 AM, Josef Bacik wrote:
> On Tue, Jan 09, 2018 at 10:46:23PM +0800, Anand Jain wrote:
>> Instead of pointer to btrfs_fs_devices from device_list_add() its
>> better to get pointer to btrfs_device, then we have both, pointer
>> to btrfs_device and btrfs_fs_devices. This is needed in preparation
>> to add handling reappearing missing device feature.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Can we just change device_list_add to return struct btrfs_device * instead, and
> just do PTR_ERR() if there's a problem?  Thanks,

That's much better. Will do.

Thanks, Anand

> Josef
> --
> 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 5/5] btrfs: move uuid_mutex closer to exclusivity

[Anand Jain]

On 01/10/2018 12:15 AM, Josef Bacik wrote:
> On Tue, Jan 09, 2018 at 10:46:25PM +0800, Anand Jain wrote:
>> move uuid_mutex with in device_list_add().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> This isn't going to work, there's a bunch of places we return errors so this
> just deadlocks the box.  Leave it like it is, it's not hurting anybody leaving
> it like this.  Thanks,

  Err. My mistake. Can't believe how did I miss it.
  Pls ignore this patch.
  Thanks for the review.

- Anand

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