On 19.07.2018 17:50, Josef Bacik 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.
>
> Signed-off-by: Josef Bacik <jo...@toxicpanda.com>
> ---
> fs/btrfs/transaction.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 4b171d8a7554..39ff9378b3db 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1919,15 +1919,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;
>
> /*
> @@ -1940,12 +1931,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle
> *trans)
> if (!list_empty(&trans->new_bgs))
> 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;
>
> @@ -2016,6 +2001,17 @@ 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) {
> + btrfs_end_transaction(trans);
> + return ret;
> + }
> +
Is this really needed since we already have code which runs delayed refs
in the transaction critical section right after btrfs_run_delayed_items.
I'd rather have a single place in commit_transaction where delayed_refs
are run. If this is needed for correctness reasons then document what
this reason is for otherwise let's just remove it.
> extwriter_counter_dec(cur_trans, trans->type);
>
> ret = btrfs_start_delalloc_flush(fs_info);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
