[PATCH v2 0/2] btrfs: clean up btrfs_iget, btrfs_iget_path usage

[PATCH v2 0/2] btrfs: clean up btrfs_iget, btrfs_iget_path usage

[Leo Martins]

The first patch moves the path allocation from inside
btrfs_read_locked_inode to btrfs_iget. This makes the code easier to
reason about as it is clear where the allocation occurs and who is in
charge of freeing the path.

The second patch moves the clean up code from btrfs_iget_path into
btrfs_read_locked_inode simplifyiung btrfs_iget_path.

Version 2 includes the modifications suggested by David Sterba:
Patch 1/2:
	- Added an ASSERT(path) to make sure that btrfs_read_locked_inode is
	never called with a null path.
	- Fixed btrfs_iget to more closely match btrfs standards.
	  Contiguous declaration at the top, with no non-trivial
	  assignments.

Leo Martins (2):
  btrfs: remove conditional path allocation from read_locked_inode, add
    path allocation to iget
  btrfs: move clean up code from btrfs_iget_path to
    btrfs_read_locked_inode

 fs/btrfs/inode.c | 128 +++++++++++++++++++++++------------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

-- 
2.43.5

[PATCH v2 1/2] btrfs: remove conditional path allocation from read_locked_inode, add path allocation to iget

[Leo Martins]

Move the path allocation from inside btrfs_read_locked_inode
to btrfs_iget. This makes the code easier to reason about as it is
clear where the allocation occurs and who is in charge of freeing the
path. I have investigated all of the callers of btrfs_iget_path to make
sure that it is never called with a null path with the expectation
of a path allocation. All of the null calls seem to come from btrfs_iget
so it makes sense to do the allocation within btrfs_iget.

---
 fs/btrfs/inode.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a8ad540d6de2..f2959803f9d7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3790,10 +3790,9 @@ static int btrfs_init_file_extent_tree(struct btrfs_inode *inode)
  * read an inode from the btree into the in-memory inode
  */
 static int btrfs_read_locked_inode(struct inode *inode,
-				   struct btrfs_path *in_path)
+				   struct btrfs_path *path)
 {
 	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
-	struct btrfs_path *path = in_path;
 	struct extent_buffer *leaf;
 	struct btrfs_inode_item *inode_item;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -3813,20 +3812,13 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	if (!ret)
 		filled = true;
 
-	if (!path) {
-		path = btrfs_alloc_path();
-		if (!path)
-			return -ENOMEM;
-	}
+	ASSERT(path);
 
 	btrfs_get_inode_key(BTRFS_I(inode), &location);
 
 	ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
-	if (ret) {
-		if (path != in_path)
-			btrfs_free_path(path);
+	if (ret)
 		return ret;
-	}
 
 	leaf = path->nodes[0];
 
@@ -3960,8 +3952,6 @@ static int btrfs_read_locked_inode(struct inode *inode,
 				  btrfs_ino(BTRFS_I(inode)),
 				  btrfs_root_id(root), ret);
 	}
-	if (path != in_path)
-		btrfs_free_path(path);
 
 	if (!maybe_acls)
 		cache_no_acl(inode);
@@ -5632,7 +5622,17 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 
 struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
 {
-	return btrfs_iget_path(ino, root, NULL);
+	struct btrfs_path *path;
+	struct inode *inode;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	inode = btrfs_iget_path(ino, root, path);
+
+	btrfs_free_path(path);
+	return inode;
 }
 
 static struct inode *new_simple_dir(struct inode *dir,
-- 
2.43.5

[PATCH v2 2/2] btrfs: move clean up code from btrfs_iget_path to btrfs_read_locked_inode

[Leo Martins]

Move all the clean up code from btrfs_iget_path to
btrfs_read_locked_inode. I had to move btrfs_add_inode_to_root as it
needs to be called by btrfs_read_locked_inode, but made no changes
to it.
---
 fs/btrfs/inode.c | 102 +++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f2959803f9d7..f4d20e590b70 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3786,6 +3786,36 @@ static int btrfs_init_file_extent_tree(struct btrfs_inode *inode)
 	return 0;
 }
 
+static int btrfs_add_inode_to_root(struct btrfs_inode *inode, bool prealloc)
+{
+	struct btrfs_root *root = inode->root;
+	struct btrfs_inode *existing;
+	const u64 ino = btrfs_ino(inode);
+	int ret;
+
+	if (inode_unhashed(&inode->vfs_inode))
+		return 0;
+
+	if (prealloc) {
+		ret = xa_reserve(&root->inodes, ino, GFP_NOFS);
+		if (ret)
+			return ret;
+	}
+
+	existing = xa_store(&root->inodes, ino, inode, GFP_ATOMIC);
+
+	if (xa_is_err(existing)) {
+		ret = xa_err(existing);
+		ASSERT(ret != -EINVAL);
+		ASSERT(ret != -ENOMEM);
+		return ret;
+	} else if (existing) {
+		WARN_ON(!(existing->vfs_inode.i_state & (I_WILL_FREE | I_FREEING)));
+	}
+
+	return 0;
+}
+
 /*
  * read an inode from the btree into the in-memory inode
  */
@@ -3806,7 +3836,7 @@ static int btrfs_read_locked_inode(struct inode *inode,
 
 	ret = btrfs_init_file_extent_tree(BTRFS_I(inode));
 	if (ret)
-		return ret;
+		goto error;
 
 	ret = btrfs_fill_inode(inode, &rdev);
 	if (!ret)
@@ -3817,8 +3847,15 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	btrfs_get_inode_key(BTRFS_I(inode), &location);
 
 	ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
-	if (ret)
-		return ret;
+	/*
+	 * ret > 0 can come from btrfs_search_slot called by
+	 * btrfs_read_locked_inode(), this means the inode item was not found.
+	 */
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0)
+		goto error;
+
 
 	leaf = path->nodes[0];
 
@@ -3978,7 +4015,16 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	}
 
 	btrfs_sync_inode_flags_to_i_flags(inode);
+
+	ret = btrfs_add_inode_to_root(BTRFS_I(inode), true);
+	if (ret < 0)
+		goto error;
+
+	unlock_new_inode(inode);
 	return 0;
+error:
+	iget_failed(inode);
+	return ret;
 }
 
 /*
@@ -5490,36 +5536,6 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
 	return err;
 }
 
-static int btrfs_add_inode_to_root(struct btrfs_inode *inode, bool prealloc)
-{
-	struct btrfs_root *root = inode->root;
-	struct btrfs_inode *existing;
-	const u64 ino = btrfs_ino(inode);
-	int ret;
-
-	if (inode_unhashed(&inode->vfs_inode))
-		return 0;
-
-	if (prealloc) {
-		ret = xa_reserve(&root->inodes, ino, GFP_NOFS);
-		if (ret)
-			return ret;
-	}
-
-	existing = xa_store(&root->inodes, ino, inode, GFP_ATOMIC);
-
-	if (xa_is_err(existing)) {
-		ret = xa_err(existing);
-		ASSERT(ret != -EINVAL);
-		ASSERT(ret != -ENOMEM);
-		return ret;
-	} else if (existing) {
-		WARN_ON(!(existing->vfs_inode.i_state & (I_WILL_FREE | I_FREEING)));
-	}
-
-	return 0;
-}
-
 static void btrfs_del_inode_from_root(struct btrfs_inode *inode)
 {
 	struct btrfs_root *root = inode->root;
@@ -5599,25 +5615,9 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 		return inode;
 
 	ret = btrfs_read_locked_inode(inode, path);
-	/*
-	 * ret > 0 can come from btrfs_search_slot called by
-	 * btrfs_read_locked_inode(), this means the inode item was not found.
-	 */
-	if (ret > 0)
-		ret = -ENOENT;
-	if (ret < 0)
-		goto error;
-
-	ret = btrfs_add_inode_to_root(BTRFS_I(inode), true);
-	if (ret < 0)
-		goto error;
-
-	unlock_new_inode(inode);
-
+	if (ret)
+		return ERR_PTR(ret);
 	return inode;
-error:
-	iget_failed(inode);
-	return ERR_PTR(ret);
 }
 
 struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
-- 
2.43.5

Re: [PATCH v2 1/2] btrfs: remove conditional path allocation from read_locked_inode, add path allocation to iget

[Josef Bacik]

On Tue, Aug 20, 2024 at 01:13:18PM -0700, Leo Martins wrote:
> Move the path allocation from inside btrfs_read_locked_inode
> to btrfs_iget. This makes the code easier to reason about as it is
> clear where the allocation occurs and who is in charge of freeing the
> path. I have investigated all of the callers of btrfs_iget_path to make
> sure that it is never called with a null path with the expectation
> of a path allocation. All of the null calls seem to come from btrfs_iget
> so it makes sense to do the allocation within btrfs_iget.

You're missing your Signed-off-by.  Also try to be less verbose in your title,
"btrfs: move path allocation into btrfs_iget" or something like that, you want
to avoid wrapping if you can.  Fix those things and you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v2 1/2] btrfs: remove conditional path allocation from read_locked_inode, add path allocation to iget

[Josef Bacik]

On Tue, Aug 20, 2024 at 01:13:18PM -0700, Leo Martins wrote:
> Move the path allocation from inside btrfs_read_locked_inode
> to btrfs_iget. This makes the code easier to reason about as it is
> clear where the allocation occurs and who is in charge of freeing the
> path. I have investigated all of the callers of btrfs_iget_path to make
> sure that it is never called with a null path with the expectation
> of a path allocation. All of the null calls seem to come from btrfs_iget
> so it makes sense to do the allocation within btrfs_iget.
> 
> ---
>  fs/btrfs/inode.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a8ad540d6de2..f2959803f9d7 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3790,10 +3790,9 @@ static int btrfs_init_file_extent_tree(struct btrfs_inode *inode)
>   * read an inode from the btree into the in-memory inode
>   */
>  static int btrfs_read_locked_inode(struct inode *inode,
> -				   struct btrfs_path *in_path)
> +				   struct btrfs_path *path)
>  {
>  	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
> -	struct btrfs_path *path = in_path;
>  	struct extent_buffer *leaf;
>  	struct btrfs_inode_item *inode_item;
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> @@ -3813,20 +3812,13 @@ static int btrfs_read_locked_inode(struct inode *inode,
>  	if (!ret)
>  		filled = true;
>  
> -	if (!path) {
> -		path = btrfs_alloc_path();
> -		if (!path)
> -			return -ENOMEM;
> -	}
> +	ASSERT(path);
>  
>  	btrfs_get_inode_key(BTRFS_I(inode), &location);
>  
>  	ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
> -	if (ret) {
> -		if (path != in_path)
> -			btrfs_free_path(path);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	leaf = path->nodes[0];
>  
> @@ -3960,8 +3952,6 @@ static int btrfs_read_locked_inode(struct inode *inode,
>  				  btrfs_ino(BTRFS_I(inode)),
>  				  btrfs_root_id(root), ret);
>  	}
> -	if (path != in_path)
> -		btrfs_free_path(path);
>  
>  	if (!maybe_acls)
>  		cache_no_acl(inode);
> @@ -5632,7 +5622,17 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
>  
>  struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
>  {
> -	return btrfs_iget_path(ino, root, NULL);
> +	struct btrfs_path *path;
> +	struct inode *inode;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return ERR_PTR(-ENOMEM);
> +

Actually now that I look at it, I don't want to do it this way.  Sorry about
that because I'm the one who suggested cleaning this all up, and I missed an
important piece here.

With btrfs_iget_path() we're doing the btrfs_iget_locked() first, which will
find the inode in cache if it can, so the path allocation isn't necessary in
that case.  I missed this when I was looking at this, so I think it's better to
move the path allocation out of btrfs_read_locked_inode(), but push it into
btrfs_iget_path() instead.  So btrfs_iget_path() will handle path == NULL the
way that btrfs_read_locked_inode() currently does, allocating if it has to and
freeing if it's necessary.  The second patch is still good.  Thanks,

Josef

Re: [PATCH v2 2/2] btrfs: move clean up code from btrfs_iget_path to btrfs_read_locked_inode

[Josef Bacik]

On Tue, Aug 20, 2024 at 01:13:19PM -0700, Leo Martins wrote:
> Move all the clean up code from btrfs_iget_path to
> btrfs_read_locked_inode. I had to move btrfs_add_inode_to_root as it
> needs to be called by btrfs_read_locked_inode, but made no changes
> to it.

Same comment as the other patch, missing your SoB, and the title is too long and
it wrapped.  Thanks,

Josef