[PATCH v2 0/9] btrfs: metadata_uuid refactors part1

[PATCH v2 0/9] btrfs: metadata_uuid refactors part1

[Anand Jain]

v2: Addressed the review comments received on some patches. Please refer
    to the individual patches for more details.

The metadata_uuid feature added later has significantly impacted code
readability due to the numerous conditions that need to be checked.

This patch set aims to improve code organization and prepares for
streamlining of the metadata_uuid checks and some simple fixes.

Anand Jain (9):
  btrfs: reduce struct btrfs_fs_devices size relocate fsid_change
  btrfs: streamline fsid checks in alloc_fs_devices
  btrfs: localise has_metadata_uuid check in alloc_fs_devices args
  btrfs: add comment about metadata_uuid in btrfs_fs_devices
  btrfs: simplify check_tree_block_fsid return arg to bool
  btrfs: refactor with match_fsid_fs_devices helper
  btrfs: refactor with match_fsid_changed helper
  btrfs: consolidate uuid memcmp in btrfs_validate_super
  btrfs: add and fix comments in btrfs_fs_devices

 fs/btrfs/disk-io.c |  24 +++++-----
 fs/btrfs/volumes.c | 110 +++++++++++++++++++++++++--------------------
 fs/btrfs/volumes.h |  37 ++++++++++-----
 3 files changed, 99 insertions(+), 72 deletions(-)

-- 
2.38.1

[PATCH v2 1/9] btrfs: reduce struct btrfs_fs_devices size relocate fsid_change

[Anand Jain]

Pack bool fsid_change and bool seeding with other bool declarations in the
struct btrfs_fs_devices, approximately 6 bytes is saved.

   before: 512 bytes
   after: 496 bytes

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Removed added code comments in v1.

 fs/btrfs/volumes.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 5cbbee32748c..236ae696c984 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -281,7 +281,6 @@ enum btrfs_read_policy {
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
 	u8 metadata_uuid[BTRFS_FSID_SIZE];
-	bool fsid_change;
 	struct list_head fs_list;
 
 	/*
@@ -337,7 +336,6 @@ struct btrfs_fs_devices {
 	struct list_head alloc_list;
 
 	struct list_head seed_list;
-	bool seeding;
 
 	int opened;
 
@@ -347,6 +345,8 @@ struct btrfs_fs_devices {
 	bool rotating;
 	/* Devices support TRIM/discard commands */
 	bool discardable;
+	bool fsid_change;
+	bool seeding;
 
 	struct btrfs_fs_info *fs_info;
 	/* sysfs kobjects */
-- 
2.38.1

Re: [PATCH v2 1/9] btrfs: reduce struct btrfs_fs_devices size relocate fsid_change

[David Sterba]

On Wed, May 24, 2023 at 08:02:35PM +0800, Anand Jain wrote:
> Pack bool fsid_change and bool seeding with other bool declarations in the
> struct btrfs_fs_devices, approximately 6 bytes is saved.
> 
>    before: 512 bytes
>    after: 496 bytes

I checked the difference on a release build and it's even better, 16
bytes, depends on the optional config features in some of the embedded
structures.

--- pre/btrfs.ko.pahole 2023-05-24 21:54:47.326166521 +0200
+++ post/btrfs.ko.pahole        2023-05-24 21:54:47.754166522 +0200
@@ -1700,49 +1700,39 @@ struct btrfs_free_space_op {
...
 
-       /* size: 344, cachelines: 6, members: 27 */
-       /* sum members: 328, holes: 3, sum holes: 16 */
-       /* last cacheline: 24 bytes */
+       /* size: 328, cachelines: 6, members: 27 */
+       /* last cacheline: 8 bytes */

[PATCH v2 2/9] btrfs: streamline fsid checks in alloc_fs_devices

[Anand Jain]

We currently have redundant checks for the non-null value of fsid
simplify it.

And, no one is using alloc_fs_devices() with a NULL metadata_uuid
while fsid is not NULL, add an assert() to verify this condition.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Simplify ASSERT condition.
    Fix space before open brackets.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1a7620680f50..4b35b28c8746 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -370,6 +370,8 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 {
 	struct btrfs_fs_devices *fs_devs;
 
+	ASSERT(fsid || !metadata_fsid);
+
 	fs_devs = kzalloc(sizeof(*fs_devs), GFP_KERNEL);
 	if (!fs_devs)
 		return ERR_PTR(-ENOMEM);
@@ -380,13 +382,12 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 	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);
 
-	if (metadata_fsid)
-		memcpy(fs_devs->metadata_uuid, metadata_fsid, BTRFS_FSID_SIZE);
-	else if (fsid)
-		memcpy(fs_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE);
+	if (fsid) {
+		memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE);
+		memcpy(fs_devs->metadata_uuid,
+		       metadata_fsid ? metadata_fsid : fsid, BTRFS_FSID_SIZE);
+	}
 
 	return fs_devs;
 }
-- 
2.38.1

[PATCH v2 3/9] btrfs: localise has_metadata_uuid check in alloc_fs_devices args

[Anand Jain]

Simplify %has_metadata_uuid checks - by localizing the
%has_metadata_uuid checked within alloc_fs_devices()'s second argument,
it improves the code readability.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: None.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4b35b28c8746..f573f93024b0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -791,12 +791,9 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 
 
 	if (!fs_devices) {
-		if (has_metadata_uuid)
-			fs_devices = alloc_fs_devices(disk_super->fsid,
-						      disk_super->metadata_uuid);
-		else
-			fs_devices = alloc_fs_devices(disk_super->fsid, NULL);
-
+		fs_devices = alloc_fs_devices(disk_super->fsid,
+					      has_metadata_uuid ?
+					      disk_super->metadata_uuid : NULL);
 		if (IS_ERR(fs_devices))
 			return ERR_CAST(fs_devices);
 
-- 
2.38.1

[PATCH v2 4/9] btrfs: add comment about metadata_uuid in btrfs_fs_devices

[Anand Jain]

Add comment about metadata_uuid in btrfs_fs_devices.
No functional change.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Fix added code comment style.

 fs/btrfs/volumes.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 236ae696c984..56633d4f9b31 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -280,7 +280,17 @@ enum btrfs_read_policy {
 
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
+
+	/*
+	 * UUID written into the btree blocks:
+	 *   If metadata_uuid != fsid then sb must have
+	 *   	BTRFS_FEATURE_INCOMPAT_METADATA_UUID flag set.
+	 *   Following shall be true at all times.
+	 *   	metadata_uuid == btrfs_header::fsid
+	 *   	metadata_uuid == btrfs_dev_item::fsid
+	 */
 	u8 metadata_uuid[BTRFS_FSID_SIZE];
+
 	struct list_head fs_list;
 
 	/*
-- 
2.38.1

[PATCH v2 5/9] btrfs: simplify check_tree_block_fsid return arg to bool

[Anand Jain]

Simplify the return type of the static function check_tree_block_fsid()
from int (1 or 0) to bool. Its only user is interested in knowing the
success or failure.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: None.

 fs/btrfs/disk-io.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 52caaf4b0678..6681e82900b0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -315,7 +315,7 @@ blk_status_t btree_csum_one_bio(struct btrfs_bio *bbio)
 	return errno_to_blk_status(ret);
 }
 
-static int check_tree_block_fsid(struct extent_buffer *eb)
+static bool 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, *seed_devs;
@@ -335,13 +335,13 @@ static int check_tree_block_fsid(struct extent_buffer *eb)
 		metadata_uuid = fs_devices->fsid;
 
 	if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE))
-		return 0;
+		return false;
 
 	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
 		if (!memcmp(fsid, seed_devs->fsid, BTRFS_FSID_SIZE))
-			return 0;
+			return false;
 
-	return 1;
+	return true;
 }
 
 /* Do basic extent buffer checks at read time */
-- 
2.38.1

[PATCH v2 6/9] btrfs: refactor with match_fsid_fs_devices helper

[Anand Jain]

Refactor the functions find_fsid() and find_fsid_with_metadata_uuid(), as
they currently share a common set of code to compare the fsid and
metadata_uuid. Create a common helper function,
match_fsid_fs_devices().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Rename helper function.

 fs/btrfs/volumes.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f573f93024b0..3d426dbd1199 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void)
 	}
 }
 
+static bool match_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
+				   const u8 *fsid, const u8 *metadata_fsid)
+{
+	if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) != 0)
+		return false;
+
+	if (!metadata_fsid)
+		return true;
+
+	if (memcmp(metadata_fsid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE) !=
+		   0)
+		return false;
+
+	return true;
+}
+
 static noinline struct btrfs_fs_devices *find_fsid(
 		const u8 *fsid, const u8 *metadata_fsid)
 {
@@ -436,15 +452,8 @@ static noinline struct btrfs_fs_devices *find_fsid(
 
 	/* Handle non-split brain cases */
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-		if (metadata_fsid) {
-			if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0
-			    && memcmp(metadata_fsid, fs_devices->metadata_uuid,
-				      BTRFS_FSID_SIZE) == 0)
-				return fs_devices;
-		} else {
-			if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0)
-				return fs_devices;
-		}
+		if (match_fsid_fs_devices(fs_devices, fsid, metadata_fsid))
+			return fs_devices;
 	}
 	return NULL;
 }
@@ -462,14 +471,15 @@ static struct btrfs_fs_devices *find_fsid_with_metadata_uuid(
 	 * at all and the CHANGING_FSID_V2 flag set.
 	 */
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-		if (fs_devices->fsid_change &&
-		    memcmp(disk_super->metadata_uuid, fs_devices->fsid,
-			   BTRFS_FSID_SIZE) == 0 &&
-		    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
-			   BTRFS_FSID_SIZE) == 0) {
+		if (!fs_devices->fsid_change)
+			continue;
+
+		if (match_fsid_fs_devices(fs_devices,
+					   disk_super->metadata_uuid,
+					   fs_devices->fsid))
 			return fs_devices;
-		}
 	}
+
 	/*
 	 * Handle scanned device having completed its fsid change but
 	 * belonging to a fs_devices that was created by a device that
-- 
2.38.1

Re: [PATCH v2 6/9] btrfs: refactor with match_fsid_fs_devices helper

[David Sterba]

On Wed, May 24, 2023 at 08:02:40PM +0800, Anand Jain wrote:
> Refactor the functions find_fsid() and find_fsid_with_metadata_uuid(), as
> they currently share a common set of code to compare the fsid and
> metadata_uuid. Create a common helper function,
> match_fsid_fs_devices().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: Rename helper function.
> 
>  fs/btrfs/volumes.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index f573f93024b0..3d426dbd1199 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void)
>  	}
>  }
>  
> +static bool match_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
> +				   const u8 *fsid, const u8 *metadata_fsid)

The match_ prefix looks on in this context.

[PATCH v2 7/9] btrfs: refactor with match_fsid_changed helper

[Anand Jain]

We often check if the metadata_uuid is not the same as fsid, and then we
check if the given fsid matches the metadata_uuid. This patch refactors
this logic into function match_fsid_changed and utilize it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Rename helper function.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3d426dbd1199..4ef2a8713628 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -458,6 +458,20 @@ static noinline struct btrfs_fs_devices *find_fsid(
 	return NULL;
 }
 
+/*
+ * First, checks if the metadata_uuid is different from the fsid in the
+ * given fs_devices. Then, checks if the given fsid is the same as the
+ * metadata_uuid in the fs_devices. If it is, returns true; otherwise,
+ * returns false.
+ */
+static inline bool match_fsid_changed(struct btrfs_fs_devices *fs_devices,
+				       u8 *fsid)
+{
+	return memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
+		       BTRFS_FSID_SIZE) != 0 &&
+		memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE) == 0;
+}
+
 static struct btrfs_fs_devices *find_fsid_with_metadata_uuid(
 				struct btrfs_super_block *disk_super)
 {
@@ -487,13 +501,11 @@ static struct btrfs_fs_devices *find_fsid_with_metadata_uuid(
 	 * CHANGING_FSID_V2 flag set.
 	 */
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-		if (fs_devices->fsid_change &&
-		    memcmp(fs_devices->metadata_uuid,
-			   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
-		    memcmp(disk_super->metadata_uuid, fs_devices->metadata_uuid,
-			   BTRFS_FSID_SIZE) == 0) {
+		if (!fs_devices->fsid_change)
+			continue;
+
+		if (match_fsid_changed(fs_devices, disk_super->metadata_uuid))
 			return fs_devices;
-		}
 	}
 
 	return find_fsid(disk_super->fsid, disk_super->metadata_uuid);
@@ -684,18 +696,16 @@ static struct btrfs_fs_devices *find_fsid_inprogress(
 	struct btrfs_fs_devices *fs_devices;
 
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-		if (memcmp(fs_devices->metadata_uuid, fs_devices->fsid,
-			   BTRFS_FSID_SIZE) != 0 &&
-		    memcmp(fs_devices->metadata_uuid, disk_super->fsid,
-			   BTRFS_FSID_SIZE) == 0 && !fs_devices->fsid_change) {
+		if (fs_devices->fsid_change)
+			continue;
+
+		if (match_fsid_changed(fs_devices,  disk_super->fsid))
 			return fs_devices;
-		}
 	}
 
 	return find_fsid(disk_super->fsid, NULL);
 }
 
-
 static struct btrfs_fs_devices *find_fsid_changed(
 					struct btrfs_super_block *disk_super)
 {
@@ -712,10 +722,7 @@ static struct btrfs_fs_devices *find_fsid_changed(
 	 */
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
 		/* Changed UUIDs */
-		if (memcmp(fs_devices->metadata_uuid, fs_devices->fsid,
-			   BTRFS_FSID_SIZE) != 0 &&
-		    memcmp(fs_devices->metadata_uuid, disk_super->metadata_uuid,
-			   BTRFS_FSID_SIZE) == 0 &&
+		if (match_fsid_changed(fs_devices, disk_super->metadata_uuid) &&
 		    memcmp(fs_devices->fsid, disk_super->fsid,
 			   BTRFS_FSID_SIZE) != 0)
 			return fs_devices;
@@ -746,11 +753,10 @@ static struct btrfs_fs_devices *find_fsid_reverted_metadata(
 	 * fs_devices equal to the FSID of the disk.
 	 */
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-		if (memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
-			   BTRFS_FSID_SIZE) != 0 &&
-		    memcmp(fs_devices->metadata_uuid, disk_super->fsid,
-			   BTRFS_FSID_SIZE) == 0 &&
-		    fs_devices->fsid_change)
+		if (!fs_devices->fsid_change)
+			continue;
+
+		if (match_fsid_changed(fs_devices, disk_super->fsid))
 			return fs_devices;
 	}
 
-- 
2.38.1

Re: [PATCH v2 7/9] btrfs: refactor with match_fsid_changed helper

[David Sterba]

On Wed, May 24, 2023 at 08:02:41PM +0800, Anand Jain wrote:
> We often check if the metadata_uuid is not the same as fsid, and then we
> check if the given fsid matches the metadata_uuid. This patch refactors
> this logic into function match_fsid_changed and utilize it.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: Rename helper function.
> 
>  fs/btrfs/volumes.c | 48 ++++++++++++++++++++++++++--------------------
>  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 3d426dbd1199..4ef2a8713628 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -458,6 +458,20 @@ static noinline struct btrfs_fs_devices *find_fsid(
>  	return NULL;
>  }
>  
> +/*
> + * First, checks if the metadata_uuid is different from the fsid in the
> + * given fs_devices. Then, checks if the given fsid is the same as the
> + * metadata_uuid in the fs_devices. If it is, returns true; otherwise,
> + * returns false.
> + */
> +static inline bool match_fsid_changed(struct btrfs_fs_devices *fs_devices,
> +				       u8 *fsid)

The argumetns can be const and I've changed the prefix to be 'check_',
we have similar helpers.

[PATCH v2 8/9] btrfs: consolidate uuid memcmp in btrfs_validate_super

[Anand Jain]

There are three ways the fsid is validated in btrfs_validate_super():

First, it verifies that super_copy::fsid is the same as fs_devices::fsid.
Second, if the metadata_uuid flag is set, it verifies if
 super_copy::metadata_uuid and fs_devices::metadata_uuid are the same.
Third, a few lines below, often missed out, it verifies if dev_item::fsid
 is the same as fs_devices::metadata_uuid.

The function btrfs_validate_super() contains multiple if-statements with
memcmp() to check UUIDs. This patch consolidates them into a single
location.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: None.

 fs/btrfs/disk-io.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6681e82900b0..d09f767c7bda 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2392,6 +2392,14 @@ int btrfs_validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
+	if (memcmp(fs_info->fs_devices->metadata_uuid, sb->dev_item.fsid,
+		   BTRFS_FSID_SIZE) != 0) {
+		btrfs_err(fs_info,
+			"dev_item UUID does not match metadata fsid: %pU != %pU",
+			fs_info->fs_devices->metadata_uuid, sb->dev_item.fsid);
+		ret = -EINVAL;
+	}
+
 	/*
 	 * Artificial requirement for block-group-tree to force newer features
 	 * (free-space-tree, no-holes) so the test matrix is smaller.
@@ -2404,14 +2412,6 @@ int btrfs_validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
-	if (memcmp(fs_info->fs_devices->metadata_uuid, sb->dev_item.fsid,
-		   BTRFS_FSID_SIZE) != 0) {
-		btrfs_err(fs_info,
-			"dev_item UUID does not match metadata fsid: %pU != %pU",
-			fs_info->fs_devices->metadata_uuid, sb->dev_item.fsid);
-		ret = -EINVAL;
-	}
-
 	/*
 	 * Hint to catch really bogus numbers, bitflips or so, more exact checks are
 	 * done later
-- 
2.38.1

[PATCH v2 9/9] btrfs: add and fix comments in btrfs_fs_devices

[Anand Jain]

(No functional changes.)

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: New patch. Part of this is from v1:patch1.

 fs/btrfs/volumes.h | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 56633d4f9b31..f64d480aed0f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -328,11 +328,11 @@ struct btrfs_fs_devices {
 	 */
 	struct btrfs_device *latest_dev;
 
-	/* all of the devices in the FS, protected by a mutex
-	 * so we can safely walk it to write out the supers without
-	 * worrying about add/remove by the multi-device code.
-	 * Scrubbing super can kick off supers writing by holding
-	 * this mutex lock.
+	/*
+	 * All of the devices in the FS, protected by a mutex so we can safely
+	 * walk it to write out the supers without worrying about add/remove by
+	 * the multi-device code. Scrubbing super can kick off supers writing by
+	 * holding this mutex lock.
 	 */
 	struct mutex device_list_mutex;
 
@@ -341,21 +341,24 @@ struct btrfs_fs_devices {
 
 	/*
 	 * Devices which can satisfy space allocation. Protected by
-	 * chunk_mutex
+	 * chunk_mutex.
 	 */
 	struct list_head alloc_list;
 
 	struct list_head seed_list;
 
+	/* Count fs-devices opened. */
 	int opened;
 
-	/* set when we find or add a device that doesn't have the
-	 * nonrot flag set
+	/*
+	 * Set when we find or add a device that doesn't have the nonrot flag
+	 * set.
 	 */
 	bool rotating;
-	/* Devices support TRIM/discard commands */
+	/* Devices support TRIM/discard commands. */
 	bool discardable;
 	bool fsid_change;
+	/* fsid is a seed filesystem. */
 	bool seeding;
 
 	struct btrfs_fs_info *fs_info;
@@ -367,7 +370,7 @@ struct btrfs_fs_devices {
 
 	enum btrfs_chunk_allocation_policy chunk_alloc_policy;
 
-	/* Policy used to read the mirrored stripes */
+	/* Policy used to read the mirrored stripes. */
 	enum btrfs_read_policy read_policy;
 };
 
-- 
2.38.1

Re: [PATCH v2 0/9] btrfs: metadata_uuid refactors part1

[David Sterba]

On Wed, May 24, 2023 at 08:02:34PM +0800, Anand Jain wrote:
> v2: Addressed the review comments received on some patches. Please refer
>     to the individual patches for more details.
> 
> The metadata_uuid feature added later has significantly impacted code
> readability due to the numerous conditions that need to be checked.
> 
> This patch set aims to improve code organization and prepares for
> streamlining of the metadata_uuid checks and some simple fixes.
> 
> Anand Jain (9):
>   btrfs: reduce struct btrfs_fs_devices size relocate fsid_change
>   btrfs: streamline fsid checks in alloc_fs_devices
>   btrfs: localise has_metadata_uuid check in alloc_fs_devices args
>   btrfs: add comment about metadata_uuid in btrfs_fs_devices
>   btrfs: simplify check_tree_block_fsid return arg to bool
>   btrfs: refactor with match_fsid_fs_devices helper
>   btrfs: refactor with match_fsid_changed helper
>   btrfs: consolidate uuid memcmp in btrfs_validate_super
>   btrfs: add and fix comments in btrfs_fs_devices

Added to misc-next with some minor adjustments, thanks.

Re: [PATCH v2 0/9] btrfs: metadata_uuid refactors part1

[Guilherme G. Piccoli]

Hi Anand, thanks for the patches, nice clean-ups / improvements!

I'm working in the same-fsid mounting [0] (guess you even commented
there already) and that touches a lot the metadata_uuid/fsid related code.

So I'd like to ask if you maybe have a "part 2" ready or almost ready,
in order I can merge it and work on top of that, avoiding too much
conflicts later.

Thanks in advance!


Guilherme


[0]
https://lore.kernel.org/linux-btrfs/20230504170708.787361-1-gpiccoli@igalia.com/

Re: [PATCH v2 0/9] btrfs: metadata_uuid refactors part1

[Anand Jain]

I have a few items still in progress, and I should be sending
them out this week. The main focus is on cleaning up the usage
of the CHANGING_FSID flag. However, it also requires changes
in the btrfs-progs and testing, which is taking longer than
I anticipated. Thank you for your patience.

Thanks, - Anand


On 27/06/2023 22:55, Guilherme G. Piccoli wrote:
> Hi Anand, thanks for the patches, nice clean-ups / improvements!
> 
> I'm working in the same-fsid mounting [0] (guess you even commented
> there already) and that touches a lot the metadata_uuid/fsid related code.
> 
> So I'd like to ask if you maybe have a "part 2" ready or almost ready,
> in order I can merge it and work on top of that, avoiding too much
> conflicts later.
> 
> Thanks in advance!
> 
> 
> Guilherme
> 
> 
> [0]
> https://lore.kernel.org/linux-btrfs/20230504170708.787361-1-gpiccoli@igalia.com/

Re: [PATCH v2 0/9] btrfs: metadata_uuid refactors part1

[Guilherme G. Piccoli]

On 28/06/2023 05:52, Anand Jain wrote:
> 
> 
> I have a few items still in progress, and I should be sending
> them out this week. The main focus is on cleaning up the usage
> of the CHANGING_FSID flag. However, it also requires changes
> in the btrfs-progs and testing, which is taking longer than
> I anticipated. Thank you for your patience.
> 
> Thanks, - Anand
> 

Thanks Anand, no hurries at all. Was just in case you already had a
branch somewhere, I could pick it and work on top of that. But instead
I'm working now on top of "6.5-rc1", which already includes part 1.

It's good and I can fix conflicts later, not a biggie. Thanks for the
very useful refactor, I confess I found the metadata_uuid stuff a bit
confusing to read, code is now better =)

Cheers!