linux-ext4 - Re: [PATCH 2/2] jbd2: commit as soon as possible after log_start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 2 Feb 2013 12:33:43 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	Theodore Ts'o <tytso@....edu>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	Eric Sandeen <sandeen@...hat.com>,
	Sedat Dilek <sedat.dilek@...il.com>, mszeredi@...e.cz
Subject: Re: [PATCH 2/2] jbd2: commit as soon as possible after log_start_commit

On 2013-01-29, at 10:39 PM, Theodore Ts'o wrote:
> Once a transaction has been requested to be committed, don't let any
> other handles start under that transaction, and don't allow any
> pending transactions to be extended (i.e., in the case of
> unlink/ftruncate).
> 
> The idea is once the transaction has had log_start_commit() called on
> it, at least one thread is blocked waiting for that transaction to
> commit, and over time, more and more threads will end up getting
> blocked.  In order to avoid high variability in file system operations
> getting blocked behind the a blocked start_this_handle(), we should
> try to get the commit started as soon as possible.

I was thinking this could break transaction batching from multiple
threads, which would hurt performance significantly in a multi-
threaded sync-heavy workload.

However, it seems that jbd2_journal_stop() blocks the h_sync thread
before it calls jbd2_log_start_commit(), so it looks like it should
be OK.

You can add my:

Acked-by: Andreas Dilger <adilger@...ger.ca>

> 
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
> Cc: Eric Sandeen <sandeen@...hat.com>
> Cc: Sedat Dilek <sedat.dilek@...il.com>
> Cc: mszeredi@...e.cz
> ---
> fs/jbd2/commit.c      | 3 ++-
> fs/jbd2/journal.c     | 1 +
> fs/jbd2/transaction.c | 6 ++++--
> include/linux/jbd2.h  | 1 +
> 4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 3091d42..fd2ac94 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -424,7 +424,8 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> 	J_ASSERT(journal->j_committing_transaction == NULL);
> 
> 	commit_transaction = journal->j_running_transaction;
> -	J_ASSERT(commit_transaction->t_state == T_RUNNING);
> +	J_ASSERT(commit_transaction->t_state == T_REQUESTED ||
> +		 commit_transaction->t_state == T_RUNNING);
> 
> 	trace_jbd2_start_commit(journal, commit_transaction);
> 	jbd_debug(1, "JBD2: starting commit of transaction %d\n",
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 1a80e31..c22773b 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -533,6 +533,7 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
> 		jbd_debug(1, "JBD2: requesting commit %d/%d\n",
> 			  journal->j_commit_request,
> 			  journal->j_commit_sequence);
> +		journal->j_running_transaction->t_state = T_REQUESTED;
> 		wake_up(&journal->j_wait_commit);
> 		return 1;
> 	} else if (!tid_geq(journal->j_commit_request, target))
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index df9f297..6daff29 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -224,7 +224,8 @@ repeat:
> 	 * If the current transaction is locked down for commit, wait for the
> 	 * lock to be released.
> 	 */
> -	if (transaction->t_state == T_LOCKED) {
> +	if ((transaction->t_state == T_LOCKED) ||
> +	    (transaction->t_state == T_REQUESTED)) {
> 		DEFINE_WAIT(wait);
> 
> 		prepare_to_wait(&journal->j_wait_transaction_locked,
> @@ -2179,7 +2180,8 @@ void __jbd2_journal_refile_buffer(struct journal_head *jh)
> 	else
> 		jlist = BJ_Reserved;
> 	__jbd2_journal_file_buffer(jh, jh->b_transaction, jlist);
> -	J_ASSERT_JH(jh, jh->b_transaction->t_state == T_RUNNING);
> +	J_ASSERT_JH(jh, (jh->b_transaction->t_state == T_RUNNING ||
> +			 jh->b_transaction->t_state == T_REQUESTED));
> 
> 	if (was_dirty)
> 		set_buffer_jbddirty(bh);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index e30b663..920a8d0 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -493,6 +493,7 @@ struct transaction_s
> 	 */
> 	enum {
> 		T_RUNNING,
> +		T_REQUESTED,
> 		T_LOCKED,
> 		T_FLUSH,
> 		T_COMMIT,
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html