All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex
@ 2020-08-14  0:03 Anand Jain
  2020-08-14  0:03 ` [PATCH 1/7] btrfs: reada: use sprout device_list_mutex Anand Jain
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

In a sprouted btrfs, the seed's fs_devices are cloned and link-
listed under the sprout's fs_devices. The fs_info::fs_devices
points to the sprout::fs_devices and various critical operations
like device-delete holds the top-level device_list_mutex
sprout::fs_devices::device_list_mutex and _not_ the seed level
mutex such as sprout::fs_devices::seed::fs_devices::device_list_mutex.

Also all related readers (except for two threads- reada and init_devices_late)
hold the sprout::fs_devices::device_list_mutex too. And those two threads
which are missing to hold the correct lock are being fixed here.

I take the approach to fix the read end instead of fixing the writer end
which are not holding the seed level mutex because to keep things
simple and there isn't much benefit burning extra CPU cycles in going 
through the lock/unlock process as we traverse through the
fs_devices::seed fs_devices (for example as in reada and init_devices_late
threads).

The first two patches (1/7, 2/7) fixes the threads using the
seed::device_list_mutex.

And rest of the patches ([3-7]/7) are cleanups and these patches
are independent by themself.

These patchset has been tested with full xfstests btrfs test cases and
found to have no new regressions.

Anand Jain (7):
  btrfs: reada: use sprout device_list_mutex
  btrfs: btrfs_init_devices_late: use sprout device_list_mutex
  btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev
  btrfs: cleanup unused return in btrfs_close_devices
  btrfs: cleanup btrfs_assign_next_active_device()
  btrfs: cleanup unnecessary goto in open_seed_device
  btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file
    global declare

 fs/btrfs/dev-replace.c | 60 +++++++++++++++++++-----------------------
 fs/btrfs/reada.c       |  4 +--
 fs/btrfs/volumes.c     | 42 +++++++++++------------------
 fs/btrfs/volumes.h     |  2 +-
 4 files changed, 46 insertions(+), 62 deletions(-)

-- 
2.18.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/7] btrfs: reada: use sprout device_list_mutex
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-14  0:03 ` [PATCH 2/7] btrfs: btrfs_init_devices_late: " Anand Jain
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

On an fs mounted using a sprout-device, the seed fs_devices are maintained
in a linked list under fs_info->fs_devices. Each seed's fs_devices also
have device_list_mutex initialized to protect against the potential race
with delete threads. But the delete thread (at btrfs_rm_device()) is holding
the fs_info::fs_devices::device_list_mutex mutex which is sprout's
device_list_mutex instead of seed's device_list_mutex. Moreover, there
aren't any significient benefits in using the seed::device_list_mutex
instead of sprout::device_list_mutex.

So this patch converts them of using the seed::device_list_mutex to
sprout::device_list_mutex.

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

diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index 243a2e44526e..c4667410b3fa 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -775,22 +775,22 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info)
 	u64 total = 0;
 	int i;
 
+	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 again:
 	do {
 		enqueued = 0;
-		mutex_lock(&fs_devices->device_list_mutex);
 		list_for_each_entry(device, &fs_devices->devices, dev_list) {
 			if (atomic_read(&device->reada_in_flight) <
 			    MAX_IN_FLIGHT)
 				enqueued += reada_start_machine_dev(device);
 		}
-		mutex_unlock(&fs_devices->device_list_mutex);
 		total += enqueued;
 	} while (enqueued && total < 10000);
 	if (fs_devices->seed) {
 		fs_devices = fs_devices->seed;
 		goto again;
 	}
+	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 
 	if (enqueued == 0)
 		return;
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/7] btrfs: btrfs_init_devices_late: use sprout device_list_mutex
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
  2020-08-14  0:03 ` [PATCH 1/7] btrfs: reada: use sprout device_list_mutex Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-14  0:03 ` [PATCH 3/7] btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev Anand Jain
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

In a mounted sprout FS, all threads now are using the
sprout::device_list_mutex, and this is the only piece of code using
the seed::device_list_mutex. This patch converts to use the sprouts
fs_info->fs_devices->device_list_mutex.

The same reasoning holds true here, that device delete is holding
the sprout::device_list_mutex.

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 ee96c5869f57..66691416ca8f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7181,14 +7181,14 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	struct btrfs_device *device;
 
+	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	while (fs_devices) {
-		mutex_lock(&fs_devices->device_list_mutex);
 		list_for_each_entry(device, &fs_devices->devices, dev_list)
 			device->fs_info = fs_info;
-		mutex_unlock(&fs_devices->device_list_mutex);
 
 		fs_devices = fs_devices->seed;
 	}
+	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 }
 
 static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/7] btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
  2020-08-14  0:03 ` [PATCH 1/7] btrfs: reada: use sprout device_list_mutex Anand Jain
  2020-08-14  0:03 ` [PATCH 2/7] btrfs: btrfs_init_devices_late: " Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-14  0:03 ` [PATCH 4/7] btrfs: cleanup unused return in btrfs_close_devices Anand Jain
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

In the function btrfs_init_dev_replace_tgtdev(), the local
variable struct list_head *devices is used only once, instead
just open code the same.

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

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index db93909b25e0..76dda11d441b 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -224,7 +224,6 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_device *device;
 	struct block_device *bdev;
-	struct list_head *devices;
 	struct rcu_string *name;
 	u64 devid = BTRFS_DEV_REPLACE_DEVID;
 	int ret = 0;
@@ -244,8 +243,7 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
 	sync_blockdev(bdev);
 
-	devices = &fs_info->fs_devices->devices;
-	list_for_each_entry(device, devices, dev_list) {
+	list_for_each_entry(device, &fs_info->fs_devices->devices, dev_list) {
 		if (device->bdev == bdev) {
 			btrfs_err(fs_info,
 				  "target device is in the filesystem!");
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/7] btrfs: cleanup unused return in btrfs_close_devices
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
                   ` (2 preceding siblings ...)
  2020-08-14  0:03 ` [PATCH 3/7] btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-14  8:53   ` Nikolay Borisov
  2020-08-14  0:03 ` [PATCH 5/7] btrfs: cleanup btrfs_assign_next_active_device() Anand Jain
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

In the function btrfs_close_devices() and its helper function
close_fs_devices(), the return value aren't used as there isn't error
return from these functions. So clean up the return argument.

Also in the function btrfs_remove_chunk() remove the local variable
%fs_devices, instead use the assigned pointer directly.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 66691416ca8f..dd867375478b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1148,12 +1148,12 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	ASSERT(atomic_read(&device->reada_in_flight) == 0);
 }
 
-static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
+static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device, *tmp;
 
 	if (--fs_devices->opened > 0)
-		return 0;
+		return;
 
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
@@ -1165,17 +1165,14 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 	WARN_ON(fs_devices->rw_devices);
 	fs_devices->opened = 0;
 	fs_devices->seeding = false;
-
-	return 0;
 }
 
-int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
+void 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);
+	close_fs_devices(fs_devices);
 	if (!fs_devices->opened) {
 		seed_devices = fs_devices->seed;
 		fs_devices->seed = NULL;
@@ -1188,7 +1185,6 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
-	return ret;
 }
 
 static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
@@ -2933,7 +2929,6 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 	struct map_lookup *map;
 	u64 dev_extent_len = 0;
 	int i, ret = 0;
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 
 	em = btrfs_get_chunk_map(fs_info, chunk_offset, 1);
 	if (IS_ERR(em)) {
@@ -2955,14 +2950,14 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 	 * a device replace operation that replaces the device object associated
 	 * with map stripes (dev-replace.c:btrfs_dev_replace_finishing()).
 	 */
-	mutex_lock(&fs_devices->device_list_mutex);
+	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	for (i = 0; i < map->num_stripes; i++) {
 		struct btrfs_device *device = map->stripes[i].dev;
 		ret = btrfs_free_dev_extent(trans, device,
 					    map->stripes[i].physical,
 					    &dev_extent_len);
 		if (ret) {
-			mutex_unlock(&fs_devices->device_list_mutex);
+			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 			btrfs_abort_transaction(trans, ret);
 			goto out;
 		}
@@ -2978,12 +2973,12 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 
 		ret = btrfs_update_device(trans, device);
 		if (ret) {
-			mutex_unlock(&fs_devices->device_list_mutex);
+			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 			btrfs_abort_transaction(trans, ret);
 			goto out;
 		}
 	}
-	mutex_unlock(&fs_devices->device_list_mutex);
+	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 
 	ret = btrfs_free_chunk(trans, chunk_offset);
 	if (ret) {
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 5eea93916fbf..76e5470e19a8 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -435,7 +435,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   fmode_t flags, void *holder);
 int btrfs_forget_devices(const char *path);
-int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
+void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_device *device,
 				     struct btrfs_device *this_dev);
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/7] btrfs: cleanup btrfs_assign_next_active_device()
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
                   ` (3 preceding siblings ...)
  2020-08-14  0:03 ` [PATCH 4/7] btrfs: cleanup unused return in btrfs_close_devices Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-14  0:03 ` [PATCH 6/7] btrfs: cleanup unnecessary goto in open_seed_device Anand Jain
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

Cleanup btrfs_assign_next_active_device(), drop %this_dev.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index dd867375478b..3e03371eade6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1956,16 +1956,13 @@ static struct btrfs_device * btrfs_find_next_active_device(
  * this_dev) which is active.
  */
 void __cold btrfs_assign_next_active_device(struct btrfs_device *device,
-				     struct btrfs_device *this_dev)
+					    struct btrfs_device *next_device)
 {
 	struct btrfs_fs_info *fs_info = device->fs_info;
-	struct btrfs_device *next_device;
 
-	if (this_dev)
-		next_device = this_dev;
-	else
+	if (!next_device)
 		next_device = btrfs_find_next_active_device(fs_info->fs_devices,
-								device);
+							    device);
 	ASSERT(next_device);
 
 	if (fs_info->sb->s_bdev &&
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/7] btrfs: cleanup unnecessary goto in open_seed_device
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
                   ` (4 preceding siblings ...)
  2020-08-14  0:03 ` [PATCH 5/7] btrfs: cleanup btrfs_assign_next_active_device() Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-14  0:03 ` [PATCH 7/7] btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file global declare Anand Jain
  2020-08-24 16:48 ` [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex David Sterba
  7 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

open_seed_devices() does goto to just return. So drop goto and just return
instead.

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 3e03371eade6..4f4e18c83a8e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6738,20 +6738,18 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	ret = open_fs_devices(fs_devices, FMODE_READ, fs_info->bdev_holder);
 	if (ret) {
 		free_fs_devices(fs_devices);
-		fs_devices = ERR_PTR(ret);
-		goto out;
+		return ERR_PTR(ret);
 	}
 
 	if (!fs_devices->seeding) {
 		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
-		fs_devices = ERR_PTR(-EINVAL);
-		goto out;
+		return ERR_PTR(-EINVAL);
 	}
 
 	fs_devices->seed = fs_info->fs_devices->seed;
 	fs_info->fs_devices->seed = fs_devices;
-out:
+
 	return fs_devices;
 }
 
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 7/7] btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file global declare
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
                   ` (5 preceding siblings ...)
  2020-08-14  0:03 ` [PATCH 6/7] btrfs: cleanup unnecessary goto in open_seed_device Anand Jain
@ 2020-08-14  0:03 ` Anand Jain
  2020-08-24 16:48 ` [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex David Sterba
  7 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2020-08-14  0:03 UTC (permalink / raw)
  To: linux-btrfs

There isn't any convoluted child functions inside the
btrfs_dev_replace_update_device_in_mapping_tree() function. With the
function moved before where it is called, we can drop its file local
declare.

No functional changes.

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

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 76dda11d441b..2706c09f117a 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -64,10 +64,6 @@
 
 static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 				       int scrub_ret);
-static void btrfs_dev_replace_update_device_in_mapping_tree(
-						struct btrfs_fs_info *fs_info,
-						struct btrfs_device *srcdev,
-						struct btrfs_device *tgtdev);
 static int btrfs_dev_replace_kthread(void *data);
 
 int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
@@ -597,6 +593,32 @@ static void btrfs_rm_dev_replace_unblocked(struct btrfs_fs_info *fs_info)
 	wake_up(&fs_info->dev_replace.replace_wait);
 }
 
+static void btrfs_dev_replace_update_device_in_mapping_tree(
+						struct btrfs_fs_info *fs_info,
+						struct btrfs_device *srcdev,
+						struct btrfs_device *tgtdev)
+{
+	struct extent_map_tree *em_tree = &fs_info->mapping_tree;
+	struct extent_map *em;
+	struct map_lookup *map;
+	u64 start = 0;
+	int i;
+
+	write_lock(&em_tree->lock);
+	do {
+		em = lookup_extent_mapping(em_tree, start, (u64)-1);
+		if (!em)
+			break;
+		map = em->map_lookup;
+		for (i = 0; i < map->num_stripes; i++)
+			if (srcdev == map->stripes[i].dev)
+				map->stripes[i].dev = tgtdev;
+		start = em->start + em->len;
+		free_extent_map(em);
+	} while (start);
+	write_unlock(&em_tree->lock);
+}
+
 static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 				       int scrub_ret)
 {
@@ -755,32 +777,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static void btrfs_dev_replace_update_device_in_mapping_tree(
-						struct btrfs_fs_info *fs_info,
-						struct btrfs_device *srcdev,
-						struct btrfs_device *tgtdev)
-{
-	struct extent_map_tree *em_tree = &fs_info->mapping_tree;
-	struct extent_map *em;
-	struct map_lookup *map;
-	u64 start = 0;
-	int i;
-
-	write_lock(&em_tree->lock);
-	do {
-		em = lookup_extent_mapping(em_tree, start, (u64)-1);
-		if (!em)
-			break;
-		map = em->map_lookup;
-		for (i = 0; i < map->num_stripes; i++)
-			if (srcdev == map->stripes[i].dev)
-				map->stripes[i].dev = tgtdev;
-		start = em->start + em->len;
-		free_extent_map(em);
-	} while (start);
-	write_unlock(&em_tree->lock);
-}
-
 /*
  * Read progress of device replace status according to the state and last
  * stored position. The value format is the same as for
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/7] btrfs: cleanup unused return in btrfs_close_devices
  2020-08-14  0:03 ` [PATCH 4/7] btrfs: cleanup unused return in btrfs_close_devices Anand Jain
@ 2020-08-14  8:53   ` Nikolay Borisov
  0 siblings, 0 replies; 10+ messages in thread
From: Nikolay Borisov @ 2020-08-14  8:53 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 14.08.20 г. 3:03 ч., Anand Jain wrote:
> In the function btrfs_close_devices() and its helper function
> close_fs_devices(), the return value aren't used as there isn't error
> return from these functions. So clean up the return argument.
> 
> Also in the function btrfs_remove_chunk() remove the local variable
> %fs_devices, instead use the assigned pointer directly.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 21 ++++++++-------------
>  fs/btrfs/volumes.h |  2 +-
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 66691416ca8f..dd867375478b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1148,12 +1148,12 @@ static void btrfs_close_one_device(struct btrfs_device *device)
>  	ASSERT(atomic_read(&device->reada_in_flight) == 0);
>  }
>  
> -static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
> +static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
>  {
>  	struct btrfs_device *device, *tmp;
>  
>  	if (--fs_devices->opened > 0)
> -		return 0;
> +		return;
>  
>  	mutex_lock(&fs_devices->device_list_mutex);
>  	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
> @@ -1165,17 +1165,14 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>  	WARN_ON(fs_devices->rw_devices);
>  	fs_devices->opened = 0;
>  	fs_devices->seeding = false;
> -
> -	return 0;
>  }
>  
> -int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
> +void 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);
> +	close_fs_devices(fs_devices);
>  	if (!fs_devices->opened) {
>  		seed_devices = fs_devices->seed;
>  		fs_devices->seed = NULL;
> @@ -1188,7 +1185,6 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
>  		close_fs_devices(fs_devices);
>  		free_fs_devices(fs_devices);
>  	}
> -	return ret;
>  }
>  

This is the relevant portions which implement what's documented. Also I
have already sent similar cleanup on 15.07.

>  static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
> @@ -2933,7 +2929,6 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
>  	struct map_lookup *map;
>  	u64 dev_extent_len = 0;
>  	int i, ret = 0;
> -	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>  
>  	em = btrfs_get_chunk_map(fs_info, chunk_offset, 1);
>  	if (IS_ERR(em)) {
> @@ -2955,14 +2950,14 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
>  	 * a device replace operation that replaces the device object associated
>  	 * with map stripes (dev-replace.c:btrfs_dev_replace_finishing()).
>  	 */
> -	mutex_lock(&fs_devices->device_list_mutex);
> +	mutex_lock(&fs_info->fs_devices->device_list_mutex);
>  	for (i = 0; i < map->num_stripes; i++) {
>  		struct btrfs_device *device = map->stripes[i].dev;
>  		ret = btrfs_free_dev_extent(trans, device,
>  					    map->stripes[i].physical,
>  					    &dev_extent_len);
>  		if (ret) {
> -			mutex_unlock(&fs_devices->device_list_mutex);
> +			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
>  			btrfs_abort_transaction(trans, ret);
>  			goto out;
>  		}
> @@ -2978,12 +2973,12 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
>  
>  		ret = btrfs_update_device(trans, device);
>  		if (ret) {
> -			mutex_unlock(&fs_devices->device_list_mutex);
> +			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
>  			btrfs_abort_transaction(trans, ret);
>  			goto out;
>  		}
>  	}
> -	mutex_unlock(&fs_devices->device_list_mutex);
> +	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
>  
>  	ret = btrfs_free_chunk(trans, chunk_offset);
>  	if (ret) {

This is unrelated cleanup...

> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 5eea93916fbf..76e5470e19a8 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -435,7 +435,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  struct btrfs_device *btrfs_scan_one_device(const char *path,
>  					   fmode_t flags, void *holder);
>  int btrfs_forget_devices(const char *path);
> -int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
> +void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
>  void btrfs_assign_next_active_device(struct btrfs_device *device,
>  				     struct btrfs_device *this_dev);
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex
  2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
                   ` (6 preceding siblings ...)
  2020-08-14  0:03 ` [PATCH 7/7] btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file global declare Anand Jain
@ 2020-08-24 16:48 ` David Sterba
  7 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2020-08-24 16:48 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Fri, Aug 14, 2020 at 08:03:45AM +0800, Anand Jain wrote:
> In a sprouted btrfs, the seed's fs_devices are cloned and link-
> listed under the sprout's fs_devices. The fs_info::fs_devices
> points to the sprout::fs_devices and various critical operations
> like device-delete holds the top-level device_list_mutex
> sprout::fs_devices::device_list_mutex and _not_ the seed level
> mutex such as sprout::fs_devices::seed::fs_devices::device_list_mutex.
> 
> Also all related readers (except for two threads- reada and init_devices_late)
> hold the sprout::fs_devices::device_list_mutex too. And those two threads
> which are missing to hold the correct lock are being fixed here.
> 
> I take the approach to fix the read end instead of fixing the writer end
> which are not holding the seed level mutex because to keep things
> simple and there isn't much benefit burning extra CPU cycles in going 
> through the lock/unlock process as we traverse through the
> fs_devices::seed fs_devices (for example as in reada and init_devices_late
> threads).
> 
> The first two patches (1/7, 2/7) fixes the threads using the
> seed::device_list_mutex.
> 
> And rest of the patches ([3-7]/7) are cleanups and these patches
> are independent by themself.
> 
> These patchset has been tested with full xfstests btrfs test cases and
> found to have no new regressions.
> 
> Anand Jain (7):
>   btrfs: reada: use sprout device_list_mutex
>   btrfs: btrfs_init_devices_late: use sprout device_list_mutex
>   btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev
>   btrfs: cleanup unused return in btrfs_close_devices
>   btrfs: cleanup btrfs_assign_next_active_device()
>   btrfs: cleanup unnecessary goto in open_seed_device
>   btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file
>     global declare

The series conflicts from patch 1 with the seed updates from Nik. The
branch is now in for-next and will be in misc-next soon.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-08-24 16:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14  0:03 [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex Anand Jain
2020-08-14  0:03 ` [PATCH 1/7] btrfs: reada: use sprout device_list_mutex Anand Jain
2020-08-14  0:03 ` [PATCH 2/7] btrfs: btrfs_init_devices_late: " Anand Jain
2020-08-14  0:03 ` [PATCH 3/7] btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev Anand Jain
2020-08-14  0:03 ` [PATCH 4/7] btrfs: cleanup unused return in btrfs_close_devices Anand Jain
2020-08-14  8:53   ` Nikolay Borisov
2020-08-14  0:03 ` [PATCH 5/7] btrfs: cleanup btrfs_assign_next_active_device() Anand Jain
2020-08-14  0:03 ` [PATCH 6/7] btrfs: cleanup unnecessary goto in open_seed_device Anand Jain
2020-08-14  0:03 ` [PATCH 7/7] btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file global declare Anand Jain
2020-08-24 16:48 ` [PATCH 0/7] btrfs: consolidate seed mutex to sprout mutex David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.