Date:   Tue, 27 Nov 2018 11:29:39 +0100
From:   Jan Kara <jack@...e.cz>
To:     Ted Tso <tytso@....edu>
Cc:     linux-ext4@...r.kernel.org,
        "Hunter, Adrian" <adrian.hunter@...el.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v2] jbd2: Avoid long hold times of j_state_lock while
 committing a transaction

On Thu 08-11-18 13:29:38, Jan Kara wrote:
> We can hold j_state_lock for writing at the beginning of
> jbd2_journal_commit_transaction() for a rather long time (reportedly for
> 30 ms) due cleaning revoke bits of all revoked buffers under it. The
> handling of revoke tables as well as cleaning of t_reserved_list, and
> checkpoint lists does not need j_state_lock for anything. It is only
> needed to prevent new handles from joining the transaction. Generally
> T_LOCKED transaction state prevents new handles from joining the
> transaction - except for reserved handles which have to allowed to join
> while we wait for other handles to complete.
> 
> To prevent reserved handles from joining the transaction while cleaning
> up lists, add new transaction state T_SWITCH and watch for it when
> starting reserved handles. With this we can just drop the lock for
> operations that don't need it.
> 
> Reported-and-tested-by: Adrian Hunter <adrian.hunter@...el.com>
> Suggested-by: "Theodore Y. Ts'o" <tytso@....edu>
> Signed-off-by: Jan Kara <jack@...e.cz>

Ping?

								Honza

> ---
>  fs/jbd2/commit.c      |  3 +++
>  fs/jbd2/transaction.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>  include/linux/jbd2.h  |  1 +
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> Changes since v1:
> - fixed handling of reserved handles
> - rebased on top of 4.20-rc1
> - tested dioread-nolock xfstest run
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 150cc030b4d7..2eb55c3361a8 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -439,6 +439,8 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		finish_wait(&journal->j_wait_updates, &wait);
>  	}
>  	spin_unlock(&commit_transaction->t_handle_lock);
> +	commit_transaction->t_state = T_SWITCH;
> +	write_unlock(&journal->j_state_lock);
>  
>  	J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <=
>  			journal->j_max_transaction_buffers);
> @@ -505,6 +507,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	atomic_sub(atomic_read(&journal->j_reserved_credits),
>  		   &commit_transaction->t_outstanding_credits);
>  
> +	write_lock(&journal->j_state_lock);
>  	trace_jbd2_commit_flushing(journal, commit_transaction);
>  	stats.run.rs_flushing = jiffies;
>  	stats.run.rs_locked = jbd2_time_diff(stats.run.rs_locked,
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index c0b66a7a795b..116d8251fbff 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -138,9 +138,9 @@ static inline void update_t_max_wait(transaction_t *transaction,
>  }
>  
>  /*
> - * Wait until running transaction passes T_LOCKED state. Also starts the commit
> - * if needed. The function expects running transaction to exist and releases
> - * j_state_lock.
> + * Wait until running transaction passes to T_FLUSH state and new transaction
> + * can thus be started. Also starts the commit if needed. The function expects
> + * running transaction to exist and releases j_state_lock.
>   */
>  static void wait_transaction_locked(journal_t *journal)
>  	__releases(journal->j_state_lock)
> @@ -160,6 +160,32 @@ static void wait_transaction_locked(journal_t *journal)
>  	finish_wait(&journal->j_wait_transaction_locked, &wait);
>  }
>  
> +/*
> + * Wait until running transaction transitions from T_SWITCH to T_FLUSH
> + * state and new transaction can thus be started. The function releases
> + * j_state_lock.
> + */
> +static void wait_transaction_switching(journal_t *journal)
> +	__releases(journal->j_state_lock)
> +{
> +	DEFINE_WAIT(wait);
> +
> +	if (WARN_ON(!journal->j_running_transaction ||
> +		    journal->j_running_transaction->t_state != T_SWITCH))
> +		return;
> +	prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
> +			TASK_UNINTERRUPTIBLE);
> +	read_unlock(&journal->j_state_lock);
> +	/*
> +	 * We don't call jbd2_might_wait_for_commit() here as there's no
> +	 * waiting for outstanding handles happening anymore in T_SWITCH state
> +	 * and handling of reserved handles actually relies on that for
> +	 * correctness.
> +	 */
> +	schedule();
> +	finish_wait(&journal->j_wait_transaction_locked, &wait);
> +}
> +
>  static void sub_reserved_credits(journal_t *journal, int blocks)
>  {
>  	atomic_sub(blocks, &journal->j_reserved_credits);
> @@ -183,7 +209,8 @@ static int add_transaction_credits(journal_t *journal, int blocks,
>  	 * If the current transaction is locked down for commit, wait
>  	 * for the lock to be released.
>  	 */
> -	if (t->t_state == T_LOCKED) {
> +	if (t->t_state != T_RUNNING) {
> +		WARN_ON_ONCE(t->t_state >= T_FLUSH);
>  		wait_transaction_locked(journal);
>  		return 1;
>  	}
> @@ -360,8 +387,14 @@ static int start_this_handle(journal_t *journal, handle_t *handle,
>  		/*
>  		 * We have handle reserved so we are allowed to join T_LOCKED
>  		 * transaction and we don't have to check for transaction size
> -		 * and journal space.
> +		 * and journal space. But we still have to wait while running
> +		 * transaction is being switched to a committing one as it
> +		 * won't wait for any handles anymore.
>  		 */
> +		if (transaction->t_state == T_SWITCH) {
> +			wait_transaction_switching(journal);
> +			goto repeat;
> +		}
>  		sub_reserved_credits(journal, blocks);
>  		handle->h_reserved = 0;
>  	}
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index b708e5169d1d..118d00a64184 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -575,6 +575,7 @@ struct transaction_s
>  	enum {
>  		T_RUNNING,
>  		T_LOCKED,
> +		T_SWITCH,
>  		T_FLUSH,
>  		T_COMMIT,
>  		T_COMMIT_DFLUSH,
> -- 
> 2.16.4
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists