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

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

[Xiaoguang Wang]

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>
---
 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

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

[Jan Kara]

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.

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.

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...

								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
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Xiaoguang Wang]

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
>>
>>

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

[Xiaoguang Wang]

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 <xiaoguang.wang@linux.alibaba.com>
---
 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

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

[Jan Kara]

On Wed 14-11-18 19:49:35, 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, drop j_checkpoint_mutex in jbd2_log_do_checkpoint()
> when we are going to wait for transaction commit.
> 
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>

Thanks for the patch! One comment below...

> 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;

Why did you change this? As far as I can tell there's no difference and the
previous condition makes it more obvious that we are still looking at the
same transaction.

Otherwise the patch looks good so after removing the above hunk feel free to
add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> @@ -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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Xiaoguang Wang]

hi,

> On Wed 14-11-18 19:49:35, 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, drop j_checkpoint_mutex in jbd2_log_do_checkpoint()
>> when we are going to wait for transaction commit.
>>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> 
> Thanks for the patch! One comment below...
> 
>> 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;
> 
> Why did you change this? As far as I can tell there's no difference and the
> previous condition makes it more obvious that we are still looking at the
> same transaction.
In this patch, we may drop j_checkpoint_mutex, then another thread may acquire
this lock, do checkpoint work and freed current transaction, "transaction->t_tid"
will cause an invalid pointer dereference.

Regards,
Xiaoguang Wang
> 
> Otherwise the patch looks good so after removing the above hunk feel free to
> add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza
> 
> 
>> @@ -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
>>

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

[Jan Kara]

On Fri 23-11-18 10:45:20, Xiaoguang Wang wrote:
> hi,
> 
> > On Wed 14-11-18 19:49:35, 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, drop j_checkpoint_mutex in jbd2_log_do_checkpoint()
> > > when we are going to wait for transaction commit.
> > > 
> > > Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> > 
> > Thanks for the patch! One comment below...
> > 
> > > 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;
> > 
> > Why did you change this? As far as I can tell there's no difference and the
> > previous condition makes it more obvious that we are still looking at the
> > same transaction.
> In this patch, we may drop j_checkpoint_mutex, then another thread may acquire
> this lock, do checkpoint work and freed current transaction, "transaction->t_tid"
> will cause an invalid pointer dereference.

That is exactly the reason why we check:

if (journal->j_checkpoint_transactions != transaction || ...

So if this test is false and so transaction->t_tid != this_tid gets
evaluated we are sure that j_checkpoint_transactions actually still points
to our transaction.

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

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

[Xiaoguang Wang]

> On Fri 23-11-18 10:45:20, Xiaoguang Wang wrote:
>> hi,
>>
>>> On Wed 14-11-18 19:49:35, 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, drop j_checkpoint_mutex in jbd2_log_do_checkpoint()
>>>> when we are going to wait for transaction commit.
>>>>
>>>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>>>
>>> Thanks for the patch! One comment below...
>>>
>>>> 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;
>>>
>>> Why did you change this? As far as I can tell there's no difference and the
>>> previous condition makes it more obvious that we are still looking at the
>>> same transaction.
>> In this patch, we may drop j_checkpoint_mutex, then another thread may acquire
>> this lock, do checkpoint work and freed current transaction, "transaction->t_tid"
>> will cause an invalid pointer dereference.
> 
> That is exactly the reason why we check:
> 
> if (journal->j_checkpoint_transactions != transaction || ...
> 
> So if this test is false and so transaction->t_tid != this_tid gets
> evaluated we are sure that j_checkpoint_transactions actually still points
> to our transaction.
I just realize that "journal->j_checkpoint_transactions != transaction" returns false, we
can make sure that transaction is valid, thanks. I'll send a patch v2 soon.

Regards,
Xiaoguang Wang
> 
> 								Honza
>