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

[Joseph Qi]

Okay, IC.
So we have to take care of all errors for ocfs2_write_begin_nolock.

On 2016/9/14 16:43, Eric Ren wrote:
> 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 at 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
>>>
>>
>>
> 
> 
> .
>