[PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[Christoph Hellwig]

btrfs_grab_root returns either the root or NULL, and the callers of
btrfs_get_global_root expect it to return the same.  But all the more
recently added roots instead return an ERR_PTR, so fix this.

Fixes: bcef60f24903 ("Btrfs: quota tree support and startup")
Fixes: f7a81ea4cc6b ("Btrfs: create UUID tree if required")
Fixes: 70f6d82ec73c ("Btrfs: add free space tree mount option")
Fixes: 14033b08a029 ("btrfs: don't save block group root into super block")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index dc2ad0bf88f84c..5dc5d733ecfa4a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1358,19 +1358,13 @@ static struct btrfs_root *btrfs_get_global_root(struct btrfs_fs_info *fs_info,
 	if (objectid == BTRFS_CSUM_TREE_OBJECTID)
 		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
 	if (objectid == BTRFS_QUOTA_TREE_OBJECTID)
-		return btrfs_grab_root(fs_info->quota_root) ?
-			fs_info->quota_root : ERR_PTR(-ENOENT);
+		return btrfs_grab_root(fs_info->quota_root);
 	if (objectid == BTRFS_UUID_TREE_OBJECTID)
-		return btrfs_grab_root(fs_info->uuid_root) ?
-			fs_info->uuid_root : ERR_PTR(-ENOENT);
+		return btrfs_grab_root(fs_info->uuid_root);
 	if (objectid == BTRFS_BLOCK_GROUP_TREE_OBJECTID)
-		return btrfs_grab_root(fs_info->block_group_root) ?
-			fs_info->block_group_root : ERR_PTR(-ENOENT);
-	if (objectid == BTRFS_FREE_SPACE_TREE_OBJECTID) {
-		struct btrfs_root *root = btrfs_global_root(fs_info, &key);
-
-		return btrfs_grab_root(root) ? root : ERR_PTR(-ENOENT);
-	}
+		return btrfs_grab_root(fs_info->block_group_root);
+	if (objectid == BTRFS_FREE_SPACE_TREE_OBJECTID)
+		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
 	return NULL;
 }
 
-- 
2.39.2

[PATCH 2/3] btrfs: convert btrfs_get_global_root to use a switch statement

[Christoph Hellwig]

Use a switch statement instead of an endless chain of if statements
to make the code a little cleaner.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5dc5d733ecfa4a..1edd6685df5760 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1347,25 +1347,28 @@ static struct btrfs_root *btrfs_get_global_root(struct btrfs_fs_info *fs_info,
 		.offset = 0,
 	};
 
-	if (objectid == BTRFS_ROOT_TREE_OBJECTID)
+	switch (objectid) {
+	case BTRFS_ROOT_TREE_OBJECTID:
 		return btrfs_grab_root(fs_info->tree_root);
-	if (objectid == BTRFS_EXTENT_TREE_OBJECTID)
+	case BTRFS_EXTENT_TREE_OBJECTID:
 		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
-	if (objectid == BTRFS_CHUNK_TREE_OBJECTID)
+	case BTRFS_CHUNK_TREE_OBJECTID:
 		return btrfs_grab_root(fs_info->chunk_root);
-	if (objectid == BTRFS_DEV_TREE_OBJECTID)
+	case BTRFS_DEV_TREE_OBJECTID:
 		return btrfs_grab_root(fs_info->dev_root);
-	if (objectid == BTRFS_CSUM_TREE_OBJECTID)
+	case BTRFS_CSUM_TREE_OBJECTID:
 		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
-	if (objectid == BTRFS_QUOTA_TREE_OBJECTID)
+	case BTRFS_QUOTA_TREE_OBJECTID:
 		return btrfs_grab_root(fs_info->quota_root);
-	if (objectid == BTRFS_UUID_TREE_OBJECTID)
+	case BTRFS_UUID_TREE_OBJECTID:
 		return btrfs_grab_root(fs_info->uuid_root);
-	if (objectid == BTRFS_BLOCK_GROUP_TREE_OBJECTID)
+	case BTRFS_BLOCK_GROUP_TREE_OBJECTID:
 		return btrfs_grab_root(fs_info->block_group_root);
-	if (objectid == BTRFS_FREE_SPACE_TREE_OBJECTID)
+	case BTRFS_FREE_SPACE_TREE_OBJECTID:
 		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
-	return NULL;
+	default:
+		return NULL;
+	}
 }
 
 int btrfs_insert_fs_root(struct btrfs_fs_info *fs_info,
-- 
2.39.2

[PATCH 3/3] btrfs: remove a pointless NULL check in btrfs_lookup_fs_root

[Christoph Hellwig]

btrfs_grab_root already checks for a NULL root itself.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1edd6685df5760..c70d9defb90fa5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1332,8 +1332,7 @@ static struct btrfs_root *btrfs_lookup_fs_root(struct btrfs_fs_info *fs_info,
 	spin_lock(&fs_info->fs_roots_radix_lock);
 	root = radix_tree_lookup(&fs_info->fs_roots_radix,
 				 (unsigned long)root_id);
-	if (root)
-		root = btrfs_grab_root(root);
+	root = btrfs_grab_root(root);
 	spin_unlock(&fs_info->fs_roots_radix_lock);
 	return root;
 }
-- 
2.39.2

Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[Johannes Thumshirn]

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

Re: [PATCH 2/3] btrfs: convert btrfs_get_global_root to use a switch statement

[Johannes Thumshirn]

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

Re: [PATCH 3/3] btrfs: remove a pointless NULL check in btrfs_lookup_fs_root

[Johannes Thumshirn]

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

Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[Anand Jain]

On 23/05/2023 16:40, Christoph Hellwig wrote:
> btrfs_grab_root returns either the root or NULL, and the callers of
> btrfs_get_global_root expect it to return the same.  But all the more
> recently added roots instead return an ERR_PTR, so fix this.
> 

Fix looks good. However, I'm curious about the Fixes commit
you're referring to as the one this fix addresses...

> Fixes: bcef60f24903 ("Btrfs: quota tree support and startup")

btrfs_read_fs_root_no_name() return value used at that commit.

> Fixes: f7a81ea4cc6b ("Btrfs: create UUID tree if required")
> Fixes: 70f6d82ec73c ("Btrfs: add free space tree mount option")

I didn't check these two Fixes.

> Fixes: 14033b08a029 ("btrfs: don't save block group root into super block")

This one is correct.

Thanks, Anand

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/disk-io.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index dc2ad0bf88f84c..5dc5d733ecfa4a 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1358,19 +1358,13 @@ static struct btrfs_root *btrfs_get_global_root(struct btrfs_fs_info *fs_info,
>   	if (objectid == BTRFS_CSUM_TREE_OBJECTID)
>   		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
>   	if (objectid == BTRFS_QUOTA_TREE_OBJECTID)
> -		return btrfs_grab_root(fs_info->quota_root) ?
> -			fs_info->quota_root : ERR_PTR(-ENOENT);
> +		return btrfs_grab_root(fs_info->quota_root);
>   	if (objectid == BTRFS_UUID_TREE_OBJECTID)
> -		return btrfs_grab_root(fs_info->uuid_root) ?
> -			fs_info->uuid_root : ERR_PTR(-ENOENT);
> +		return btrfs_grab_root(fs_info->uuid_root);
>   	if (objectid == BTRFS_BLOCK_GROUP_TREE_OBJECTID)
> -		return btrfs_grab_root(fs_info->block_group_root) ?
> -			fs_info->block_group_root : ERR_PTR(-ENOENT);
> -	if (objectid == BTRFS_FREE_SPACE_TREE_OBJECTID) {
> -		struct btrfs_root *root = btrfs_global_root(fs_info, &key);
> -
> -		return btrfs_grab_root(root) ? root : ERR_PTR(-ENOENT);
> -	}
> +		return btrfs_grab_root(fs_info->block_group_root);
> +	if (objectid == BTRFS_FREE_SPACE_TREE_OBJECTID)
> +		return btrfs_grab_root(btrfs_global_root(fs_info, &key));
>   	return NULL;
>   }
>   

Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[Christoph Hellwig]

On Tue, May 23, 2023 at 07:37:23PM +0800, Anand Jain wrote:
> On 23/05/2023 16:40, Christoph Hellwig wrote:
>> btrfs_grab_root returns either the root or NULL, and the callers of
>> btrfs_get_global_root expect it to return the same.  But all the more
>> recently added roots instead return an ERR_PTR, so fix this.
>>
>
> Fix looks good. However, I'm curious about the Fixes commit
> you're referring to as the one this fix addresses...
>
>> Fixes: bcef60f24903 ("Btrfs: quota tree support and startup")
>
> btrfs_read_fs_root_no_name() return value used at that commit.

Indeed. open_ctree also used to check IS_ERR after the NULL check.
So while this commit was really odd in that it added the first ERR_PTR
return to the otherwise NULL as failure btrfs_read_fs_root_no_name,
that was actually handled.  Looks like the first fixes would be for
whoever dropped that check, but finding code removals in git-blame
tends to be a bit hard.  I'll see if I can track down the culprit.

Otherwise I'm fine with just dropping the extra Fixes tags.

Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[David Sterba]

On Tue, May 23, 2023 at 01:48:27PM +0200, Christoph Hellwig wrote:
> On Tue, May 23, 2023 at 07:37:23PM +0800, Anand Jain wrote:
> > On 23/05/2023 16:40, Christoph Hellwig wrote:
> >> btrfs_grab_root returns either the root or NULL, and the callers of
> >> btrfs_get_global_root expect it to return the same.  But all the more
> >> recently added roots instead return an ERR_PTR, so fix this.
> >>
> >
> > Fix looks good. However, I'm curious about the Fixes commit
> > you're referring to as the one this fix addresses...
> >
> >> Fixes: bcef60f24903 ("Btrfs: quota tree support and startup")
> >
> > btrfs_read_fs_root_no_name() return value used at that commit.
> 
> Indeed. open_ctree also used to check IS_ERR after the NULL check.
> So while this commit was really odd in that it added the first ERR_PTR
> return to the otherwise NULL as failure btrfs_read_fs_root_no_name,
> that was actually handled.  Looks like the first fixes would be for
> whoever dropped that check, but finding code removals in git-blame
> tends to be a bit hard.  I'll see if I can track down the culprit.
> 
> Otherwise I'm fine with just dropping the extra Fixes tags.

No need to track it down to the specific commits in this case, it could
be tricky and without much use. I'll add a CC:stable for 6.1 which is
relevant, 5.15 might be also relevant but the patch does not apply
cleanly. The global roots are a recent abstraction of direct root
pointers that are not unique in the extent tree v2 design.

Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value

[David Sterba]

On Tue, May 23, 2023 at 10:40:18AM +0200, Christoph Hellwig wrote:
> btrfs_grab_root returns either the root or NULL, and the callers of
> btrfs_get_global_root expect it to return the same.  But all the more
> recently added roots instead return an ERR_PTR, so fix this.
> 
> Fixes: bcef60f24903 ("Btrfs: quota tree support and startup")
> Fixes: f7a81ea4cc6b ("Btrfs: create UUID tree if required")
> Fixes: 70f6d82ec73c ("Btrfs: add free space tree mount option")
> Fixes: 14033b08a029 ("btrfs: don't save block group root into super block")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

1-3 added to misc-next, thanks. I dropped the Fixes: lines here,
replaced by CC:stable 6.1+.