| Message ID | 20230523084020.336697-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] btrfs: fix the btrfs_get_global_root return value | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
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; > } >
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.
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.
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+.
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; }
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(-)