| Message ID | 20230219181022.3499088-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: move all btree initialization into btrfs_init_btree_inode | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Sun, Feb 19, 2023 at 07:10:22PM +0100, Christoph Hellwig wrote: > Move the remaining code that deals with initializing the btree > inode into btrfs_init_btree_inode instead of splitting it between > that helpers and its only caller. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Added to misc-next, thanks.
On Mon, Feb 20, 2023 at 09:19:05PM +0100, David Sterba wrote: > On Sun, Feb 19, 2023 at 07:10:22PM +0100, Christoph Hellwig wrote: > > Move the remaining code that deals with initializing the btree > > inode into btrfs_init_btree_inode instead of splitting it between > > that helpers and its only caller. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Added to misc-next, thanks. Btw, in case you need to rebase again, the subject needs a 'inode' after 'btree'.
On Tue, Feb 21, 2023 at 03:28:18PM +0100, Christoph Hellwig wrote: > On Mon, Feb 20, 2023 at 09:19:05PM +0100, David Sterba wrote: > > On Sun, Feb 19, 2023 at 07:10:22PM +0100, Christoph Hellwig wrote: > > > Move the remaining code that deals with initializing the btree > > > inode into btrfs_init_btree_inode instead of splitting it between > > > that helpers and its only caller. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > Added to misc-next, thanks. > > Btw, in case you need to rebase again, the subject needs a > 'inode' after 'btree'. I see, fixed, thanks.
On 2/20/23 02:10, Christoph Hellwig wrote: > Move the remaining code that deals with initializing the btree > inode into btrfs_init_btree_inode instead of splitting it between > that helpers and its only caller. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/disk-io.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index b53f0e30ce2b3b..981973b40b065a 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2125,11 +2125,16 @@ static void btrfs_init_balance(struct btrfs_fs_info *fs_info) > atomic_set(&fs_info->reloc_cancel_req, 0); > } > > -static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) > +static int btrfs_init_btree_inode(struct super_block *sb) > { > - struct inode *inode = fs_info->btree_inode; > + struct btrfs_fs_info *fs_info = btrfs_sb(sb); > unsigned long hash = btrfs_inode_hash(BTRFS_BTREE_INODE_OBJECTID, > fs_info->tree_root); > + struct inode *inode; > + > + inode = new_inode(sb); > + if (!inode) > + return -ENOMEM; > > inode->i_ino = BTRFS_BTREE_INODE_OBJECTID; > set_nlink(inode, 1); > @@ -2140,6 +2145,7 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) > */ > inode->i_size = OFFSET_MAX; > inode->i_mapping->a_ops = &btree_aops; > + mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS); > > RB_CLEAR_NODE(&BTRFS_I(inode)->rb_node); > extent_io_tree_init(fs_info, &BTRFS_I(inode)->io_tree, > @@ -2152,6 +2158,8 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) > BTRFS_I(inode)->location.offset = 0; > set_bit(BTRFS_INODE_DUMMY, &BTRFS_I(inode)->runtime_flags); > __insert_inode_hash(inode, hash); > + fs_info->btree_inode = inode; > + return 0; > } > > static void btrfs_init_dev_replace_locks(struct btrfs_fs_info *fs_info) > @@ -3351,13 +3359,9 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device > goto fail; > } > > - fs_info->btree_inode = new_inode(sb); > - if (!fs_info->btree_inode) { > - err = -ENOMEM; > + err = btrfs_init_btree_inode(sb); > + if (err) > goto fail; > - } > - mapping_set_gfp_mask(fs_info->btree_inode->i_mapping, GFP_NOFS); > - btrfs_init_btree_inode(fs_info); > > invalidate_bdev(fs_devices->latest_dev->bdev); > Dave, This patch is causing a regression, as reported here: https://patchwork.kernel.org/project/linux-btrfs/patch/2de92bdcebd36e4119467797dedae8a9d97a9df3.1677314616.git.wqu@suse.com/ There are many child functions in open_ctree() that rely on the default value of @err, which is -EINVAL, to return an error instead of ret. The issue is that @err is being overwritten by the return value of btrfs_init_btree_inode() in this patch. To fix this issue, please fold the following diff into the patch. Once that's done, you can add: Reviewed-by: Anand Jain <anand.jain@oracle.com> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 48368d4bc331..0e0c30fe6df6 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3360,9 +3360,11 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device goto fail; } - err = btrfs_init_btree_inode(sb); - if (err) + ret = btrfs_init_btree_inode(sb); + if (ret) { + err = ret; goto fail; + } invalidate_bdev(fs_devices->latest_dev->bdev);
Thanks Anand. The fix looks good to me. The error handling is a bit confsing and hopefully we can get it fixed up eventually. Just curious how you found the error as I didn't see anything in xfstest runs.
On Mon, Feb 27, 2023 at 03:35:53PM +0800, Anand Jain wrote: > On 2/20/23 02:10, Christoph Hellwig wrote: > > > > Dave, > > This patch is causing a regression, as reported here: > > > https://patchwork.kernel.org/project/linux-btrfs/patch/2de92bdcebd36e4119467797dedae8a9d97a9df3.1677314616.git.wqu@suse.com/ > > There are many child functions in open_ctree() that rely on the default > value of @err, which is -EINVAL, to return an error instead of ret. > The issue is that @err is being overwritten by the return value of > btrfs_init_btree_inode() in this patch. > > To fix this issue, please fold the following diff into the patch. Thanks. open_ctree has mixed the original and newish style of error variables. In some btrfs functions but more widely seen in other VFS code the error value is set once and the errors only go to return, while what I think we should unify is the error set right before goto or inherited from the call itself. It's a few lines more but the exact error value is at the place where it happens and not somewhere in the function and possibly changed on any line between. Feel free to clean up open_ctree().
在 2023/2/20 04:40, Christoph Hellwig 写道: > Move the remaining code that deals with initializing the btree > inode into btrfs_init_btree_inode instead of splitting it between > that helpers and its only caller. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Qu Wenruo <wqu@suse.com> Just one small nitpick. > --- > fs/btrfs/disk-io.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index b53f0e30ce2b3b..981973b40b065a 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2125,11 +2125,16 @@ static void btrfs_init_balance(struct btrfs_fs_info *fs_info) > atomic_set(&fs_info->reloc_cancel_req, 0); > } > > -static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) > +static int btrfs_init_btree_inode(struct super_block *sb) It's preferred to use btrfs_fs_info * as a parameter, just to keep the consistency of all btrfs interfaces. Thanks, Qu > { > - struct inode *inode = fs_info->btree_inode; > + struct btrfs_fs_info *fs_info = btrfs_sb(sb); > unsigned long hash = btrfs_inode_hash(BTRFS_BTREE_INODE_OBJECTID, > fs_info->tree_root); > + struct inode *inode; > + > + inode = new_inode(sb); > + if (!inode) > + return -ENOMEM; > > inode->i_ino = BTRFS_BTREE_INODE_OBJECTID; > set_nlink(inode, 1); > @@ -2140,6 +2145,7 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) > */ > inode->i_size = OFFSET_MAX; > inode->i_mapping->a_ops = &btree_aops; > + mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS); > > RB_CLEAR_NODE(&BTRFS_I(inode)->rb_node); > extent_io_tree_init(fs_info, &BTRFS_I(inode)->io_tree, > @@ -2152,6 +2158,8 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) > BTRFS_I(inode)->location.offset = 0; > set_bit(BTRFS_INODE_DUMMY, &BTRFS_I(inode)->runtime_flags); > __insert_inode_hash(inode, hash); > + fs_info->btree_inode = inode; > + return 0; > } > > static void btrfs_init_dev_replace_locks(struct btrfs_fs_info *fs_info) > @@ -3351,13 +3359,9 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device > goto fail; > } > > - fs_info->btree_inode = new_inode(sb); > - if (!fs_info->btree_inode) { > - err = -ENOMEM; > + err = btrfs_init_btree_inode(sb); > + if (err) > goto fail; > - } > - mapping_set_gfp_mask(fs_info->btree_inode->i_mapping, GFP_NOFS); > - btrfs_init_btree_inode(fs_info); > > invalidate_bdev(fs_devices->latest_dev->bdev); >
On Wed, Feb 14, 2024 at 04:58:55PM +1030, Qu Wenruo wrote: > > > 在 2023/2/20 04:40, Christoph Hellwig 写道: > > Move the remaining code that deals with initializing the btree > > inode into btrfs_init_btree_inode instead of splitting it between > > that helpers and its only caller. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Just one small nitpick. The patch is almost one year old, it does not make much sense to send reviews, if there's something to be fixed please send a new patch.
在 2024/2/14 17:42, David Sterba 写道: > On Wed, Feb 14, 2024 at 04:58:55PM +1030, Qu Wenruo wrote: >> >> >> 在 2023/2/20 04:40, Christoph Hellwig 写道: >>> Move the remaining code that deals with initializing the btree >>> inode into btrfs_init_btree_inode instead of splitting it between >>> that helpers and its only caller. >>> >>> Signed-off-by: Christoph Hellwig <hch@lst.de> >> >> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> Just one small nitpick. > > The patch is almost one year old, it does not make much sense to send > reviews, if there's something to be fixed please send a new patch. My bad, I'm re-setting up my mail client, and this one popped up and I thought it's new... Sorry for the noise, Qu
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index b53f0e30ce2b3b..981973b40b065a 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2125,11 +2125,16 @@ static void btrfs_init_balance(struct btrfs_fs_info *fs_info) atomic_set(&fs_info->reloc_cancel_req, 0); } -static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) +static int btrfs_init_btree_inode(struct super_block *sb) { - struct inode *inode = fs_info->btree_inode; + struct btrfs_fs_info *fs_info = btrfs_sb(sb); unsigned long hash = btrfs_inode_hash(BTRFS_BTREE_INODE_OBJECTID, fs_info->tree_root); + struct inode *inode; + + inode = new_inode(sb); + if (!inode) + return -ENOMEM; inode->i_ino = BTRFS_BTREE_INODE_OBJECTID; set_nlink(inode, 1); @@ -2140,6 +2145,7 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) */ inode->i_size = OFFSET_MAX; inode->i_mapping->a_ops = &btree_aops; + mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS); RB_CLEAR_NODE(&BTRFS_I(inode)->rb_node); extent_io_tree_init(fs_info, &BTRFS_I(inode)->io_tree, @@ -2152,6 +2158,8 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info) BTRFS_I(inode)->location.offset = 0; set_bit(BTRFS_INODE_DUMMY, &BTRFS_I(inode)->runtime_flags); __insert_inode_hash(inode, hash); + fs_info->btree_inode = inode; + return 0; } static void btrfs_init_dev_replace_locks(struct btrfs_fs_info *fs_info) @@ -3351,13 +3359,9 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device goto fail; } - fs_info->btree_inode = new_inode(sb); - if (!fs_info->btree_inode) { - err = -ENOMEM; + err = btrfs_init_btree_inode(sb); + if (err) goto fail; - } - mapping_set_gfp_mask(fs_info->btree_inode->i_mapping, GFP_NOFS); - btrfs_init_btree_inode(fs_info); invalidate_bdev(fs_devices->latest_dev->bdev);
Move the remaining code that deals with initializing the btree inode into btrfs_init_btree_inode instead of splitting it between that helpers and its only caller. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/disk-io.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)