We catch an assert problem in jbd2_journal_commit_transaction() when
doing fsstress and request falut injection tests. The problem is
happened in a race condition between jbd2_journal_commit_transaction()
and ext4_end_io_end(). Firstly, ext4_writepages() writeback dirty pages
and start reserved handle, and then the journal was aborted due to some
previous metadata IO error, jbd2_journal_abort() start to commit current
running transaction, the committing procedure could be raced by
ext4_end_io_end() and lead to subtract j_reserved_credits twice from
commit_transaction->t_outstanding_credits, finally the
t_outstanding_credits is mistakenly smaller than t_nr_buffers and
trigger assert.
kjournald2 kworker
jbd2_journal_commit_transaction()
write_unlock(&journal->j_state_lock);
atomic_sub(j_reserved_credits, t_outstanding_credits); //sub once
jbd2_journal_start_reserved()
start_this_handle() //detect aborted journal
jbd2_journal_free_reserved() //get running transaction
read_lock(&journal->j_state_lock)
__jbd2_journal_unreserve_handle()
atomic_sub(j_reserved_credits, t_outstanding_credits);
//sub again
read_unlock(&journal->j_state_lock);
journal->j_running_transaction = NULL;
J_ASSERT(t_nr_buffers <= t_outstanding_credits) //bomb!!!
Fix this issue by using journal->j_state_lock to protect the subtraction
in jbd2_journal_commit_transaction().
Fixes: 96f1e0974575 ("jbd2: avoid long hold times of j_state_lock while committing a transaction")
Signed-off-by: Zhang Yi <[email protected]>
---
fs/jbd2/commit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index eb315e81f1a6..af1a9191368c 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -553,13 +553,13 @@ void jbd2_journal_commit_transaction(journal_t *journal)
*/
jbd2_journal_switch_revoke_table(journal);
+ write_lock(&journal->j_state_lock);
/*
* Reserved credits cannot be claimed anymore, free them
*/
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.31.1
On Sat 11-06-22 21:04:26, Zhang Yi wrote:
> We catch an assert problem in jbd2_journal_commit_transaction() when
> doing fsstress and request falut injection tests. The problem is
> happened in a race condition between jbd2_journal_commit_transaction()
> and ext4_end_io_end(). Firstly, ext4_writepages() writeback dirty pages
> and start reserved handle, and then the journal was aborted due to some
> previous metadata IO error, jbd2_journal_abort() start to commit current
> running transaction, the committing procedure could be raced by
> ext4_end_io_end() and lead to subtract j_reserved_credits twice from
> commit_transaction->t_outstanding_credits, finally the
> t_outstanding_credits is mistakenly smaller than t_nr_buffers and
> trigger assert.
>
> kjournald2 kworker
>
> jbd2_journal_commit_transaction()
> write_unlock(&journal->j_state_lock);
> atomic_sub(j_reserved_credits, t_outstanding_credits); //sub once
>
> jbd2_journal_start_reserved()
> start_this_handle() //detect aborted journal
> jbd2_journal_free_reserved() //get running transaction
> read_lock(&journal->j_state_lock)
> __jbd2_journal_unreserve_handle()
> atomic_sub(j_reserved_credits, t_outstanding_credits);
> //sub again
> read_unlock(&journal->j_state_lock);
>
> journal->j_running_transaction = NULL;
> J_ASSERT(t_nr_buffers <= t_outstanding_credits) //bomb!!!
>
> Fix this issue by using journal->j_state_lock to protect the subtraction
> in jbd2_journal_commit_transaction().
>
> Fixes: 96f1e0974575 ("jbd2: avoid long hold times of j_state_lock while committing a transaction")
> Signed-off-by: Zhang Yi <[email protected]>
Thanks for the analysis and the fix! This is indeed subtle. This fix looks
good to me. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/jbd2/commit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index eb315e81f1a6..af1a9191368c 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -553,13 +553,13 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> */
> jbd2_journal_switch_revoke_table(journal);
>
> + write_lock(&journal->j_state_lock);
> /*
> * Reserved credits cannot be claimed anymore, free them
> */
> 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.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat, 11 Jun 2022 21:04:26 +0800, Zhang Yi wrote:
> We catch an assert problem in jbd2_journal_commit_transaction() when
> doing fsstress and request falut injection tests. The problem is
> happened in a race condition between jbd2_journal_commit_transaction()
> and ext4_end_io_end(). Firstly, ext4_writepages() writeback dirty pages
> and start reserved handle, and then the journal was aborted due to some
> previous metadata IO error, jbd2_journal_abort() start to commit current
> running transaction, the committing procedure could be raced by
> ext4_end_io_end() and lead to subtract j_reserved_credits twice from
> commit_transaction->t_outstanding_credits, finally the
> t_outstanding_credits is mistakenly smaller than t_nr_buffers and
> trigger assert.
>
> [...]
Applied, thanks!
[1/1] jbd2: fix outstanding credits assert in jbd2_journal_commit_transaction()
commit: f8dc286e4d942dab79d1814e0708ac91052a34fa
Best regards,
--
Theodore Ts'o <[email protected]>