| Message ID | 100D68C7BA14664A8938383216E40DE04217F70D@FMSMSX114.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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; >> } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. + */
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; >>> } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }