[PATCH v4 0/2] btrfs: iget_path cleanup

[PATCH v4 0/2] btrfs: iget_path cleanup

[Leo Martins]

Updates from v3:
Previously I allocated a path in btrfs_iget and called btrfs_iget_path
with it. However, Josef pointed out that there is a case in
btrfs_iget_path where the inode was found in cache and no path
allocation was necessary. In this patch series I no longer call
btrfs_iget_path from btrfs_iget, instead I duplicated the code from
btrfs_iget_path with a path allocation.

This patch series is a cleanup of btrfs_iget_path and btrfs_iget. It
moves some cleanup and error handling from btrfs_iget_path into
read_locked_inode. In addition it also removes a conditional path
allocation that occurs in read_locked_inode, instead reworking
btrfs_iget to allocate and free the path.

Leo Martins (2):
  btrfs: push btrfs_iget_path cleanup into btrfs_read_locked_inode
  btrfs:

 fs/btrfs/inode.c | 143 +++++++++++++++++++++++++++--------------------
 1 file changed, 81 insertions(+), 62 deletions(-)

-- 
2.43.5

[PATCH v4 1/2] btrfs: push cleanup into read_locked_inode

[Leo Martins]

Move btrfs_add_inode_to_root so it can be called from
btrfs_read_locked_inode, no changes were made to the function.

Move cleanup code from btrfs_iget_path to btrfs_read_locked_inode.
This improves readability and improves a leaky abstraction. Previously
btrfs_iget_path had to handle a positive error case as a result of a
call to btrfs_search_slot, but it makes more sense to handle this closer
to the source of the call.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 fs/btrfs/inode.c | 99 +++++++++++++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e96b63d7e8fd..df0fe5e79ea2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3786,8 +3786,39 @@ 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
+ * read a locked inode from the btree into the in-memory inode
+ * on failure clean up the inode
  */
 static int btrfs_read_locked_inode(struct inode *inode,
 				   struct btrfs_path *in_path)
@@ -3807,7 +3838,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)
@@ -3816,7 +3847,7 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	if (!path) {
 		path = btrfs_alloc_path();
 		if (!path)
-			return -ENOMEM;
+			goto error;
 	}
 
 	btrfs_get_inode_key(BTRFS_I(inode), &location);
@@ -3825,7 +3856,13 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	if (ret) {
 		if (path != in_path)
 			btrfs_free_path(path);
-		return ret;
+		/*
+		 * ret > 0 can come from btrfs_search_slot called by
+		 * btrfs_lookup_inode(), this means the inode was not found.
+		 */
+		if (ret > 0)
+			ret = -ENOENT;
+		goto error;
 	}
 
 	leaf = path->nodes[0];
@@ -3988,7 +4025,15 @@ 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)
+		goto error;
+
 	return 0;
+error:
+	iget_failed(inode);
+	return ret;
 }
 
 /*
@@ -5500,35 +5545,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)
 {
@@ -5609,25 +5626,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;
+	if (ret)
+		return ERR_PTR(ret);
 
 	unlock_new_inode(inode);
-
 	return inode;
-error:
-	iget_failed(inode);
-	return ERR_PTR(ret);
 }
 
 struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
-- 
2.43.5

[PATCH v4 2/2] btrfs: remove conditional path allocation

[Leo Martins]

Remove conditional path allocation from btrfs_read_locked_inode. Add
an ASSERT(path) to indicate it should never be called with a NULL path.

Call btrfs_read_locked_inode directly from btrfs_iget. This causes
code duplication between btrfs_iget and btrfs_iget_path, but I
think this is justifiable as it removes the need for conditionally
allocating the path inside of read_locked_inode. This makes the code
easier to reason about and makes it clear who has the responsibility of
allocating and freeing the path.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 fs/btrfs/inode.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index df0fe5e79ea2..a4b4e4190624 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3821,10 +3821,9 @@ static int btrfs_add_inode_to_root(struct btrfs_inode *inode, bool prealloc)
  * on failure clean up the 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;
@@ -3844,18 +3843,12 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	if (!ret)
 		filled = true;
 
-	if (!path) {
-		path = btrfs_alloc_path();
-		if (!path)
-			goto error;
-	}
+	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);
 		/*
 		 * ret > 0 can come from btrfs_search_slot called by
 		 * btrfs_lookup_inode(), this means the inode was not found.
@@ -3997,8 +3990,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);
@@ -5608,9 +5599,8 @@ static struct inode *btrfs_iget_locked(u64 ino, struct btrfs_root *root)
 
 /*
  * Get an inode object given its inode number and corresponding root.
- * Path can be preallocated to prevent recursing back to iget through
- * allocator. NULL is also valid but may require an additional allocation
- * later.
+ * Path is preallocated to prevent recursing back to iget through
+ * allocator.
  */
 struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 			      struct btrfs_path *path)
@@ -5633,9 +5623,35 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 	return inode;
 }
 
+/*
+ * Get an inode object given its inode number and corresponding root.
+ */
 struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
 {
-	return btrfs_iget_path(ino, root, NULL);
+	struct inode *inode;
+	struct btrfs_path *path;
+	int ret;
+
+	inode = btrfs_iget_locked(ino, root);
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	if (!(inode->i_state & I_NEW))
+		return inode;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	ret = btrfs_read_locked_inode(inode, path);
+
+	btrfs_free_path(path);
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	unlock_new_inode(inode);
+	return inode;
 }
 
 static struct inode *new_simple_dir(struct inode *dir,
-- 
2.43.5

Re: [PATCH v4 0/2] btrfs: iget_path cleanup

[David Sterba]

On Fri, Aug 30, 2024 at 01:24:53PM -0700, Leo Martins wrote:
> Updates from v3:
> Previously I allocated a path in btrfs_iget and called btrfs_iget_path
> with it. However, Josef pointed out that there is a case in
> btrfs_iget_path where the inode was found in cache and no path
> allocation was necessary. In this patch series I no longer call
> btrfs_iget_path from btrfs_iget, instead I duplicated the code from
> btrfs_iget_path with a path allocation.
> 
> This patch series is a cleanup of btrfs_iget_path and btrfs_iget. It
> moves some cleanup and error handling from btrfs_iget_path into
> read_locked_inode. In addition it also removes a conditional path
> allocation that occurs in read_locked_inode, instead reworking
> btrfs_iget to allocate and free the path.
> 
> Leo Martins (2):
>   btrfs: push btrfs_iget_path cleanup into btrfs_read_locked_inode
>   btrfs:

I had the two patches in my misc-next, I was not sure about something
that I forgot meanwhile. Removing the conditional parameter is a useful
cleanup.  After a fresh look I moved the patches to for-next with some
minor tweaks. Thanks.