[PATCH] btrfs: avoid unnecessary ref initialization when freeing log tree block

[PATCH] btrfs: avoid unnecessary ref initialization when freeing log tree block

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At btrfs_free_tree_block(), we are always initializing a delayed reference
to drop the given extent buffer but we only use if it does not belong to a
log root tree. So we are doing unnecessary work here and increasing the
duration of a critical section as this is normally called while holding a
lock on the parent tree block (if any) and while holding a log transaction
open.

So initialize the delayed reference only if the extent buffer is not from
a log tree, avoiding unnecessary work and making the code also a bit
easier to follow.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent-tree.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 0d72d0f7cefc..beedd6ed64d3 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3458,16 +3458,17 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 			   u64 parent, int last_ref)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
-	struct btrfs_ref generic_ref = { 0 };
 	struct btrfs_block_group *bg;
 	int ret;
 
-	btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
-			       buf->start, buf->len, parent, btrfs_header_owner(buf));
-	btrfs_init_tree_ref(&generic_ref, btrfs_header_level(buf),
-			    root_id, 0, false);
-
 	if (root_id != BTRFS_TREE_LOG_OBJECTID) {
+		struct btrfs_ref generic_ref = { 0 };
+
+		btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
+				       buf->start, buf->len, parent,
+				       btrfs_header_owner(buf));
+		btrfs_init_tree_ref(&generic_ref, btrfs_header_level(buf),
+				    root_id, 0, false);
 		btrfs_ref_tree_mod(fs_info, &generic_ref);
 		ret = btrfs_add_delayed_tree_ref(trans, &generic_ref, NULL);
 		BUG_ON(ret); /* -ENOMEM */
-- 
2.40.1

Re: [PATCH] btrfs: avoid unnecessary ref initialization when freeing log tree block

[David Sterba]

On Mon, Feb 19, 2024 at 01:00:22PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At btrfs_free_tree_block(), we are always initializing a delayed reference
> to drop the given extent buffer but we only use if it does not belong to a
> log root tree. So we are doing unnecessary work here and increasing the
> duration of a critical section as this is normally called while holding a
> lock on the parent tree block (if any) and while holding a log transaction
> open.
> 
> So initialize the delayed reference only if the extent buffer is not from
> a log tree, avoiding unnecessary work and making the code also a bit
> easier to follow.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH] btrfs: avoid unnecessary ref initialization when freeing log tree block

[Naohiro Aota]

On Mon, Feb 19, 2024 at 01:00:22PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At btrfs_free_tree_block(), we are always initializing a delayed reference
> to drop the given extent buffer but we only use if it does not belong to a
> log root tree. So we are doing unnecessary work here and increasing the
> duration of a critical section as this is normally called while holding a
> lock on the parent tree block (if any) and while holding a log transaction
> open.
> 
> So initialize the delayed reference only if the extent buffer is not from
> a log tree, avoiding unnecessary work and making the code also a bit
> easier to follow.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Exactly what I also thought while debugging around this function.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>