[PATCH 0/4] metadata_uuid misc cleanup and fixes part2

[PATCH 0/4] metadata_uuid misc cleanup and fixes part2

[Anand Jain]

These patches are independent and not related. Please ref to the
patch for details.

Anand Jain (4):
  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

 fs/btrfs/disk-io.c | 36 ++++++++++++++++++------------------
 fs/btrfs/volumes.c | 12 ++++--------
 2 files changed, 22 insertions(+), 26 deletions(-)

-- 
2.38.1

[PATCH 1/4] btrfs: simplify memcpy for either metadata_uuid or fsid.

[Anand Jain]

This change makes the code more readable.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5678ca9b6281..4ce6c63ab868 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -833,14 +833,10 @@ 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,
+			       has_metadata_uuid ?
+				disk_super->metadata_uuid : disk_super->fsid,
+			       BTRFS_FSID_SIZE);
 
 			fs_devices->fsid_change = false;
 		}
-- 
2.38.1

[PATCH 2/4] 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, ..)

But, currently we 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.38.1

[PATCH 3/4] 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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f2279eb93370..8d6d7c23d37d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2278,6 +2278,14 @@ static int btrfs_read_roots(struct btrfs_fs_info *fs_info)
 	return ret;
 }
 
+static 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;
+}
+
 /*
  * Real super block validation
  * NOTE: super csum type and incompat features will not be checked here.
@@ -2380,13 +2388,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);
+			  sb->metadata_uuid, fs_info->fs_devices->metadata_uuid);
 		ret = -EINVAL;
 	}
 
-- 
2.38.1

[PATCH 4/4] 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 8d6d7c23d37d..b42de42bafb2 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.38.1

Re: [PATCH 1/4] btrfs: simplify memcpy for either metadata_uuid or fsid.

[Filipe Manana]

On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> This change makes the code more readable.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5678ca9b6281..4ce6c63ab868 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -833,14 +833,10 @@ 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,
> +                              has_metadata_uuid ?
> +                               disk_super->metadata_uuid : disk_super->fsid,
> +                              BTRFS_FSID_SIZE);

While there's less lines of code, I don't find having a long ternary
operation in the middle of a function call, split in two lines, more
readable than the existing if-else statement, quite the contrary.

Maybe I'm just being picky...

Thanks.

>
>                         fs_devices->fsid_change = false;
>                 }
> --
> 2.38.1
>

Re: [PATCH 1/4] btrfs: simplify memcpy for either metadata_uuid or fsid.

[David Sterba]

On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote:
> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> wrote:
> >
> > This change makes the code more readable.
> >
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > ---
> >  fs/btrfs/volumes.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 5678ca9b6281..4ce6c63ab868 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -833,14 +833,10 @@ 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,
> > +                              has_metadata_uuid ?
> > +                               disk_super->metadata_uuid : disk_super->fsid,
> > +                              BTRFS_FSID_SIZE);
> 
> While there's less lines of code, I don't find having a long ternary
> operation in the middle of a function call, split in two lines, more
> readable than the existing if-else statement, quite the contrary.

Agreed, one line of code doing one thing is readable.

Re: [PATCH 1/4] btrfs: simplify memcpy for either metadata_uuid or fsid.

[Anand Jain]

On 29/07/2023 02:39, David Sterba wrote:
> On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote:
>> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> wrote:
>>>
>>> This change makes the code more readable.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/volumes.c | 12 ++++--------
>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 5678ca9b6281..4ce6c63ab868 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -833,14 +833,10 @@ 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,
>>> +                              has_metadata_uuid ?
>>> +                               disk_super->metadata_uuid : disk_super->fsid,
>>> +                              BTRFS_FSID_SIZE);
>>
>> While there's less lines of code, I don't find having a long ternary
>> operation in the middle of a function call, split in two lines, more
>> readable than the existing if-else statement, quite the contrary.
> 
> Agreed, one line of code doing one thing is readable.

My POV was one memcpy() per destination argument makes it better
summarized at the function level. Anyway, I am okay with dropping
this patch.

Thanks.

Re: [PATCH 1/4] btrfs: simplify memcpy for either metadata_uuid or fsid.

[Anand Jain]

On 29/07/2023 06:23, Anand Jain wrote:
> 
> 
> On 29/07/2023 02:39, David Sterba wrote:
>> On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote:
>>> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> 
>>> wrote:
>>>>
>>>> This change makes the code more readable.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>>   fs/btrfs/volumes.c | 12 ++++--------
>>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 5678ca9b6281..4ce6c63ab868 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -833,14 +833,10 @@ 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,
>>>> +                              has_metadata_uuid ?
>>>> +                               disk_super->metadata_uuid : 
>>>> disk_super->fsid,
>>>> +                              BTRFS_FSID_SIZE);
>>>
>>> While there's less lines of code, I don't find having a long ternary
>>> operation in the middle of a function call, split in two lines, more
>>> readable than the existing if-else statement, quite the contrary.
>>
>> Agreed, one line of code doing one thing is readable.
> 
> My POV was one memcpy() per destination argument makes it better
> summarized at the function level. Anyway, I am okay with dropping
> this patch.
>

I missed something. I have a helper function btrfs_sb_fsid_ptr() in the
patch 3/4 which reads either metadata_uuid or fsid as per METADATA_UUID
flag. Which in that case the code shall be

memcpy(fs_devices->metadata_uuid, btrfs_sb_fsid_ptr(disk_super),
	BTRFS_FSID_SIZE);

I think this is better?

> Thanks.

Re: [PATCH 1/4] btrfs: simplify memcpy for either metadata_uuid or fsid.

[Anand Jain]

On 29/7/23 12:22, Anand Jain wrote:
> On 29/07/2023 06:23, Anand Jain wrote:
>>
>>
>> On 29/07/2023 02:39, David Sterba wrote:
>>> On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote:
>>>> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> 
>>>> wrote:
>>>>>
>>>>> This change makes the code more readable.
>>>>>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>> ---
>>>>>   fs/btrfs/volumes.c | 12 ++++--------
>>>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>>> index 5678ca9b6281..4ce6c63ab868 100644
>>>>> --- a/fs/btrfs/volumes.c
>>>>> +++ b/fs/btrfs/volumes.c
>>>>> @@ -833,14 +833,10 @@ 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,
>>>>> +                              has_metadata_uuid ?
>>>>> +                               disk_super->metadata_uuid : 
>>>>> disk_super->fsid,
>>>>> +                              BTRFS_FSID_SIZE);
>>>>
>>>> While there's less lines of code, I don't find having a long ternary
>>>> operation in the middle of a function call, split in two lines, more
>>>> readable than the existing if-else statement, quite the contrary.
>>>
>>> Agreed, one line of code doing one thing is readable.
>>
>> My POV was one memcpy() per destination argument makes it better
>> summarized at the function level. Anyway, I am okay with dropping
>> this patch.
>>
> 
> I missed something. I have a helper function btrfs_sb_fsid_ptr() in the
> patch 3/4 which reads either metadata_uuid or fsid as per METADATA_UUID
> flag. Which in that case the code shall be
> 
> memcpy(fs_devices->metadata_uuid, btrfs_sb_fsid_ptr(disk_super),
>      BTRFS_FSID_SIZE);
> 
> I think this is better?

Sent v2 with this changed. Thx.

> 
>> Thanks.
>