[PATCH 0/7 v2] metadata_uuid misc cleanup and fixes part2

[PATCH 0/7 v2] metadata_uuid misc cleanup and fixes part2

[Anand Jain]

v2:
The new patch 1/7 peels btrfs_sb_fsid_prt() helper function from 1/4 in v1,
This function is used in patch 2/7 and patch 4/7 in v2.

Patch 6/7, adds btrfs_sb_metadata_uuid_or_null() another helper func, used
in 7/7 in volumes.c. Further, used in the upcoming fsid_changing patch.
Pushing this set now which are ready. This has passed fstests -g quick.

----- original -----
These patches are cleanups related to metadata_uuid. Please ref to the
patch for details.

Anand Jain (7):
  btrfs: add a helper to read the superblock metadata_uuid
  btrfs: simplify memcpy either of metadata_uuid or fsid
  btrfs: fix fsid in btrfs_validate_super
  btrfs: fix metadata_uuid in btrfs_validate_super
  btrfs: drop redundant check to use fs_devices::metadata_uuid
  btrfs: add a superblock helper to read metadata_uuid or NULL
  btrfs: use btrfs_sb_metadata_uuid_or_null

 fs/btrfs/disk-io.c | 28 ++++++++++------------------
 fs/btrfs/volumes.c | 29 +++++++++++++++++++----------
 fs/btrfs/volumes.h |  1 +
 3 files changed, 30 insertions(+), 28 deletions(-)

-- 
2.39.2

[PATCH 1/7] btrfs: add a helper to read the superblock metadata_uuid

[Anand Jain]

In some cases, we need to read the FSID from the superblock when the
metadata_uuid is not set, and otherwise, read the metadata_uuid. So,
add a helper.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 70d69d4b44d2..2020b1fac764 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -681,6 +681,14 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	return -EINVAL;
 }
 
+u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb)
+{
+	bool has_metadata_uuid = (btrfs_super_incompat_flags(sb) &
+				  BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
+
+	return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
+}
+
 /*
  * Handle scanned device having its CHANGING_FSID_V2 flag set and the fs_devices
  * being created with a disk that has already completed its fsid change. Such
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index b8c51f16ba86..0f87057bb575 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -749,5 +749,6 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info);
 bool btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical);
 
 bool btrfs_pinned_by_swapfile(struct btrfs_fs_info *fs_info, void *ptr);
+u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb);
 
 #endif
-- 
2.39.2

[PATCH 2/7] btrfs: simplify memcpy either of metadata_uuid or fsid

[Anand Jain]

There is a helper which provides either metadata_uuid or fsid as per
METADATA_UUID flag. So use it.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2020b1fac764..2f470404ff83 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -841,15 +841,8 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		    found_transid > fs_devices->latest_generation) {
 			memcpy(fs_devices->fsid, disk_super->fsid,
 					BTRFS_FSID_SIZE);
-
-			if (has_metadata_uuid)
-				memcpy(fs_devices->metadata_uuid,
-				       disk_super->metadata_uuid,
-				       BTRFS_FSID_SIZE);
-			else
-				memcpy(fs_devices->metadata_uuid,
-				       disk_super->fsid, BTRFS_FSID_SIZE);
-
+			memcpy(fs_devices->metadata_uuid,
+			       btrfs_sb_fsid_ptr(disk_super), BTRFS_FSID_SIZE);
 			fs_devices->fsid_change = false;
 		}
 	}
-- 
2.39.2

[PATCH 3/7] btrfs: fix fsid in btrfs_validate_super

[Anand Jain]

The function btrfs_validate_super() should verify the fsid in the provided
superblock argument. Because, all its callers expects it to do that.

Such as in the following stack:

   write_all_supers()
       sb = fs_info->super_for_commit;
       btrfs_validate_write_super(.., sb)
         btrfs_validate_super(.., sb, ..)

   scrub_one_super()
	btrfs_validate_super(.., sb, ..)

And
   check_dev_super()
	btrfs_validate_super(.., sb, ..)

However, it currently verifies the fs_info::super_copy::fsid instead,
which does not help.

Fix this using the correct fsid in the superblock argument.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b4495d4c1533..f2279eb93370 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2373,11 +2373,10 @@ int btrfs_validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
-	if (memcmp(fs_info->fs_devices->fsid, fs_info->super_copy->fsid,
-		   BTRFS_FSID_SIZE)) {
+	if (memcmp(fs_info->fs_devices->fsid, sb->fsid, BTRFS_FSID_SIZE)) {
 		btrfs_err(fs_info,
 		"superblock fsid doesn't match fsid of fs_devices: %pU != %pU",
-			fs_info->super_copy->fsid, fs_info->fs_devices->fsid);
+			  sb->fsid, fs_info->fs_devices->fsid);
 		ret = -EINVAL;
 	}
 
-- 
2.39.2

[PATCH 4/7] btrfs: fix metadata_uuid in btrfs_validate_super

[Anand Jain]

The function btrfs_validate_super() should verify the metadata_uuid in
the provided superblock argument. Because, all its callers expects it to
do that.

Such as in the following stacks:

  write_all_supers()
   sb = fs_info->super_for_commit;
   btrfs_validate_write_super(.., sb)
     btrfs_validate_super(.., sb, ..)

  scrub_one_super()
	btrfs_validate_super(.., sb, ..)

And
   check_dev_super()
	btrfs_validate_super(.., sb, ..)

However, it currently verifies the fs_info::super_copy::metadata_uuid
instead.

Fix this using the correct metadata_uuid in the superblock argument.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f2279eb93370..e2fb11e89279 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2380,13 +2380,11 @@ int btrfs_validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
-	if (btrfs_fs_incompat(fs_info, METADATA_UUID) &&
-	    memcmp(fs_info->fs_devices->metadata_uuid,
-		   fs_info->super_copy->metadata_uuid, BTRFS_FSID_SIZE)) {
+	if (memcmp(fs_info->fs_devices->metadata_uuid, btrfs_sb_fsid_ptr(sb),
+		   BTRFS_FSID_SIZE)) {
 		btrfs_err(fs_info,
 "superblock metadata_uuid doesn't match metadata uuid of fs_devices: %pU != %pU",
-			fs_info->super_copy->metadata_uuid,
-			fs_info->fs_devices->metadata_uuid);
+			  btrfs_sb_fsid_ptr(sb), fs_info->fs_devices->metadata_uuid);
 		ret = -EINVAL;
 	}
 
-- 
2.39.2

[PATCH 5/7] btrfs: drop redundant check to use fs_devices::metadata_uuid

[Anand Jain]

fs_devices::metadata_uuid value is already updated based on the
super_block::METADATA_UUID flag for either fsid or metadata_uuid as
appropriate. So, fs_devices::metadata_uuid can be used directly.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e2fb11e89279..902dfc4aec53 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -313,21 +313,16 @@ 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;
 	u8 fsid[BTRFS_FSID_SIZE];
-	u8 *metadata_uuid;
 
 	read_extent_buffer(eb, fsid, offsetof(struct btrfs_header, fsid),
 			   BTRFS_FSID_SIZE);
+
 	/*
-	 * 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
+	 * alloc_fs_devices() copies the fsid into metadata_uuid if the
+	 * metadata_uuid is unset in the superblock, including for a seed device.
+	 * So, we can use fs_devices->metadata_uuid.
 	 */
-	if (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))
+	if (!memcmp(fsid, fs_info->fs_devices->metadata_uuid, BTRFS_FSID_SIZE))
 		return false;
 
 	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
-- 
2.39.2

[PATCH 6/7] btrfs: add a superblock helper to read metadata_uuid or NULL

[Anand Jain]

This is preparatory patch next two patches.

In some parts of the code, when the METADATA_UUID flag is set, we need to
use metadata_uuid otherwise NULL. Add a helper function that do it.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2f470404ff83..661dc69543eb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -681,6 +681,14 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	return -EINVAL;
 }
 
+static u8 *btrfs_sb_metadata_uuid_or_null(struct btrfs_super_block *sb)
+{
+	bool has_metadata_uuid = (btrfs_super_incompat_flags(sb) &
+				  BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
+
+	return has_metadata_uuid ? sb->metadata_uuid : NULL;
+}
+
 u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb)
 {
 	bool has_metadata_uuid = (btrfs_super_incompat_flags(sb) &
-- 
2.39.2

[PATCH 7/7] btrfs: use btrfs_sb_metadata_uuid_or_null

[Anand Jain]

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 661dc69543eb..316839d2aecc 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -821,7 +821,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 
 	if (!fs_devices) {
 		fs_devices = alloc_fs_devices(disk_super->fsid,
-				has_metadata_uuid ? disk_super->metadata_uuid : NULL);
+				btrfs_sb_metadata_uuid_or_null(disk_super));
 		if (IS_ERR(fs_devices))
 			return ERR_CAST(fs_devices);
 
-- 
2.39.2

Re: [PATCH 1/7] btrfs: add a helper to read the superblock metadata_uuid

[Johannes Thumshirn]

On 31.07.23 13:18, Anand Jain wrote:
> +u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb)
> +{
> +	bool has_metadata_uuid = (btrfs_super_incompat_flags(sb) &
> +				  BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
> +
> +	return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
> +}

Not a huge fan of the overly long variable name and first line, but the 
concept looks good:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/7] btrfs: simplify memcpy either of metadata_uuid or fsid

[Johannes Thumshirn]

On 31.07.23 13:18, Anand Jain wrote:
> There is a helper which provides either metadata_uuid or fsid as per 
> METADATA_UUID flag. So use it.

Now that we have 'btrfs_sb_fsid_ptr()' use it to simplify metadata uuid 
copying in device_list_add()?

Code wise,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/7] btrfs: fix fsid in btrfs_validate_super

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/7] btrfs: fix metadata_uuid in btrfs_validate_super

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 5/7] btrfs: drop redundant check to use fs_devices::metadata_uuid

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/7 v2] metadata_uuid misc cleanup and fixes part2

[Guilherme G. Piccoli]

Thanks for the patches Anand! Feel free to add (to the whole series):

Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>


I'm working a feature for btrfs that relies on metadata_uuid
infrastructure, so I did rebase on top of your patches and tested, it
works fine.

Cheers,


Guilherme

Re: [PATCH 1/7] btrfs: add a helper to read the superblock metadata_uuid

[David Sterba]

On Tue, Aug 01, 2023 at 01:16:42PM +0000, Johannes Thumshirn wrote:
> On 31.07.23 13:18, Anand Jain wrote:
> > +u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb)
> > +{
> > +	bool has_metadata_uuid = (btrfs_super_incompat_flags(sb) &
> > +				  BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
> > +
> > +	return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
> > +}
> 
> Not a huge fan of the overly long variable name and first line, but the 
> concept looks good:

For the incompat bit checks (btrfs_fs_incompat & co) we have the macros
that glue the prefix of the bit name but that's for the fs_info data.
For raw buffers of super block we use the unpreprocessed helpers/macros,
there are like 3 in total so that's probably not worth a helper.

Re: [PATCH 3/7] btrfs: fix fsid in btrfs_validate_super

[David Sterba]

On Mon, Jul 31, 2023 at 07:16:34PM +0800, Anand Jain wrote:
> The function btrfs_validate_super() should verify the fsid in the provided
> superblock argument. Because, all its callers expects it to do that.
> 
> Such as in the following stack:
> 
>    write_all_supers()
>        sb = fs_info->super_for_commit;
>        btrfs_validate_write_super(.., sb)
>          btrfs_validate_super(.., sb, ..)
> 
>    scrub_one_super()
> 	btrfs_validate_super(.., sb, ..)
> 
> And
>    check_dev_super()
> 	btrfs_validate_super(.., sb, ..)
> 
> However, it currently verifies the fs_info::super_copy::fsid instead,
> which does not help.
> 
> Fix this using the correct fsid in the superblock argument.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/disk-io.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b4495d4c1533..f2279eb93370 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2373,11 +2373,10 @@ int btrfs_validate_super(struct btrfs_fs_info *fs_info,
>  		ret = -EINVAL;
>  	}
>  
> -	if (memcmp(fs_info->fs_devices->fsid, fs_info->super_copy->fsid,
> -		   BTRFS_FSID_SIZE)) {
> +	if (memcmp(fs_info->fs_devices->fsid, sb->fsid, BTRFS_FSID_SIZE)) {

For memcpy and strcpy please compare it against 0 so it follows the
semantics of "is equal" or "is not equal".

Re: [PATCH 6/7] btrfs: add a superblock helper to read metadata_uuid or NULL

[David Sterba]

On Mon, Jul 31, 2023 at 07:16:37PM +0800, Anand Jain wrote:
> This is preparatory patch next two patches.

There's only one more patch 7/7 and given how short the helper is it
could be done in one patch. The split for perparatory work and actual
use applies more to bigger helpers.

Re: [PATCH 7/7] btrfs: use btrfs_sb_metadata_uuid_or_null

[David Sterba]

On Mon, Jul 31, 2023 at 07:16:38PM +0800, Anand Jain wrote:
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 661dc69543eb..316839d2aecc 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -821,7 +821,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  
>  	if (!fs_devices) {
>  		fs_devices = alloc_fs_devices(disk_super->fsid,
> -				has_metadata_uuid ? disk_super->metadata_uuid : NULL);
> +				btrfs_sb_metadata_uuid_or_null(disk_super));

This is the only place that passes a non-NULL pointer to
alloc_fs_devices, so it might be better to drop the 2nd argument
completely and just set the metadata_uuid here, which is basically
copying fsid to metadata_uuid.

Re: [PATCH 0/7 v2] metadata_uuid misc cleanup and fixes part2

[David Sterba]

On Mon, Jul 31, 2023 at 07:16:31PM +0800, Anand Jain wrote:
> v2:
> The new patch 1/7 peels btrfs_sb_fsid_prt() helper function from 1/4 in v1,
> This function is used in patch 2/7 and patch 4/7 in v2.
> 
> Patch 6/7, adds btrfs_sb_metadata_uuid_or_null() another helper func, used
> in 7/7 in volumes.c. Further, used in the upcoming fsid_changing patch.
> Pushing this set now which are ready. This has passed fstests -g quick.
> 
> ----- original -----
> These patches are cleanups related to metadata_uuid. Please ref to the
> patch for details.
> 
> Anand Jain (7):
>   btrfs: add a helper to read the superblock metadata_uuid
>   btrfs: simplify memcpy either of metadata_uuid or fsid
>   btrfs: fix fsid in btrfs_validate_super
>   btrfs: fix metadata_uuid in btrfs_validate_super
>   btrfs: drop redundant check to use fs_devices::metadata_uuid

The above added to misc-next, thanks.

>   btrfs: add a superblock helper to read metadata_uuid or NULL
>   btrfs: use btrfs_sb_metadata_uuid_or_null

I think this can simplify the alloc_fs_devices a bit more as commented,
please resend. Thanks.

Re: [PATCH 7/7] btrfs: use btrfs_sb_metadata_uuid_or_null

[Anand Jain]

On 11/08/2023 23:42, David Sterba wrote:
> On Mon, Jul 31, 2023 at 07:16:38PM +0800, Anand Jain wrote:
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 661dc69543eb..316839d2aecc 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -821,7 +821,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>>   
>>   	if (!fs_devices) {
>>   		fs_devices = alloc_fs_devices(disk_super->fsid,
>> -				has_metadata_uuid ? disk_super->metadata_uuid : NULL);
>> +				btrfs_sb_metadata_uuid_or_null(disk_super));
> 
> This is the only place that passes a non-NULL pointer to
> alloc_fs_devices, so it might be better to drop the 2nd argument
> completely and just set the metadata_uuid here, which is basically
> copying fsid to metadata_uuid.

  Oh right. I am sending v2. Thanks.

Re: [PATCH 6/7] btrfs: add a superblock helper to read metadata_uuid or NULL

[Anand Jain]

On 11/08/2023 23:39, David Sterba wrote:
> On Mon, Jul 31, 2023 at 07:16:37PM +0800, Anand Jain wrote:
>> This is preparatory patch next two patches.
> 
> There's only one more patch 7/7 and given how short the helper is it
> could be done in one patch. The split for perparatory work and actual
> use applies more to bigger helpers.

  There was one more patch which used this helper initially.
  I'll merge this with 7/7.

Thanks.

[PATCH] btrfs: simplify alloc_fs_devices() remove arg2

[Anand Jain]

Among all the callers, only the device_list_add() function uses the
second argument of alloc_fs_devices(). It passes metadata_uuid when
available; otherwise, it passes NULL. And in turn, alloc_fs_devices()
is designed to copy either metadata_uuid or fsid into
fs_devices::metadata_uuid.

So eliminate the second argument in alloc_fs_devices(), and copy the
fsid alway. In the caller device_list_add() function, we will overwrite
it with metadata_uuid when it is available.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c |  7 ++++---
 fs/btrfs/volumes.c | 25 ++++++++++++-------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0a96ea8c1d3a..9858c77b99e6 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -318,9 +318,10 @@ static bool check_tree_block_fsid(struct extent_buffer *eb)
 			   BTRFS_FSID_SIZE);
 
 	/*
-	 * alloc_fs_devices() copies the fsid into metadata_uuid if the
-	 * metadata_uuid is unset in the superblock, including for a seed device.
-	 * So, we can use fs_devices->metadata_uuid.
+	 * alloc_fsid_devices() copies the fsid into fs_devices::metadata_uuid.
+	 * This is then overwritten by metadata_uuid if it is present in the
+	 * device_list_add(). The same true for a seed device as well. So use of
+	 * fs_devices::metadata_uuid is appropriate here.
 	 */
 	if (memcmp(fsid, fs_info->fs_devices->metadata_uuid, BTRFS_FSID_SIZE) == 0)
 		return false;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 189da583bb67..852590e06d76 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -358,20 +358,17 @@ struct list_head * __attribute_const__ btrfs_get_fs_uuids(void)
 
 /*
  * alloc_fs_devices - allocate struct btrfs_fs_devices
- * @fsid:		if not NULL, copy the UUID to fs_devices::fsid
- * @metadata_fsid:	if not NULL, copy the UUID to fs_devices::metadata_fsid
+ * @fsid:		if not NULL, copy the UUID to fs_devices::fsid and to
+ * 			fs_devices::metadata_fsid
  *
  * Return a pointer to a new struct btrfs_fs_devices on success, or ERR_PTR().
  * The returned struct is not linked onto any lists and can be destroyed with
  * kfree() right away.
  */
-static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
-						 const u8 *metadata_fsid)
+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);
@@ -385,8 +382,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 
 	if (fsid) {
 		memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE);
-		memcpy(fs_devs->metadata_uuid,
-		       metadata_fsid ?: fsid, BTRFS_FSID_SIZE);
+		memcpy(fs_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE);
 	}
 
 	return fs_devs;
@@ -812,8 +808,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 
 
 	if (!fs_devices) {
-		fs_devices = alloc_fs_devices(disk_super->fsid,
-				has_metadata_uuid ? disk_super->metadata_uuid : NULL);
+		fs_devices = alloc_fs_devices(disk_super->fsid);
+		if (has_metadata_uuid)
+			memcpy(fs_devices->metadata_uuid,
+			       disk_super->metadata_uuid, BTRFS_FSID_SIZE);
+
 		if (IS_ERR(fs_devices))
 			return ERR_CAST(fs_devices);
 
@@ -997,7 +996,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 
 	lockdep_assert_held(&uuid_mutex);
 
-	fs_devices = alloc_fs_devices(orig->fsid, NULL);
+	fs_devices = alloc_fs_devices(orig->fsid);
 	if (IS_ERR(fs_devices))
 		return fs_devices;
 
@@ -2454,7 +2453,7 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
 	 * Private copy of the seed devices, anchored at
 	 * fs_info->fs_devices->seed_list
 	 */
-	seed_devices = alloc_fs_devices(NULL, NULL);
+	seed_devices = alloc_fs_devices(NULL);
 	if (IS_ERR(seed_devices))
 		return seed_devices;
 
@@ -6905,7 +6904,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		if (!btrfs_test_opt(fs_info, DEGRADED))
 			return ERR_PTR(-ENOENT);
 
-		fs_devices = alloc_fs_devices(fsid, NULL);
+		fs_devices = alloc_fs_devices(fsid);
 		if (IS_ERR(fs_devices))
 			return fs_devices;
 
-- 
2.38.1

[PATCH resend] btrfs: simplify alloc_fs_devices() remove arg2

[Anand Jain]

Among all the callers, only the device_list_add() function uses the
second argument of alloc_fs_devices(). It passes metadata_uuid when
available; otherwise, it passes NULL. And in turn, alloc_fs_devices()
is designed to copy either metadata_uuid or fsid into
fs_devices::metadata_uuid.

So eliminate the second argument in alloc_fs_devices(), and copy the
fsid alway. In the caller device_list_add() function, we will overwrite
it with metadata_uuid when it is available.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c |  7 ++++---
 fs/btrfs/volumes.c | 25 ++++++++++++-------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0a96ea8c1d3a..9858c77b99e6 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -318,9 +318,10 @@ static bool check_tree_block_fsid(struct extent_buffer *eb)
 			   BTRFS_FSID_SIZE);
 
 	/*
-	 * alloc_fs_devices() copies the fsid into metadata_uuid if the
-	 * metadata_uuid is unset in the superblock, including for a seed device.
-	 * So, we can use fs_devices->metadata_uuid.
+	 * alloc_fsid_devices() copies the fsid into fs_devices::metadata_uuid.
+	 * This is then overwritten by metadata_uuid if it is present in the
+	 * device_list_add(). The same true for a seed device as well. So use of
+	 * fs_devices::metadata_uuid is appropriate here.
 	 */
 	if (memcmp(fsid, fs_info->fs_devices->metadata_uuid, BTRFS_FSID_SIZE) == 0)
 		return false;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 189da583bb67..852590e06d76 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -358,20 +358,17 @@ struct list_head * __attribute_const__ btrfs_get_fs_uuids(void)
 
 /*
  * alloc_fs_devices - allocate struct btrfs_fs_devices
- * @fsid:		if not NULL, copy the UUID to fs_devices::fsid
- * @metadata_fsid:	if not NULL, copy the UUID to fs_devices::metadata_fsid
+ * @fsid:		if not NULL, copy the UUID to fs_devices::fsid and to
+ * 			fs_devices::metadata_fsid
  *
  * Return a pointer to a new struct btrfs_fs_devices on success, or ERR_PTR().
  * The returned struct is not linked onto any lists and can be destroyed with
  * kfree() right away.
  */
-static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
-						 const u8 *metadata_fsid)
+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);
@@ -385,8 +382,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
 
 	if (fsid) {
 		memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE);
-		memcpy(fs_devs->metadata_uuid,
-		       metadata_fsid ?: fsid, BTRFS_FSID_SIZE);
+		memcpy(fs_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE);
 	}
 
 	return fs_devs;
@@ -812,8 +808,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 
 
 	if (!fs_devices) {
-		fs_devices = alloc_fs_devices(disk_super->fsid,
-				has_metadata_uuid ? disk_super->metadata_uuid : NULL);
+		fs_devices = alloc_fs_devices(disk_super->fsid);
+		if (has_metadata_uuid)
+			memcpy(fs_devices->metadata_uuid,
+			       disk_super->metadata_uuid, BTRFS_FSID_SIZE);
+
 		if (IS_ERR(fs_devices))
 			return ERR_CAST(fs_devices);
 
@@ -997,7 +996,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 
 	lockdep_assert_held(&uuid_mutex);
 
-	fs_devices = alloc_fs_devices(orig->fsid, NULL);
+	fs_devices = alloc_fs_devices(orig->fsid);
 	if (IS_ERR(fs_devices))
 		return fs_devices;
 
@@ -2454,7 +2453,7 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
 	 * Private copy of the seed devices, anchored at
 	 * fs_info->fs_devices->seed_list
 	 */
-	seed_devices = alloc_fs_devices(NULL, NULL);
+	seed_devices = alloc_fs_devices(NULL);
 	if (IS_ERR(seed_devices))
 		return seed_devices;
 
@@ -6905,7 +6904,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		if (!btrfs_test_opt(fs_info, DEGRADED))
 			return ERR_PTR(-ENOENT);
 
-		fs_devices = alloc_fs_devices(fsid, NULL);
+		fs_devices = alloc_fs_devices(fsid);
 		if (IS_ERR(fs_devices))
 			return fs_devices;
 
-- 
2.38.1

Re: [PATCH resend] btrfs: simplify alloc_fs_devices() remove arg2

[David Sterba]

On Wed, Aug 23, 2023 at 10:52:13PM +0800, Anand Jain wrote:
> Among all the callers, only the device_list_add() function uses the
> second argument of alloc_fs_devices(). It passes metadata_uuid when
> available; otherwise, it passes NULL. And in turn, alloc_fs_devices()
> is designed to copy either metadata_uuid or fsid into
> fs_devices::metadata_uuid.
> 
> So eliminate the second argument in alloc_fs_devices(), and copy the
> fsid alway. In the caller device_list_add() function, we will overwrite
> it with metadata_uuid when it is available.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Added to misc-next, thanks.