[PATCH] Btrfs: simpler and more efficient cleanup of a log tree's extent io tree

[PATCH] Btrfs: simpler and more efficient cleanup of a log tree's extent io tree

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We currently are in a loop finding each range (corresponding to a btree
node/leaf) in a log root's extent io tree and then clean it up. This is a
waste of time since we are traversing the extent io tree's rb_tree more
times then needed (one for a range lookup and another for cleaning it up)
without any good reason.

We free the log trees when we are in the critical section of a transaction
commit (the transaction state is set to TRANS_STATE_COMMIT_DOING), so it's
of great convenience to do everything as fast as possible in order to
reduce the time we block other tasks from starting a new transaction.

So fix this by traversing the extent io tree once and cleaning up all its
records in one go while traversing it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index d49edd25f2e5..aac3749f697f 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3204,8 +3204,6 @@ static void free_log_tree(struct btrfs_trans_handle *trans,
 			  struct btrfs_root *log)
 {
 	int ret;
-	u64 start;
-	u64 end;
 	struct walk_control wc = {
 		.free = 1,
 		.process_func = process_one_buffer
@@ -3216,18 +3214,8 @@ static void free_log_tree(struct btrfs_trans_handle *trans,
 	if (ret)
 		btrfs_abort_transaction(trans, ret);
 
-	while (1) {
-		ret = find_first_extent_bit(&log->dirty_log_pages,
-				0, &start, &end,
-				EXTENT_DIRTY | EXTENT_NEW | EXTENT_NEED_WAIT,
-				NULL);
-		if (ret)
-			break;
-
-		clear_extent_bits(&log->dirty_log_pages, start, end,
-				  EXTENT_DIRTY | EXTENT_NEW | EXTENT_NEED_WAIT);
-	}
-
+	clear_extent_bits(&log->dirty_log_pages, 0, (u64)-1,
+			  EXTENT_DIRTY | EXTENT_NEW | EXTENT_NEED_WAIT);
 	free_extent_buffer(log->node);
 	kfree(log);
 }
-- 
2.11.0

Re: [PATCH] Btrfs: simpler and more efficient cleanup of a log tree's extent io tree

[Nikolay Borisov]

On 9.11.18 г. 12:43 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We currently are in a loop finding each range (corresponding to a btree
> node/leaf) in a log root's extent io tree and then clean it up. This is a
> waste of time since we are traversing the extent io tree's rb_tree more
> times then needed (one for a range lookup and another for cleaning it up)
> without any good reason.
> 
> We free the log trees when we are in the critical section of a transaction
> commit (the transaction state is set to TRANS_STATE_COMMIT_DOING), so it's
> of great convenience to do everything as fast as possible in order to
> reduce the time we block other tasks from starting a new transaction.
> 
> So fix this by traversing the extent io tree once and cleaning up all its
> records in one go while traversing it.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  fs/btrfs/tree-log.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index d49edd25f2e5..aac3749f697f 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3204,8 +3204,6 @@ static void free_log_tree(struct btrfs_trans_handle *trans,
>  			  struct btrfs_root *log)
>  {
>  	int ret;
> -	u64 start;
> -	u64 end;
>  	struct walk_control wc = {
>  		.free = 1,
>  		.process_func = process_one_buffer
> @@ -3216,18 +3214,8 @@ static void free_log_tree(struct btrfs_trans_handle *trans,
>  	if (ret)
>  		btrfs_abort_transaction(trans, ret);
>  
> -	while (1) {
> -		ret = find_first_extent_bit(&log->dirty_log_pages,
> -				0, &start, &end,
> -				EXTENT_DIRTY | EXTENT_NEW | EXTENT_NEED_WAIT,
> -				NULL);
> -		if (ret)
> -			break;
> -
> -		clear_extent_bits(&log->dirty_log_pages, start, end,
> -				  EXTENT_DIRTY | EXTENT_NEW | EXTENT_NEED_WAIT);
> -	}
> -
> +	clear_extent_bits(&log->dirty_log_pages, 0, (u64)-1,
> +			  EXTENT_DIRTY | EXTENT_NEW | EXTENT_NEED_WAIT);
>  	free_extent_buffer(log->node);
>  	kfree(log);
>  }
> 

Re: [PATCH] Btrfs: simpler and more efficient cleanup of a log tree's extent io tree

[Josef Bacik]

On Fri, Nov 09, 2018 at 10:43:08AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We currently are in a loop finding each range (corresponding to a btree
> node/leaf) in a log root's extent io tree and then clean it up. This is a
> waste of time since we are traversing the extent io tree's rb_tree more
> times then needed (one for a range lookup and another for cleaning it up)
> without any good reason.
> 
> We free the log trees when we are in the critical section of a transaction
> commit (the transaction state is set to TRANS_STATE_COMMIT_DOING), so it's
> of great convenience to do everything as fast as possible in order to
> reduce the time we block other tasks from starting a new transaction.
> 
> So fix this by traversing the extent io tree once and cleaning up all its
> records in one go while traversing it.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Sheesh

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] Btrfs: simpler and more efficient cleanup of a log tree's extent io tree

[David Sterba]

On Fri, Nov 09, 2018 at 10:43:08AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We currently are in a loop finding each range (corresponding to a btree
> node/leaf) in a log root's extent io tree and then clean it up. This is a
> waste of time since we are traversing the extent io tree's rb_tree more
> times then needed (one for a range lookup and another for cleaning it up)
> without any good reason.
> 
> We free the log trees when we are in the critical section of a transaction
> commit (the transaction state is set to TRANS_STATE_COMMIT_DOING), so it's
> of great convenience to do everything as fast as possible in order to
> reduce the time we block other tasks from starting a new transaction.
> 
> So fix this by traversing the extent io tree once and cleaning up all its
> records in one go while traversing it.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.