Yet another borken page_count() check in invalidate_inode_pages2()....

Yet another borken page_count() check in invalidate_inode_pages2()....

[Trond Myklebust]

I'm once again getting bogus errors from invalidate_inode_pages2() due
to a VM bug. See the third line of remove_mapping().

Cheers,
  Trond

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Andrew Morton]

On Wed, 15 Nov 2006 00:33:39 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> I'm once again getting bogus errors from invalidate_inode_pages2() due
> to a VM bug. See the third line of remove_mapping().
> 

invalidate_inode_pages2() doesn't use remove_mapping().

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Trond Myklebust]

On Wed, 2006-11-15 at 00:33 -0500, Trond Myklebust wrote:
> I'm once again getting bogus errors from invalidate_inode_pages2() due
> to a VM bug. See the third line of remove_mapping().

Argh... Never try debugging past midnight: the above was a red herring.
I've been hitting the WARN_ON in invalidate_inode_pages2() reliably when
running NetApp's simulated i/o tool on the NFS client. It looks as if
I'm hitting a race in which writeback starts on the page after the call
to wait_on_page_writeback(), probably as a consequence of
unmap_mapping_range().

Anyhow, when we call try_to_release_page() with the GFP_WAIT argument,
it seems unnecessary that it should fail immediately if the page is
under writeback. How about something like the following patch?

Cheers,
 Trond
------------------------------------------------------------------
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Wed, 15 Nov 2006 08:02:30 -0500
MM: Fix a loophole in try_to_release_page()

The following patch allows try_to_release_page() to wait on page writeback
instead of failing if the user specified __GFP_WAIT.

The reason is that when running NetApp's simulated I/O tool (sio_ntap) on
the NFS client, I can currently reliably trigger the WARN_ON() in
invalidate_inode_pages2().
Whereas we do wait on page_writeback in invalidate_inode_pages2_range(), we
do so before we unmap the page. There is still a race which will cause the
call to try_to_release_page() to fail the test for PageWriteback(page).

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 mm/filemap.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 7b84dc8..d37f77b 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2445,7 +2445,9 @@ int try_to_release_page(struct page *pag
 	struct address_space * const mapping = page->mapping;
 
 	BUG_ON(!PageLocked(page));
-	if (PageWriteback(page))
+	if (gfp_mask & __GFP_WAIT)
+		wait_on_page_writeback(page);
+	else if (PageWriteback(page))
 		return 0;
 
 	if (mapping && mapping->a_ops->releasepage)

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Andrew Morton]

On Wed, 15 Nov 2006 08:18:09 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> The following patch allows try_to_release_page() to wait on page writeback
> instead of failing if the user specified __GFP_WAIT.
> 
> The reason is that when running NetApp's simulated I/O tool (sio_ntap) on
> the NFS client, I can currently reliably trigger the WARN_ON() in
> invalidate_inode_pages2().
> Whereas we do wait on page_writeback in invalidate_inode_pages2_range(), we
> do so before we unmap the page. There is still a race which will cause the
> call to try_to_release_page() to fail the test for PageWriteback(page).
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  mm/filemap.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 7b84dc8..d37f77b 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2445,7 +2445,9 @@ int try_to_release_page(struct page *pag
>  	struct address_space * const mapping = page->mapping;
>  
>  	BUG_ON(!PageLocked(page));
> -	if (PageWriteback(page))
> +	if (gfp_mask & __GFP_WAIT)
> +		wait_on_page_writeback(page);
> +	else if (PageWriteback(page))
>  		return 0;
>  
>  	if (mapping && mapping->a_ops->releasepage)

The change probably makes sense.  Need to think about that a bit more and
review callers..

But I don't see how it can change invalidate_inode_pages2().  What we
would effectively have is: 

invalidate_inode_pages2_range()
{
	lock_page(page);
	wait_on_page_writeback(page);

	...

				
	wait_on_page_writeback(page);

but nobody could have started another writeback after the "..." because they
couldn't have got the lock_page(), and lock_page() is required for
->writepage()?

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Trond Myklebust]

On Wed, 2006-11-15 at 08:46 -0800, Andrew Morton wrote:

> but nobody could have started another writeback after the "..." because they
> couldn't have got the lock_page(), and lock_page() is required for
> ->writepage()?

Nothing can have called writepage(), but something may be calling
->writepages(). That may call set_page_writeback without taking the page
lock.

Cheers,
  Trond

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Andrew Morton]

On Wed, 15 Nov 2006 13:05:13 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Wed, 2006-11-15 at 08:46 -0800, Andrew Morton wrote:
> 
> > but nobody could have started another writeback after the "..." because they
> > couldn't have got the lock_page(), and lock_page() is required for
> > ->writepage()?
> 
> Nothing can have called writepage(), but something may be calling
> ->writepages(). That may call set_page_writeback without taking the page
> lock.
> 

The protocol is

	lock_page()
	set_page_writeback()
	->writepage()

and there are various places which assume that nobody will start new
writeout of a locked page.  But I forget where they are - things have always
been this way.

If NFS is running set_page_writeback() against an unlocked page then I
don't know what will break.  I didn't know it was doing that.

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Trond Myklebust]

On Wed, 2006-11-15 at 11:24 -0800, Andrew Morton wrote:

> The protocol is
> 
> 	lock_page()
> 	set_page_writeback()
> 	->writepage()

We're not using ->writepage().

> and there are various places which assume that nobody will start new
> writeout of a locked page.  But I forget where they are - things have always
> been this way.

Huh? There has never been a requirement to lock the page if all you want
to do is call set_page_writeback(). The only reason why we want to do
that at all is to allow the VM to track that the page is under I/O. All
other operations involved in scheduling writes are protected by internal
NFS locks.

Re: Yet another borken page_count() check in invalidate_inode_pages2()....

[Andrew Morton]

On Wed, 15 Nov 2006 15:57:45 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Wed, 2006-11-15 at 11:24 -0800, Andrew Morton wrote:
> 
> > The protocol is
> > 
> > 	lock_page()
> > 	set_page_writeback()
> > 	->writepage()
> 
> We're not using ->writepage().

I think you know what I mean.

> > and there are various places which assume that nobody will start new
> > writeout of a locked page.  But I forget where they are - things have always
> > been this way.
> 
> Huh? There has never been a requirement to lock the page if all you want
> to do is call set_page_writeback().

The protocol is, and always has been

	lock_page()
	set_page_writeback();
	start-io
	unlock_page();

end_io:
	end_page_writeback()


and there are places in the VM which rely upon some or all of that.  I'd
need to go on a big hunt to remember where they are.  One of them is
invalidate_inode_pages2(), as you've just discovered.

> The only reason why we want to do
> that at all is to allow the VM to track that the page is under I/O. All
> other operations involved in scheduling writes are protected by internal
> NFS locks.

Well the VM uses lock_page() for this synchronisation.  If NFS has gone and
decided not to do that then we'll need to either

a) Make NFS follow the protocol or

b) Put stuff in NFS to allow the VM to work correctly (until we change it) or

c) Put very-clearly-commented NFS exception code into the VM.