| 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
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(-)