linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Convert seed devices to proper list API
@ 2020-07-15 10:48 Nikolay Borisov
  2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov
                   ` (6 more replies)
  0 siblings, 7 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

This series converts the existing seed devices pointer in btrfs_fs_devices to
proper list api. First 4 patches are cleanups preparing the code for the switch.
Patch 5 has more information about the required transformations, it might seem
lengthy but he logic everywhere is pretty much the same so shouldn't be that
cumbersome to review.

This series survived xfstest runs and even caught 2 bugs while developing it.

Nikolay Borisov (5):
  btrfs: Factor out reada loop in __reada_start_machine
  btrfs: Factor out loop logic from btrfs_free_extra_devids
  btrfs: Make close_fs_devices return void
  btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices
  btrfs: Switch seed device to list api

 fs/btrfs/disk-io.c |  39 +++++------
 fs/btrfs/reada.c   |  26 ++++---
 fs/btrfs/sysfs.c   |   4 --
 fs/btrfs/volumes.c | 166 ++++++++++++++++++++-------------------------
 fs/btrfs/volumes.h |   6 +-
 5 files changed, 112 insertions(+), 129 deletions(-)

--
2.17.1


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

* [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
@ 2020-07-15 10:48 ` Nikolay Borisov
  2020-08-18 15:02   ` Josef Bacik
  2020-08-29 15:06   ` Anand Jain
  2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

This is in preparation for moving fs_devices to proper lists.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/reada.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index 243a2e44526e..aa9d24ed56d7 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -767,15 +767,14 @@ static void reada_start_machine_worker(struct btrfs_work *work)
 	kfree(rmw);
 }
 
-static void __reada_start_machine(struct btrfs_fs_info *fs_info)
+
+/* Try to start up to 10k READA requests for a group of devices. */
+static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices)
 {
-	struct btrfs_device *device;
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	u64 enqueued;
 	u64 total = 0;
-	int i;
+	struct btrfs_device *device;
 
-again:
 	do {
 		enqueued = 0;
 		mutex_lock(&fs_devices->device_list_mutex);
@@ -787,6 +786,18 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info)
 		mutex_unlock(&fs_devices->device_list_mutex);
 		total += enqueued;
 	} while (enqueued && total < 10000);
+
+	return total;
+}
+
+static void __reada_start_machine(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	int i;
+	u64 enqueued = 0;
+
+again:
+	enqueued += __reada_start_for_fsdevs(fs_devices);
 	if (fs_devices->seed) {
 		fs_devices = fs_devices->seed;
 		goto again;
-- 
2.17.1


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

* [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
  2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov
@ 2020-07-15 10:48 ` Nikolay Borisov
  2020-07-15 12:32   ` kernel test robot
                     ` (2 more replies)
  2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov
                   ` (4 subsequent siblings)
  6 siblings, 3 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

This prepares the code to switching seeds devices to a proper list.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ce01e44f8134..db29fc4fbe89 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1024,28 +1024,24 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 	return ERR_PTR(ret);
 }
 
-/*
- * After we have read the system tree and know devids belonging to
- * this filesystem, remove the device which does not belong there.
- */
-void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
+
+
+void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step,
+			       struct btrfs_device **latest_dev)
 {
 	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) {
 		if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
-							&device->dev_state)) {
+			     &device->dev_state)) {
 			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-			     &device->dev_state) &&
+				      &device->dev_state) &&
 			    !test_bit(BTRFS_DEV_STATE_MISSING,
 				      &device->dev_state) &&
-			     (!latest_dev ||
-			      device->generation > latest_dev->generation)) {
-				latest_dev = device;
+			    (!*latest_dev ||
+			     device->generation > (*latest_dev)->generation)) {
+				*latest_dev = device;
 			}
 			continue;
 		}
@@ -1083,6 +1079,18 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 		btrfs_free_device(device);
 	}
 
+}
+/*
+ * After we have read the system tree and know devids belonging to
+ * this filesystem, remove the device which does not belong there.
+ */
+void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
+{
+	struct btrfs_device *latest_dev = NULL;
+
+	mutex_lock(&uuid_mutex);
+again:
+	__btrfs_free_extra_devids(fs_devices, step, &latest_dev);
 	if (fs_devices->seed) {
 		fs_devices = fs_devices->seed;
 		goto again;
-- 
2.17.1


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

* [PATCH 3/5] btrfs: Make close_fs_devices return void
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
  2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov
  2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov
@ 2020-07-15 10:48 ` Nikolay Borisov
  2020-08-18 15:05   ` Josef Bacik
  2020-08-29 15:14   ` Anand Jain
  2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

The return value of this function conveys absolutely no information.
All callers already check the state of  fs_devices->opened to decide
how to proceed. So conver the function to returning void. While at it
make btrfs_close_devices also return void.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 12 ++++--------
 fs/btrfs/volumes.h |  2 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index db29fc4fbe89..6de021c78277 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1146,12 +1146,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) {
@@ -1163,17 +1163,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;
@@ -1186,7 +1183,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,
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.17.1


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

* [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
                   ` (2 preceding siblings ...)
  2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov
@ 2020-07-15 10:48 ` Nikolay Borisov
  2020-08-18 15:08   ` Josef Bacik
  2020-08-26 10:50   ` Anand Jain
  2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

It makes no sense to have sysfs-related routines be responsible for
properly initialising the fs_info pointer of struct btrfs_fs_device.
Instead this can be streamlined by making it the responsibility of
btrfs_init_devices_late to initialize it. That function already
initializes fs_info of every individual device in btrfs_fs_devices.

As far as clearing it is concerned it makes sense to move it to
close_fs_devices. That function is only called when struct
btrfs_fs_devices is no longer in use - either for holding seeds or main
devices for a mounted filesystem.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/sysfs.c   |  4 ----
 fs/btrfs/volumes.c | 22 ++--------------------
 fs/btrfs/volumes.h |  2 --
 3 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 38c0b95e0e7f..24f6bbd655e3 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -939,8 +939,6 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 {
 	struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj;
 
-	btrfs_reset_fs_info_ptr(fs_info);
-
 	sysfs_remove_link(fsid_kobj, "bdi");
 
 	if (fs_info->space_info_kobj) {
@@ -1397,8 +1395,6 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
 	struct btrfs_fs_devices *fs_devs = fs_info->fs_devices;
 	struct kobject *fsid_kobj = &fs_devs->fsid_kobj;
 
-	btrfs_set_fs_info_ptr(fs_info);
-
 	error = btrfs_sysfs_add_devices_dir(fs_devs, NULL);
 	if (error)
 		return error;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6de021c78277..a0cdd027e99c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1163,6 +1163,7 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
 	WARN_ON(fs_devices->rw_devices);
 	fs_devices->opened = 0;
 	fs_devices->seeding = false;
+	fs_devices->fs_info = NULL;
 }
 
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
@@ -7149,6 +7150,7 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
 			device->fs_info = fs_info;
 		mutex_unlock(&fs_devices->device_list_mutex);
 
+		fs_devices->fs_info = fs_info;
 		fs_devices = fs_devices->seed;
 	}
 }
@@ -7447,24 +7449,6 @@ void btrfs_commit_device_sizes(struct btrfs_transaction *trans)
 	mutex_unlock(&trans->fs_info->chunk_mutex);
 }
 
-void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info)
-{
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
-	while (fs_devices) {
-		fs_devices->fs_info = fs_info;
-		fs_devices = fs_devices->seed;
-	}
-}
-
-void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info)
-{
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
-	while (fs_devices) {
-		fs_devices->fs_info = NULL;
-		fs_devices = fs_devices->seed;
-	}
-}
-
 /*
  * Multiplicity factor for simple profiles: DUP, RAID1-like and RAID10.
  */
@@ -7475,8 +7459,6 @@ int btrfs_bg_type_to_factor(u64 flags)
 	return btrfs_raid_array[index].ncopies;
 }
 
-
-
 static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 				 u64 chunk_offset, u64 devid,
 				 u64 physical_offset, u64 physical_len)
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 76e5470e19a8..fc283fdbcece 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -569,8 +569,6 @@ static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
 void btrfs_commit_device_sizes(struct btrfs_transaction *trans);
 
 struct list_head * __attribute_const__ btrfs_get_fs_uuids(void);
-void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info);
-void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info);
 bool btrfs_check_rw_degradable(struct btrfs_fs_info *fs_info,
 					struct btrfs_device *failing_dev);
 
-- 
2.17.1


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

* [PATCH 5/5] btrfs: Switch seed device to list api
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
                   ` (3 preceding siblings ...)
  2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov
@ 2020-07-15 10:48 ` Nikolay Borisov
  2020-07-15 13:14   ` kernel test robot
                     ` (3 more replies)
  2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba
  2020-08-17 19:19 ` Nikolay Borisov
  6 siblings, 4 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

While this patch touches a bunch of files the conversion is
straighforward. Instead of using the implicit linked list anchored at
btrfs_fs_devices::seed the code is switched to using
list_for_each_entry. Previous patches in the series already factored out
code that processed both main and seed devices so in those cases
the factored out functions are called on the main fs_devices and then
on every seed dev inside list_for_each_entry.

Using list api also allows to simplify deletion from the seed dev list
performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by
substituting a while() loop with a simple list_del_init.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c |  39 ++++++++---------
 fs/btrfs/reada.c   |   9 ++--
 fs/btrfs/volumes.c | 102 ++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |   2 +-
 4 files changed, 72 insertions(+), 80 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2fb0b43bc1c5..8767d47f6e29 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -545,33 +545,30 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 static int check_tree_block_fsid(struct extent_buffer *eb)
 {
 	struct btrfs_fs_info *fs_info = eb->fs_info;
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	u8 fsid[BTRFS_FSID_SIZE];
-	int ret = 1;
+	u8 *metadata_uuid;
 
 	read_extent_buffer(eb, fsid, offsetof(struct btrfs_header, fsid),
 			   BTRFS_FSID_SIZE);
-	while (fs_devices) {
-		u8 *metadata_uuid;
+	/*
+	 * Checking the incompat flag is only valid for the current
+	 * fs. For seed devices it's forbidden to have their uuid
+	 * changed so reading ->fsid in this case is fine
+	 */
+	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
+		metadata_uuid = fs_devices->metadata_uuid;
+	else
+		metadata_uuid = fs_devices->fsid;
 
-		/*
-		 * Checking the incompat flag is only valid for the current
-		 * fs. For seed devices it's forbidden to have their uuid
-		 * changed so reading ->fsid in this case is fine
-		 */
-		if (fs_devices == fs_info->fs_devices &&
-		    btrfs_fs_incompat(fs_info, METADATA_UUID))
-			metadata_uuid = fs_devices->metadata_uuid;
-		else
-			metadata_uuid = fs_devices->fsid;
+	if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE))
+		return 0;
 
-		if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE)) {
-			ret = 0;
-			break;
-		}
-		fs_devices = fs_devices->seed;
-	}
-	return ret;
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
+		if (!memcmp(fsid, seed_devs->fsid, BTRFS_FSID_SIZE))
+			return 0;
+
+	return 1;
 }
 
 static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index aa9d24ed56d7..ac5b07ded0fe 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -792,16 +792,13 @@ static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices)
 
 static void __reada_start_machine(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	int i;
 	u64 enqueued = 0;
 
-again:
 	enqueued += __reada_start_for_fsdevs(fs_devices);
-	if (fs_devices->seed) {
-		fs_devices = fs_devices->seed;
-		goto again;
-	}
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
+		enqueued += __reada_start_for_fsdevs(seed_devs);
 
 	if (enqueued == 0)
 		return;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a0cdd027e99c..7fce7a220a76 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -350,6 +350,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 	INIT_LIST_HEAD(&fs_devs->devices);
 	INIT_LIST_HEAD(&fs_devs->alloc_list);
 	INIT_LIST_HEAD(&fs_devs->fs_list);
+	INIT_LIST_HEAD(&fs_devs->seed_list);
 	if (fsid)
 		memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE);
 
@@ -1087,14 +1088,13 @@ void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step,
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 {
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_fs_devices *seed_dev;
 
 	mutex_lock(&uuid_mutex);
-again:
 	__btrfs_free_extra_devids(fs_devices, step, &latest_dev);
-	if (fs_devices->seed) {
-		fs_devices = fs_devices->seed;
-		goto again;
-	}
+
+	list_for_each_entry(seed_dev, &fs_devices->seed_list, seed_list)
+		__btrfs_free_extra_devids(seed_dev, step, &latest_dev);
 
 	fs_devices->latest_bdev = latest_dev->bdev;
 
@@ -1168,20 +1168,18 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
 
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
-	struct btrfs_fs_devices *seed_devices = NULL;
+	LIST_HEAD(list);
+	struct btrfs_fs_devices *tmp;
 
 	mutex_lock(&uuid_mutex);
 	close_fs_devices(fs_devices);
-	if (!fs_devices->opened) {
-		seed_devices = fs_devices->seed;
-		fs_devices->seed = NULL;
-	}
+	if (!fs_devices->opened)
+		list_splice_init(&fs_devices->seed_list, &list);
 	mutex_unlock(&uuid_mutex);
 
-	while (seed_devices) {
-		fs_devices = seed_devices;
-		seed_devices = fs_devices->seed;
+	list_for_each_entry_safe(fs_devices, tmp, &list, seed_list) {
 		close_fs_devices(fs_devices);
+		list_del(&fs_devices->seed_list);
 		free_fs_devices(fs_devices);
 	}
 }
@@ -2154,14 +2152,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	btrfs_free_device(device);
 
 	if (cur_devices->open_devices == 0) {
-		while (fs_devices) {
-			if (fs_devices->seed == cur_devices) {
-				fs_devices->seed = cur_devices->seed;
-				break;
-			}
-			fs_devices = fs_devices->seed;
-		}
-		cur_devices->seed = NULL;
+		list_del_init(&cur_devices->seed_list);
 		close_fs_devices(cur_devices);
 		free_fs_devices(cur_devices);
 	}
@@ -2236,14 +2227,7 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_device *srcdev)
 		ASSERT(fs_devices->seeding);
 
 		tmp_fs_devices = fs_info->fs_devices;
-		while (tmp_fs_devices) {
-			if (tmp_fs_devices->seed == fs_devices) {
-				tmp_fs_devices->seed = fs_devices->seed;
-				break;
-			}
-			tmp_fs_devices = tmp_fs_devices->seed;
-		}
-		fs_devices->seed = NULL;
+		list_del_init(&fs_devices->seed_list);
 		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
@@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	fs_devices->open_devices = 0;
 	fs_devices->missing_devices = 0;
 	fs_devices->rotating = false;
-	fs_devices->seed = seed_devices;
+	list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list);
 
 	generate_random_uuid(fs_devices->fsid);
 	memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE);
@@ -6430,11 +6414,23 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
 				       bool seed)
 {
 	struct btrfs_device *device;
+	struct btrfs_fs_devices *seed_devs;
 
-	while (fs_devices) {
+	if (!fsid ||
+	    !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
+		list_for_each_entry(device, &fs_devices->devices,
+				    dev_list) {
+			if (device->devid == devid &&
+			    (!uuid || memcmp(device->uuid, uuid,
+					     BTRFS_UUID_SIZE) == 0))
+				return device;
+		}
+	}
+
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
 		if (!fsid ||
-		    !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
-			list_for_each_entry(device, &fs_devices->devices,
+		    !memcmp(seed_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
+			list_for_each_entry(device, &seed_devs->devices,
 					    dev_list) {
 				if (device->devid == devid &&
 				    (!uuid || memcmp(device->uuid, uuid,
@@ -6442,11 +6438,8 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
 					return device;
 			}
 		}
-		if (seed)
-			fs_devices = fs_devices->seed;
-		else
-			return NULL;
 	}
+
 	return NULL;
 }
 
@@ -6688,13 +6681,10 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	lockdep_assert_held(&uuid_mutex);
 	ASSERT(fsid);
 
-	fs_devices = fs_info->fs_devices->seed;
-	while (fs_devices) {
+	list_for_each_entry(fs_devices, &fs_info->fs_devices->seed_list, seed_list)
 		if (!memcmp(fs_devices->fsid, fsid, BTRFS_FSID_SIZE))
 			return fs_devices;
 
-		fs_devices = fs_devices->seed;
-	}
 
 	fs_devices = find_fsid(fsid, NULL);
 	if (!fs_devices) {
@@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	fs_devices->seed = fs_info->fs_devices->seed;
-	fs_info->fs_devices->seed = fs_devices;
+	ASSERT(list_empty(&fs_devices->seed_list));
+	list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list);
 out:
 	return fs_devices;
 }
@@ -7141,17 +7131,23 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 
 void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	struct btrfs_device *device;
 
-	while (fs_devices) {
-		mutex_lock(&fs_devices->device_list_mutex);
-		list_for_each_entry(device, &fs_devices->devices, dev_list)
+	fs_devices->fs_info = fs_info;
+
+	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);
+
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
+		mutex_lock(&seed_devs->device_list_mutex);
+		list_for_each_entry(device, &seed_devs->devices, dev_list)
 			device->fs_info = fs_info;
-		mutex_unlock(&fs_devices->device_list_mutex);
+		mutex_unlock(&seed_devs->device_list_mutex);
 
-		fs_devices->fs_info = fs_info;
-		fs_devices = fs_devices->seed;
+		seed_devs->fs_info = fs_info;
 	}
 }
 
@@ -7527,8 +7523,10 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 
 	/* It's possible this device is a dummy for seed device */
 	if (dev->disk_total_bytes == 0) {
-		dev = btrfs_find_device(fs_info->fs_devices->seed, devid, NULL,
-					NULL, false);
+		struct btrfs_fs_devices *devs;
+		devs = list_first_entry(&fs_info->fs_devices->seed_list,
+					struct btrfs_fs_devices, seed_list);
+		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
 		if (!dev) {
 			btrfs_err(fs_info, "failed to find seed devid %llu",
 				  devid);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index fc283fdbcece..709f4aacbd3f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -246,7 +246,7 @@ struct btrfs_fs_devices {
 	 */
 	struct list_head alloc_list;
 
-	struct btrfs_fs_devices *seed;
+	struct list_head seed_list;
 	bool seeding;
 
 	int opened;
-- 
2.17.1


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

* Re: [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids
  2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov
@ 2020-07-15 12:32   ` kernel test robot
  2020-07-15 12:39     ` Nikolay Borisov
  2020-07-16  7:17   ` [PATCH v2] " Nikolay Borisov
  2020-08-18 15:03   ` [PATCH 2/5] " Josef Bacik
  2 siblings, 1 reply; 31+ messages in thread
From: kernel test robot @ 2020-07-15 12:32 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs; +Cc: kbuild-all, Nikolay Borisov

[-- Attachment #1: Type: text/plain, Size: 3949 bytes --]

Hi Nikolay,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.8-rc5 next-20200715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Convert-seed-devices-to-proper-list-API/20200715-185102
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-rhel-7.6-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from fs/btrfs/volumes.c:17:
   fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
    2271 | size_t __const btrfs_get_num_csums(void);
         |        ^~~~~~~
   In file included from fs/btrfs/volumes.c:28:
   fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
      16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set);
         | ^~~~~
>> fs/btrfs/volumes.c:1029:6: warning: no previous prototype for '__btrfs_free_extra_devids' [-Wmissing-prototypes]
    1029 | void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/__btrfs_free_extra_devids +1029 fs/btrfs/volumes.c

  1026	
  1027	
  1028	
> 1029	void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step,
  1030				       struct btrfs_device **latest_dev)
  1031	{
  1032		struct btrfs_device *device, *next;
  1033	
  1034		/* This is the initialized path, it is safe to release the devices. */
  1035		list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
  1036			if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
  1037				     &device->dev_state)) {
  1038				if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
  1039					      &device->dev_state) &&
  1040				    !test_bit(BTRFS_DEV_STATE_MISSING,
  1041					      &device->dev_state) &&
  1042				    (!*latest_dev ||
  1043				     device->generation > (*latest_dev)->generation)) {
  1044					*latest_dev = device;
  1045				}
  1046				continue;
  1047			}
  1048	
  1049			if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
  1050				/*
  1051				 * In the first step, keep the device which has
  1052				 * the correct fsid and the devid that is used
  1053				 * for the dev_replace procedure.
  1054				 * In the second step, the dev_replace state is
  1055				 * read from the device tree and it is known
  1056				 * whether the procedure is really active or
  1057				 * not, which means whether this device is
  1058				 * used or whether it should be removed.
  1059				 */
  1060				if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
  1061							  &device->dev_state)) {
  1062					continue;
  1063				}
  1064			}
  1065			if (device->bdev) {
  1066				blkdev_put(device->bdev, device->mode);
  1067				device->bdev = NULL;
  1068				fs_devices->open_devices--;
  1069			}
  1070			if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
  1071				list_del_init(&device->dev_alloc_list);
  1072				clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
  1073				if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
  1074					      &device->dev_state))
  1075					fs_devices->rw_devices--;
  1076			}
  1077			list_del_init(&device->dev_list);
  1078			fs_devices->num_devices--;
  1079			btrfs_free_device(device);
  1080		}
  1081	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50003 bytes --]

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

* Re: [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids
  2020-07-15 12:32   ` kernel test robot
@ 2020-07-15 12:39     ` Nikolay Borisov
  0 siblings, 0 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-15 12:39 UTC (permalink / raw)
  To: linux-btrfs



On 15.07.20 г. 15:32 ч., kernel test robot wrote:
> Hi Nikolay,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on kdave/for-next]
> [also build test WARNING on v5.8-rc5 next-20200715]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Convert-seed-devices-to-proper-list-API/20200715-185102
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: x86_64-rhel-7.6-kselftests (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
> reproduce (this is a W=1 build):
>         # save the attached .config to linux build tree
>         make W=1 ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from fs/btrfs/volumes.c:17:
>    fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
>     2271 | size_t __const btrfs_get_num_csums(void);
>          |        ^~~~~~~
>    In file included from fs/btrfs/volumes.c:28:
>    fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
>       16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set);
>          | ^~~~~
>>> fs/btrfs/volumes.c:1029:6: warning: no previous prototype for '__btrfs_free_extra_devids' [-Wmissing-prototypes]
>     1029 | void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step,
>          |      ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> vim +/__btrfs_free_extra_devids +1029 fs/btrfs/volumes.c
> 
>   1026	
>   1027	
>   1028	
>> 1029	void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step



This function is missing a static modifier. It shouldn't be a reason to
block review.

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

* Re: [PATCH 5/5] btrfs: Switch seed device to list api
  2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
@ 2020-07-15 13:14   ` kernel test robot
  2020-07-16  7:25   ` [PATCH v2] " Nikolay Borisov
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 31+ messages in thread
From: kernel test robot @ 2020-07-15 13:14 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs; +Cc: kbuild-all, Nikolay Borisov

[-- Attachment #1: Type: text/plain, Size: 5808 bytes --]

Hi Nikolay,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20200715]
[cannot apply to v5.8-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Convert-seed-devices-to-proper-list-API/20200715-185102
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-rhel-7.6-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from fs/btrfs/volumes.c:17:
   fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
    2271 | size_t __const btrfs_get_num_csums(void);
         |        ^~~~~~~
   In file included from fs/btrfs/volumes.c:28:
   fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
      16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set);
         | ^~~~~
   fs/btrfs/volumes.c:1030:6: warning: no previous prototype for '__btrfs_free_extra_devids' [-Wmissing-prototypes]
    1030 | void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~
   fs/btrfs/volumes.c: In function 'btrfs_rm_dev_replace_free_srcdev':
>> fs/btrfs/volumes.c:2219:28: warning: variable 'tmp_fs_devices' set but not used [-Wunused-but-set-variable]
    2219 |   struct btrfs_fs_devices *tmp_fs_devices;
         |                            ^~~~~~~~~~~~~~
--
   In file included from fs/btrfs/delayed-inode.h:17,
                    from fs/btrfs/super.c:30:
   fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
    2271 | size_t __const btrfs_get_num_csums(void);
         |        ^~~~~~~
   In file included from fs/btrfs/super.c:46:
   fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
      16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set);
         | ^~~~~
   fs/btrfs/super.c: In function 'btrfs_show_devname':
>> fs/btrfs/super.c:2404:28: error: 'struct btrfs_fs_devices' has no member named 'seed'
    2404 |   cur_devices = cur_devices->seed;
         |                            ^~

vim +2404 fs/btrfs/super.c

9e7cc91a6d18a4 Wang Xiaoguang  2016-08-01  2377  
9c5085c147989d Josef Bacik     2012-06-05  2378  static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
9c5085c147989d Josef Bacik     2012-06-05  2379  {
9c5085c147989d Josef Bacik     2012-06-05  2380  	struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);
9c5085c147989d Josef Bacik     2012-06-05  2381  	struct btrfs_fs_devices *cur_devices;
9c5085c147989d Josef Bacik     2012-06-05  2382  	struct btrfs_device *dev, *first_dev = NULL;
9c5085c147989d Josef Bacik     2012-06-05  2383  	struct list_head *head;
9c5085c147989d Josef Bacik     2012-06-05  2384  
88c14590cdd6f3 David Sterba    2018-03-16  2385  	/*
88c14590cdd6f3 David Sterba    2018-03-16  2386  	 * Lightweight locking of the devices. We should not need
88c14590cdd6f3 David Sterba    2018-03-16  2387  	 * device_list_mutex here as we only read the device data and the list
88c14590cdd6f3 David Sterba    2018-03-16  2388  	 * is protected by RCU.  Even if a device is deleted during the list
88c14590cdd6f3 David Sterba    2018-03-16  2389  	 * traversals, we'll get valid data, the freeing callback will wait at
52042d8e82ff50 Andrea Gelmini  2018-11-28  2390  	 * least until the rcu_read_unlock.
88c14590cdd6f3 David Sterba    2018-03-16  2391  	 */
88c14590cdd6f3 David Sterba    2018-03-16  2392  	rcu_read_lock();
9c5085c147989d Josef Bacik     2012-06-05  2393  	cur_devices = fs_info->fs_devices;
9c5085c147989d Josef Bacik     2012-06-05  2394  	while (cur_devices) {
9c5085c147989d Josef Bacik     2012-06-05  2395  		head = &cur_devices->devices;
88c14590cdd6f3 David Sterba    2018-03-16  2396  		list_for_each_entry_rcu(dev, head, dev_list) {
e6e674bd4d54fe Anand Jain      2017-12-04  2397  			if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
aa9ddcd4b55571 Josef Bacik     2012-08-02  2398  				continue;
0aeb8a6e67cdde Anand Jain      2014-06-30  2399  			if (!dev->name)
0aeb8a6e67cdde Anand Jain      2014-06-30  2400  				continue;
9c5085c147989d Josef Bacik     2012-06-05  2401  			if (!first_dev || dev->devid < first_dev->devid)
9c5085c147989d Josef Bacik     2012-06-05  2402  				first_dev = dev;
9c5085c147989d Josef Bacik     2012-06-05  2403  		}
9c5085c147989d Josef Bacik     2012-06-05 @2404  		cur_devices = cur_devices->seed;
9c5085c147989d Josef Bacik     2012-06-05  2405  	}
9c5085c147989d Josef Bacik     2012-06-05  2406  
672d599041c862 Misono Tomohiro 2018-08-02  2407  	if (first_dev)
672d599041c862 Misono Tomohiro 2018-08-02  2408  		seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
672d599041c862 Misono Tomohiro 2018-08-02  2409  	else
9c5085c147989d Josef Bacik     2012-06-05  2410  		WARN_ON(1);
88c14590cdd6f3 David Sterba    2018-03-16  2411  	rcu_read_unlock();
9c5085c147989d Josef Bacik     2012-06-05  2412  	return 0;
9c5085c147989d Josef Bacik     2012-06-05  2413  }
9c5085c147989d Josef Bacik     2012-06-05  2414  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50003 bytes --]

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

* [PATCH v2] btrfs: Factor out loop logic from btrfs_free_extra_devids
  2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov
  2020-07-15 12:32   ` kernel test robot
@ 2020-07-16  7:17   ` Nikolay Borisov
  2020-08-29 15:13     ` Anand Jain
  2020-08-18 15:03   ` [PATCH 2/5] " Josef Bacik
  2 siblings, 1 reply; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-16  7:17 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

This prepares the code to switching seeds devices to a proper list.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2:
 * Added missing static modifier to the factored out function. Reported by
 kernel test robot

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ce01e44f8134..76a68edb3127 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1024,28 +1024,24 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 	return ERR_PTR(ret);
 }

-/*
- * After we have read the system tree and know devids belonging to
- * this filesystem, remove the device which does not belong there.
- */
-void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
+
+
+static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
+				      int step, struct btrfs_device **latest_dev)
 {
 	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) {
 		if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
-							&device->dev_state)) {
+			     &device->dev_state)) {
 			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-			     &device->dev_state) &&
+				      &device->dev_state) &&
 			    !test_bit(BTRFS_DEV_STATE_MISSING,
 				      &device->dev_state) &&
-			     (!latest_dev ||
-			      device->generation > latest_dev->generation)) {
-				latest_dev = device;
+			    (!*latest_dev ||
+			     device->generation > (*latest_dev)->generation)) {
+				*latest_dev = device;
 			}
 			continue;
 		}
@@ -1083,6 +1079,18 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 		btrfs_free_device(device);
 	}

+}
+/*
+ * After we have read the system tree and know devids belonging to
+ * this filesystem, remove the device which does not belong there.
+ */
+void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
+{
+	struct btrfs_device *latest_dev = NULL;
+
+	mutex_lock(&uuid_mutex);
+again:
+	__btrfs_free_extra_devids(fs_devices, step, &latest_dev);
 	if (fs_devices->seed) {
 		fs_devices = fs_devices->seed;
 		goto again;
--
2.17.1


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

* [PATCH v2] btrfs: Switch seed device to list api
  2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
  2020-07-15 13:14   ` kernel test robot
@ 2020-07-16  7:25   ` Nikolay Borisov
  2020-08-18 15:19     ` Josef Bacik
  2020-08-30 14:39     ` Anand Jain
  2020-07-24  7:36   ` [PATCH 5/5] " Nikolay Borisov
  2020-09-02 15:58   ` Anand Jain
  3 siblings, 2 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-16  7:25 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

While this patch touches a bunch of files the conversion is
straighforward. Instead of using the implicit linked list anchored at
btrfs_fs_devices::seed the code is switched to using
list_for_each_entry. Previous patches in the series already factored out
code that processed both main and seed devices so in those cases
the factored out functions are called on the main fs_devices and then
on every seed dev inside list_for_each_entry.

Using list api also allows to simplify deletion from the seed dev list
performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by
substituting a while() loop with a simple list_del_init.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2:
 * Removed tmp_fs_devices from btrfs_rm_dev_replace_free_srcdev. Reported by
 kernel test robot

 fs/btrfs/disk-io.c |  39 ++++++++---------
 fs/btrfs/reada.c   |   9 ++--
 fs/btrfs/volumes.c | 105 +++++++++++++++++++++------------------------
 fs/btrfs/volumes.h |   2 +-
 4 files changed, 72 insertions(+), 83 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2fb0b43bc1c5..8767d47f6e29 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -545,33 +545,30 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 static int check_tree_block_fsid(struct extent_buffer *eb)
 {
 	struct btrfs_fs_info *fs_info = eb->fs_info;
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	u8 fsid[BTRFS_FSID_SIZE];
-	int ret = 1;
+	u8 *metadata_uuid;

 	read_extent_buffer(eb, fsid, offsetof(struct btrfs_header, fsid),
 			   BTRFS_FSID_SIZE);
-	while (fs_devices) {
-		u8 *metadata_uuid;
+	/*
+	 * Checking the incompat flag is only valid for the current
+	 * fs. For seed devices it's forbidden to have their uuid
+	 * changed so reading ->fsid in this case is fine
+	 */
+	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
+		metadata_uuid = fs_devices->metadata_uuid;
+	else
+		metadata_uuid = fs_devices->fsid;

-		/*
-		 * Checking the incompat flag is only valid for the current
-		 * fs. For seed devices it's forbidden to have their uuid
-		 * changed so reading ->fsid in this case is fine
-		 */
-		if (fs_devices == fs_info->fs_devices &&
-		    btrfs_fs_incompat(fs_info, METADATA_UUID))
-			metadata_uuid = fs_devices->metadata_uuid;
-		else
-			metadata_uuid = fs_devices->fsid;
+	if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE))
+		return 0;

-		if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE)) {
-			ret = 0;
-			break;
-		}
-		fs_devices = fs_devices->seed;
-	}
-	return ret;
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
+		if (!memcmp(fsid, seed_devs->fsid, BTRFS_FSID_SIZE))
+			return 0;
+
+	return 1;
 }

 static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index aa9d24ed56d7..ac5b07ded0fe 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -792,16 +792,13 @@ static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices)

 static void __reada_start_machine(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	int i;
 	u64 enqueued = 0;

-again:
 	enqueued += __reada_start_for_fsdevs(fs_devices);
-	if (fs_devices->seed) {
-		fs_devices = fs_devices->seed;
-		goto again;
-	}
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
+		enqueued += __reada_start_for_fsdevs(seed_devs);

 	if (enqueued == 0)
 		return;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 70e58d724384..b05dbb448ee0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -350,6 +350,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 	INIT_LIST_HEAD(&fs_devs->devices);
 	INIT_LIST_HEAD(&fs_devs->alloc_list);
 	INIT_LIST_HEAD(&fs_devs->fs_list);
+	INIT_LIST_HEAD(&fs_devs->seed_list);
 	if (fsid)
 		memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE);

@@ -1087,14 +1088,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 {
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_fs_devices *seed_dev;

 	mutex_lock(&uuid_mutex);
-again:
 	__btrfs_free_extra_devids(fs_devices, step, &latest_dev);
-	if (fs_devices->seed) {
-		fs_devices = fs_devices->seed;
-		goto again;
-	}
+
+	list_for_each_entry(seed_dev, &fs_devices->seed_list, seed_list)
+		__btrfs_free_extra_devids(seed_dev, step, &latest_dev);

 	fs_devices->latest_bdev = latest_dev->bdev;

@@ -1168,20 +1168,18 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)

 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
-	struct btrfs_fs_devices *seed_devices = NULL;
+	LIST_HEAD(list);
+	struct btrfs_fs_devices *tmp;

 	mutex_lock(&uuid_mutex);
 	close_fs_devices(fs_devices);
-	if (!fs_devices->opened) {
-		seed_devices = fs_devices->seed;
-		fs_devices->seed = NULL;
-	}
+	if (!fs_devices->opened)
+		list_splice_init(&fs_devices->seed_list, &list);
 	mutex_unlock(&uuid_mutex);

-	while (seed_devices) {
-		fs_devices = seed_devices;
-		seed_devices = fs_devices->seed;
+	list_for_each_entry_safe(fs_devices, tmp, &list, seed_list) {
 		close_fs_devices(fs_devices);
+		list_del(&fs_devices->seed_list);
 		free_fs_devices(fs_devices);
 	}
 }
@@ -2154,14 +2152,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	btrfs_free_device(device);

 	if (cur_devices->open_devices == 0) {
-		while (fs_devices) {
-			if (fs_devices->seed == cur_devices) {
-				fs_devices->seed = cur_devices->seed;
-				break;
-			}
-			fs_devices = fs_devices->seed;
-		}
-		cur_devices->seed = NULL;
+		list_del_init(&cur_devices->seed_list);
 		close_fs_devices(cur_devices);
 		free_fs_devices(cur_devices);
 	}
@@ -2225,8 +2216,6 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_device *srcdev)

 	/* if this is no devs we rather delete the fs_devices */
 	if (!fs_devices->num_devices) {
-		struct btrfs_fs_devices *tmp_fs_devices;
-
 		/*
 		 * On a mounted FS, num_devices can't be zero unless it's a
 		 * seed. In case of a seed device being replaced, the replace
@@ -2235,15 +2224,7 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_device *srcdev)
 		 */
 		ASSERT(fs_devices->seeding);

-		tmp_fs_devices = fs_info->fs_devices;
-		while (tmp_fs_devices) {
-			if (tmp_fs_devices->seed == fs_devices) {
-				tmp_fs_devices->seed = fs_devices->seed;
-				break;
-			}
-			tmp_fs_devices = tmp_fs_devices->seed;
-		}
-		fs_devices->seed = NULL;
+		list_del_init(&fs_devices->seed_list);
 		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
@@ -2397,7 +2378,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	fs_devices->open_devices = 0;
 	fs_devices->missing_devices = 0;
 	fs_devices->rotating = false;
-	fs_devices->seed = seed_devices;
+	list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list);

 	generate_random_uuid(fs_devices->fsid);
 	memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE);
@@ -6430,11 +6411,23 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
 				       bool seed)
 {
 	struct btrfs_device *device;
+	struct btrfs_fs_devices *seed_devs;
+
+	if (!fsid ||
+	    !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
+		list_for_each_entry(device, &fs_devices->devices,
+				    dev_list) {
+			if (device->devid == devid &&
+			    (!uuid || memcmp(device->uuid, uuid,
+					     BTRFS_UUID_SIZE) == 0))
+				return device;
+		}
+	}

-	while (fs_devices) {
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
 		if (!fsid ||
-		    !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
-			list_for_each_entry(device, &fs_devices->devices,
+		    !memcmp(seed_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
+			list_for_each_entry(device, &seed_devs->devices,
 					    dev_list) {
 				if (device->devid == devid &&
 				    (!uuid || memcmp(device->uuid, uuid,
@@ -6442,11 +6435,8 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
 					return device;
 			}
 		}
-		if (seed)
-			fs_devices = fs_devices->seed;
-		else
-			return NULL;
 	}
+
 	return NULL;
 }

@@ -6688,13 +6678,10 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	lockdep_assert_held(&uuid_mutex);
 	ASSERT(fsid);

-	fs_devices = fs_info->fs_devices->seed;
-	while (fs_devices) {
+	list_for_each_entry(fs_devices, &fs_info->fs_devices->seed_list, seed_list)
 		if (!memcmp(fs_devices->fsid, fsid, BTRFS_FSID_SIZE))
 			return fs_devices;

-		fs_devices = fs_devices->seed;
-	}

 	fs_devices = find_fsid(fsid, NULL);
 	if (!fs_devices) {
@@ -6728,8 +6715,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		goto out;
 	}

-	fs_devices->seed = fs_info->fs_devices->seed;
-	fs_info->fs_devices->seed = fs_devices;
+	ASSERT(list_empty(&fs_devices->seed_list));
+	list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list);
 out:
 	return fs_devices;
 }
@@ -7141,17 +7128,23 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)

 void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	struct btrfs_device *device;

-	while (fs_devices) {
-		mutex_lock(&fs_devices->device_list_mutex);
-		list_for_each_entry(device, &fs_devices->devices, dev_list)
+	fs_devices->fs_info = fs_info;
+
+	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);
+
+	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
+		mutex_lock(&seed_devs->device_list_mutex);
+		list_for_each_entry(device, &seed_devs->devices, dev_list)
 			device->fs_info = fs_info;
-		mutex_unlock(&fs_devices->device_list_mutex);
+		mutex_unlock(&seed_devs->device_list_mutex);

-		fs_devices->fs_info = fs_info;
-		fs_devices = fs_devices->seed;
+		seed_devs->fs_info = fs_info;
 	}
 }

@@ -7527,8 +7520,10 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,

 	/* It's possible this device is a dummy for seed device */
 	if (dev->disk_total_bytes == 0) {
-		dev = btrfs_find_device(fs_info->fs_devices->seed, devid, NULL,
-					NULL, false);
+		struct btrfs_fs_devices *devs;
+		devs = list_first_entry(&fs_info->fs_devices->seed_list,
+					struct btrfs_fs_devices, seed_list);
+		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
 		if (!dev) {
 			btrfs_err(fs_info, "failed to find seed devid %llu",
 				  devid);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index fc283fdbcece..709f4aacbd3f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -246,7 +246,7 @@ struct btrfs_fs_devices {
 	 */
 	struct list_head alloc_list;

-	struct btrfs_fs_devices *seed;
+	struct list_head seed_list;
 	bool seeding;

 	int opened;
--
2.17.1


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

* Re: [PATCH 0/5] Convert seed devices to proper list API
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
                   ` (4 preceding siblings ...)
  2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
@ 2020-07-22 14:26 ` David Sterba
  2020-07-23  8:02   ` Nikolay Borisov
  2020-08-17 19:19 ` Nikolay Borisov
  6 siblings, 1 reply; 31+ messages in thread
From: David Sterba @ 2020-07-22 14:26 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: linux-btrfs

On Wed, Jul 15, 2020 at 01:48:45PM +0300, Nikolay Borisov wrote:
> This series converts the existing seed devices pointer in btrfs_fs_devices to
> proper list api. First 4 patches are cleanups preparing the code for the switch.

> Patch 5 has more information about the required transformations, it might seem
> lengthy but he logic everywhere is pretty much the same so shouldn't be that
> cumbersome to review.

The first 3 patches are clear, I start to have doubts in 4 and 5 was a
stop for me, but I'm not saying it's wrong. Just that I always thought
the seed devices + the sprout are close to a tree structure but you're
switching that to a linear list.

I'll add the first three patches now, but would appreciate some help
with 4 and 5.

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

* Re: [PATCH 0/5] Convert seed devices to proper list API
  2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba
@ 2020-07-23  8:02   ` Nikolay Borisov
  2020-08-21 14:33     ` David Sterba
  0 siblings, 1 reply; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-23  8:02 UTC (permalink / raw)
  To: dsterba, linux-btrfs



On 22.07.20 г. 17:26 ч., David Sterba wrote:
> On Wed, Jul 15, 2020 at 01:48:45PM +0300, Nikolay Borisov wrote:
>> This series converts the existing seed devices pointer in btrfs_fs_devices to
>> proper list api. First 4 patches are cleanups preparing the code for the switch.
> 
>> Patch 5 has more information about the required transformations, it might seem
>> lengthy but he logic everywhere is pretty much the same so shouldn't be that
>> cumbersome to review.
> 
> The first 3 patches are clear, I start to have doubts in 4 and 5 was a
> stop for me, but I'm not saying it's wrong. Just that I always thought
> the seed devices + the sprout are close to a tree structure but you're
> switching that to a linear list.
> 
> I'll add the first three patches now, but would appreciate some help
> with 4 and 5.
> 

Ok, let's take it one at a time, patch 4:

Originally fs_info for fs_devices is set by btrfs_set_fs_info_ptr called
by btrfs_sysfs_add_mounted. THe latter is called in open_ctree before
block_groups are read i.e on before the fs is exposed. My patch instead
changes this so that fs_info is set in btrfs_init_devices_late, called
from btrfs_read_roots, called by init_tree_roots. The last function is
called before btrfs_sysfs_add_mounted. This means as far as setting
fs_devices is concerned its fs_info is being set earlier. And this is
correct.

Now, let's think about clearing it, the old code cleared it in :

btrfs_sysfs_remove_mounted which is called from:

1. fail_sysfs label in open_ctree
2. close_ctree
3. failure in btrfs_sysfs_add_mounted which does goto fail_fsdev_sysfs

So how are those 3 situation handled by the new code :

1 and 3 - the last function called in open_ctree is btrfs_close_devices
in case of errors and both failure labels are before it, so we are
guaranteed that on failure in open_ctree fs_devices will have it's
fs_info being NULL.

2. Again, last function called in close_ctree is btrfs_close_devices so
we are guaranteed on unmount fs_devices will have fs_info being NULL.

Thus the patch doesn't introduce any functional changes but IMO makes
the code more coherent.


Regarding patch 5 - I don't know what made you think there is a
tree-like structure involved. Simply looking at the old way seeds are
iterated:

	while (seed_devices) {
		fs_devices = seed_devices;
		seed_devices = fs_devices->seed;
 		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}


There's no conditional deciding if we should go left/right of the tree.
Or let's take for example deleting from a list of seed devices:

		tmp_fs_devices = fs_info->fs_devices;
		while (tmp_fs_devices) {
			if (tmp_fs_devices->seed == fs_devices) {
				tmp_fs_devices->seed = fs_devices->seed;
				break;
			}
			tmp_fs_devices = tmp_fs_devices->seed;
		}

Here a simple linear search is performed and when a member of the seed
list matches our fs_devices it simply pointed 1 past our fs_devices

When the if inside the loop matches we get the following situation:

[tmp_fs_devices]->[fs_devices]->[fs_devices->seeds]

Then we perform [tmp_fs_devices] -> [fs_devices->seed]

So a simple deletion, just the fact you were confused shows the old code
is rather wonky.



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

* Re: [PATCH 5/5] btrfs: Switch seed device to list api
  2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
  2020-07-15 13:14   ` kernel test robot
  2020-07-16  7:25   ` [PATCH v2] " Nikolay Borisov
@ 2020-07-24  7:36   ` Nikolay Borisov
  2020-09-02 15:58   ` Anand Jain
  3 siblings, 0 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-07-24  7:36 UTC (permalink / raw)
  To: linux-btrfs



On 15.07.20 г. 13:48 ч., Nikolay Borisov wrote:
> While this patch touches a bunch of files the conversion is
> straighforward. Instead of using the implicit linked list anchored at
> btrfs_fs_devices::seed the code is switched to using
> list_for_each_entry. Previous patches in the series already factored out
> code that processed both main and seed devices so in those cases
> the factored out functions are called on the main fs_devices and then
> on every seed dev inside list_for_each_entry.
> 
> Using list api also allows to simplify deletion from the seed dev list
> performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by
> substituting a while() loop with a simple list_del_init.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

<snip>

> @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>  		goto out;
>  	}
>  
> -	fs_devices->seed = fs_info->fs_devices->seed;
> -	fs_info->fs_devices->seed = fs_devices;
> +	ASSERT(list_empty(&fs_devices->seed_list));

That assert is useless, since fs_devices at this point are a clone from
clone_fs_devices hence its seed_list cannot be non-empty. Consider this
when removing merging it.

> +	list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list);
>  out:
>  	return fs_devices;
>  }
> @@ -7141,17 +7131,23 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
>  
>  void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
>  {
> -	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
> +	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
>  	struct btrfs_device *device;
>  
> -	while (fs_devices) {
> -		mutex_lock(&fs_devices->device_list_mutex);
> -		list_for_each_entry(device, &fs_devices->devices, dev_list)
> +	fs_devices->fs_info = fs_info;
> +
> +	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);
> +
> +	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
> +		mutex_lock(&seed_devs->device_list_mutex);
> +		list_for_each_entry(device, &seed_devs->devices, dev_list)
>  			device->fs_info = fs_info;
> -		mutex_unlock(&fs_devices->device_list_mutex);
> +		mutex_unlock(&seed_devs->device_list_mutex);
>  
> -		fs_devices->fs_info = fs_info;
> -		fs_devices = fs_devices->seed;
> +		seed_devs->fs_info = fs_info;
>  	}
>  }
>  
> @@ -7527,8 +7523,10 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>  
>  	/* It's possible this device is a dummy for seed device */
>  	if (dev->disk_total_bytes == 0) {
> -		dev = btrfs_find_device(fs_info->fs_devices->seed, devid, NULL,
> -					NULL, false);
> +		struct btrfs_fs_devices *devs;
> +		devs = list_first_entry(&fs_info->fs_devices->seed_list,
> +					struct btrfs_fs_devices, seed_list);
> +		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
>  		if (!dev) {
>  			btrfs_err(fs_info, "failed to find seed devid %llu",
>  				  devid);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index fc283fdbcece..709f4aacbd3f 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -246,7 +246,7 @@ struct btrfs_fs_devices {
>  	 */
>  	struct list_head alloc_list;
>  
> -	struct btrfs_fs_devices *seed;
> +	struct list_head seed_list;
>  	bool seeding;
>  
>  	int opened;
> 

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

* Re: [PATCH 0/5] Convert seed devices to proper list API
  2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
                   ` (5 preceding siblings ...)
  2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba
@ 2020-08-17 19:19 ` Nikolay Borisov
  6 siblings, 0 replies; 31+ messages in thread
From: Nikolay Borisov @ 2020-08-17 19:19 UTC (permalink / raw)
  To: linux-btrfs



On 15.07.20 г. 13:48 ч., Nikolay Borisov wrote:
> This series converts the existing seed devices pointer in btrfs_fs_devices to
> proper list api. First 4 patches are cleanups preparing the code for the switch.
> Patch 5 has more information about the required transformations, it might seem
> lengthy but he logic everywhere is pretty much the same so shouldn't be that
> cumbersome to review.
> 
> This series survived xfstest runs and even caught 2 bugs while developing it.
> 
> Nikolay Borisov (5):
>   btrfs: Factor out reada loop in __reada_start_machine
>   btrfs: Factor out loop logic from btrfs_free_extra_devids
>   btrfs: Make close_fs_devices return void
>   btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices
>   btrfs: Switch seed device to list api
> 
>  fs/btrfs/disk-io.c |  39 +++++------
>  fs/btrfs/reada.c   |  26 ++++---
>  fs/btrfs/sysfs.c   |   4 --
>  fs/btrfs/volumes.c | 166 ++++++++++++++++++++-------------------------
>  fs/btrfs/volumes.h |   6 +-
>  5 files changed, 112 insertions(+), 129 deletions(-)
> 
> --
> 2.17.1
> 

Gentle ping

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

* Re: [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine
  2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov
@ 2020-08-18 15:02   ` Josef Bacik
  2020-08-29 15:06   ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Josef Bacik @ 2020-08-18 15:02 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 7/15/20 6:48 AM, Nikolay Borisov wrote:
> This is in preparation for moving fs_devices to proper lists.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

* Re: [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids
  2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov
  2020-07-15 12:32   ` kernel test robot
  2020-07-16  7:17   ` [PATCH v2] " Nikolay Borisov
@ 2020-08-18 15:03   ` Josef Bacik
  2 siblings, 0 replies; 31+ messages in thread
From: Josef Bacik @ 2020-08-18 15:03 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 7/15/20 6:48 AM, Nikolay Borisov wrote:
> This prepares the code to switching seeds devices to a proper list.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

* Re: [PATCH 3/5] btrfs: Make close_fs_devices return void
  2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov
@ 2020-08-18 15:05   ` Josef Bacik
  2020-08-29 15:14   ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Josef Bacik @ 2020-08-18 15:05 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 7/15/20 6:48 AM, Nikolay Borisov wrote:
> The return value of this function conveys absolutely no information.
> All callers already check the state of  fs_devices->opened to decide
> how to proceed. So conver the function to returning void. While at it
> make btrfs_close_devices also return void.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

* Re: [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices
  2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov
@ 2020-08-18 15:08   ` Josef Bacik
  2020-08-26 10:50   ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Josef Bacik @ 2020-08-18 15:08 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 7/15/20 6:48 AM, Nikolay Borisov wrote:
> It makes no sense to have sysfs-related routines be responsible for
> properly initialising the fs_info pointer of struct btrfs_fs_device.
> Instead this can be streamlined by making it the responsibility of
> btrfs_init_devices_late to initialize it. That function already
> initializes fs_info of every individual device in btrfs_fs_devices.
> 
> As far as clearing it is concerned it makes sense to move it to
> close_fs_devices. That function is only called when struct
> btrfs_fs_devices is no longer in use - either for holding seeds or main
> devices for a mounted filesystem.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

I was sort of worried we inadvertently depended on some other locking from where 
btrfs_sysfs_add_mounted() was called, but we don't

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

* Re: [PATCH v2] btrfs: Switch seed device to list api
  2020-07-16  7:25   ` [PATCH v2] " Nikolay Borisov
@ 2020-08-18 15:19     ` Josef Bacik
  2020-08-30 14:39     ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Josef Bacik @ 2020-08-18 15:19 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 7/16/20 3:25 AM, Nikolay Borisov wrote:
> While this patch touches a bunch of files the conversion is
> straighforward. Instead of using the implicit linked list anchored at
> btrfs_fs_devices::seed the code is switched to using
> list_for_each_entry. Previous patches in the series already factored out
> code that processed both main and seed devices so in those cases
> the factored out functions are called on the main fs_devices and then
> on every seed dev inside list_for_each_entry.
> 
> Using list api also allows to simplify deletion from the seed dev list
> performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by
> substituting a while() loop with a simple list_del_init.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef


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

* Re: [PATCH 0/5] Convert seed devices to proper list API
  2020-07-23  8:02   ` Nikolay Borisov
@ 2020-08-21 14:33     ` David Sterba
  0 siblings, 0 replies; 31+ messages in thread
From: David Sterba @ 2020-08-21 14:33 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: dsterba, linux-btrfs

On Thu, Jul 23, 2020 at 11:02:14AM +0300, Nikolay Borisov wrote:
> Regarding patch 5 - I don't know what made you think there is a
> tree-like structure involved. Simply looking at the old way seeds are
> iterated:
> 
> 	while (seed_devices) {
> 		fs_devices = seed_devices;
> 		seed_devices = fs_devices->seed;
>  		close_fs_devices(fs_devices);
>  		free_fs_devices(fs_devices);
>  	}
> 
> There's no conditional deciding if we should go left/right of the tree.

A tree structure that has only one direction arrows, from leaves to the
root. Where the root is the seed fs and the leaves are the sprouts.

	 fs1 ---> seed
		  ^ ^
	 fs2 -----| |
                    |
	 fs3 -------|

The while loop in each fs goes by the pointers up to the seeding device
and always removes it's references.

I'm not sure about a deeper tree structure here, so the fs3 -> fs2 would
be possible.

> Or let's take for example deleting from a list of seed devices:
> 
> 		tmp_fs_devices = fs_info->fs_devices;
> 		while (tmp_fs_devices) {
> 			if (tmp_fs_devices->seed == fs_devices) {
> 				tmp_fs_devices->seed = fs_devices->seed;
> 				break;
> 			}
> 			tmp_fs_devices = tmp_fs_devices->seed;
> 		}
> 
> Here a simple linear search is performed and when a member of the seed
> list matches our fs_devices it simply pointed 1 past our fs_devices
> 
> When the if inside the loop matches we get the following situation:
> 
> [tmp_fs_devices]->[fs_devices]->[fs_devices->seeds]
> 
> Then we perform [tmp_fs_devices] -> [fs_devices->seed]
> 
> So a simple deletion, just the fact you were confused shows the old code
> is rather wonky.

Well, I still am after reading the above, the missing part is about the
device cloning and that each fs has it's own copy. The tree structure
implies sharing and would need reference counting, but this does not
seem to be so.

I'll add the series to for-next so we have a base for the other seeding
patches but I haven't reviewed it to the point where I'm convinced it's
correct.

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

* Re: [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices
  2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov
  2020-08-18 15:08   ` Josef Bacik
@ 2020-08-26 10:50   ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Anand Jain @ 2020-08-26 10:50 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 15/7/20 6:48 pm, Nikolay Borisov wrote:
> It makes no sense to have sysfs-related routines be responsible for
> properly initialising the fs_info pointer of struct btrfs_fs_device.
> Instead this can be streamlined by making it the responsibility of
> btrfs_init_devices_late to initialize it. That function already
> initializes fs_info of every individual device in btrfs_fs_devices.
> 
> As far as clearing it is concerned it makes sense to move it to
> close_fs_devices. That function is only called when struct
> btrfs_fs_devices is no longer in use - either for holding seeds or main
> devices for a mounted filesystem.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>


Reviewed-by: Anand Jain <anand.jain@oracle.com>


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

* Re: [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine
  2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov
  2020-08-18 15:02   ` Josef Bacik
@ 2020-08-29 15:06   ` Anand Jain
  2020-08-31 12:24     ` David Sterba
  1 sibling, 1 reply; 31+ messages in thread
From: Anand Jain @ 2020-08-29 15:06 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 15/7/20 6:48 pm, Nikolay Borisov wrote:
> This is in preparation for moving fs_devices to proper lists.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/reada.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
> index 243a2e44526e..aa9d24ed56d7 100644
> --- a/fs/btrfs/reada.c
> +++ b/fs/btrfs/reada.c
> @@ -767,15 +767,14 @@ static void reada_start_machine_worker(struct btrfs_work *work)
>   	kfree(rmw);
>   }
>   
> -static void __reada_start_machine(struct btrfs_fs_info *fs_info)
> +
> +/* Try to start up to 10k READA requests for a group of devices. */
> +static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices)

  In my experience David doesn't prefer __ prefix for helper functions.

>   {
> -	struct btrfs_device *device;
> -	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>   	u64 enqueued;
>   	u64 total = 0;
> -	int i;
> +	struct btrfs_device *device;
>   
> -again:
>   	do {
>   		enqueued = 0;
>   		mutex_lock(&fs_devices->device_list_mutex);
> @@ -787,6 +786,18 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info)
>   		mutex_unlock(&fs_devices->device_list_mutex);
>   		total += enqueued;
>   	} while (enqueued && total < 10000);
> +
> +	return total;
> +}
> +
> +static void __reada_start_machine(struct btrfs_fs_info *fs_info)
> +{
> +	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
> +	int i;
> +	u64 enqueued = 0;
> +
> +again:
> +	enqueued += __reada_start_for_fsdevs(fs_devices);
>   	if (fs_devices->seed) {
>   		fs_devices = fs_devices->seed;
>   		goto again;
> 

There wasn't any need to create another helper function here IMO.

Anyway changes looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


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

* Re: [PATCH v2] btrfs: Factor out loop logic from btrfs_free_extra_devids
  2020-07-16  7:17   ` [PATCH v2] " Nikolay Borisov
@ 2020-08-29 15:13     ` Anand Jain
  0 siblings, 0 replies; 31+ messages in thread
From: Anand Jain @ 2020-08-29 15:13 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 16/7/20 3:17 pm, Nikolay Borisov wrote:
> This prepares the code to switching seeds devices to a proper list.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V2:
>   * Added missing static modifier to the factored out function. Reported by
>   kernel test robot
> 
>   fs/btrfs/volumes.c | 34 +++++++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ce01e44f8134..76a68edb3127 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1024,28 +1024,24 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
>   	return ERR_PTR(ret);
>   }
> 
> -/*
> - * After we have read the system tree and know devids belonging to
> - * this filesystem, remove the device which does not belong there.
> - */
> -void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
> +
> +
> +static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> +				      int step, struct btrfs_device **latest_dev)

David doesn't prefer __ prefix for helpers.

>   {
>   	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) {
>   		if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
> -							&device->dev_state)) {
> +			     &device->dev_state)) {
>   			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> -			     &device->dev_state) &&
> +				      &device->dev_state) &&
>   			    !test_bit(BTRFS_DEV_STATE_MISSING,
>   				      &device->dev_state) &&
> -			     (!latest_dev ||
> -			      device->generation > latest_dev->generation)) {
> -				latest_dev = device;
> +			    (!*latest_dev ||
> +			     device->generation > (*latest_dev)->generation)) {
> +				*latest_dev = device;
>   			}
>   			continue;
>   		}
> @@ -1083,6 +1079,18 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
>   		btrfs_free_device(device);
>   	}
> 
> +}
> +/*
> + * After we have read the system tree and know devids belonging to
> + * this filesystem, remove the device which does not belong there.
> + */
> +void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
> +{
> +	struct btrfs_device *latest_dev = NULL;
> +
> +	mutex_lock(&uuid_mutex);
> +again:
> +	__btrfs_free_extra_devids(fs_devices, step, &latest_dev);


>   	if (fs_devices->seed) {
>   		fs_devices = fs_devices->seed;
>   		goto again;
> --
> 2.17.1
> 


Looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>


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

* Re: [PATCH 3/5] btrfs: Make close_fs_devices return void
  2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov
  2020-08-18 15:05   ` Josef Bacik
@ 2020-08-29 15:14   ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Anand Jain @ 2020-08-29 15:14 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 15/7/20 6:48 pm, Nikolay Borisov wrote:
> The return value of this function conveys absolutely no information.
> All callers already check the state of  fs_devices->opened to decide
> how to proceed. So conver the function to returning void. While at it
> make btrfs_close_devices also return void.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/volumes.c | 12 ++++--------
>   fs/btrfs/volumes.h |  2 +-
>   2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index db29fc4fbe89..6de021c78277 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1146,12 +1146,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) {
> @@ -1163,17 +1163,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;
> @@ -1186,7 +1183,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,
> 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);
> 


Reviewed-by: Anand Jain <anand.jain@oracle.com>

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

* Re: [PATCH v2] btrfs: Switch seed device to list api
  2020-07-16  7:25   ` [PATCH v2] " Nikolay Borisov
  2020-08-18 15:19     ` Josef Bacik
@ 2020-08-30 14:39     ` Anand Jain
  1 sibling, 0 replies; 31+ messages in thread
From: Anand Jain @ 2020-08-30 14:39 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

On 16/7/20 3:25 pm, Nikolay Borisov wrote:
> While this patch touches a bunch of files the conversion is
> straighforward. Instead of using the implicit linked list anchored at
> btrfs_fs_devices::seed the code is switched to using
> list_for_each_entry. Previous patches in the series already factored out
> code that processed both main and seed devices so in those cases
> the factored out functions are called on the main fs_devices and then
> on every seed dev inside list_for_each_entry.
> 
> Using list api also allows to simplify deletion from the seed dev list
> performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by
> substituting a while() loop with a simple list_del_init.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V2:
>   * Removed tmp_fs_devices from btrfs_rm_dev_replace_free_srcdev. Reported by
>   kernel test robot
> 

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand


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

* Re: [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine
  2020-08-29 15:06   ` Anand Jain
@ 2020-08-31 12:24     ` David Sterba
  0 siblings, 0 replies; 31+ messages in thread
From: David Sterba @ 2020-08-31 12:24 UTC (permalink / raw)
  To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs

On Sat, Aug 29, 2020 at 11:06:32PM +0800, Anand Jain wrote:
> On 15/7/20 6:48 pm, Nikolay Borisov wrote:
> > This is in preparation for moving fs_devices to proper lists.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> >   fs/btrfs/reada.c | 21 ++++++++++++++++-----
> >   1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
> > index 243a2e44526e..aa9d24ed56d7 100644
> > --- a/fs/btrfs/reada.c
> > +++ b/fs/btrfs/reada.c
> > @@ -767,15 +767,14 @@ static void reada_start_machine_worker(struct btrfs_work *work)
> >   	kfree(rmw);
> >   }
> >   
> > -static void __reada_start_machine(struct btrfs_fs_info *fs_info)
> > +
> > +/* Try to start up to 10k READA requests for a group of devices. */
> > +static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices)
> 
>   In my experience David doesn't prefer __ prefix for helper functions.

Right, I've updated the patch to drop the __ as there's no other
function without the underscores (unlike __reada_start_machine/reada_start_machine)

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

* Re: [PATCH 5/5] btrfs: Switch seed device to list api
  2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
                     ` (2 preceding siblings ...)
  2020-07-24  7:36   ` [PATCH 5/5] " Nikolay Borisov
@ 2020-09-02 15:58   ` Anand Jain
  2020-09-03  9:03     ` Nikolay Borisov
  3 siblings, 1 reply; 31+ messages in thread
From: Anand Jain @ 2020-09-02 15:58 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



The seed of the current sprout should rather be at the head instead of 
at the bottom.


> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>   	fs_devices->open_devices = 0;
>   	fs_devices->missing_devices = 0;
>   	fs_devices->rotating = false;
> -	fs_devices->seed = seed_devices;
> +	list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list);

  It should be list_add_head.

>   
>   	generate_random_uuid(fs_devices->fsid);
>   	memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE);




> @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>   		goto out;
>   	}
>   
> -	fs_devices->seed = fs_info->fs_devices->seed;
> -	fs_info->fs_devices->seed = fs_devices;
> +	ASSERT(list_empty(&fs_devices->seed_list));
> +	list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list);

  It should be list_add_head.

>   out:
>   	return fs_devices;
>   }


Thanks, Anand

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

* Re: [PATCH 5/5] btrfs: Switch seed device to list api
  2020-09-02 15:58   ` Anand Jain
@ 2020-09-03  9:03     ` Nikolay Borisov
  2020-09-03  9:33       ` Anand Jain
  0 siblings, 1 reply; 31+ messages in thread
From: Nikolay Borisov @ 2020-09-03  9:03 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 2.09.20 г. 18:58 ч., Anand Jain wrote:
> 
> 
> The seed of the current sprout should rather be at the head instead of
> at the bottom.
> 
> 
>> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct
>> btrfs_fs_info *fs_info)
>>       fs_devices->open_devices = 0;
>>       fs_devices->missing_devices = 0;
>>       fs_devices->rotating = false;
>> -    fs_devices->seed = seed_devices;
>> +    list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list);
> 
>  It should be list_add_head.

Generally yes, but in this case I don't think it makes any functional
differences so even adding at the tail is fine.

> 
>>         generate_random_uuid(fs_devices->fsid);
>>       memcpy(fs_devices->metadata_uuid, fs_devices->fsid,
>> BTRFS_FSID_SIZE);
> 
> 
> 
> 
>> @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices
>> *open_seed_devices(struct btrfs_fs_info *fs_info,
>>           goto out;
>>       }
>>   -    fs_devices->seed = fs_info->fs_devices->seed;
>> -    fs_info->fs_devices->seed = fs_devices;
>> +    ASSERT(list_empty(&fs_devices->seed_list));
>> +    list_add_tail(&fs_devices->seed_list,
>> &fs_info->fs_devices->seed_list);
> 
>  It should be list_add_head.
> 
>>   out:
>>       return fs_devices;
>>   }
> 
> 
> Thanks, Anand
> 

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

* Re: [PATCH 5/5] btrfs: Switch seed device to list api
  2020-09-03  9:03     ` Nikolay Borisov
@ 2020-09-03  9:33       ` Anand Jain
  2020-09-10 16:28         ` David Sterba
  0 siblings, 1 reply; 31+ messages in thread
From: Anand Jain @ 2020-09-03  9:33 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



On 3/9/20 5:03 pm, Nikolay Borisov wrote:
> 
> 
> On 2.09.20 г. 18:58 ч., Anand Jain wrote:
>>
>>
>> The seed of the current sprout should rather be at the head instead of
>> at the bottom.
>>
>>
>>> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct
>>> btrfs_fs_info *fs_info)
>>>        fs_devices->open_devices = 0;
>>>        fs_devices->missing_devices = 0;
>>>        fs_devices->rotating = false;
>>> -    fs_devices->seed = seed_devices;
>>> +    list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list);
>>
>>   It should be list_add_head.
> 
> Generally yes, but in this case I don't think it makes any functional
> differences so even adding at the tail is fine.
> 

  Hm No. Adding to the head matches to the order of dependency. As it 
was in the while loop.

Thanks, Anand



>>
>>>          generate_random_uuid(fs_devices->fsid);
>>>        memcpy(fs_devices->metadata_uuid, fs_devices->fsid,
>>> BTRFS_FSID_SIZE);
>>
>>
>>
>>
>>> @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices
>>> *open_seed_devices(struct btrfs_fs_info *fs_info,
>>>            goto out;
>>>        }
>>>    -    fs_devices->seed = fs_info->fs_devices->seed;
>>> -    fs_info->fs_devices->seed = fs_devices;
>>> +    ASSERT(list_empty(&fs_devices->seed_list));
>>> +    list_add_tail(&fs_devices->seed_list,
>>> &fs_info->fs_devices->seed_list);
>>
>>   It should be list_add_head.
>>
>>>    out:
>>>        return fs_devices;
>>>    }
>>
>>
>> Thanks, Anand
>>

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

* Re: [PATCH 5/5] btrfs: Switch seed device to list api
  2020-09-03  9:33       ` Anand Jain
@ 2020-09-10 16:28         ` David Sterba
  0 siblings, 0 replies; 31+ messages in thread
From: David Sterba @ 2020-09-10 16:28 UTC (permalink / raw)
  To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs

On Thu, Sep 03, 2020 at 05:33:26PM +0800, Anand Jain wrote:
> 
> 
> On 3/9/20 5:03 pm, Nikolay Borisov wrote:
> > 
> > 
> > On 2.09.20 г. 18:58 ч., Anand Jain wrote:
> >>
> >>
> >> The seed of the current sprout should rather be at the head instead of
> >> at the bottom.
> >>
> >>
> >>> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct
> >>> btrfs_fs_info *fs_info)
> >>>        fs_devices->open_devices = 0;
> >>>        fs_devices->missing_devices = 0;
> >>>        fs_devices->rotating = false;
> >>> -    fs_devices->seed = seed_devices;
> >>> +    list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list);
> >>
> >>   It should be list_add_head.
> > 
> > Generally yes, but in this case I don't think it makes any functional
> > differences so even adding at the tail is fine.
> > 
> 
>   Hm No. Adding to the head matches to the order of dependency. As it 
> was in the while loop.

Following the same order sounds like a better idea to me. If there's
really no differece and we want to add the entry to the tail, then it
would be another patch with proper reasoning. I'll update it to do
list_head.

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

end of thread, other threads:[~2020-09-10 16:44 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov
2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov
2020-08-18 15:02   ` Josef Bacik
2020-08-29 15:06   ` Anand Jain
2020-08-31 12:24     ` David Sterba
2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov
2020-07-15 12:32   ` kernel test robot
2020-07-15 12:39     ` Nikolay Borisov
2020-07-16  7:17   ` [PATCH v2] " Nikolay Borisov
2020-08-29 15:13     ` Anand Jain
2020-08-18 15:03   ` [PATCH 2/5] " Josef Bacik
2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov
2020-08-18 15:05   ` Josef Bacik
2020-08-29 15:14   ` Anand Jain
2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov
2020-08-18 15:08   ` Josef Bacik
2020-08-26 10:50   ` Anand Jain
2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov
2020-07-15 13:14   ` kernel test robot
2020-07-16  7:25   ` [PATCH v2] " Nikolay Borisov
2020-08-18 15:19     ` Josef Bacik
2020-08-30 14:39     ` Anand Jain
2020-07-24  7:36   ` [PATCH 5/5] " Nikolay Borisov
2020-09-02 15:58   ` Anand Jain
2020-09-03  9:03     ` Nikolay Borisov
2020-09-03  9:33       ` Anand Jain
2020-09-10 16:28         ` David Sterba
2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba
2020-07-23  8:02   ` Nikolay Borisov
2020-08-21 14:33     ` David Sterba
2020-08-17 19:19 ` Nikolay Borisov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).