* [PATCH v4 1/2] btrfs: push cleanup into read_locked_inode
2024-08-30 20:24 [PATCH v4 0/2] btrfs: iget_path cleanup Leo Martins
@ 2024-08-30 20:24 ` Leo Martins
2024-08-30 20:24 ` [PATCH v4 2/2] btrfs: remove conditional path allocation Leo Martins
2024-10-29 23:53 ` [PATCH v4 0/2] btrfs: iget_path cleanup David Sterba
2 siblings, 0 replies; 4+ messages in thread
From: Leo Martins @ 2024-08-30 20:24 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v4 2/2] btrfs: remove conditional path allocation
2024-08-30 20:24 [PATCH v4 0/2] btrfs: iget_path cleanup Leo Martins
2024-08-30 20:24 ` [PATCH v4 1/2] btrfs: push cleanup into read_locked_inode Leo Martins
@ 2024-08-30 20:24 ` Leo Martins
2024-10-29 23:53 ` [PATCH v4 0/2] btrfs: iget_path cleanup David Sterba
2 siblings, 0 replies; 4+ messages in thread
From: Leo Martins @ 2024-08-30 20:24 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4 0/2] btrfs: iget_path cleanup
2024-08-30 20:24 [PATCH v4 0/2] btrfs: iget_path cleanup Leo Martins
2024-08-30 20:24 ` [PATCH v4 1/2] btrfs: push cleanup into read_locked_inode Leo Martins
2024-08-30 20:24 ` [PATCH v4 2/2] btrfs: remove conditional path allocation Leo Martins
@ 2024-10-29 23:53 ` David Sterba
2 siblings, 0 replies; 4+ messages in thread
From: David Sterba @ 2024-10-29 23:53 UTC (permalink / raw)
To: Leo Martins; +Cc: linux-btrfs, kernel-team
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.
^ permalink raw reply [flat|nested] 4+ messages in thread