On Thu, Nov 22, 2018 at 12:35 AM Josef Bacik <[email protected]> wrote:
>
> I noticed in a giant dbench run that we spent a lot of time on lock
> contention while running transaction commit. This is because dbench
> results in a lot of fsync()'s that do a btrfs_transaction_commit(), and
> they all run the delayed refs first thing, so they all contend with
> each other. This leads to seconds of 0 throughput. Change this to only
> run the delayed refs if we're the ones committing the transaction. This
> makes the latency go away and we get no more lock contention.
Can you share the following in the changelog please?
1) How did you ran dbench (parameters, config).
2) What results did you get before and after this change. So that we all get
an idea of how good the impact is.
While the reduced contention makes all sense and seems valid, I'm not
sure this is always a win.
It certainly is when multiple tasks are calling
btrfs_commit_transaction() simultaneously, but,
what about when only one does it?
By running all delayed references inside the critical section of the
transaction commit
(state == TRANS_STATE_COMMIT_START), instead of running most of them
outside/before,
we will be blocking for a longer a time other tasks calling
btrfs_start_transaction() (which is used
a lot - creating files, unlinking files, adding links, etc, and even fsync).
Won't there by any other types of workload and tests other then dbench
that can get increased
latency and/or smaller throughput?
I find that sort of information useful to have in the changelog. If
you verified that or you think
it's irrelevant to measure/consider, it would be great to have it
mentioned in the changelog
(and explained).
Thanks.
>
> Reviewed-by: Omar Sandoval <[email protected]>
> Signed-off-by: Josef Bacik <[email protected]>
> ---
> fs/btrfs/transaction.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 3c1be9db897c..41cc96cc59a3 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1918,15 +1918,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle
> *trans)
> btrfs_trans_release_metadata(trans);
> trans->block_rsv = NULL;
>
> - /* make a pass through all the delayed refs we have so far
> - * any runnings procs may add more while we are here
> - */
> - ret = btrfs_run_delayed_refs(trans, 0);
> - if (ret) {
> - btrfs_end_transaction(trans);
> - return ret;
> - }
> -
> cur_trans = trans->transaction;
>
> /*
> @@ -1938,12 +1929,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle
> *trans)
>
> btrfs_create_pending_block_groups(trans);
>
> - ret = btrfs_run_delayed_refs(trans, 0);
> - if (ret) {
> - btrfs_end_transaction(trans);
> - return ret;
> - }
> -
> if (!test_bit(BTRFS_TRANS_DIRTY_BG_RUN, &cur_trans->flags)) {
> int run_it = 0;
>
> @@ -2014,6 +1999,15 @@ int btrfs_commit_transaction(struct btrfs_trans_handle
> *trans)
> spin_unlock(&fs_info->trans_lock);
> }
>
> + /*
> + * We are now the only one in the commit area, we can run delayed refs
> + * without hitting a bunch of lock contention from a lot of people
> + * trying to commit the transaction at once.
> + */
> + ret = btrfs_run_delayed_refs(trans, 0);
> + if (ret)
> + goto cleanup_transaction;
> +
> extwriter_counter_dec(cur_trans, trans->type);
>
> ret = btrfs_start_delalloc_flush(fs_info);
> --
> 2.14.3
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
