Re: jbd2_clear_buffer_revoked_flags() takes a long time

[Jan Kara <jack@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Wed 10-10-18 13:49:34, Theodore Y. Ts'o wrote:
> On Wed, Oct 10, 2018 at 04:43:27PM +0300, Adrian Hunter wrote:
> > Hi
> > 
> > I have a case on a v4.14 kernel where the EXT4 journal commit disables
> > preemption for 30ms due to jbd2_clear_buffer_revoked_flags().  That in turn
> > disables preemption on other CPUs as they come to spin waiting for the same
> > lock.  The side-effect of that is that it periodically blocks high priority
> > tasks from running.
> > 
> > I see jbd2_clear_buffer_revoked_flags() iterating 32768 times calling
> > __find_get_block().
> > 
> > Is there any way to make jbd2_clear_buffer_revoked_flags() take less time,
> > or move its work out from under write_lock(&journal->j_state_lock)?
> 
> Hmm.... I'd have to look a bit more carefully and then run some tests,
> but I *think* we can drop the j_state_lock at the beginning of JBD2
> commit phase 1, and then grab it again right before we set
> commit_transaction->t_state to T_FLUSH.
> 
> That should be safe because while the transaction state is T_LOCKED,
> we can't start any new handles, so there can't be any new blocks added
> to the revoke table.
> 
> Can you give that a try and see whether that solves your priority
> inversion problem?

Agreed. Something like attached patch (compile-tested only)?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-jbd2-Avoid-long-hold-times-of-j_state_lock-while-com.patch --]
[-- Type: text/x-patch, Size: 2286 bytes --]

>From 3627b5f30996504019cd84f326402fccbb9a298b Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Thu, 11 Oct 2018 13:04:44 +0200
Subject: [PATCH] jbd2: Avoid long hold times of j_state_lock while committing
 a transaction

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. Furthermore
the transaction is in T_LOCKED state and we waited for all outstanding
handles so nobody is going to be adding anything to the transaction.

Just drop the lock for unnecessary operations.

Reported-by: Adrian Hunter <adrian.hunter@intel.com>
Suggested-by: "Theodore Y. Ts'o" <tytso@mit.edu>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 150cc030b4d7..356b75fa3101 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -422,6 +422,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 				       stats.run.rs_locked);
 	stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
 					      stats.run.rs_locked);
+	write_unlock(&journal->j_state_lock);
 
 	spin_lock(&commit_transaction->t_handle_lock);
 	while (atomic_read(&commit_transaction->t_updates)) {
@@ -431,9 +432,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 					TASK_UNINTERRUPTIBLE);
 		if (atomic_read(&commit_transaction->t_updates)) {
 			spin_unlock(&commit_transaction->t_handle_lock);
-			write_unlock(&journal->j_state_lock);
 			schedule();
-			write_lock(&journal->j_state_lock);
 			spin_lock(&commit_transaction->t_handle_lock);
 		}
 		finish_wait(&journal->j_wait_updates, &wait);
@@ -505,6 +504,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,
-- 
2.16.4