[PATCH 0/9] btrfs: metadata_uuid refactors part1

[PATCH 0/9] btrfs: metadata_uuid refactors part1

[Anand Jain]

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 memcmp_fsid_fs_devices helper
  btrfs: refactor with memcmp_fsid_changed helper
  btrfs: consolidate uuid memcmp in btrfs_validate_super
  btrfs: fix source code style in find_fsid

 fs/btrfs/disk-io.c |  24 +++++-----
 fs/btrfs/volumes.c | 114 +++++++++++++++++++++++++--------------------
 fs/btrfs/volumes.h |  21 +++++++--
 3 files changed, 94 insertions(+), 65 deletions(-)

-- 
2.39.2

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

[Anand Jain]

By relocating the bool fsid_change near other bool declarations in the
struct btrfs_fs_devices, approximately 6 bytes is saved.

   before: 512 bytes
   after: 496 bytes

Furthermore, adding comments.

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

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 5cbbee32748c..a9a86c9220b3 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,17 +336,24 @@ struct btrfs_fs_devices {
 	struct list_head alloc_list;
 
 	struct list_head seed_list;
-	bool seeding;
 
+	/* count fs-devices opened */
 	int opened;
 
-	/* set when we find or add a device that doesn't have the
+	/*
+	 * set when we find or add a device that doesn't have the
 	 * nonrot flag set
 	 */
 	bool rotating;
+
 	/* Devices support TRIM/discard commands */
 	bool discardable;
 
+	bool fsid_change;
+
+	/* fsid is a seed filesystem */
+	bool seeding;
+
 	struct btrfs_fs_info *fs_info;
 	/* sysfs kobjects */
 	struct kobject fsid_kobj;
-- 
2.39.2

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

[Christoph Hellwig]

On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote:
> By relocating the bool fsid_change near other bool declarations in the
> struct btrfs_fs_devices, approximately 6 bytes is saved.
> 
>    before: 512 bytes
>    after: 496 bytes
> 
> Furthermore, adding comments.

I like the better backing.  But what looks access to fsid_change
and the other bools?  For sub-word members atomicy guarantees are
very limited, so they'd better all use the same lock.

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

[David Sterba]

On Tue, May 23, 2023 at 09:27:44AM -0700, Christoph Hellwig wrote:
> On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote:
> > By relocating the bool fsid_change near other bool declarations in the
> > struct btrfs_fs_devices, approximately 6 bytes is saved.
> > 
> >    before: 512 bytes
> >    after: 496 bytes
> > 
> > Furthermore, adding comments.
> 
> I like the better backing.  But what looks access to fsid_change
> and the other bools?  For sub-word members atomicy guarantees are
> very limited, so they'd better all use the same lock.

Do you have an example where the reordered structure would become
problematic? The fs_devices locking is non-standard, the structures are
accessed from module context or from filesystem context. There's the
uuid_mutex as a big lock for fs_devices, and for access of the
individual devices is device_list_mutex.

It's possible that there's something wrong still but after a quick look
I don't see anything obvious. Because of the complex locking there are
no optimizations like unlocked access to bool members.

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

[Christoph Hellwig]

On Tue, May 23, 2023 at 10:59:17PM +0200, David Sterba wrote:
> On Tue, May 23, 2023 at 09:27:44AM -0700, Christoph Hellwig wrote:
> > On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote:
> > > By relocating the bool fsid_change near other bool declarations in the
> > > struct btrfs_fs_devices, approximately 6 bytes is saved.
> > > 
> > >    before: 512 bytes
> > >    after: 496 bytes
> > > 
> > > Furthermore, adding comments.
> > 
> > I like the better backing.  But what looks access to fsid_change
> > and the other bools?  For sub-word members atomicy guarantees are
> > very limited, so they'd better all use the same lock.
> 
> Do you have an example where the reordered structure would become
> problematic? The fs_devices locking is non-standard, the structures are
> accessed from module context or from filesystem context. There's the
> uuid_mutex as a big lock for fs_devices, and for access of the
> individual devices is device_list_mutex.

No, I'm mostly asking for Anand to document the rationale why this
is fine.  If there is an issue, it's probably pre-existing.

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

[Anand Jain]

On 24/05/2023 13:56, Christoph Hellwig wrote:
> On Tue, May 23, 2023 at 10:59:17PM +0200, David Sterba wrote:
>> On Tue, May 23, 2023 at 09:27:44AM -0700, Christoph Hellwig wrote:
>>> On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote:
>>>> By relocating the bool fsid_change near other bool declarations in the
>>>> struct btrfs_fs_devices, approximately 6 bytes is saved.
>>>>
>>>>     before: 512 bytes
>>>>     after: 496 bytes
>>>>
>>>> Furthermore, adding comments.
>>>
>>> I like the better backing.  But what looks access to fsid_change
>>> and the other bools?  For sub-word members atomicy guarantees are
>>> very limited, so they'd better all use the same lock.
>>
>> Do you have an example where the reordered structure would become
>> problematic? The fs_devices locking is non-standard, the structures are
>> accessed from module context or from filesystem context. There's the
>> uuid_mutex as a big lock for fs_devices, and for access of the
>> individual devices is device_list_mutex.
> 
> No, I'm mostly asking for Anand to document the rationale why this
> is fine.  If there is an issue, it's probably pre-existing.

Expanding on David's comment, these bool variables are not modified
after the fs is mounted. However, here is a known bug - %discardable
and %rotating aren't updated after device replace/add/remove.

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

[David Sterba]

On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote:
> By relocating the bool fsid_change near other bool declarations in the
> struct btrfs_fs_devices, approximately 6 bytes is saved.
> 
>    before: 512 bytes
>    after: 496 bytes
> 
> Furthermore, adding comments.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.h | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 5cbbee32748c..a9a86c9220b3 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,17 +336,24 @@ struct btrfs_fs_devices {
>  	struct list_head alloc_list;
>  
>  	struct list_head seed_list;
> -	bool seeding;
>  
> +	/* count fs-devices opened */
>  	int opened;
>  
> -	/* set when we find or add a device that doesn't have the
> +	/*
> +	 * set when we find or add a device that doesn't have the
>  	 * nonrot flag set

Please reformat comments so they follow the most up to date style, which
is to be a full sentence (with "." at the end) and if it fits on one
line then it's the /* text */ otherwise multiline /* ... */.

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

[Anand Jain]

On 24/5/23 05:31, David Sterba wrote:
> On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote:
>> By relocating the bool fsid_change near other bool declarations in the
>> struct btrfs_fs_devices, approximately 6 bytes is saved.
>>
>>     before: 512 bytes
>>     after: 496 bytes
>>
>> Furthermore, adding comments.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.h | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index 5cbbee32748c..a9a86c9220b3 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,17 +336,24 @@ struct btrfs_fs_devices {
>>   	struct list_head alloc_list;
>>   
>>   	struct list_head seed_list;
>> -	bool seeding;
>>   
>> +	/* count fs-devices opened */
>>   	int opened;
>>   
>> -	/* set when we find or add a device that doesn't have the
>> +	/*
>> +	 * set when we find or add a device that doesn't have the
>>   	 * nonrot flag set
> 
> Please reformat comments so they follow the most up to date style, which
> is to be a full sentence (with "." at the end) and if it fits on one
> line then it's the /* text */ otherwise multiline /* ... */.

Sure. I will fix them.

Also, I'll split into two patch adding/fixing comments and moving
%fsid_change.

Thanks, Anand

[PATCH 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>
---
 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..6f231e679667 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 == NULL && metadata_fsid != NULL));
+
 	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.39.2

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

[Christoph Hellwig]

On Tue, May 23, 2023 at 06:03:16PM +0800, Anand Jain wrote:
> +	ASSERT(!(fsid == NULL && metadata_fsid != NULL));

The more readable way to write this would be;

	ASSERT(fsid || !metadata_fsid);

or just throw a 

	ASSERT(!metadata_fsid);

into the else branch of the if (fsid) check.

> +	if (fsid){

missing whitespace before the opening brace.

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

[Anand Jain]

On 24/05/2023 00:30, Christoph Hellwig wrote:
> On Tue, May 23, 2023 at 06:03:16PM +0800, Anand Jain wrote:
>> +	ASSERT(!(fsid == NULL && metadata_fsid != NULL));
> 
> The more readable way to write this would be;
> 
> 	ASSERT(fsid || !metadata_fsid);
> 

  Yeah. This is better.

> or just throw a
> 
> 	ASSERT(!metadata_fsid);
> 
> into the else branch of the if (fsid) check.
> 
>> +	if (fsid){
> 
> missing whitespace before the opening brace.
> 

  Both fixed locally.

Thanks, Anand

[PATCH 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>
---
 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 6f231e679667..95b87e9a0a73 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.39.2

[PATCH 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>
---
 fs/btrfs/volumes.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index a9a86c9220b3..35d135bcdee4 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -280,7 +280,16 @@ enum btrfs_read_policy {
 
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
+
+	/*
+	 * UUID written into the btree blocks:
+	 * Relations:
+	 *   metadata_uuid != fsid must set BTRFS_FEATURE_INCOMPAT_METADATA_UUID
+	 *   metadata_uuid == btrfs_header::fsid at all times.
+	 *   metadata_uuid == btrfs_dev_item::fsid at all times.
+	 */
 	u8 metadata_uuid[BTRFS_FSID_SIZE];
+
 	struct list_head fs_list;
 
 	/*
-- 
2.39.2

[PATCH 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>
---
 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 dc2ad0bf88f8..5a3e92fedcde 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -396,7 +396,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;
@@ -416,13 +416,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.39.2

[PATCH 6/9] btrfs: refactor with memcmp_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,
memcmp_fsid_fs_devices().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 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 95b87e9a0a73..8738c8027421 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void)
 	}
 }
 
+static bool memcmp_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 (memcmp_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 (memcmp_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.39.2

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

[David Sterba]

On Tue, May 23, 2023 at 06:03:21PM +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,
> memcmp_fsid_fs_devices().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  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 95b87e9a0a73..8738c8027421 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void)
>  	}
>  }
>  
> +static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
> +				   const u8 *fsid, const u8 *metadata_fsid)

It's confusing when function is named memcmp_* but does not have the
same return value semantics as memcmp().

We could use "memeq_" prefix but I don't see it used in kernel so
there's not a pattern to follow but for readability, either the helpers
should be exactly memcmp() semantics or rename to something better
reflecting that it's a equal or not.

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

[Anand Jain]

On 24/5/23 05:23, David Sterba wrote:
> On Tue, May 23, 2023 at 06:03:21PM +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,
>> memcmp_fsid_fs_devices().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   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 95b87e9a0a73..8738c8027421 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void)
>>   	}
>>   }
>>   
>> +static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
>> +				   const u8 *fsid, const u8 *metadata_fsid)
> 
> It's confusing when function is named memcmp_* but does not have the
> same return value semantics as memcmp().
> 
> We could use "memeq_" prefix but I don't see it used in kernel so
> there's not a pattern to follow but for readability, either the helpers
> should be exactly memcmp() semantics or rename to something better
> reflecting that it's a equal or not.

Yeah, calling it "memcmp_" with different return semantic will be
confusing. I'll rename. Howabout, "match_"?

Thanks, Anand

[PATCH 7/9] btrfs: refactor with memcmp_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 a function and utilize it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 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 8738c8027421..730fc723524e 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 memcmp_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 (memcmp_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 (memcmp_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 (memcmp_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 (memcmp_fsid_changed(fs_devices, disk_super->fsid))
 			return fs_devices;
 	}
 
-- 
2.39.2

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

[Christoph Hellwig]

On Tue, May 23, 2023 at 06:03:23PM +0800, Anand Jain wrote:
> +	return (memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
> +		       BTRFS_FSID_SIZE) != 0 &&
> +		memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE) == 0);

no need for the outer braces.

[PATCH 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>
---
 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 5a3e92fedcde..85e75d84675f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2574,6 +2574,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.
@@ -2586,14 +2594,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.39.2

[PATCH 9/9] btrfs: fix source code style in find_fsid

[Anand Jain]

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 730fc723524e..db46df2f8fb2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -443,8 +443,8 @@ static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
 	return true;
 }
 
-static noinline struct btrfs_fs_devices *find_fsid(
-		const u8 *fsid, const u8 *metadata_fsid)
+static noinline struct btrfs_fs_devices *find_fsid(const u8 *fsid,
+						   const u8 *metadata_fsid)
 {
 	struct btrfs_fs_devices *fs_devices;
 
-- 
2.39.2

Re: [PATCH 9/9] btrfs: fix source code style in find_fsid

[David Sterba]

On Tue, May 23, 2023 at 06:03:25PM +0800, Anand Jain wrote:
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 730fc723524e..db46df2f8fb2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -443,8 +443,8 @@ static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
>  	return true;
>  }
>  
> -static noinline struct btrfs_fs_devices *find_fsid(
> -		const u8 *fsid, const u8 *metadata_fsid)
> +static noinline struct btrfs_fs_devices *find_fsid(const u8 *fsid,
> +						   const u8 *metadata_fsid)

We have lots of mixed coding styles, patches that change just one place
are not so useful, it's better to fix coding style when the code is
changed. If there's some consistent anti-pattern it might be worth
fixing separately but function argument indentation is in the category
of harmless but unfortunately vastly inconsistent.

Re: [PATCH 9/9] btrfs: fix source code style in find_fsid

[Anand Jain]

On 24/5/23 05:29, David Sterba wrote:
> On Tue, May 23, 2023 at 06:03:25PM +0800, Anand Jain wrote:
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 730fc723524e..db46df2f8fb2 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -443,8 +443,8 @@ static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices,
>>   	return true;
>>   }
>>   
>> -static noinline struct btrfs_fs_devices *find_fsid(
>> -		const u8 *fsid, const u8 *metadata_fsid)
>> +static noinline struct btrfs_fs_devices *find_fsid(const u8 *fsid,
>> +						   const u8 *metadata_fsid)
> 
> We have lots of mixed coding styles, patches that change just one place
> are not so useful, it's better to fix coding style when the code is
> changed. If there's some consistent anti-pattern it might be worth
> fixing separately but function argument indentation is in the category
> of harmless but unfortunately vastly inconsistent.

Yeah, I agree. I'll drop this patch in v2.

Thanks, Anand

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

[David Sterba]

On Tue, May 23, 2023 at 06:03:14PM +0800, Anand Jain wrote:
> 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.

In general the cleanups look good, the checks get simplified. Please
fix the coding style, thanks.