[PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[Yangtao Li]

Pass path objects from btrfs_del_inode_ref() to
btrfs_del_inode_extref(), which reducing path allocations
and potential failures.

BTW convert to use BTRFS_PATH_AUTO_FREE macro.

Suggested-by: Daniel Vacek <neelx@suse.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/inode-item.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 3530de0618c8..693cd47668eb 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -105,11 +105,11 @@ btrfs_lookup_inode_extref(struct btrfs_trans_handle *trans,
 
 static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 				  struct btrfs_root *root,
+				  struct btrfs_path *path,
 				  const struct fscrypt_str *name,
 				  u64 inode_objectid, u64 ref_objectid,
 				  u64 *index)
 {
-	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_inode_extref *extref;
 	struct extent_buffer *leaf;
@@ -123,15 +123,11 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 	key.type = BTRFS_INODE_EXTREF_KEY;
 	key.offset = btrfs_extref_hash(ref_objectid, name->name, name->len);
 
-	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
-
 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
 	if (ret > 0)
 		ret = -ENOENT;
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	/*
 	 * Sanity check - did we find the right item for this name?
@@ -142,8 +138,7 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 						ref_objectid, name);
 	if (!extref) {
 		btrfs_abort_transaction(trans, -ENOENT);
-		ret = -ENOENT;
-		goto out;
+		return -ENOENT;
 	}
 
 	leaf = path->nodes[0];
@@ -156,8 +151,7 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 		 * Common case only one ref in the item, remove the
 		 * whole item.
 		 */
-		ret = btrfs_del_item(trans, root, path);
-		goto out;
+		return btrfs_del_item(trans, root, path);
 	}
 
 	ptr = (unsigned long)extref;
@@ -168,17 +162,14 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 
 	btrfs_truncate_item(trans, path, item_size - del_len, 1);
 
-out:
-	btrfs_free_path(path);
-
-	return ret;
+	return 0;
 }
 
 int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
 			struct btrfs_root *root, const struct fscrypt_str *name,
 			u64 inode_objectid, u64 ref_objectid, u64 *index)
 {
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	struct btrfs_inode_ref *ref;
 	struct extent_buffer *leaf;
@@ -230,7 +221,7 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
 			      item_size - (ptr + sub_item_len - item_start));
 	btrfs_truncate_item(trans, path, item_size - sub_item_len, 1);
 out:
-	btrfs_free_path(path);
+	btrfs_release_path(path);
 
 	if (search_ext_refs) {
 		/*
@@ -238,7 +229,7 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
 		 * name in our ref array. Find and remove the extended
 		 * inode ref then.
 		 */
-		return btrfs_del_inode_extref(trans, root, name,
+		return btrfs_del_inode_extref(trans, root, path, name,
 					      inode_objectid, ref_objectid, index);
 	}
 
-- 
2.39.0

[PATCH 2/3] btrfs: get rid of path allocation in btrfs_insert_inode_extref()

[Yangtao Li]

Pass path objects from btrfs_insert_inode_ref() to
btrfs_insert_inode_extref(), which reducing path allocations
and potential failures.

BTW convert to use BTRFS_PATH_AUTO_FREE macro.

Suggested-by: Daniel Vacek <neelx@suse.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/inode-item.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 693cd47668eb..ff775dfbe6b7 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -243,6 +243,7 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
  */
 static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 				     struct btrfs_root *root,
+				     struct btrfs_path *path,
 				     const struct fscrypt_str *name,
 				     u64 inode_objectid, u64 ref_objectid,
 				     u64 index)
@@ -251,7 +252,6 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 	int ret;
 	int ins_len = name->len + sizeof(*extref);
 	unsigned long ptr;
-	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct extent_buffer *leaf;
 
@@ -259,10 +259,6 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 	key.type = BTRFS_INODE_EXTREF_KEY;
 	key.offset = btrfs_extref_hash(ref_objectid, name->name, name->len);
 
-	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
-
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      ins_len);
 	if (ret == -EEXIST) {
@@ -270,13 +266,13 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 						   path->slots[0],
 						   ref_objectid,
 						   name))
-			goto out;
+			return ret;
 
 		btrfs_extend_item(trans, path, ins_len);
 		ret = 0;
 	}
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	leaf = path->nodes[0];
 	ptr = (unsigned long)btrfs_item_ptr(leaf, path->slots[0], char);
@@ -289,9 +285,8 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 
 	ptr = (unsigned long)&extref->name;
 	write_extent_buffer(path->nodes[0], name->name, ptr, name->len);
-out:
-	btrfs_free_path(path);
-	return ret;
+
+	return 0;
 }
 
 /* Will return 0, -ENOMEM, -EMLINK, or -EEXIST or anything from the CoW path */
@@ -300,7 +295,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans,
 			   u64 inode_objectid, u64 ref_objectid, u64 index)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	struct btrfs_inode_ref *ref;
 	unsigned long ptr;
@@ -353,7 +348,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans,
 	}
 	write_extent_buffer(path->nodes[0], name->name, ptr, name->len);
 out:
-	btrfs_free_path(path);
+	btrfs_release_path(path);
 
 	if (ret == -EMLINK) {
 		struct btrfs_super_block *disk_super = fs_info->super_copy;
@@ -361,7 +356,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans,
 		 * add an extended ref. */
 		if (btrfs_super_incompat_flags(disk_super)
 		    & BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF)
-			ret = btrfs_insert_inode_extref(trans, root, name,
+			ret = btrfs_insert_inode_extref(trans, root, path, name,
 							inode_objectid,
 							ref_objectid, index);
 	}
-- 
2.39.0

[PATCH 3/3] btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_truncate_inode_items()

[Yangtao Li]

All cleanup paths lead to btrfs_path_free so we can define path with the
automatic free callback.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/inode-item.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index ff775dfbe6b7..c702597174f7 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -442,7 +442,7 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
 			       struct btrfs_truncate_control *control)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_file_extent_item *fi;
 	struct btrfs_key key;
@@ -729,6 +729,5 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
 	if (!ret && control->last_size > new_size)
 		control->last_size = new_size;
 
-	btrfs_free_path(path);
 	return ret;
 }
-- 
2.39.0

Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[Sun YangKai]

It seems a nice try to reduce path allocation and improve performance.

But it also seems make the code less maintainable. I would prefer to  have a 
comment saying something like the @path argument is just for reuse the 
btrfs_path allocation and only a released or empty btrfs_path should be used 
here.

Also, although the path passed is released, it seems the bit flags are still 
passed, which makes the behavior of the functions a little different. But it 
seems fine since those bit flags are never set in this code path.

Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[David Sterba]

On Tue, Apr 15, 2025 at 10:45:06PM +0800, Sun YangKai wrote:
> It seems a nice try to reduce path allocation and improve performance.
> 
> But it also seems make the code less maintainable. I would prefer to  have a 
> comment saying something like the @path argument is just for reuse the 
> btrfs_path allocation and only a released or empty btrfs_path should be used 
> here.

Yes, this should be there, though we use the pattern of passing existing
path to functions so this within what'd consider OK.

> Also, although the path passed is released, it seems the bit flags are still 
> passed, which makes the behavior of the functions a little different. But it 
> seems fine since those bit flags are never set in this code path.

Also a good point, the path should be in a pristine state, as if it were
just allocated. Releasing paths in other functions may want to keep the
bits but in this case we're crossing a function boundary and the same
assumptions may not be the same.

Release resets the ->nodes, so what's left is from ->slots until the the
end of the structure. And a helper for that would be desirable rather
than opencoding that.

回复: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[李扬韬]

> Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.

> Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.

IIUC, use btrfs_reset_path instead of btrfs_release_path?

noinline void btrfs_reset_path(struct btrfs_path *p)
{
        int i;

        for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
                if (!p->nodes[i])
                        continue;
                if (p->locks[i])
                        btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
                free_extent_buffer(p->nodes[i]);
        }
        memset(p, 0, sizeof(struct btrfs_path));
}

BTW, I have seen released paths being passed across functions in some other paths.

Should these also be changed to reset paths, or should these flags be cleared in the release path?

Thx,
Yangtao

Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[Filipe Manana]

On Wed, Apr 16, 2025 at 2:24 PM 李扬韬 <frank.li@vivo.com> wrote:
>
>
>
> > Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.
>
> > Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.
>
> IIUC, use btrfs_reset_path instead of btrfs_release_path?
>
> noinline void btrfs_reset_path(struct btrfs_path *p)
> {
>         int i;
>
>         for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
>                 if (!p->nodes[i])
>                         continue;
>                 if (p->locks[i])
>                         btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
>                 free_extent_buffer(p->nodes[i]);
>         }
>         memset(p, 0, sizeof(struct btrfs_path));
> }
>
> BTW, I have seen released paths being passed across functions in some other paths.
>
> Should these also be changed to reset paths, or should these flags be cleared in the release path?

Please don't complicate things unnecessarily.
The patch is fine, all that needs to be done is to call
btrfs_release_path() before passing the path to
btrfs_del_inode_extref(), which resets nodes, slots and locks.

>
> Thx,
> Yangtao

Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[David Sterba]

On Wed, Apr 16, 2025 at 02:37:33PM +0100, Filipe Manana wrote:
> On Wed, Apr 16, 2025 at 2:24 PM 李扬韬 <frank.li@vivo.com> wrote:
> >
> >
> >
> > > Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.
> >
> > > Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.
> >
> > IIUC, use btrfs_reset_path instead of btrfs_release_path?
> >
> > noinline void btrfs_reset_path(struct btrfs_path *p)
> > {
> >         int i;
> >
> >         for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
> >                 if (!p->nodes[i])
> >                         continue;
> >                 if (p->locks[i])
> >                         btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
> >                 free_extent_buffer(p->nodes[i]);
> >         }
> >         memset(p, 0, sizeof(struct btrfs_path));
> > }
> >
> > BTW, I have seen released paths being passed across functions in some other paths.
> >
> > Should these also be changed to reset paths, or should these flags be cleared in the release path?
> 
> Please don't complicate things unnecessarily.
> The patch is fine, all that needs to be done is to call
> btrfs_release_path() before passing the path to
> btrfs_del_inode_extref(), which resets nodes, slots and locks.

But this leaves the bits set, btrfs_insert_inode_ref() sets
path->skip_release_on_error, this should be reset. In this case it may
not be significant but I'd rather make the path reusing pattern correct
from the beginning.

My idea was to add only

btrfs_reset_path() {
        memset(p, 0, sizeof(struct btrfs_path));
}

and use it in conection with btrfs_release_path() only in case it's
optimizing the allocation.

Re: 回复: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[David Sterba]

On Wed, Apr 16, 2025 at 01:24:30PM +0000, 李扬韬 wrote:
> 
> 
> > Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.
> 
> > Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.
> 
> IIUC, use btrfs_reset_path instead of btrfs_release_path?
> 
> noinline void btrfs_reset_path(struct btrfs_path *p)
> {
>         int i;
> 
>         for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
>                 if (!p->nodes[i])
>                         continue;
>                 if (p->locks[i])
>                         btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
>                 free_extent_buffer(p->nodes[i]);
>         }
>         memset(p, 0, sizeof(struct btrfs_path));
> }
> 
> BTW, I have seen released paths being passed across functions in some other paths.
> 
> Should these also be changed to reset paths, or should these flags be cleared in the release path?

No, a path may be passed among several functions but the path bits may
be set up in a particular way and must be preserved. E.g. if the first
caller sets up path to search commit root the next call expect this bit
to be set as well so clearing it would be a bug.

Example is resolve_indirect_ref(), resolve_indirect_refs() and
find_parent_nodes() that set skip_locks and search_commit_root.

Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[Filipe Manana]

On Wed, Apr 16, 2025 at 8:11 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Apr 16, 2025 at 02:37:33PM +0100, Filipe Manana wrote:
> > On Wed, Apr 16, 2025 at 2:24 PM 李扬韬 <frank.li@vivo.com> wrote:
> > >
> > >
> > >
> > > > Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.
> > >
> > > > Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.
> > >
> > > IIUC, use btrfs_reset_path instead of btrfs_release_path?
> > >
> > > noinline void btrfs_reset_path(struct btrfs_path *p)
> > > {
> > >         int i;
> > >
> > >         for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
> > >                 if (!p->nodes[i])
> > >                         continue;
> > >                 if (p->locks[i])
> > >                         btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
> > >                 free_extent_buffer(p->nodes[i]);
> > >         }
> > >         memset(p, 0, sizeof(struct btrfs_path));
> > > }
> > >
> > > BTW, I have seen released paths being passed across functions in some other paths.
> > >
> > > Should these also be changed to reset paths, or should these flags be cleared in the release path?
> >
> > Please don't complicate things unnecessarily.
> > The patch is fine, all that needs to be done is to call
> > btrfs_release_path() before passing the path to
> > btrfs_del_inode_extref(), which resets nodes, slots and locks.
>
> But this leaves the bits set, btrfs_insert_inode_ref() sets
> path->skip_release_on_error, this should be reset. In this case it may
> not be significant but I'd rather make the path reusing pattern correct
> from the beginning.
>
> My idea was to add only
>
> btrfs_reset_path() {
>         memset(p, 0, sizeof(struct btrfs_path));
> }
>
> and use it in conection with btrfs_release_path() only in case it's
> optimizing the allocation.

Honestly I don't like adding yet another function to do such "reset" thing.

Leaving path->skip_release_on_error is perfectly fine in this scenario.
If that bothers anyone so much, just set path->skip_release_on_error
to 0 after calling btrfs_release_path() and before passing the path to
btrfs_insert_inode_extref().

This is the sort of optimization that is not worth spending this much
time and adding new APIs - freeing and allocating a path shortly after
is almost always fast as we're using a slab, plus this is a rarely hit
use case - having to use extrefs, meaning we have a very large number
of inode refs.

回复: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[李扬韬]

> Honestly I don't like adding yet another function to do such "reset" thing.
>
> Leaving path->skip_release_on_error is perfectly fine in this scenario.
> If that bothers anyone so much, just set path->skip_release_on_error to 0 after calling btrfs_release_path() and before passing the path to btrfs_insert_inode_extref().
>
> This is the sort of optimization that is not worth spending this much time and adding new APIs - freeing and allocating a path shortly after is almost always fast as we're using a slab, plus this is a rarely hit use case - having to use extrefs, meaning we have a very large number of inode refs.

I am fine to add btrfs_reset_path or just clear path->skip_release_on_error.

David, what do you think?

Thx,
Yangtao

Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

[Filipe Manana]

On Thu, Apr 17, 2025 at 3:15 PM 李扬韬 <frank.li@vivo.com> wrote:
>
> > Honestly I don't like adding yet another function to do such "reset" thing.
> >
> > Leaving path->skip_release_on_error is perfectly fine in this scenario.
> > If that bothers anyone so much, just set path->skip_release_on_error to 0 after calling btrfs_release_path() and before passing the path to btrfs_insert_inode_extref().
> >
> > This is the sort of optimization that is not worth spending this much time and adding new APIs - freeing and allocating a path shortly after is almost always fast as we're using a slab, plus this is a rarely hit use case - having to use extrefs, meaning we have a very large number of inode refs.
>
> I am fine to add btrfs_reset_path or just clear path->skip_release_on_error.

For god's sake, just keep it simple - either do nothing or set
path->skip_release_on_error to 0.

>
> David, what do you think?
>
> Thx,
> Yangtao