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

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

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 | 123 ++++++++++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 61 deletions(-)

-- 
2.43.5

[PATCH 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 | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 07858d63378f..b89b4b1bd3da 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,18 +3812,10 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	if (!ret)
 		filled = true;
 
-	if (!path) {
-		path = btrfs_alloc_path();
-		if (!path)
-			return -ENOMEM;
-	}
-
 	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);
 		return ret;
 	}
 
@@ -3960,8 +3951,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);
@@ -5631,7 +5620,15 @@ 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 = btrfs_alloc_path();
+
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	struct inode *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

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

[David Sterba]

On Tue, Aug 13, 2024 at 01:27:33PM -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.

This looks like a good cleanup. The path argument was added to fix some
problem in the past, 4222ea7100c0e3 ("Btrfs: fix deadlock on tree root
leaf when finding free extent") but doing the allocation in the caller
makes things more clear.

> ---
>  fs/btrfs/inode.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 07858d63378f..b89b4b1bd3da 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,18 +3812,10 @@ static int btrfs_read_locked_inode(struct inode *inode,
>  	if (!ret)
>  		filled = true;

You can add an ASSERT(path)

>  
> -	if (!path) {
> -		path = btrfs_alloc_path();
> -		if (!path)
> -			return -ENOMEM;
> -	}
> -
>  	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);
>  		return ret;
>  	}
>  
> @@ -3960,8 +3951,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);
> @@ -5631,7 +5620,15 @@ 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 = btrfs_alloc_path();
> +
> +	if (!path)
> +		return ERR_PTR(-ENOMEM);
> +
> +	struct inode *inode = btrfs_iget_path(ino, root, path);

In kernel the declaration block should be contiguous, ie. not mixed with
statements like that. Also, we don't do allocatios (and other
non-trivial assignments that require error handling) in the declaration
block. This may different in other parts of kernel, in btrfs it's the
preferred way.

[PATCH 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 | 100 ++++++++++++++++++++++++-----------------------
 1 file changed, 52 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b89b4b1bd3da..01a7677fef5c 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)
@@ -3815,9 +3845,14 @@ 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_lookup_inode(), this means the inode item was not found.
+	 */
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0)
+		goto error;
 
 	leaf = path->nodes[0];
 
@@ -3977,7 +4012,18 @@ 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;
 }
 
 /*
@@ -5488,35 +5534,7 @@ 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)
 {
@@ -5597,25 +5615,11 @@ 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