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

Xiaoguang Wang
2018-11-24T14:27:35Z

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