* [PATCH 2/3] btrfs: convert btrfs_get_global_root to use a switch statement
2023-05-23 8:40 [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Christoph Hellwig
@ 2023-05-23 8:40 ` Christoph Hellwig
2023-05-23 10:19 ` Johannes Thumshirn
2023-05-23 8:40 ` [PATCH 3/3] btrfs: remove a pointless NULL check in btrfs_lookup_fs_root Christoph Hellwig
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2023-05-23 8:40 UTC (permalink / raw)
To: clm, josef, dsterba; +Cc: linux-btrfs
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/3] btrfs: remove a pointless NULL check in btrfs_lookup_fs_root
2023-05-23 8:40 [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Christoph Hellwig
2023-05-23 8:40 ` [PATCH 2/3] btrfs: convert btrfs_get_global_root to use a switch statement Christoph Hellwig
@ 2023-05-23 8:40 ` Christoph Hellwig
2023-05-23 10:20 ` Johannes Thumshirn
2023-05-23 10:11 ` [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Johannes Thumshirn
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2023-05-23 8:40 UTC (permalink / raw)
To: clm, josef, dsterba; +Cc: linux-btrfs
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value
2023-05-23 8:40 [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Christoph Hellwig
2023-05-23 8:40 ` [PATCH 2/3] btrfs: convert btrfs_get_global_root to use a switch statement Christoph Hellwig
2023-05-23 8:40 ` [PATCH 3/3] btrfs: remove a pointless NULL check in btrfs_lookup_fs_root Christoph Hellwig
@ 2023-05-23 10:11 ` Johannes Thumshirn
2023-05-23 11:37 ` Anand Jain
2023-05-25 23:15 ` David Sterba
4 siblings, 0 replies; 10+ messages in thread
From: Johannes Thumshirn @ 2023-05-23 10:11 UTC (permalink / raw)
To: Christoph Hellwig, clm@fb.com, josef@toxicpanda.com,
dsterba@suse.com
Cc: linux-btrfs@vger.kernel.org
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value
2023-05-23 8:40 [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Christoph Hellwig
` (2 preceding siblings ...)
2023-05-23 10:11 ` [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Johannes Thumshirn
@ 2023-05-23 11:37 ` Anand Jain
2023-05-23 11:48 ` Christoph Hellwig
2023-05-25 23:15 ` David Sterba
4 siblings, 1 reply; 10+ messages in thread
From: Anand Jain @ 2023-05-23 11:37 UTC (permalink / raw)
To: Christoph Hellwig, clm, josef, dsterba; +Cc: linux-btrfs
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;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value
2023-05-23 11:37 ` Anand Jain
@ 2023-05-23 11:48 ` Christoph Hellwig
2023-05-25 23:12 ` David Sterba
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2023-05-23 11:48 UTC (permalink / raw)
To: Anand Jain; +Cc: Christoph Hellwig, clm, josef, dsterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value
2023-05-23 11:48 ` Christoph Hellwig
@ 2023-05-25 23:12 ` David Sterba
0 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2023-05-25 23:12 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Anand Jain, clm, josef, dsterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value
2023-05-23 8:40 [PATCH 1/3] btrfs: fix the btrfs_get_global_root return value Christoph Hellwig
` (3 preceding siblings ...)
2023-05-23 11:37 ` Anand Jain
@ 2023-05-25 23:15 ` David Sterba
4 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2023-05-25 23:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: clm, josef, dsterba, linux-btrfs
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+.
^ permalink raw reply [flat|nested] 10+ messages in thread