[PATCH 0/15] Review uuid_mutex usage

[PATCH 0/15] Review uuid_mutex usage

[Anand Jain]

uuid_mutex lock is not a per-fs lock but a global lock. The main aim of
this patch-set is to critically review the usage of this lock, and delete
the unnecessary once. By doing this we improve the concurrency of
device operations across multiple btrfs filesystems is in the system.

patch 1: Was sent before, I am including it here, as its about uuid_mutex.

patch 2-9: Are cleanup and or preparatory patches.

patch 10-14: Drops the uuid_mutex and makes sure there is enough lock,
as discussed in the patch change log.

patch 15: A generic cleanup patch around functions in the same context.

These patches are on top of
  https://github.com/kdave/btrfs-devel.git remove-volume-mutex
And it will be a good idea to go along with the kill-volume-mutex patches.

This is tested with xfstests and there are no _new_ regression. And I am
trying to understand the old regressions, and notice that they are
inconsistent.

Anand Jain (15):
  btrfs: optimize move uuid_mutex closer to the critical section
  btrfs: rename struct btrfs_fs_devices::list
  btrfs: cleanup __btrfs_open_devices() drop head pointer
  btrfs: rename __btrfs_close_devices to close_fs_devices
  btrfs: rename __btrfs_open_devices to open_fs_devices
  btrfs: cleanup find_device() drop list_head pointer
  btrfs: cleanup btrfs_rm_device() promote fs_devices pointer
  btrfs: cleanup btrfs_rm_device() use cur_devices
  btrfs: uuid_mutex in read_chunk_tree, add a comment
  btrfs: drop uuid_mutex in btrfs_free_extra_devids()
  btrfs: drop uuid_mutex in btrfs_open_devices()
  btrfs: drop uuid_mutex in close_fs_devices()
  btrfs: drop uuid_mutex in btrfs_dev_replace_finishing()
  btrfs: drop uuid_mutex in btrfs_destroy_dev_replace_tgtdev()
  btrfs: cleanup btrfs_destroy_dev_replace_tgtdev() localize
    btrfs_fs_devices

 fs/btrfs/dev-replace.c |   3 --
 fs/btrfs/sysfs.c       |   2 +-
 fs/btrfs/volumes.c     | 113 ++++++++++++++++++++++++-------------------------
 fs/btrfs/volumes.h     |   2 +-
 4 files changed, 58 insertions(+), 62 deletions(-)

-- 
2.7.0

[PATCH 01/15] btrfs: optimize move uuid_mutex closer to the critical section

[Anand Jain]

Move uuid_mutex closer to the exclusion section.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1d9f20fc3392..7ccd1a38c634 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1206,31 +1206,29 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	 */
 	bytenr = btrfs_sb_offset(0);
 	flags |= FMODE_EXCL;
-	mutex_lock(&uuid_mutex);
 
 	bdev = blkdev_get_by_path(path, flags, holder);
-	if (IS_ERR(bdev)) {
-		ret = PTR_ERR(bdev);
-		goto error;
-	}
+	if (IS_ERR(bdev))
+		return PTR_ERR(bdev);
 
 	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
 		ret = -EINVAL;
 		goto error_bdev_put;
 	}
 
+	mutex_lock(&uuid_mutex);
 	device = device_list_add(path, disk_super);
 	if (IS_ERR(device))
 		ret = PTR_ERR(device);
 	else
 		*fs_devices_ret = device->fs_devices;
+	mutex_unlock(&uuid_mutex);
 
 	btrfs_release_disk_super(page);
 
 error_bdev_put:
 	blkdev_put(bdev, flags);
-error:
-	mutex_unlock(&uuid_mutex);
+
 	return ret;
 }
 
-- 
2.7.0

[PATCH 02/15] btrfs: rename struct btrfs_fs_devices::list

[Anand Jain]

btrfs_fs_devices::list is the list of BTRFS fsid in the kernel, a generic
name 'list' makes it's search very difficult, rename it to fs_list.

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

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index ca067471cd46..ba14d79cf5d2 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -602,7 +602,7 @@ void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs)
 		return;
 	}
 
-	list_for_each_entry(fs_devs, fs_uuids, list) {
+	list_for_each_entry(fs_devs, fs_uuids, fs_list) {
 		__btrfs_sysfs_remove_fsid(fs_devs);
 	}
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7ccd1a38c634..aacaf6c9feb7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -232,7 +232,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid)
 	INIT_LIST_HEAD(&fs_devs->devices);
 	INIT_LIST_HEAD(&fs_devs->resized_devices);
 	INIT_LIST_HEAD(&fs_devs->alloc_list);
-	INIT_LIST_HEAD(&fs_devs->list);
+	INIT_LIST_HEAD(&fs_devs->fs_list);
 	if (fsid)
 		memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE);
 
@@ -278,8 +278,8 @@ void __exit btrfs_cleanup_fs_uuids(void)
 
 	while (!list_empty(&fs_uuids)) {
 		fs_devices = list_entry(fs_uuids.next,
-					struct btrfs_fs_devices, list);
-		list_del(&fs_devices->list);
+					struct btrfs_fs_devices, fs_list);
+		list_del(&fs_devices->fs_list);
 		free_fs_devices(fs_devices);
 	}
 }
@@ -348,7 +348,7 @@ static noinline struct btrfs_fs_devices *find_fsid(u8 *fsid)
 {
 	struct btrfs_fs_devices *fs_devices;
 
-	list_for_each_entry(fs_devices, &fs_uuids, list) {
+	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
 		if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0)
 			return fs_devices;
 	}
@@ -612,7 +612,7 @@ static void btrfs_free_stale_devices(const char *path,
 	struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
 	struct btrfs_device *dev, *tmp_dev;
 
-	list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
+	list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, fs_list) {
 
 		if (fs_devs->opened)
 			continue;
@@ -637,7 +637,7 @@ static void btrfs_free_stale_devices(const char *path,
 			/* delete the stale device */
 			if (fs_devs->num_devices == 1) {
 				btrfs_sysfs_remove_fsid(fs_devs);
-				list_del(&fs_devs->list);
+				list_del(&fs_devs->fs_list);
 				free_fs_devices(fs_devs);
 				break;
 			} else {
@@ -737,7 +737,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		if (IS_ERR(fs_devices))
 			return ERR_CAST(fs_devices);
 
-		list_add(&fs_devices->list, &fs_uuids);
+		list_add(&fs_devices->fs_list, &fs_uuids);
 
 		device = NULL;
 	} else {
@@ -2256,7 +2256,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 		return PTR_ERR(old_devices);
 	}
 
-	list_add(&old_devices->list, &fs_uuids);
+	list_add(&old_devices->fs_list, &fs_uuids);
 
 	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
 	seed_devices->opened = 1;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 681be2dc0c6a..ef220d541d4b 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -221,6 +221,7 @@ BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
 
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
+	struct list_head fs_list;
 
 	u64 num_devices;
 	u64 open_devices;
@@ -242,7 +243,6 @@ struct btrfs_fs_devices {
 	struct list_head resized_devices;
 	/* devices not currently being allocated */
 	struct list_head alloc_list;
-	struct list_head list;
 
 	struct btrfs_fs_devices *seed;
 	int seeding;
-- 
2.7.0

[PATCH 03/15] btrfs: cleanup __btrfs_open_devices() drop head pointer

[Anand Jain]

__btrfs_open_devices() declares struct list_head *head, however head is
used only once, instead use btrfs_fs_devices::devices directly.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index aacaf6c9feb7..793471eb3f51 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1074,14 +1074,13 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 static int __btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 				fmode_t flags, void *holder)
 {
-	struct list_head *head = &fs_devices->devices;
 	struct btrfs_device *device;
 	struct btrfs_device *latest_dev = NULL;
 	int ret = 0;
 
 	flags |= FMODE_EXCL;
 
-	list_for_each_entry(device, head, dev_list) {
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		/* Just open everything we can; ignore failures here */
 		if (btrfs_open_one_device(fs_devices, device, flags, holder))
 			continue;
-- 
2.7.0

[PATCH 04/15] btrfs: rename __btrfs_close_devices to close_fs_devices

[Anand Jain]

__btrfs_close_devices() is un-exported, drop the __ prefix and rename it
to close_fs_devices().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 793471eb3f51..ef8578bf6b99 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1010,7 +1010,7 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
 	new_device->fs_devices = device->fs_devices;
 }
 
-static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
+static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device, *tmp;
 	struct list_head pending_put;
@@ -1055,7 +1055,7 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 	int ret;
 
 	mutex_lock(&uuid_mutex);
-	ret = __btrfs_close_devices(fs_devices);
+	ret = close_fs_devices(fs_devices);
 	if (!fs_devices->opened) {
 		seed_devices = fs_devices->seed;
 		fs_devices->seed = NULL;
@@ -1065,7 +1065,7 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 	while (seed_devices) {
 		fs_devices = seed_devices;
 		seed_devices = fs_devices->seed;
-		__btrfs_close_devices(fs_devices);
+		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
 	return ret;
@@ -2031,7 +2031,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 			fs_devices = fs_devices->seed;
 		}
 		cur_devices->seed = NULL;
-		__btrfs_close_devices(cur_devices);
+		close_fs_devices(cur_devices);
 		free_fs_devices(cur_devices);
 	}
 
@@ -2112,7 +2112,7 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 			tmp_fs_devices = tmp_fs_devices->seed;
 		}
 		fs_devices->seed = NULL;
-		__btrfs_close_devices(fs_devices);
+		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
 }
@@ -6578,7 +6578,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	}
 
 	if (!fs_devices->seeding) {
-		__btrfs_close_devices(fs_devices);
+		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 		fs_devices = ERR_PTR(-EINVAL);
 		goto out;
-- 
2.7.0

[PATCH 05/15] btrfs: rename __btrfs_open_devices to open_fs_devices

[Anand Jain]

__btrfs_open_devices() is un-exported drop __ prefix and rename it to
open_fs_devices().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ef8578bf6b99..9ed670ffa6a5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1071,7 +1071,7 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 	return ret;
 }
 
-static int __btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
+static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 				fmode_t flags, void *holder)
 {
 	struct btrfs_device *device;
@@ -1125,7 +1125,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		ret = 0;
 	} else {
 		list_sort(NULL, &fs_devices->devices, devid_cmp);
-		ret = __btrfs_open_devices(fs_devices, flags, holder);
+		ret = open_fs_devices(fs_devices, flags, holder);
 	}
 	mutex_unlock(&uuid_mutex);
 	return ret;
@@ -6569,8 +6569,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	if (IS_ERR(fs_devices))
 		return fs_devices;
 
-	ret = __btrfs_open_devices(fs_devices, FMODE_READ,
-				   fs_info->bdev_holder);
+	ret = open_fs_devices(fs_devices, FMODE_READ, fs_info->bdev_holder);
 	if (ret) {
 		free_fs_devices(fs_devices);
 		fs_devices = ERR_PTR(ret);
-- 
2.7.0

[PATCH 06/15] btrfs: cleanup find_device() drop list_head pointer

[Anand Jain]

find_device() declares struct list_head *head pointer and used only once,
instead just use it directly.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9ed670ffa6a5..5714cdaf9c03 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -332,10 +332,9 @@ static struct btrfs_device *__alloc_device(void)
 static struct btrfs_device *find_device(struct btrfs_fs_devices *fs_devices,
 		u64 devid, const u8 *uuid)
 {
-	struct list_head *head = &fs_devices->devices;
 	struct btrfs_device *dev;
 
-	list_for_each_entry(dev, head, dev_list) {
+	list_for_each_entry(dev, &fs_devices->devices, dev_list) {
 		if (dev->devid == devid &&
 		    (!uuid || !memcmp(dev->uuid, uuid, BTRFS_UUID_SIZE))) {
 			return dev;
-- 
2.7.0

[PATCH 07/15] btrfs: cleanup btrfs_rm_device() promote fs_devices pointer

[Anand Jain]

This function uses fs_info::fs_devices number of time, however we declare
and use it only at the end, instead do it in the beginning of the function
and use it.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5714cdaf9c03..db077c505fe0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1918,12 +1918,13 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *cur_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	u64 num_devices;
 	int ret = 0;
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = fs_info->fs_devices->num_devices;
+	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);
@@ -1987,7 +1988,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	 */
 
 	cur_devices = device->fs_devices;
-	mutex_lock(&fs_info->fs_devices->device_list_mutex);
+	mutex_lock(&fs_devices->device_list_mutex);
 	list_del_rcu(&device->dev_list);
 
 	device->fs_devices->num_devices--;
@@ -2001,12 +2002,12 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (device->bdev) {
 		device->fs_devices->open_devices--;
 		/* remove sysfs entry */
-		btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
+		btrfs_sysfs_rm_device_link(fs_devices, device);
 	}
 
 	num_devices = btrfs_super_num_devices(fs_info->super_copy) - 1;
 	btrfs_set_super_num_devices(fs_info->super_copy, num_devices);
-	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
+	mutex_unlock(&fs_devices->device_list_mutex);
 
 	/*
 	 * at this point, the device is zero sized and detached from
@@ -2020,8 +2021,6 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	call_rcu(&device->rcu, free_device_rcu);
 
 	if (cur_devices->open_devices == 0) {
-		struct btrfs_fs_devices *fs_devices;
-		fs_devices = fs_info->fs_devices;
 		while (fs_devices) {
 			if (fs_devices->seed == cur_devices) {
 				fs_devices->seed = cur_devices->seed;
@@ -2042,7 +2041,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 		mutex_lock(&fs_info->chunk_mutex);
 		list_add(&device->dev_alloc_list,
-			 &fs_info->fs_devices->alloc_list);
+			 &fs_devices->alloc_list);
 		device->fs_devices->rw_devices++;
 		mutex_unlock(&fs_info->chunk_mutex);
 	}
-- 
2.7.0

[PATCH 08/15] btrfs: cleanup btrfs_rm_device() use cur_devices

[Anand Jain]

Instead of de-referencing the device->fs_devices use cur_devices
which points to the same fs_devices.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index db077c505fe0..29799d39554a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1987,20 +1987,25 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	 * (super_copy) should hold the device list mutex.
 	 */
 
+	/*
+	 * In normal cases the cur_devices == fs_devices. But in case
+	 * of deleting a seed device, the cur_devices should point to
+	 * its own fs_devices listed under the fs_devices->seed.
+	 */
 	cur_devices = device->fs_devices;
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_del_rcu(&device->dev_list);
 
-	device->fs_devices->num_devices--;
-	device->fs_devices->total_devices--;
+	cur_devices->num_devices--;
+	cur_devices->total_devices--;
 
 	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
-		device->fs_devices->missing_devices--;
+		cur_devices->missing_devices--;
 
 	btrfs_assign_next_active_device(fs_info, device, NULL);
 
 	if (device->bdev) {
-		device->fs_devices->open_devices--;
+		cur_devices->open_devices--;
 		/* remove sysfs entry */
 		btrfs_sysfs_rm_device_link(fs_devices, device);
 	}
-- 
2.7.0

[PATCH 09/15] btrfs: uuid_mutex in read_chunk_tree, add a comment

[Anand Jain]

read_chunk_tree() calls read_one_dev(), but for seed device we have
to search the fs_uuids list, so we need the uuid_mutex. Add a comment
comment, so that we can improve this part.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 29799d39554a..8996a1df9a21 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6889,6 +6889,10 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 	if (!path)
 		return -ENOMEM;
 
+	/*
+	 * uuid_mutex is needed only if we are mounting a sprout FS
+	 * otherwise we don't need it.
+	 */
 	mutex_lock(&uuid_mutex);
 	mutex_lock(&fs_info->chunk_mutex);
 
-- 
2.7.0

[PATCH 10/15] btrfs: drop uuid_mutex in btrfs_free_extra_devids()

[Anand Jain]

btrfs_free_extra_devids() frees the orphan fsid::devid but its search is
limited to btrfs_fs_devices::devices, so we dont need uuid_mutex.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 8996a1df9a21..85948ef12ecf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -897,7 +897,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 	struct btrfs_device *device, *next;
 	struct btrfs_device *latest_dev = NULL;
 
-	mutex_lock(&uuid_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) {
@@ -951,8 +950,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 	}
 
 	fs_devices->latest_bdev = latest_dev->bdev;
-
-	mutex_unlock(&uuid_mutex);
 }
 
 static void free_device_rcu(struct rcu_head *head)
-- 
2.7.0

Re: [PATCH 10/15] btrfs: drop uuid_mutex in btrfs_free_extra_devids()

[David Sterba]

On Thu, Apr 12, 2018 at 10:29:33AM +0800, Anand Jain wrote:
> btrfs_free_extra_devids() frees the orphan fsid::devid but its search is
> limited to btrfs_fs_devices::devices, so we dont need uuid_mutex.

>From that it's not clear why there's no locking at all now:

> @@ -897,7 +897,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
>  	struct btrfs_device *device, *next;
>  	struct btrfs_device *latest_dev = NULL;
>  
> -	mutex_lock(&uuid_mutex);

ie. why is this not replaced by the device_list_lock. Please resend this
patch and explain in the changelog.

>  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) {
> @@ -951,8 +950,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
>  	}
>  
>  	fs_devices->latest_bdev = latest_dev->bdev;
> -
> -	mutex_unlock(&uuid_mutex);
>  }
>  
>  static void free_device_rcu(struct rcu_head *head)

Re: [PATCH 10/15] btrfs: drop uuid_mutex in btrfs_free_extra_devids()

[Anand Jain]

On 05/16/2018 12:26 AM, David Sterba wrote:
> On Thu, Apr 12, 2018 at 10:29:33AM +0800, Anand Jain wrote:
>> btrfs_free_extra_devids() frees the orphan fsid::devid but its search is
>> limited to btrfs_fs_devices::devices, so we dont need uuid_mutex.
> 
>  From that it's not clear why there's no locking at all now:
> 
>> @@ -897,7 +897,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
>>   	struct btrfs_device *device, *next;
>>   	struct btrfs_device *latest_dev = NULL;
>>   
>> -	mutex_lock(&uuid_mutex);
> 
> ie. why is this not replaced by the device_list_lock. Please resend this
> patch and explain in the changelog.

  My apologies for the delay.

  btrfs_free_extra_devids() needs device_list_mutex, will send a v2 for
  this.

  Thanks.

>>   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) {
>> @@ -951,8 +950,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
>>   	}
>>   
>>   	fs_devices->latest_bdev = latest_dev->bdev;
>> -
>> -	mutex_unlock(&uuid_mutex);
>>   }
>>   
>>   static void free_device_rcu(struct rcu_head *head)
> --
> 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
>

[PATCH v3] 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>
---
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.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b6757b53c297..a9c1f4f7ebd0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -925,7 +925,7 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 	struct btrfs_device *device, *next;
 	struct btrfs_device *latest_dev = NULL;
 
-	mutex_lock(&uuid_mutex);
+	mutex_lock(&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) {
@@ -979,8 +979,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(&fs_devices->device_list_mutex);
 }
 
 static void free_device_rcu(struct rcu_head *head)
-- 
2.7.0

Re: [PATCH v3] btrfs: drop uuid_mutex in btrfs_free_extra_devids()

[David Sterba]

On Wed, May 23, 2018 at 10:54:22AM +0800, Anand Jain wrote:
> 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>

Thanks, this explanation is much better and addresses the questions I
have while reading the code.

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

> ---
> Currently device_list_add() is very lean on its device_list_mutex usage,
> a cleanup and fix is wip.

I also have a WIP patch to rewrite device_list_add. There were quite
some changes around the device locking so I'd need to refresh it on top
of current code first, it's not in a shape to be posted yet.

[PATCH v4] 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>
---
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 top most
	fs_devices pointer.
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 b6757b53c297..f03719221fca 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -924,8 +924,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) {
@@ -979,8 +980,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 11/15] btrfs: drop uuid_mutex in btrfs_open_devices()

[Anand Jain]

btrfs_open_devices() is using uuid_mutex, but as the btrfs_open_devices()
is just limited to open all the devices under a given fsid, so we don't
need uuid_mutex.

Instead it should hold the device_list_mutex as it updated the status
of the btrfs_fs_devices and btrfs_device of a fsid.

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 85948ef12ecf..dfebf8f29916 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1115,7 +1115,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 {
 	int ret;
 
-	mutex_lock(&uuid_mutex);
+	mutex_lock(&fs_devices->device_list_mutex);
 	if (fs_devices->opened) {
 		fs_devices->opened++;
 		ret = 0;
@@ -1123,7 +1123,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		list_sort(NULL, &fs_devices->devices, devid_cmp);
 		ret = open_fs_devices(fs_devices, flags, holder);
 	}
-	mutex_unlock(&uuid_mutex);
+	mutex_unlock(&fs_devices->device_list_mutex);
 	return ret;
 }
 
-- 
2.7.0

[PATCH 12/15] btrfs: drop uuid_mutex in close_fs_devices()

[Anand Jain]

close_fs_devices() closes devices of a given fsid, and it is limited
to all the devices of a fsid, so we don't have to hold the global
uuid_mutex, instead we need the device_list_mutex as the device state is
being changed.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index dfebf8f29916..4c29214e0c18 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -995,7 +995,6 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
 					device->uuid);
 	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
 
-	/* Safe because we are under uuid_mutex */
 	if (device->name) {
 		name = rcu_string_strdup(device->name->str, GFP_NOFS);
 		BUG_ON(!name); /* -ENOMEM */
@@ -1013,10 +1012,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 
 	INIT_LIST_HEAD(&pending_put);
 
-	if (--fs_devices->opened > 0)
+	mutex_lock(&fs_devices->device_list_mutex);
+	if (--fs_devices->opened > 0) {
+		mutex_unlock(&fs_devices->device_list_mutex);
 		return 0;
+	}
 
-	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
 		btrfs_prepare_close_one_device(device);
 		list_add(&device->dev_list, &pending_put);
@@ -1050,13 +1051,11 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 	struct btrfs_fs_devices *seed_devices = NULL;
 	int ret;
 
-	mutex_lock(&uuid_mutex);
 	ret = close_fs_devices(fs_devices);
 	if (!fs_devices->opened) {
 		seed_devices = fs_devices->seed;
 		fs_devices->seed = NULL;
 	}
-	mutex_unlock(&uuid_mutex);
 
 	while (seed_devices) {
 		fs_devices = seed_devices;
-- 
2.7.0

Re: [PATCH 12/15] btrfs: drop uuid_mutex in close_fs_devices()

[David Sterba]

On Thu, Apr 12, 2018 at 10:29:35AM +0800, Anand Jain wrote:
> close_fs_devices() closes devices of a given fsid, and it is limited
> to all the devices of a fsid, so we don't have to hold the global
> uuid_mutex, instead we need the device_list_mutex as the device state is
> being changed.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index dfebf8f29916..4c29214e0c18 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -995,7 +995,6 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
>  					device->uuid);
>  	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
>  
> -	/* Safe because we are under uuid_mutex */
>  	if (device->name) {
>  		name = rcu_string_strdup(device->name->str, GFP_NOFS);
>  		BUG_ON(!name); /* -ENOMEM */
> @@ -1013,10 +1012,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>  
>  	INIT_LIST_HEAD(&pending_put);
>  
> -	if (--fs_devices->opened > 0)
> +	mutex_lock(&fs_devices->device_list_mutex);
> +	if (--fs_devices->opened > 0) {
> +		mutex_unlock(&fs_devices->device_list_mutex);
>  		return 0;
> +	}
>  
> -	mutex_lock(&fs_devices->device_list_mutex);
>  	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
>  		btrfs_prepare_close_one_device(device);
>  		list_add(&device->dev_list, &pending_put);
> @@ -1050,13 +1051,11 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
>  	struct btrfs_fs_devices *seed_devices = NULL;
>  	int ret;
>  
> -	mutex_lock(&uuid_mutex);
>  	ret = close_fs_devices(fs_devices);
>  	if (!fs_devices->opened) {
>  		seed_devices = fs_devices->seed;
>  		fs_devices->seed = NULL;

This still touches ->seed, and also reads ->opened, some locking is
required here or it has to be clearly explained why it's not.

>  	}
> -	mutex_unlock(&uuid_mutex);
>  
>  	while (seed_devices) {
>  		fs_devices = seed_devices;
> -- 
> 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 12/15] btrfs: drop uuid_mutex in close_fs_devices()

[Anand Jain]

On 05/16/2018 12:30 AM, David Sterba wrote:
> On Thu, Apr 12, 2018 at 10:29:35AM +0800, Anand Jain wrote:
>> close_fs_devices() closes devices of a given fsid, and it is limited
>> to all the devices of a fsid, so we don't have to hold the global
>> uuid_mutex, instead we need the device_list_mutex as the device state is
>> being changed.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index dfebf8f29916..4c29214e0c18 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -995,7 +995,6 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
>>   					device->uuid);
>>   	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
>>   
>> -	/* Safe because we are under uuid_mutex */
>>   	if (device->name) {
>>   		name = rcu_string_strdup(device->name->str, GFP_NOFS);
>>   		BUG_ON(!name); /* -ENOMEM */
>> @@ -1013,10 +1012,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>>   
>>   	INIT_LIST_HEAD(&pending_put);
>>   
>> -	if (--fs_devices->opened > 0)
>> +	mutex_lock(&fs_devices->device_list_mutex);
>> +	if (--fs_devices->opened > 0) {
>> +		mutex_unlock(&fs_devices->device_list_mutex);
>>   		return 0;
>> +	}
>>   
>> -	mutex_lock(&fs_devices->device_list_mutex);
>>   	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
>>   		btrfs_prepare_close_one_device(device);
>>   		list_add(&device->dev_list, &pending_put);
>> @@ -1050,13 +1051,11 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
>>   	struct btrfs_fs_devices *seed_devices = NULL;
>>   	int ret;
>>   
>> -	mutex_lock(&uuid_mutex);
>>   	ret = close_fs_devices(fs_devices);
>>   	if (!fs_devices->opened) {
>>   		seed_devices = fs_devices->seed;
>>   		fs_devices->seed = NULL;
> 
> This still touches ->seed, and also reads ->opened, some locking is
> required here or it has to be clearly explained why it's not.

  The ->seed here is %fs_devices local so we don't need the
  global uuid_mutex. We cloned it in btrfs_prepare_sprout().

  Will add comments and send v2.

Thanks, Anand

>>   	}
>> -	mutex_unlock(&uuid_mutex);
>>   
>>   	while (seed_devices) {
>>   		fs_devices = seed_devices;
>> -- 
>> 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
> 

[PATCH 13/15] btrfs: drop uuid_mutex in btrfs_dev_replace_finishing()

[Anand Jain]

btrfs_dev_replace_finish() updates devices (soruce and target) which
are within the btrfs_fs_devices::devices or withint the cloned seed
devices (btrfs_fs_devices::seed::devices), so we don't need the global
uuid_mutex.

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

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index ba011af9b0d2..da937eb064aa 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -621,7 +621,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	ret = btrfs_commit_transaction(trans);
 	WARN_ON(ret);
 
-	mutex_lock(&uuid_mutex);
 	/* keep away write_all_supers() during the finishing procedure */
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	mutex_lock(&fs_info->chunk_mutex);
@@ -648,7 +647,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 		btrfs_dev_replace_write_unlock(dev_replace);
 		mutex_unlock(&fs_info->chunk_mutex);
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
-		mutex_unlock(&uuid_mutex);
 		btrfs_rm_dev_replace_blocked(fs_info);
 		if (tgt_device)
 			btrfs_destroy_dev_replace_tgtdev(fs_info, tgt_device);
@@ -699,7 +697,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	 */
 	mutex_unlock(&fs_info->chunk_mutex);
 	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
-	mutex_unlock(&uuid_mutex);
 
 	/* replace the sysfs entry */
 	btrfs_sysfs_rm_device_link(fs_info->fs_devices, src_device);
-- 
2.7.0

[PATCH 14/15] btrfs: drop uuid_mutex in btrfs_destroy_dev_replace_tgtdev()

[Anand Jain]

Delete the uuid_mutex lock here as this thread access the
btrfs_fs_devices::devices only. And the device_list_mutex lock is already
in place.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4c29214e0c18..1f694083c15f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2119,7 +2119,6 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 				      struct btrfs_device *tgtdev)
 {
-	mutex_lock(&uuid_mutex);
 	WARN_ON(!tgtdev);
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 
@@ -2135,7 +2134,6 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 	list_del_rcu(&tgtdev->dev_list);
 
 	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
-	mutex_unlock(&uuid_mutex);
 
 	/*
 	 * The update_dev_time() with in btrfs_scratch_superblocks()
-- 
2.7.0

[PATCH 15/15] btrfs: cleanup btrfs_destroy_dev_replace_tgtdev() localize btrfs_fs_devices

[Anand Jain]

Declare a local btrfs_fs_devices pointer to access the same, as there are
more than one access to the pointer.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1f694083c15f..d5f8f9a81536 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2119,21 +2119,23 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 				      struct btrfs_device *tgtdev)
 {
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+
 	WARN_ON(!tgtdev);
-	mutex_lock(&fs_info->fs_devices->device_list_mutex);
+	mutex_lock(&fs_devices->device_list_mutex);
 
-	btrfs_sysfs_rm_device_link(fs_info->fs_devices, tgtdev);
+	btrfs_sysfs_rm_device_link(fs_devices, tgtdev);
 
 	if (tgtdev->bdev)
-		fs_info->fs_devices->open_devices--;
+		fs_devices->open_devices--;
 
-	fs_info->fs_devices->num_devices--;
+	fs_devices->num_devices--;
 
 	btrfs_assign_next_active_device(fs_info, tgtdev, NULL);
 
 	list_del_rcu(&tgtdev->dev_list);
 
-	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
+	mutex_unlock(&fs_devices->device_list_mutex);
 
 	/*
 	 * The update_dev_time() with in btrfs_scratch_superblocks()
-- 
2.7.0

Re: [PATCH 0/15] Review uuid_mutex usage

[David Sterba]

On Thu, Apr 12, 2018 at 10:29:23AM +0800, Anand Jain wrote:
> uuid_mutex lock is not a per-fs lock but a global lock. The main aim of
> this patch-set is to critically review the usage of this lock, and delete
> the unnecessary once. By doing this we improve the concurrency of
> device operations across multiple btrfs filesystems is in the system.
> 
> patch 1: Was sent before, I am including it here, as its about uuid_mutex.
> 
> patch 2-9: Are cleanup and or preparatory patches.
> 
> patch 10-14: Drops the uuid_mutex and makes sure there is enough lock,
> as discussed in the patch change log.
> 
> patch 15: A generic cleanup patch around functions in the same context.
> 
> These patches are on top of
>   https://github.com/kdave/btrfs-devel.git remove-volume-mutex
> And it will be a good idea to go along with the kill-volume-mutex patches.
> 
> This is tested with xfstests and there are no _new_ regression. And I am
> trying to understand the old regressions, and notice that they are
> inconsistent.

The series looks good to me, I don't see any obvious breakage when the
uuid mutex is being removed, most of the device list consistency is
guarded by the device_list_mutex.

It indeed looks like uuid_mutex is overused and I've noticed that the
lock description in volumes.c is not entirely correct as it says that
eg. the device count and count of rw devices is proteced by that lock.

This would be good to update too, so it matches the code.

I'll add the branch to for-next to expose it to more testing.

[PATCH] btrfs: update uuid_mutex and device_list_mutex comments

[Anand Jain]

Make the uuid_mutex and device_list_mutex comments inline with
the changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Hi David,

 Can you kindly add this patch to the set: 'Review uuid_mutex usage'

Thanks, Anand

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6d7f10729713..12617a8a7083 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -155,29 +155,26 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
  *
  * uuid_mutex (global lock)
  * ------------------------
- * protects the fs_uuids list that tracks all per-fs fs_devices, resulting from
+ * Protects the fs_uuids list that tracks all per-fs fs_devices, resulting from
  * the SCAN_DEV ioctl registration or from mount either implicitly (the first
- * device) or requested by the device= mount option
- *
- * the mutex can be very coarse and can cover long-running operations
- *
- * protects: updates to fs_devices counters like missing devices, rw devices,
- * seeding, structure cloning, openning/closing devices at mount/umount time
+ * device) or requested by the device= mount option.
  *
  * global::fs_devs - add, remove, updates to the global list
  *
- * does not protect: manipulation of the fs_devices::devices list!
+ * Does not protect: manipulation of the fs_devices::devices list!
  *
  * btrfs_device::name - renames (write side), read is RCU
  *
  * fs_devices::device_list_mutex (per-fs, with RCU)
  * ------------------------------------------------
- * protects updates to fs_devices::devices, ie. adding and deleting
+ * Protects updates to fs_devices::devices, ie. adding and deleting, and its
+ * counters like missing devices, rw devices, seeding, structure cloning,
+ * openning/closing devices at mount/umount time.
  *
- * simple list traversal with read-only actions can be done with RCU protection
+ * Simple list traversal with read-only actions can be done with RCU protection.
  *
- * may be used to exclude some operations from running concurrently without any
- * modifications to the list (see write_all_supers)
+ * May be used to exclude some operations from running concurrently without any
+ * modifications to the list (see write_all_supers).
  *
  * balance_mutex
  * -------------
-- 
2.15.0

Re: [PATCH] btrfs: update uuid_mutex and device_list_mutex comments

[David Sterba]

On Wed, Apr 18, 2018 at 05:56:31PM +0800, Anand Jain wrote:
> @@ -155,29 +155,26 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>   *
>   * uuid_mutex (global lock)
>   * ------------------------
> - * protects the fs_uuids list that tracks all per-fs fs_devices, resulting from
> + * Protects the fs_uuids list that tracks all per-fs fs_devices, resulting from
>   * the SCAN_DEV ioctl registration or from mount either implicitly (the first
> - * device) or requested by the device= mount option
> - *
> - * the mutex can be very coarse and can cover long-running operations
> - *
> - * protects: updates to fs_devices counters like missing devices, rw devices,
> - * seeding, structure cloning, openning/closing devices at mount/umount time
> + * device) or requested by the device= mount option.
>   *
>   * global::fs_devs - add, remove, updates to the global list
              ^^^^^^^

My typo, this should be fs_uuids.

>   * fs_devices::device_list_mutex (per-fs, with RCU)
>   * ------------------------------------------------
> - * protects updates to fs_devices::devices, ie. adding and deleting
> + * Protects updates to fs_devices::devices, ie. adding and deleting, and its
> + * counters like missing devices, rw devices, seeding, structure cloning,
> + * openning/closing devices at mount/umount time.
>   *
> - * simple list traversal with read-only actions can be done with RCU protection
> + * Simple list traversal with read-only actions can be done with RCU protection.
>   *
> - * may be used to exclude some operations from running concurrently without any
> - * modifications to the list (see write_all_supers)
> + * May be used to exclude some operations from running concurrently without any
> + * modifications to the list (see write_all_supers).

The uuid_mutex usage is a bit muddy, so far I think that most uses are
not necessary so this is in line with this patchset. In some cases we
might need to add the device_list_mutex once uuid mutex is gone.

The clear usage of the uuid_mutex is when a new fs_devices is added to
the global fs_uuids, to prevent concurrent access by device scan and
mount. Another one is the seed fs manipulation, that on the higher level
works on the linked fs_devices. And the last one is the device renames
that happen after a device appears under a different name.

So far I haven't noticed any problems during tests of for-next with this
patchset, so I guess we'd have to try harder to trigger the potential
races. Thre's no device add/delete/replace/scan stress tests. The
seeding is not very well covered by tests, so I'll keep the branch in
for-next, but more tests are welcome.

Re: [PATCH] btrfs: update uuid_mutex and device_list_mutex comments

[Anand Jain]

On 04/24/2018 11:48 PM, David Sterba wrote:
> On Wed, Apr 18, 2018 at 05:56:31PM +0800, Anand Jain wrote:
>> @@ -155,29 +155,26 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>>    *
>>    * uuid_mutex (global lock)
>>    * ------------------------
>> - * protects the fs_uuids list that tracks all per-fs fs_devices, resulting from
>> + * Protects the fs_uuids list that tracks all per-fs fs_devices, resulting from
>>    * the SCAN_DEV ioctl registration or from mount either implicitly (the first
>> - * device) or requested by the device= mount option
>> - *
>> - * the mutex can be very coarse and can cover long-running operations
>> - *
>> - * protects: updates to fs_devices counters like missing devices, rw devices,
>> - * seeding, structure cloning, openning/closing devices at mount/umount time
>> + * device) or requested by the device= mount option.
>>    *
>>    * global::fs_devs - add, remove, updates to the global list
>                ^^^^^^^
> 
> My typo, this should be fs_uuids.

right. Corrected in v2.

> 
>>    * fs_devices::device_list_mutex (per-fs, with RCU)
>>    * ------------------------------------------------
>> - * protects updates to fs_devices::devices, ie. adding and deleting
>> + * Protects updates to fs_devices::devices, ie. adding and deleting, and its
>> + * counters like missing devices, rw devices, seeding, structure cloning,
>> + * openning/closing devices at mount/umount time.
>>    *
>> - * simple list traversal with read-only actions can be done with RCU protection
>> + * Simple list traversal with read-only actions can be done with RCU protection.
>>    *
>> - * may be used to exclude some operations from running concurrently without any
>> - * modifications to the list (see write_all_supers)
>> + * May be used to exclude some operations from running concurrently without any
>> + * modifications to the list (see write_all_supers).
> 
> The uuid_mutex usage is a bit muddy, so far I think that most uses are
> not necessary so this is in line with this patchset. In some cases we
> might need to add the device_list_mutex once uuid mutex is gone.

> The clear usage of the uuid_mutex is when a new fs_devices is added to
> the global fs_uuids, to prevent concurrent access by device scan and
> mount.

  Yes. I have part#2 of uuid_mutex which will cleanup this part.
  I got diverged into something else before I could send. Will send soon.
  Sorry for the delay.

> Another one is the seed fs manipulation, that on the higher level
> works on the linked fs_devices. And the last one is the device renames
> that happen after a device appears under a different name.

  Sprout doesn't need uuid_mutex, it would need device_list_mutex of
  the respective seed fs_devices. I am planning this in part#3.
  As of now its ok to continue to use uuid_mutex.

> So far I haven't noticed any problems during tests of for-next with this
> patchset, so I guess we'd have to try harder to trigger the potential
> races.

> Thre's no device add/delete/replace/scan stress tests.

   stress tests - to exerciser concurrency - right.

> The
> seeding is not very well covered by tests, so I'll keep the branch in
> for-next, but more tests are welcome.

  Let me find time to add in part#3 as above.

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 0/15] Review uuid_mutex usage

[David Sterba]

On Thu, Apr 12, 2018 at 10:29:23AM +0800, Anand Jain wrote:
> uuid_mutex lock is not a per-fs lock but a global lock. The main aim of
> this patch-set is to critically review the usage of this lock, and delete
> the unnecessary once. By doing this we improve the concurrency of
> device operations across multiple btrfs filesystems is in the system.
> 
> patch 1: Was sent before, I am including it here, as its about uuid_mutex.
> 
> patch 2-9: Are cleanup and or preparatory patches.
> 
> patch 10-14: Drops the uuid_mutex and makes sure there is enough lock,
> as discussed in the patch change log.
> 
> patch 15: A generic cleanup patch around functions in the same context.
> 
> These patches are on top of
>   https://github.com/kdave/btrfs-devel.git remove-volume-mutex
> And it will be a good idea to go along with the kill-volume-mutex patches.
> 
> This is tested with xfstests and there are no _new_ regression. And I am
> trying to understand the old regressions, and notice that they are
> inconsistent.
> 
> Anand Jain (15):
>   btrfs: optimize move uuid_mutex closer to the critical section

FYI, the patches:

>   btrfs: rename struct btrfs_fs_devices::list
>   btrfs: cleanup __btrfs_open_devices() drop head pointer
>   btrfs: rename __btrfs_close_devices to close_fs_devices
>   btrfs: rename __btrfs_open_devices to open_fs_devices
>   btrfs: cleanup find_device() drop list_head pointer
>   btrfs: cleanup btrfs_rm_device() promote fs_devices pointer

have been added to misc-next as they're independent cleanups. The rest
is still under review.

Re: [PATCH 0/15] Review uuid_mutex usage

[David Sterba]

On Thu, Apr 12, 2018 at 10:29:23AM +0800, Anand Jain wrote:
> uuid_mutex lock is not a per-fs lock but a global lock. The main aim of
> this patch-set is to critically review the usage of this lock, and delete
> the unnecessary once. By doing this we improve the concurrency of
> device operations across multiple btrfs filesystems is in the system.
> 
> patch 1: Was sent before, I am including it here, as its about uuid_mutex.
> 
> patch 2-9: Are cleanup and or preparatory patches.
> 
> patch 10-14: Drops the uuid_mutex and makes sure there is enough lock,
> as discussed in the patch change log.
> 
> patch 15: A generic cleanup patch around functions in the same context.
> 
> These patches are on top of
>   https://github.com/kdave/btrfs-devel.git remove-volume-mutex
> And it will be a good idea to go along with the kill-volume-mutex patches.
> 
> This is tested with xfstests and there are no _new_ regression. And I am
> trying to understand the old regressions, and notice that they are
> inconsistent.
> 
> Anand Jain (15):
>   btrfs: optimize move uuid_mutex closer to the critical section
>   btrfs: rename struct btrfs_fs_devices::list
>   btrfs: cleanup __btrfs_open_devices() drop head pointer
>   btrfs: rename __btrfs_close_devices to close_fs_devices
>   btrfs: rename __btrfs_open_devices to open_fs_devices
>   btrfs: cleanup find_device() drop list_head pointer
>   btrfs: cleanup btrfs_rm_device() promote fs_devices pointer
>   btrfs: cleanup btrfs_rm_device() use cur_devices
>   btrfs: uuid_mutex in read_chunk_tree, add a comment
>   btrfs: drop uuid_mutex in btrfs_free_extra_devids()
>   btrfs: drop uuid_mutex in btrfs_open_devices()
>   btrfs: drop uuid_mutex in close_fs_devices()
>   btrfs: drop uuid_mutex in btrfs_dev_replace_finishing()
>   btrfs: drop uuid_mutex in btrfs_destroy_dev_replace_tgtdev()
>   btrfs: cleanup btrfs_destroy_dev_replace_tgtdev() localize
>     btrfs_fs_devices

Patches 10 and 12 haven't been merged, the rest is now in misc-next.
Testing hasn't revealed any problems related to the uuid/device locks
but as said before we don't have stress tests.

Re: [PATCH 0/15] Review uuid_mutex usage

[Anand Jain]

On 05/16/2018 12:33 AM, David Sterba wrote:
> On Thu, Apr 12, 2018 at 10:29:23AM +0800, Anand Jain wrote:
>> uuid_mutex lock is not a per-fs lock but a global lock. The main aim of
>> this patch-set is to critically review the usage of this lock, and delete
>> the unnecessary once. By doing this we improve the concurrency of
>> device operations across multiple btrfs filesystems is in the system.
>>
>> patch 1: Was sent before, I am including it here, as its about uuid_mutex.
>>
>> patch 2-9: Are cleanup and or preparatory patches.
>>
>> patch 10-14: Drops the uuid_mutex and makes sure there is enough lock,
>> as discussed in the patch change log.
>>
>> patch 15: A generic cleanup patch around functions in the same context.
>>
>> These patches are on top of
>>    https://github.com/kdave/btrfs-devel.git remove-volume-mutex
>> And it will be a good idea to go along with the kill-volume-mutex patches.
>>
>> This is tested with xfstests and there are no _new_ regression. And I am
>> trying to understand the old regressions, and notice that they are
>> inconsistent.
>>
>> Anand Jain (15):
>>    btrfs: optimize move uuid_mutex closer to the critical section
>>    btrfs: rename struct btrfs_fs_devices::list
>>    btrfs: cleanup __btrfs_open_devices() drop head pointer
>>    btrfs: rename __btrfs_close_devices to close_fs_devices
>>    btrfs: rename __btrfs_open_devices to open_fs_devices
>>    btrfs: cleanup find_device() drop list_head pointer
>>    btrfs: cleanup btrfs_rm_device() promote fs_devices pointer
>>    btrfs: cleanup btrfs_rm_device() use cur_devices
>>    btrfs: uuid_mutex in read_chunk_tree, add a comment
>>    btrfs: drop uuid_mutex in btrfs_free_extra_devids()
>>    btrfs: drop uuid_mutex in btrfs_open_devices()
>>    btrfs: drop uuid_mutex in close_fs_devices()
>>    btrfs: drop uuid_mutex in btrfs_dev_replace_finishing()
>>    btrfs: drop uuid_mutex in btrfs_destroy_dev_replace_tgtdev()
>>    btrfs: cleanup btrfs_destroy_dev_replace_tgtdev() localize
>>      btrfs_fs_devices
> 
> Patches 10 and 12 haven't been merged, the rest is now in misc-next.
> Testing hasn't revealed any problems related to the uuid/device locks
> but as said before we don't have stress tests.

  Our test cases are sequential. We need same device related test cases
  in a concurrent manner.

  Just for experiment, I ran two instances of xfstests concurrently (on a
  separate set of test and scratch devices), they ran fine with these
  patches, not a perfect solution though. We need to think of better
  ways.

  The main challenge with testing concurrency / racing is how to
  synchronize racing threads.

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
>