Re: [PATCH] ext4: fix deadlock while checkpoint thread waits commit thread to finish

[Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>]

hi,

> On Tue 13-11-18 16:59:07, Xiaoguang Wang wrote:
>> 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, here we introduce a new j_loginfo_mutex to protect
>> concurrent modifications to journal log tail info.
>>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> 
> Thanks for the analysis and the patch. I agree this deadlock is possible
> however I'm not sure your solution is quite correct. There are other places
> besides __jbd2_update_log_tail() that do update log tail. The most common
> one is jbd2_mark_journal_empty() and the functions calling it but there are
> a few other places as well. All these need to be synchronized properly so
> that journal tail does not get corrupted. The your patch needs some more
> work.
Yes, thanks for pointing this out.

> 
> But in general I like the idea of a special lock protecting journal tail
> (I'd just call it j_tail_mutex to explicitely express that) so that
> pushing of the journal tail possibly done during transaction commit would not
> be blocked by checkpointing. That can be a performance win as well.
Agree with you :)

> 
> Since proper locking change is going to be a bit more involved, can you
> perhaps fix this deadlock by just dropping j_checkpoint_mutex in
> log_do_checkpoint() when we are going to wait for transaction commit. I've
> checked and that should be fine and that is going to be much easier change
> to backport into stable kernels...
OK, I'll try this method and have a test, thanks.

Regards,
Xiaoguang Wang

> 
> 								Honza
> 
> 
>> ---
>>   fs/jbd2/checkpoint.c |  2 +-
>>   fs/jbd2/commit.c     |  6 +++---
>>   fs/jbd2/journal.c    | 16 +++++++++-------
>>   include/linux/jbd2.h |  9 ++++++++-
>>   4 files changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
>> index c125d662777c..1729d6298895 100644
>> --- a/fs/jbd2/checkpoint.c
>> +++ b/fs/jbd2/checkpoint.c
>> @@ -404,7 +404,7 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
>>   	if (journal->j_flags & JBD2_BARRIER)
>>   		blkdev_issue_flush(journal->j_fs_dev, GFP_NOFS, NULL);
>>   
>> -	return __jbd2_update_log_tail(journal, first_tid, blocknr);
>> +	return jbd2_update_log_tail(journal, first_tid, blocknr);
>>   }
>>   
>>   
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index 150cc030b4d7..f139f5465687 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -383,9 +383,9 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>>   	/* Do we need to erase the effects of a prior jbd2_journal_flush? */
>>   	if (journal->j_flags & JBD2_FLUSHED) {
>>   		jbd_debug(3, "super block updated\n");
>> -		mutex_lock_io(&journal->j_checkpoint_mutex);
>> +		mutex_lock_io(&journal->j_loginfo_mutex);
>>   		/*
>> -		 * We hold j_checkpoint_mutex so tail cannot change under us.
>> +		 * We hold j_loginfo_mutex so tail cannot change under us.
>>   		 * We don't need any special data guarantees for writing sb
>>   		 * since journal is empty and it is ok for write to be
>>   		 * flushed only with transaction commit.
>> @@ -394,7 +394,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>>   						journal->j_tail_sequence,
>>   						journal->j_tail,
>>   						REQ_SYNC);
>> -		mutex_unlock(&journal->j_checkpoint_mutex);
>> +		mutex_unlock(&journal->j_loginfo_mutex);
>>   	} else {
>>   		jbd_debug(3, "superblock not updated\n");
>>   	}
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 8ef6b6daaa7a..be2c10ff5bae 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -940,8 +940,6 @@ int __jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block)
>>   	unsigned long freed;
>>   	int ret;
>>   
>> -	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
>> -
>>   	/*
>>   	 * We cannot afford for write to remain in drive's caches since as
>>   	 * soon as we update j_tail, next transaction can start reusing journal
>> @@ -978,12 +976,16 @@ int __jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block)
>>    * provided log tail and locks j_checkpoint_mutex. So it is safe against races
>>    * with other threads updating log tail.
>>    */
>> -void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block)
>> +int jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block)
>>   {
>> -	mutex_lock_io(&journal->j_checkpoint_mutex);
>> +	int ret = 0;
>> +
>> +	mutex_lock_io(&journal->j_loginfo_mutex);
>>   	if (tid_gt(tid, journal->j_tail_sequence))
>> -		__jbd2_update_log_tail(journal, tid, block);
>> -	mutex_unlock(&journal->j_checkpoint_mutex);
>> +		ret = __jbd2_update_log_tail(journal, tid, block);
>> +	mutex_unlock(&journal->j_loginfo_mutex);
>> +
>> +	return ret;
>>   }
>>   
>>   struct jbd2_stats_proc_session {
>> @@ -1147,6 +1149,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
>>   	init_waitqueue_head(&journal->j_wait_reserved);
>>   	mutex_init(&journal->j_barrier);
>>   	mutex_init(&journal->j_checkpoint_mutex);
>> +	mutex_init(&journal->j_loginfo_mutex);
>>   	spin_lock_init(&journal->j_revoke_lock);
>>   	spin_lock_init(&journal->j_list_lock);
>>   	rwlock_init(&journal->j_state_lock);
>> @@ -1420,7 +1423,6 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
>>   	if (is_journal_aborted(journal))
>>   		return -EIO;
>>   
>> -	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
>>   	jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
>>   		  tail_block, tail_tid);
>>   
>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
>> index b708e5169d1d..a9c2928aea35 100644
>> --- a/include/linux/jbd2.h
>> +++ b/include/linux/jbd2.h
>> @@ -862,6 +862,13 @@ struct journal_s
>>   	 */
>>   	struct buffer_head	*j_chkpt_bhs[JBD2_NR_BATCH];
>>   
>> +	/**
>> +	 * @j_loginfo_mutex:
>> +	 *
>> +	 * Semaphore for locking against concurrent update journal info.
>> +	 */
>> +	struct mutex		j_loginfo_mutex;
>> +
>>   	/**
>>   	 * @j_head:
>>   	 *
>> @@ -1265,7 +1272,7 @@ int jbd2_journal_next_log_block(journal_t *, unsigned long long *);
>>   int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
>>   			      unsigned long *block);
>>   int __jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block);
>> -void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block);
>> +int jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block);
>>   
>>   /* Commit management */
>>   extern void jbd2_journal_commit_transaction(journal_t *);
>> -- 
>> 2.14.4.44.g2045bb6
>>
>>