DAX: bug in COW no page fault?

DAX: bug in COW no page fault?

[Jared Hulbert]

Why isn't this required?

diff --git a/fs/dax.c b/fs/dax.c
index a7f77e1..30f2abe 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
                                error = -EIO;
                                goto out;
                        }
+                       i_mmap_unlock_read(mapping);
                }
                return VM_FAULT_LOCKED;
        }

RE: bug in COW no page fault?

[Wilcox, Matthew R]

Because we need to hold the i_mmap_sem until the page is inserted into the page tables to avoid racing with truncate.  Therefore it is unlocked by the MM code.

Did you try this patch?  It should have BUGged immediately that you hit this case.  If not, you have insufficient CONFIG_DEBUG enabled.

________________________________________
From: Jared Hulbert [jaredeh@gmail.com]
Sent: January 29, 2016 1:38 PM
To: linux-fsdevel@vger.kernel.org; Wilcox, Matthew R; ross.zwisler@linux.intel.com
Subject: DAX: bug in COW no page fault?

Why isn't this required?

diff --git a/fs/dax.c b/fs/dax.c
index a7f77e1..30f2abe 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
                                error = -EIO;
                                goto out;
                        }
+                       i_mmap_unlock_read(mapping);
                }
                return VM_FAULT_LOCKED;
        }

Re: bug in COW no page fault?

[Jared Hulbert]

Which page?  The unmatched call for i_mmap_lock_read() is only called
if(!page).  Ahh, are we locking for the cow_page?  Okay, then why
don't we lock when page != NULL in the COW case?

On Fri, Jan 29, 2016 at 1:53 PM, Wilcox, Matthew R
<matthew.r.wilcox@intel.com> wrote:
> Because we need to hold the i_mmap_sem until the page is inserted into the page tables to avoid racing with truncate.  Therefore it is unlocked by the MM code.
>
> Did you try this patch?  It should have BUGged immediately that you hit this case.  If not, you have insufficient CONFIG_DEBUG enabled.
>
> ________________________________________
> From: Jared Hulbert [jaredeh@gmail.com]
> Sent: January 29, 2016 1:38 PM
> To: linux-fsdevel@vger.kernel.org; Wilcox, Matthew R; ross.zwisler@linux.intel.com
> Subject: DAX: bug in COW no page fault?
>
> Why isn't this required?
>
> diff --git a/fs/dax.c b/fs/dax.c
> index a7f77e1..30f2abe 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
>                                 error = -EIO;
>                                 goto out;
>                         }
> +                       i_mmap_unlock_read(mapping);
>                 }
>                 return VM_FAULT_LOCKED;
>         }

RE: bug in COW no page fault?

[Wilcox, Matthew R]

Because then we lock the page instead of the i_mmap_sem.  The code in mm/memory.c isn't /that/ hard to understand, is it?
________________________________________
From: Jared Hulbert [jaredeh@gmail.com]
Sent: January 29, 2016 2:12 PM
To: Wilcox, Matthew R
Cc: linux-fsdevel@vger.kernel.org; ross.zwisler@linux.intel.com
Subject: Re: bug in COW no page fault?

Which page?  The unmatched call for i_mmap_lock_read() is only called
if(!page).  Ahh, are we locking for the cow_page?  Okay, then why
don't we lock when page != NULL in the COW case?

On Fri, Jan 29, 2016 at 1:53 PM, Wilcox, Matthew R
<matthew.r.wilcox@intel.com> wrote:
> Because we need to hold the i_mmap_sem until the page is inserted into the page tables to avoid racing with truncate.  Therefore it is unlocked by the MM code.
>
> Did you try this patch?  It should have BUGged immediately that you hit this case.  If not, you have insufficient CONFIG_DEBUG enabled.
>
> ________________________________________
> From: Jared Hulbert [jaredeh@gmail.com]
> Sent: January 29, 2016 1:38 PM
> To: linux-fsdevel@vger.kernel.org; Wilcox, Matthew R; ross.zwisler@linux.intel.com
> Subject: DAX: bug in COW no page fault?
>
> Why isn't this required?
>
> diff --git a/fs/dax.c b/fs/dax.c
> index a7f77e1..30f2abe 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
>                                 error = -EIO;
>                                 goto out;
>                         }
> +                       i_mmap_unlock_read(mapping);
>                 }
>                 return VM_FAULT_LOCKED;
>         }

Re: bug in COW no page fault?

[Jared Hulbert]

oh...  Look!  There it is, right where it should be.  Sorry.

Recovering from years of Android and VMware, please be patient.

On Fri, Jan 29, 2016 at 2:36 PM, Wilcox, Matthew R
<matthew.r.wilcox@intel.com> wrote:
> Because then we lock the page instead of the i_mmap_sem.  The code in mm/memory.c isn't /that/ hard to understand, is it?
> ________________________________________
> From: Jared Hulbert [jaredeh@gmail.com]
> Sent: January 29, 2016 2:12 PM
> To: Wilcox, Matthew R
> Cc: linux-fsdevel@vger.kernel.org; ross.zwisler@linux.intel.com
> Subject: Re: bug in COW no page fault?
>
> Which page?  The unmatched call for i_mmap_lock_read() is only called
> if(!page).  Ahh, are we locking for the cow_page?  Okay, then why
> don't we lock when page != NULL in the COW case?
>
> On Fri, Jan 29, 2016 at 1:53 PM, Wilcox, Matthew R
> <matthew.r.wilcox@intel.com> wrote:
>> Because we need to hold the i_mmap_sem until the page is inserted into the page tables to avoid racing with truncate.  Therefore it is unlocked by the MM code.
>>
>> Did you try this patch?  It should have BUGged immediately that you hit this case.  If not, you have insufficient CONFIG_DEBUG enabled.
>>
>> ________________________________________
>> From: Jared Hulbert [jaredeh@gmail.com]
>> Sent: January 29, 2016 1:38 PM
>> To: linux-fsdevel@vger.kernel.org; Wilcox, Matthew R; ross.zwisler@linux.intel.com
>> Subject: DAX: bug in COW no page fault?
>>
>> Why isn't this required?
>>
>> diff --git a/fs/dax.c b/fs/dax.c
>> index a7f77e1..30f2abe 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
>>                                 error = -EIO;
>>                                 goto out;
>>                         }
>> +                       i_mmap_unlock_read(mapping);
>>                 }
>>                 return VM_FAULT_LOCKED;
>>         }

RE: bug in COW no page fault?

[Wilcox, Matthew R]

I remembered that Ross had a similar question, so it must be hard to understand.  How does this comment work for both of you?

+               /*
+                * A truncate must remove COWs of pages that are removed
+                * from the file.  If we have a struct page, the normal
+                * page lock mechanism prevents truncate from missing the
+                * COWed page.  If not, the i_mmap_lock can provide the
+                * same guarantee.  It is dropped by the caller after the
+                * page is safely in the page tables.
+                */

________________________________________
From: Jared Hulbert [jaredeh@gmail.com]
Sent: January 29, 2016 3:16 PM
To: Wilcox, Matthew R
Cc: linux-fsdevel@vger.kernel.org; ross.zwisler@linux.intel.com
Subject: Re: bug in COW no page fault?

oh...  Look!  There it is, right where it should be.  Sorry.

Recovering from years of Android and VMware, please be patient.

On Fri, Jan 29, 2016 at 2:36 PM, Wilcox, Matthew R
<matthew.r.wilcox@intel.com> wrote:
> Because then we lock the page instead of the i_mmap_sem.  The code in mm/memory.c isn't /that/ hard to understand, is it?
> ________________________________________
> From: Jared Hulbert [jaredeh@gmail.com]
> Sent: January 29, 2016 2:12 PM
> To: Wilcox, Matthew R
> Cc: linux-fsdevel@vger.kernel.org; ross.zwisler@linux.intel.com
> Subject: Re: bug in COW no page fault?
>
> Which page?  The unmatched call for i_mmap_lock_read() is only called
> if(!page).  Ahh, are we locking for the cow_page?  Okay, then why
> don't we lock when page != NULL in the COW case?
>
> On Fri, Jan 29, 2016 at 1:53 PM, Wilcox, Matthew R
> <matthew.r.wilcox@intel.com> wrote:
>> Because we need to hold the i_mmap_sem until the page is inserted into the page tables to avoid racing with truncate.  Therefore it is unlocked by the MM code.
>>
>> Did you try this patch?  It should have BUGged immediately that you hit this case.  If not, you have insufficient CONFIG_DEBUG enabled.
>>
>> ________________________________________
>> From: Jared Hulbert [jaredeh@gmail.com]
>> Sent: January 29, 2016 1:38 PM
>> To: linux-fsdevel@vger.kernel.org; Wilcox, Matthew R; ross.zwisler@linux.intel.com
>> Subject: DAX: bug in COW no page fault?
>>
>> Why isn't this required?
>>
>> diff --git a/fs/dax.c b/fs/dax.c
>> index a7f77e1..30f2abe 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
>>                                 error = -EIO;
>>                                 goto out;
>>                         }
>> +                       i_mmap_unlock_read(mapping);
>>                 }
>>                 return VM_FAULT_LOCKED;
>>         }

Re: bug in COW no page fault?

[Jared Hulbert]

That's helpful.  Thanks.

On Fri, Jan 29, 2016 at 9:47 PM, Wilcox, Matthew R
<matthew.r.wilcox@intel.com> wrote:
> I remembered that Ross had a similar question, so it must be hard to understand.  How does this comment work for both of you?
>
> +               /*
> +                * A truncate must remove COWs of pages that are removed
> +                * from the file.  If we have a struct page, the normal
> +                * page lock mechanism prevents truncate from missing the
> +                * COWed page.  If not, the i_mmap_lock can provide the
> +                * same guarantee.  It is dropped by the caller after the
> +                * page is safely in the page tables.
> +                */
>
> ________________________________________
> From: Jared Hulbert [jaredeh@gmail.com]
> Sent: January 29, 2016 3:16 PM
> To: Wilcox, Matthew R
> Cc: linux-fsdevel@vger.kernel.org; ross.zwisler@linux.intel.com
> Subject: Re: bug in COW no page fault?
>
> oh...  Look!  There it is, right where it should be.  Sorry.
>
> Recovering from years of Android and VMware, please be patient.
>
> On Fri, Jan 29, 2016 at 2:36 PM, Wilcox, Matthew R
> <matthew.r.wilcox@intel.com> wrote:
>> Because then we lock the page instead of the i_mmap_sem.  The code in mm/memory.c isn't /that/ hard to understand, is it?
>> ________________________________________
>> From: Jared Hulbert [jaredeh@gmail.com]
>> Sent: January 29, 2016 2:12 PM
>> To: Wilcox, Matthew R
>> Cc: linux-fsdevel@vger.kernel.org; ross.zwisler@linux.intel.com
>> Subject: Re: bug in COW no page fault?
>>
>> Which page?  The unmatched call for i_mmap_lock_read() is only called
>> if(!page).  Ahh, are we locking for the cow_page?  Okay, then why
>> don't we lock when page != NULL in the COW case?
>>
>> On Fri, Jan 29, 2016 at 1:53 PM, Wilcox, Matthew R
>> <matthew.r.wilcox@intel.com> wrote:
>>> Because we need to hold the i_mmap_sem until the page is inserted into the page tables to avoid racing with truncate.  Therefore it is unlocked by the MM code.
>>>
>>> Did you try this patch?  It should have BUGged immediately that you hit this case.  If not, you have insufficient CONFIG_DEBUG enabled.
>>>
>>> ________________________________________
>>> From: Jared Hulbert [jaredeh@gmail.com]
>>> Sent: January 29, 2016 1:38 PM
>>> To: linux-fsdevel@vger.kernel.org; Wilcox, Matthew R; ross.zwisler@linux.intel.com
>>> Subject: DAX: bug in COW no page fault?
>>>
>>> Why isn't this required?
>>>
>>> diff --git a/fs/dax.c b/fs/dax.c
>>> index a7f77e1..30f2abe 100644
>>> --- a/fs/dax.c
>>> +++ b/fs/dax.c
>>> @@ -416,6 +416,7 @@ int __dax_fault(struct vm_area_struct *vma, struct vm_fault
>>>                                 error = -EIO;
>>>                                 goto out;
>>>                         }
>>> +                       i_mmap_unlock_read(mapping);
>>>                 }
>>>                 return VM_FAULT_LOCKED;
>>>         }

Re: bug in COW no page fault?

[Ross Zwisler]

On Sat, Jan 30, 2016 at 05:47:18AM +0000, Wilcox, Matthew R wrote:
> I remembered that Ross had a similar question, so it must be hard to understand.  How does this comment work for both of you?
> 
> +               /*
> +                * A truncate must remove COWs of pages that are removed
> +                * from the file.  If we have a struct page, the normal
> +                * page lock mechanism prevents truncate from missing the
> +                * COWed page.  If not, the i_mmap_lock can provide the
> +                * same guarantee.  It is dropped by the caller after the
> +                * page is safely in the page tables.
> +                */

Yep, this makes sense.  It may be worthwhile to explicitly say "It is dropped
by do_cow_fault() after the page is safely in the page tables." so the less
experienced among us (me) can easily find the match to the i_mmap_lock_read().