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

[Eric Ren]

Hi,

On 09/12/2016 11:06 AM, Eric Ren wrote:
> Hi,
>>> 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.
>>
>> 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?
> 1. ocfs2_unlock_pages() will be called in ocfs2_write_end_nolock(), in this case, we
> definitely want to return a locked mmaped page
> to VM code (do_page_mkwrite) when VM_FAULT_LOCKED is set.
>
> 2. But there's indeed a potential existing deadlock situation:
>                                  ocfs2_grab_pages_for_write()  ==> return -ENOMEM and with
> the mmaped page locked
>                                  ocfs2_free_write_ctxt() ==> leave the mmapped page locked
>                            ocfs2_write_begin_nolock() ==> return -ENOMEM
>                      __ocfs2_page_mkwrite() ==> return VM_FAULT_OMM
>                __do_page_mkwrite() ==> deadlock here
> (https://github.com/torvalds/linux/blob/master/mm/memory.c#L2054)
> This is another corner case, right?
>
> Anyway, I think this patch is good for the -ENOSPC case. And another patch should be
> proposed for -ENOMEM case?
Yes, I think we can catch both -ENOSPC and -ENOMEM cases in the failure path by unlocking the
mmaped page after ocfs2_free_write_ctx(), right?

Eric
>
> Thanks,
> Eric
>
>> 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
>