[Ocfs2-devel] [PATCH] ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

From: Eric Ren <zren@suse.com>
To: ocfs2-devel@oss.oracle.com
Subject: [Ocfs2-devel] [PATCH] ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()
Date: Wed, 14 Sep 2016 16:43:51 +0800	[thread overview]
Message-ID: <e870faba-261c-f60a-d57a-47c9ac9386b3@suse.com> (raw)
In-Reply-To: <57D90981.4080504@huawei.com>

Hi Joseph,

On 09/14/2016 04:25 PM, Joseph Qi wrote:
> Hi Eric,
> Sorry for the delayed response.
> I have got your explanation. So we have to unlock the page only in case
> of retry, right?
> If so, I think the unlock should be right before "goto try_again".
No, the mmapped page should be unlocked as long as we cannot return VM_FAULT_LOCKED
to do_page_mkpage(). Otherwise, the deadlock will happen in do_page_mkpage(). Please
see the recent 2 mails;-)

Eric
>
> Thanks,
> Joseph
>
> On 2016/9/14 16:04, Eric Ren wrote:
>> Hi Joseph,
>>>>> In ocfs2_write_begin_nolock(), we first grab the pages and then
>>>>> allocate disk space for this write; ocfs2_try_to_free_truncate_log()
>>>>> will be called if ENOSPC is turned; if we're lucky to get enough clusters,
>>>>> which is usually the case, we start over again. But in ocfs2_free_write_ctxt()
>>>>> the target page isn't unlocked, so we will deadlock when trying to grab
>>>>> the target page again.
>>>> IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and
>>>> w_target_locked is set to true, and then will be unlocked by
>>>> ocfs2_unlock_pages in ocfs2_free_write_ctxt.
>>>> So I'm not getting the case "page isn't unlock". Could you please explain
>>>> it in more detail?
>>> Thanks for review;-) Follow up the calling chain:
>>>
>>> ocfs2_free_write_ctxt()
>>>   ->ocfs2_unlock_pages()
>>>
>>> in ocfs2_unlock_pages (https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we
>>> can see the code just put_page(target_page), but not unlock it.
>> Did this answer your question?
>>
>> Thanks,
>> Eric
>>> Yeah, I will think this a bit more like:
>>> why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but
>>> other possible error code?
>>>
>>> Thanks,
>>> Eric
>>>
>>>> Thanks,
>>>> Joseph
>>>>
>>>>> Fix this issue by unlocking the target page after we fail to allocate
>>>>> enough space at the first time.
>>>>>
>>>>> Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.
>>>>>
>>>>> Signed-off-by: Eric Ren <zren@suse.com>
>>>>> ---
>>>>>    fs/ocfs2/aops.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>>>>> index 98d3654..78d1d67 100644
>>>>> --- a/fs/ocfs2/aops.c
>>>>> +++ b/fs/ocfs2/aops.c
>>>>> @@ -1860,6 +1860,13 @@ out:
>>>>>             */
>>>>>            try_free = 0;
>>>>>    +        /*
>>>>> +         * Unlock mmap_page because the page has been locked when we
>>>>> +         * are here.
>>>>> +         */
>>>>> +        if (mmap_page)
>>>>> +            unlock_page(mmap_page);
>>>>> +
>>>>>            ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
>>>>>            if (ret1 == 1)
>>>>>                goto try_again;
>>>>>
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Ocfs2-devel mailing list
>>> Ocfs2-devel at oss.oracle.com
>>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
>
>