Re: NFS BUG_ON in nfs_do_writepage

Re: NFS BUG_ON in nfs_do_writepage

[Nick Piggin]

On Sat, Apr 25, 2009 at 10:57:08AM -0400, Trond Myklebust wrote:
> On Fri, 2009-04-24 at 05:26 -0400, Rince wrote:
> > Applied try 3 of Nick Piggin's patch to 2.6.30-rc3 (cleanly, no less!)
> > 
> > Doesn't appear to have helped at all - I received my favorite BUG ON
> > write.c:252 just like always, within 24 hours of booting the kernel,
> > even.
> 
> Can you apply the following incremental patch on top of Nick's. This
> appears to suffice to close the race on my setup.

Thanks, yes that looks good. Note: I deliberately didn't try to
convert filesystems because it needs much better understanding
of each one. So any fs maintainers using page_mkwrite I hope have
looked at these patches and considered whether they need to
do anything differently (ditto for the page_mkwrite return value
fixup patch).

Thanks,
Nick

> 
> Cheers
>   Trond
> ---------------------------------------------------------------------
> >From f0258852dcb43c748854d2ee550c9c270bb25f21 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
> Date: Fri, 24 Apr 2009 17:32:22 -0400
> Subject: [PATCH] NFS: Close page_mkwrite() races
> 
> Follow up to Nick Piggin's patches to ensure that nfs_vm_page_mkwrite
> returns with the page lock held, and sets the VM_FAULT_LOCKED flag.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/nfs/file.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 5a97bcf..ec7e27d 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -517,10 +517,10 @@ static int nfs_vm_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>  
>  	ret = nfs_updatepage(filp, page, 0, pagelen);
>  out_unlock:
> +	if (!ret)
> +		return VM_FAULT_LOCKED;
>  	unlock_page(page);
> -	if (ret)
> -		ret = VM_FAULT_SIGBUS;
> -	return ret;
> +	return VM_FAULT_SIGBUS;
>  }
>  
>  static struct vm_operations_struct nfs_file_vm_ops = {
> -- 
> 1.6.0.6
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS BUG_ON in nfs_do_writepage

[Trond Myklebust]

On Sun, 2009-04-26 at 08:40 +0200, Nick Piggin wrote:
> On Sat, Apr 25, 2009 at 10:57:08AM -0400, Trond Myklebust wrote:
> > On Fri, 2009-04-24 at 05:26 -0400, Rince wrote:
> > > Applied try 3 of Nick Piggin's patch to 2.6.30-rc3 (cleanly, no less!)
> > > 
> > > Doesn't appear to have helped at all - I received my favorite BUG ON
> > > write.c:252 just like always, within 24 hours of booting the kernel,
> > > even.
> > 
> > Can you apply the following incremental patch on top of Nick's. This
> > appears to suffice to close the race on my setup.
> 
> Thanks, yes that looks good. Note: I deliberately didn't try to
> convert filesystems because it needs much better understanding
> of each one. So any fs maintainers using page_mkwrite I hope have
> looked at these patches and considered whether they need to
> do anything differently (ditto for the page_mkwrite return value
> fixup patch).

Note that after applying this, I put a WARN_ON(!PageDirty()) in the NFS
set_page_dirty() method, and ran some mmap stress tests.

As far as I can tell, the WARN_ON is never triggering. I take this to
mean that the remaining cases where the VM is calling set_page_dirty()
are basically all cases where we've already seen a page fault and set
the page dirty flag, but haven't yet written it out (i.e. we haven't yet
called clear_page_dirty_for_io() and so the pte is still marked as
dirty).
That again implies that set_page_dirty() is now fully redundant for
filesystems that define page_mkwrite(), provided that the filesystem
takes charge of marking the page as dirty.

This suggests an alternative fix for the stable kernels in the form of
the following patch.
Comments?

Cheers
  Trond
------------------------------------------------------------------
commit 684049bf73059aa9be35f9cdf07acda29ebb0340
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Sun Apr 26 10:14:34 2009 -0400

    NFS: Fix page dirtying races in NFS
    
    If a filesystem defines a page_mkwrite() callback that also marks the page
    as being dirty, then we don't need to define a set_page_dirty() callback.
    
    The following patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=12913
    by eliminating a race in do_wp_page() and __do_fault(). The latter two
    mark the page as dirty after the call to page_mkwrite(). Since
    nfs_vm_page_mkwrite() has already marked the page as dirty, this means that
    there is a race whereby the filesystem may actually have cleaned the
    page by the time it is marked as dirty (again) by the VM.
    
    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 5a97bcf..21bffaf 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -465,10 +465,19 @@ static int nfs_launder_page(struct page *page)
 	return nfs_wb_page(inode, page);
 }
 
+static int nfs_set_page_dirty(struct page *page)
+{
+	/* We don't need to have the VM mark the page as dirty, since
+	 * nfs_updatepage() will do it. This eliminates the race
+	 * that caused http://bugzilla.kernel.org/show_bug.cgi?id=12913
+	 */
+	return 0;
+}
+
 const struct address_space_operations nfs_file_aops = {
 	.readpage = nfs_readpage,
 	.readpages = nfs_readpages,
-	.set_page_dirty = __set_page_dirty_nobuffers,
+	.set_page_dirty = nfs_set_page_dirty,
 	.writepage = nfs_writepage,
 	.writepages = nfs_writepages,
 	.write_begin = nfs_write_begin,

Re: NFS BUG_ON in nfs_do_writepage

[Nick Piggin]

On Sun, Apr 26, 2009 at 10:18:29AM -0400, Trond Myklebust wrote:
> On Sun, 2009-04-26 at 08:40 +0200, Nick Piggin wrote:
> > On Sat, Apr 25, 2009 at 10:57:08AM -0400, Trond Myklebust wrote:
> > > On Fri, 2009-04-24 at 05:26 -0400, Rince wrote:
> > > > Applied try 3 of Nick Piggin's patch to 2.6.30-rc3 (cleanly, no less!)
> > > > 
> > > > Doesn't appear to have helped at all - I received my favorite BUG ON
> > > > write.c:252 just like always, within 24 hours of booting the kernel,
> > > > even.
> > > 
> > > Can you apply the following incremental patch on top of Nick's. This
> > > appears to suffice to close the race on my setup.
> > 
> > Thanks, yes that looks good. Note: I deliberately didn't try to
> > convert filesystems because it needs much better understanding
> > of each one. So any fs maintainers using page_mkwrite I hope have
> > looked at these patches and considered whether they need to
> > do anything differently (ditto for the page_mkwrite return value
> > fixup patch).
> 
> Note that after applying this, I put a WARN_ON(!PageDirty()) in the NFS
> set_page_dirty() method, and ran some mmap stress tests.
> 
> As far as I can tell, the WARN_ON is never triggering. I take this to
> mean that the remaining cases where the VM is calling set_page_dirty()
> are basically all cases where we've already seen a page fault and set
> the page dirty flag, but haven't yet written it out (i.e. we haven't yet
> called clear_page_dirty_for_io() and so the pte is still marked as
> dirty).
> That again implies that set_page_dirty() is now fully redundant for
> filesystems that define page_mkwrite(), provided that the filesystem
> takes charge of marking the page as dirty.
> 
> This suggests an alternative fix for the stable kernels in the form of
> the following patch.
> Comments?

This doesn't seem to fix the race, though... on kernels with the
race still there, it will just open a window where you can have
a dirty pte but the page not written out.

I don't understand.

> Cheers
>   Trond
> ------------------------------------------------------------------
> commit 684049bf73059aa9be35f9cdf07acda29ebb0340
> Author: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date:   Sun Apr 26 10:14:34 2009 -0400
> 
>     NFS: Fix page dirtying races in NFS
>     
>     If a filesystem defines a page_mkwrite() callback that also marks the page
>     as being dirty, then we don't need to define a set_page_dirty() callback.
>     
>     The following patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=12913
>     by eliminating a race in do_wp_page() and __do_fault(). The latter two
>     mark the page as dirty after the call to page_mkwrite(). Since
>     nfs_vm_page_mkwrite() has already marked the page as dirty, this means that
>     there is a race whereby the filesystem may actually have cleaned the
>     page by the time it is marked as dirty (again) by the VM.
>     
>     Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 5a97bcf..21bffaf 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -465,10 +465,19 @@ static int nfs_launder_page(struct page *page)
>  	return nfs_wb_page(inode, page);
>  }
>  
> +static int nfs_set_page_dirty(struct page *page)
> +{
> +	/* We don't need to have the VM mark the page as dirty, since
> +	 * nfs_updatepage() will do it. This eliminates the race
> +	 * that caused http://bugzilla.kernel.org/show_bug.cgi?id=12913
> +	 */
> +	return 0;
> +}
> +
>  const struct address_space_operations nfs_file_aops = {
>  	.readpage = nfs_readpage,
>  	.readpages = nfs_readpages,
> -	.set_page_dirty = __set_page_dirty_nobuffers,
> +	.set_page_dirty = nfs_set_page_dirty,
>  	.writepage = nfs_writepage,
>  	.writepages = nfs_writepages,
>  	.write_begin = nfs_write_begin,
> 

Re: NFS BUG_ON in nfs_do_writepage

[Trond Myklebust]

On Sun, 2009-04-26 at 17:13 +0200, Nick Piggin wrote:
> On Sun, Apr 26, 2009 at 10:18:29AM -0400, Trond Myklebust wrote:
> > On Sun, 2009-04-26 at 08:40 +0200, Nick Piggin wrote:
> > > On Sat, Apr 25, 2009 at 10:57:08AM -0400, Trond Myklebust wrote:
> > > > On Fri, 2009-04-24 at 05:26 -0400, Rince wrote:
> > > > > Applied try 3 of Nick Piggin's patch to 2.6.30-rc3 (cleanly, no less!)
> > > > > 
> > > > > Doesn't appear to have helped at all - I received my favorite BUG ON
> > > > > write.c:252 just like always, within 24 hours of booting the kernel,
> > > > > even.
> > > > 
> > > > Can you apply the following incremental patch on top of Nick's. This
> > > > appears to suffice to close the race on my setup.
> > > 
> > > Thanks, yes that looks good. Note: I deliberately didn't try to
> > > convert filesystems because it needs much better understanding
> > > of each one. So any fs maintainers using page_mkwrite I hope have
> > > looked at these patches and considered whether they need to
> > > do anything differently (ditto for the page_mkwrite return value
> > > fixup patch).
> > 
> > Note that after applying this, I put a WARN_ON(!PageDirty()) in the NFS
> > set_page_dirty() method, and ran some mmap stress tests.
> > 
> > As far as I can tell, the WARN_ON is never triggering. I take this to
> > mean that the remaining cases where the VM is calling set_page_dirty()
> > are basically all cases where we've already seen a page fault and set
> > the page dirty flag, but haven't yet written it out (i.e. we haven't yet
> > called clear_page_dirty_for_io() and so the pte is still marked as
> > dirty).
> > That again implies that set_page_dirty() is now fully redundant for
> > filesystems that define page_mkwrite(), provided that the filesystem
> > takes charge of marking the page as dirty.
> > 
> > This suggests an alternative fix for the stable kernels in the form of
> > the following patch.
> > Comments?
> 
> This doesn't seem to fix the race, though... on kernels with the
> race still there, it will just open a window where you can have
> a dirty pte but the page not written out.
> 
> I don't understand.

I'm just pointing out that the NFS client already calls
__set_page_dirty_nobuffers() while holding the page lock inside the
nfs_vm_page_mkwrite() call, so having the VM do it too in the call to
set_page_dirty_balance() is actually redundant. IOW: as far as the NFS
code is concerned, we can get rid of the ->set_page_dirty() callback in
that situation.

I couldn't find any other places in the VM code where we can have a
dirty pte without also having called page_mkwrite() (and hence
__set_page_dirty_nobuffers). As I said, adding a WARN_ON(!PageDirty())
in ->set_page_dirty() didn't ever trigger any cases where the
set_page_dirty() was actually setting the dirty bit (except in the case
where we race with page writeout in do_wp_page() and __do_fault()).

That's why I believe disabling ->set_page_dirty() is safe here, and will
in fact suffice to fix the page writeout race.

Cheers
  Trond

Re: NFS BUG_ON in nfs_do_writepage

[Nick Piggin]

On Sun, Apr 26, 2009 at 01:55:22PM -0400, Trond Myklebust wrote:
> On Sun, 2009-04-26 at 17:13 +0200, Nick Piggin wrote:
> > This doesn't seem to fix the race, though... on kernels with the
> > race still there, it will just open a window where you can have
> > a dirty pte but the page not written out.
> > 
> > I don't understand.
> 
> I'm just pointing out that the NFS client already calls
> __set_page_dirty_nobuffers() while holding the page lock inside the
> nfs_vm_page_mkwrite() call, so having the VM do it too in the call to
> set_page_dirty_balance() is actually redundant. IOW: as far as the NFS
> code is concerned, we can get rid of the ->set_page_dirty() callback in
> that situation.
> 
> I couldn't find any other places in the VM code where we can have a
> dirty pte without also having called page_mkwrite() (and hence
> __set_page_dirty_nobuffers). As I said, adding a WARN_ON(!PageDirty())
> in ->set_page_dirty() didn't ever trigger any cases where the
> set_page_dirty() was actually setting the dirty bit (except in the case
> where we race with page writeout in do_wp_page() and __do_fault()).
> 
> That's why I believe disabling ->set_page_dirty() is safe here, and will
> in fact suffice to fix the page writeout race.

Ah, no I don't think so because it opens another race where the
pte is dity but the page is marked clean.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS BUG_ON in nfs_do_writepage

[Trond Myklebust]

On Tue, 2009-04-28 at 06:27 +0200, Nick Piggin wrote:
> On Sun, Apr 26, 2009 at 01:55:22PM -0400, Trond Myklebust wrote:
> > On Sun, 2009-04-26 at 17:13 +0200, Nick Piggin wrote:
> > > This doesn't seem to fix the race, though... on kernels with the
> > > race still there, it will just open a window where you can have
> > > a dirty pte but the page not written out.
> > > 
> > > I don't understand.
> > 
> > I'm just pointing out that the NFS client already calls
> > __set_page_dirty_nobuffers() while holding the page lock inside the
> > nfs_vm_page_mkwrite() call, so having the VM do it too in the call to
> > set_page_dirty_balance() is actually redundant. IOW: as far as the NFS
> > code is concerned, we can get rid of the ->set_page_dirty() callback in
> > that situation.
> > 
> > I couldn't find any other places in the VM code where we can have a
> > dirty pte without also having called page_mkwrite() (and hence
> > __set_page_dirty_nobuffers). As I said, adding a WARN_ON(!PageDirty())
> > in ->set_page_dirty() didn't ever trigger any cases where the
> > set_page_dirty() was actually setting the dirty bit (except in the case
> > where we race with page writeout in do_wp_page() and __do_fault()).
> > 
> > That's why I believe disabling ->set_page_dirty() is safe here, and will
> > in fact suffice to fix the page writeout race.
> 
> Ah, no I don't think so because it opens another race where the
> pte is dity but the page is marked clean.

So how can that happen?

AFAICS, when the pte is dirtied, we should get a page fault, which
causes the page itself to be marked dirty by the nfs_vm_page_mkwrite()
callback.
When the page gets written out, the VM calls clear_page_dirty_for_io()
which also causes the pte to be cleaned.

At what point can you therefore have a situation where the pte is dirty
without the page being marked as dirty too?


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS BUG_ON in nfs_do_writepage

[Nick Piggin]

On Tue, Apr 28, 2009 at 07:45:17AM -0400, Trond Myklebust wrote:
> On Tue, 2009-04-28 at 06:27 +0200, Nick Piggin wrote:
> > On Sun, Apr 26, 2009 at 01:55:22PM -0400, Trond Myklebust wrote:
> > > On Sun, 2009-04-26 at 17:13 +0200, Nick Piggin wrote:
> > > > This doesn't seem to fix the race, though... on kernels with the
> > > > race still there, it will just open a window where you can have
> > > > a dirty pte but the page not written out.
> > > > 
> > > > I don't understand.
> > > 
> > > I'm just pointing out that the NFS client already calls
> > > __set_page_dirty_nobuffers() while holding the page lock inside the
> > > nfs_vm_page_mkwrite() call, so having the VM do it too in the call to
> > > set_page_dirty_balance() is actually redundant. IOW: as far as the NFS
> > > code is concerned, we can get rid of the ->set_page_dirty() callback in
> > > that situation.
> > > 
> > > I couldn't find any other places in the VM code where we can have a
> > > dirty pte without also having called page_mkwrite() (and hence
> > > __set_page_dirty_nobuffers). As I said, adding a WARN_ON(!PageDirty())
> > > in ->set_page_dirty() didn't ever trigger any cases where the
> > > set_page_dirty() was actually setting the dirty bit (except in the case
> > > where we race with page writeout in do_wp_page() and __do_fault()).
> > > 
> > > That's why I believe disabling ->set_page_dirty() is safe here, and will
> > > in fact suffice to fix the page writeout race.
> > 
> > Ah, no I don't think so because it opens another race where the
> > pte is dity but the page is marked clean.
> 
> So how can that happen?

If the page gets cleaned after page_mkwrite and before the page
table locks are taken again in order to set the pte writeable.
(actually, page_mkclean only runs if it finds mapcount elevated,
so it is enough to clean the page even after the locks are taken
and before mapcount is incremented in the case of __do_fault).

 
> AFAICS, when the pte is dirtied, we should get a page fault, which
> causes the page itself to be marked dirty by the nfs_vm_page_mkwrite()
> callback.
> When the page gets written out, the VM calls clear_page_dirty_for_io()
> which also causes the pte to be cleaned.
> 
> At what point can you therefore have a situation where the pte is dirty
> without the page being marked as dirty too?

 

Re: NFS BUG_ON in nfs_do_writepage

[Trond Myklebust]

On Tue, 2009-04-28 at 13:54 +0200, Nick Piggin wrote:
> If the page gets cleaned after page_mkwrite and before the page
> table locks are taken again in order to set the pte writeable.
> (actually, page_mkclean only runs if it finds mapcount elevated,
> so it is enough to clean the page even after the locks are taken
> and before mapcount is incremented in the case of __do_fault).

OK. This was what I was missing...

Thanks!