[PATCH 0/6] fix device_list_mutex usage and cleanups in device_list_add()

[PATCH 0/6] fix device_list_mutex usage and cleanups in device_list_add()

[Anand Jain]

Patch 1/6 is a preparatory patch to add the required device_list_mutex
in device_list_add() in the patch 2/6.

Patch 3/6 updates the comments on device_list_mutex.

Patch 4/6 is just a cleanup patch no functional changes.

Patch 5/6 adds the required device_list_mutex in
btrfs_free_stale_device().

Patch 6/6 renames btrfs_free_stale_device() to free_stale_device(), no
functional changes.

Anand Jain (6):
  btrfs: do btrfs_free_stale_devices() outside of device_list_add()
  btrfs: fix device_list_add() missing device_list_mutex()
  btrfs: update device_list_mutex protected items
  btrfs: btrfs_free_stale_devices() rename local variables
  btrfs: fix btrfs_free_stale_devices() with needed locks
  btrfs: rename btrfs_free_stale_devices drop btrfs prefix

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

-- 
2.7.0

[PATCH 1/6] btrfs: do btrfs_free_stale_devices() outside of device_list_add()

[Anand Jain]

btrfs_free_stale_devices() looks for device path reused for another
fsid, and deletes the older fs_devices::device entry.

In preparation to handle proper locks in the device_list_add(),
move the btrfs_free_stale_devices() outside of the device_list_add()
as these two functions has two distinct purpose.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0a47b4162d12..4edba7c5c050 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -750,7 +750,8 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
  * error pointer when failed
  */
 static noinline struct btrfs_device *device_list_add(const char *path,
-			   struct btrfs_super_block *disk_super)
+			   struct btrfs_super_block *disk_super,
+			   bool *new_device_added)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices;
@@ -796,7 +797,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		mutex_unlock(&fs_devices->device_list_mutex);
 
 		device->fs_devices = fs_devices;
-		btrfs_free_stale_devices(path, device);
+		*new_device_added = true;
 
 		if (disk_super->label[0])
 			pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
@@ -1223,6 +1224,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;
+	bool new_device_added = false;
 	struct btrfs_device *device;
 	struct block_device *bdev;
 	struct page *page;
@@ -1248,11 +1250,14 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	}
 
 	mutex_lock(&uuid_mutex);
-	device = device_list_add(path, disk_super);
-	if (IS_ERR(device))
+	device = device_list_add(path, disk_super, &new_device_added);
+	if (IS_ERR(device)) {
 		ret = PTR_ERR(device);
-	else
+	} else {
 		*fs_devices_ret = device->fs_devices;
+		if (new_device_added)
+			btrfs_free_stale_devices(path, device);
+	}
 	mutex_unlock(&uuid_mutex);
 
 	btrfs_release_disk_super(page);
-- 
2.7.0

[PATCH 2/6] btrfs: fix device_list_add() missing device_list_mutex()

[Anand Jain]

When the device is added or if the device pointer is retrieved
for writing, make sure that device_list_mutex is held. Also
make sure it is held when we check fs_devices::opened and
fs_devices::total_devices is updated.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4edba7c5c050..0fdb9b717398 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -765,21 +765,26 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		if (IS_ERR(fs_devices))
 			return ERR_CAST(fs_devices);
 
+		mutex_lock(&fs_devices->device_list_mutex);
 		list_add(&fs_devices->fs_list, &fs_uuids);
 
 		device = NULL;
 	} else {
+		mutex_lock(&fs_devices->device_list_mutex);
 		device = find_device(fs_devices, devid,
 				disk_super->dev_item.uuid);
 	}
 
 	if (!device) {
-		if (fs_devices->opened)
+		if (fs_devices->opened) {
+			mutex_unlock(&fs_devices->device_list_mutex);
 			return ERR_PTR(-EBUSY);
+		}
 
 		device = btrfs_alloc_device(NULL, &devid,
 					    disk_super->dev_item.uuid);
 		if (IS_ERR(device)) {
+			mutex_unlock(&fs_devices->device_list_mutex);
 			/* we can safely leave the fs_devices entry around */
 			return device;
 		}
@@ -787,14 +792,13 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		name = rcu_string_strdup(path, GFP_NOFS);
 		if (!name) {
 			btrfs_free_device(device);
+			mutex_unlock(&fs_devices->device_list_mutex);
 			return ERR_PTR(-ENOMEM);
 		}
 		rcu_assign_pointer(device->name, name);
 
-		mutex_lock(&fs_devices->device_list_mutex);
 		list_add_rcu(&device->dev_list, &fs_devices->devices);
 		fs_devices->num_devices++;
-		mutex_unlock(&fs_devices->device_list_mutex);
 
 		device->fs_devices = fs_devices;
 		*new_device_added = true;
@@ -841,12 +845,15 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			 * with larger generation number or the last-in if
 			 * generation are equal.
 			 */
+			mutex_unlock(&fs_devices->device_list_mutex);
 			return ERR_PTR(-EEXIST);
 		}
 
 		name = rcu_string_strdup(path, GFP_NOFS);
-		if (!name)
+		if (!name) {
+			mutex_unlock(&fs_devices->device_list_mutex);
 			return ERR_PTR(-ENOMEM);
+		}
 		rcu_string_free(device->name);
 		rcu_assign_pointer(device->name, name);
 		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) {
@@ -866,6 +873,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 
 	fs_devices->total_devices = btrfs_super_num_devices(disk_super);
 
+	mutex_unlock(&fs_devices->device_list_mutex);
 	return device;
 }
 
-- 
2.7.0

[PATCH 3/6] btrfs: update device_list_mutex protected items

[Anand Jain]

Include items of fs_uuids::fs_devices to be protected by
device_list_mutex, like (but not limited to) fs_devices::opened,
fs_devices::total_devices and fs_devices::num_devices. This is
been done in the code already.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0fdb9b717398..e3dd25238eb9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -164,7 +164,8 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
  *
  * fs_devices::device_list_mutex (per-fs, with RCU)
  * ------------------------------------------------
- * protects updates to fs_devices::devices, ie. adding and deleting
+ * Protects updates to fs_uuids::fs_devices items, and fs_devices::devices, ie.
+ * adding and deleting.
  *
  * simple list traversal with read-only actions can be done with RCU protection
  *
-- 
2.7.0

[PATCH 4/6] btrfs: btrfs_free_stale_devices() rename local variables

[Anand Jain]

Over the years we named %fs_devices and %devices to represent,
the struct btrfs_fs_devices and the struct btrfs_device. So
follow the same norm here too. No functional changes.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e3dd25238eb9..fd656eb75225 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -635,43 +635,44 @@ static void pending_bios_fn(struct btrfs_work *work)
  *		devices.
  */
 static void btrfs_free_stale_devices(const char *path,
-				     struct btrfs_device *skip_dev)
+				     struct btrfs_device *skip_device)
 {
-	struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
-	struct btrfs_device *dev, *tmp_dev;
+	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
+	struct btrfs_device *device, *tmp_device;
 
-	list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, fs_list) {
+	list_for_each_entry_safe(fs_devices, tmp_fs_devices, &fs_uuids,
+				 fs_list) {
 
-		if (fs_devs->opened)
+		if (fs_devices->opened)
 			continue;
 
-		list_for_each_entry_safe(dev, tmp_dev,
-					 &fs_devs->devices, dev_list) {
+		list_for_each_entry_safe(device, tmp_device,
+					 &fs_devices->devices, dev_list) {
 			int not_found = 0;
 
-			if (skip_dev && skip_dev == dev)
+			if (skip_device && skip_device == device)
 				continue;
-			if (path && !dev->name)
+			if (path && !device->name)
 				continue;
 
 			rcu_read_lock();
 			if (path)
-				not_found = strcmp(rcu_str_deref(dev->name),
+				not_found = strcmp(rcu_str_deref(device->name),
 						   path);
 			rcu_read_unlock();
 			if (not_found)
 				continue;
 
 			/* delete the stale device */
-			if (fs_devs->num_devices == 1) {
-				btrfs_sysfs_remove_fsid(fs_devs);
-				list_del(&fs_devs->fs_list);
-				free_fs_devices(fs_devs);
+			if (fs_devices->num_devices == 1) {
+				btrfs_sysfs_remove_fsid(fs_devices);
+				list_del(&fs_devices->fs_list);
+				free_fs_devices(fs_devices);
 				break;
 			} else {
-				fs_devs->num_devices--;
-				list_del(&dev->dev_list);
-				btrfs_free_device(dev);
+				fs_devices->num_devices--;
+				list_del(&device->dev_list);
+				btrfs_free_device(device);
 			}
 		}
 	}
-- 
2.7.0

[PATCH 5/6] btrfs: fix btrfs_free_stale_devices() with needed locks

[Anand Jain]

btrfs_free_stale_devices() finds the stale (not opened) matching
device path in the fs_uuid list. We are already under uuid_mutex
so when we check for each fs_devices, hold the respective
device_list_mutex.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fd656eb75225..a6a23b0185e5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -643,8 +643,11 @@ static void btrfs_free_stale_devices(const char *path,
 	list_for_each_entry_safe(fs_devices, tmp_fs_devices, &fs_uuids,
 				 fs_list) {
 
-		if (fs_devices->opened)
+		mutex_lock(&fs_devices->device_list_mutex);
+		if (fs_devices->opened) {
+			mutex_unlock(&fs_devices->device_list_mutex);
 			continue;
+		}
 
 		list_for_each_entry_safe(device, tmp_device,
 					 &fs_devices->devices, dev_list) {
@@ -664,16 +667,18 @@ static void btrfs_free_stale_devices(const char *path,
 				continue;
 
 			/* delete the stale device */
-			if (fs_devices->num_devices == 1) {
-				btrfs_sysfs_remove_fsid(fs_devices);
-				list_del(&fs_devices->fs_list);
-				free_fs_devices(fs_devices);
+			fs_devices->num_devices--;
+			list_del(&device->dev_list);
+			btrfs_free_device(device);
+
+			if (fs_devices->num_devices == 0)
 				break;
-			} else {
-				fs_devices->num_devices--;
-				list_del(&device->dev_list);
-				btrfs_free_device(device);
-			}
+		}
+		mutex_unlock(&fs_devices->device_list_mutex);
+		if (fs_devices->num_devices == 0) {
+			btrfs_sysfs_remove_fsid(fs_devices);
+			list_del(&fs_devices->fs_list);
+			free_fs_devices(fs_devices);
 		}
 	}
 }
-- 
2.7.0

[PATCH 6/6] btrfs: rename btrfs_free_stale_devices drop btrfs prefix

[Anand Jain]

btrfs_free_stale_devices() is a static function drop the btrfs prefix.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 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 a6a23b0185e5..afb3504f4097 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -634,7 +634,7 @@ static void pending_bios_fn(struct btrfs_work *work)
  *  skip_dev:	Optional. Will skip this device when searching for the stale
  *		devices.
  */
-static void btrfs_free_stale_devices(const char *path,
+static void free_stale_devices(const char *path,
 				     struct btrfs_device *skip_device)
 {
 	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
@@ -1271,7 +1271,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	} else {
 		*fs_devices_ret = device->fs_devices;
 		if (new_device_added)
-			btrfs_free_stale_devices(path, device);
+			free_stale_devices(path, device);
 	}
 	mutex_unlock(&uuid_mutex);
 
-- 
2.7.0

Re: [PATCH 6/6] btrfs: rename btrfs_free_stale_devices drop btrfs prefix

[Nikolay Borisov]

On 30.05.2018 09:58, Anand Jain wrote:
> btrfs_free_stale_devices() is a static function drop the btrfs prefix.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Nit: I think this patch could be dropped, it's not a set-in-stone rule
that static function shouldn't have btrfs_ prefix. Personally, I'd
prefer all subsys function to be prefixed so that in the call stack it's
clear when we are in generic code and when in fs code.

In the end it's the call of the maintainer.

> ---
>  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 a6a23b0185e5..afb3504f4097 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -634,7 +634,7 @@ static void pending_bios_fn(struct btrfs_work *work)
>   *  skip_dev:	Optional. Will skip this device when searching for the stale
>   *		devices.
>   */
> -static void btrfs_free_stale_devices(const char *path,
> +static void free_stale_devices(const char *path,
>  				     struct btrfs_device *skip_device)
>  {
>  	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
> @@ -1271,7 +1271,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>  	} else {
>  		*fs_devices_ret = device->fs_devices;
>  		if (new_device_added)
> -			btrfs_free_stale_devices(path, device);
> +			free_stale_devices(path, device);
>  	}
>  	mutex_unlock(&uuid_mutex);
>  
>