Re: [PATCH 4/5] btrfs: run delayed refs less often in commit_cowonly_roots

[Nikolay Borisov <nborisov@suse.com>]

On 13.03.20 г. 23:12 ч., Josef Bacik wrote:
> We love running delayed refs in commit_cowonly_roots, but it is a bit

You refer to commit_cowonly_roots but in fact are changing
create_pending_snapshot. Between calling
create_pending_snapshots->create_pending_snapshot and
commit_cowonly_roots we have 2 calls to btrfs_run_delayed_refs. My point
is by the time commit_cowonly_roots is called we've already long flushed
the delayed refs generated from create_pending_snapshot. IMO referring
to commit_cowonly_roots in this commit is wrong?


> excessive.  I was seeing cases of running 3 or 4 refs a few times in a
> row during this time.  Instead simply update all of the roots first,
By "simply update all of the roots" I assume you meant the call to
commit_fs_roots or the changes happening to the snapshoted roots in
create_pending_snapshot? If it's the latter I'd rather you made the text
more explicit by referring to the fact the refs are generated from
snapshots e.g.

Instead simply run all snapshot-related work, then drain delayed refs
resulting from this ...

> then run delayed refs, then handle the empty block groups case, and then
> if we have any more dirty roots do the whole thing again.  This allows
> us to be much more efficient with our delayed ref running, as we can
> batch a few more operations at once.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/transaction.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 3e7fd8a934c1..c3b3b524b8c3 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1646,12 +1646,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>  		goto fail;
>  	}
>  
> -	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto fail;
> -	}
> -
>  	/*
>  	 * Do special qgroup accounting for snapshot, as we do some qgroup
>  	 * snapshot hack to do fast snapshot.
> @@ -1698,12 +1692,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>  		}
>  	}
>  
> -	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto fail;
> -	}
> -
>  fail:
>  	pending->error = ret;
>  dir_item_existed:
>