| Message ID | 20250415033854.848776-1-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref() | expand |
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.
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.
> 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
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
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.
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.
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.
> 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
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
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); }
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(-)