[PATCH v3 0/2] clean up iget_path, read_locked_inode

[PATCH v3 0/2] clean up iget_path, read_locked_inode

[Leo Martins]

The first patch moves the path allocation from
btrfs_read_locked_inode to btrfs_iget_path. It makes
more sense to handle a path allocation outside of 
read_locked_inode.

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

Leo Martins (2):
  btrfs: move path allocation to btrfs_iget_path
  btrfs: move cleanup code to read_locked_inode

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

-- 
2.43.5

[PATCH v3 1/2] btrfs: move path allocation to btrfs_iget_path

[Leo Martins]

Since we're going to keep the conditional path allocation
does it still make sense to move the path allocation from
read_locked_inode?

My understanding of the benefits of moving the path allocation to
btrfs_iget is that there is no need for a conditional path allocation
and as a result the code is easier to reason about and
responsibilities are clearer.

I don't think it makes much of a difference at this point to allocate
in btrfs_iget_path vs. btrfs_read_locked_inode. If I'm missing
something please let me know.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a8ad540d6de25..74d23d0cd1eb9 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,11 @@ 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);
+	if (ret)
 		return ret;
-	}
 
 	leaf = path->nodes[0];
 
@@ -3960,8 +3950,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);
@@ -5596,8 +5584,9 @@ static struct inode *btrfs_iget_locked(u64 ino, struct btrfs_root *root)
  * later.
  */
 struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
-			      struct btrfs_path *path)
+			      struct btrfs_path *in_path)
 {
+	struct btrfs_path *path = in_path;
 	struct inode *inode;
 	int ret;
 
@@ -5608,7 +5597,20 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 	if (!(inode->i_state & I_NEW))
 		return inode;
 
+	if (!path) {
+		path = btrfs_alloc_path();
+		if (!path) {
+			ret = -ENOMEM;
+			goto error;
+		}
+
+	}
+
 	ret = btrfs_read_locked_inode(inode, path);
+
+	if (path != in_path)
+		btrfs_free_path(path);
+
 	/*
 	 * ret > 0 can come from btrfs_search_slot called by
 	 * btrfs_read_locked_inode(), this means the inode item was not found.
-- 
2.43.5

[PATCH v3 2/2] btrfs: move cleanup code to 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.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 109 +++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 56 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 74d23d0cd1eb9..23733cb590633 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,8 +3845,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];
 
@@ -3976,7 +4013,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;
 }
 
 /*
@@ -5488,36 +5534,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,11 +5615,8 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 
 	if (!path) {
 		path = btrfs_alloc_path();
-		if (!path) {
-			ret = -ENOMEM;
-			goto error;
-		}
-
+		if (!path)
+			return ERR_PTR(-ENOMEM);
 	}
 
 	ret = btrfs_read_locked_inode(inode, path);
@@ -5611,25 +5624,9 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 	if (path != in_path)
 		btrfs_free_path(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