Return-Path: Received: from out30-132.freemail.mail.aliyun.com ([115.124.30.132]:60578 "EHLO out30-132.freemail.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728071AbeKNVyX (ORCPT ); Wed, 14 Nov 2018 16:54:23 -0500 From: Xiaoguang Wang To: linux-ext4@vger.kernel.org Cc: Xiaoguang Wang Subject: [PATCH] ext4: fix deadlock while checkpoint thread waits commit thread to finish Date: Wed, 14 Nov 2018 19:49:35 +0800 Message-Id: <20181114114935.5250-1-xiaoguang.wang@linux.alibaba.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: This issue was found when I tried to put checkpoint work in a separate thread, the deadlock below happened: Thread1 | Thread2 __jbd2_log_wait_for_space | jbd2_log_do_checkpoint (hold j_checkpoint_mutex)| if (jh->b_transaction != NULL) | ... | jbd2_log_start_commit(journal, tid); |jbd2_update_log_tail | will lock j_checkpoint_mutex, | but will be blocked here. | jbd2_log_wait_commit(journal, tid); | wait_event(journal->j_wait_done_commit, | !tid_gt(tid, journal->j_commit_sequence)); | ... |wake_up(j_wait_done_commit) } | then deadlock occurs, Thread1 will never be waken up. To fix this issue, drop j_checkpoint_mutex in jbd2_log_do_checkpoint() when we are going to wait for transaction commit. Signed-off-by: Xiaoguang Wang --- fs/jbd2/checkpoint.c | 21 +++++++++++++++++---- fs/jbd2/journal.c | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index 26f8d7e46462..e728844f2f0e 100644 --- a/fs/jbd2/checkpoint.c +++ b/fs/jbd2/checkpoint.c @@ -113,7 +113,7 @@ void __jbd2_log_wait_for_space(journal_t *journal) nblocks = jbd2_space_needed(journal); while (jbd2_log_space_left(journal) < nblocks) { write_unlock(&journal->j_state_lock); - mutex_lock(&journal->j_checkpoint_mutex); + mutex_lock_io(&journal->j_checkpoint_mutex); /* * Test again, another process may have checkpointed while we @@ -241,8 +241,8 @@ int jbd2_log_do_checkpoint(journal_t *journal) * done (maybe it's a new transaction, but it fell at the same * address). */ - if (journal->j_checkpoint_transactions != transaction || - transaction->t_tid != this_tid) + if (journal->j_checkpoint_transactions == NULL || + journal->j_checkpoint_transactions->t_tid != this_tid) goto out; /* checkpoint all of the transaction's buffers */ @@ -276,9 +276,22 @@ int jbd2_log_do_checkpoint(journal_t *journal) "JBD2: %s: Waiting for Godot: block %llu\n", journal->j_devname, (unsigned long long) bh->b_blocknr); + if (batch_count) + __flush_batch(journal, &batch_count); jbd2_log_start_commit(journal, tid); + /* + * jbd2_journal_commit_transaction() may want + * to take the checkpoint_mutex if JBD2_FLUSHED + * is set, jbd2_update_log_tail() called by + * jbd2_journal_commit_transaction() may also take + * checkpoint_mutex. So we need to temporarily + * drop it. + */ + mutex_unlock(&journal->j_checkpoint_mutex); jbd2_log_wait_commit(journal, tid); - goto retry; + mutex_lock_io(&journal->j_checkpoint_mutex); + spin_lock(&journal->j_list_lock); + goto restart; } if (!buffer_dirty(bh)) { if (unlikely(buffer_write_io_error(bh)) && !result) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 8ef6b6daaa7a..88d8f22d2cba 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -2067,7 +2067,7 @@ int jbd2_journal_wipe(journal_t *journal, int write) err = jbd2_journal_skip_recovery(journal); if (write) { /* Lock to make assertions happy... */ - mutex_lock(&journal->j_checkpoint_mutex); + mutex_lock_io(&journal->j_checkpoint_mutex); jbd2_mark_journal_empty(journal, REQ_SYNC | REQ_FUA); mutex_unlock(&journal->j_checkpoint_mutex); } -- 2.17.2