[PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[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>
---
 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);
 
-- 
2.39.1

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[Johannes Thumshirn]

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

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[David Sterba]

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.

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[Christoph Hellwig]

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'.

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[David Sterba]

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.

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[Anand Jain]

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);

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[Christoph Hellwig]

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.

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[David Sterba]

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().

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[Qu Wenruo]

在 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);
>

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[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.

Re: [PATCH] btrfs: move all btree initialization into btrfs_init_btree_inode

[Qu Wenruo]

在 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