[PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

If try_to_release_page() is called with a zero gfp mask, then the
filesystem is effectively denied the possibility of sleeping while
attempting to release the page. There doesn't appear to be any valid
reason why this should be banned, given that we're not calling this from
a memory allocation context.
 
For this reason, change the gfp_mask argument of the call to GFP_KERNEL.
    
Note: I am less sure of what the callers of invalidate_complete_page()
require, and so this patch does not touch that mask.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
diff --git a/mm/truncate.c b/mm/truncate.c
index f4edbc1..49c1ffd 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -302,7 +302,7 @@ invalidate_complete_page2(struct address
 	if (page->mapping != mapping)
 		return 0;
 
-	if (PagePrivate(page) && !try_to_release_page(page, 0))
+	if (PagePrivate(page) && !try_to_release_page(page, GFP_KERNEL))
 		return 0;
 
 	write_lock_irq(&mapping->tree_lock);

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Steve Dickson]

Trond Myklebust wrote:
> If try_to_release_page() is called with a zero gfp mask, then the
> filesystem is effectively denied the possibility of sleeping while
> attempting to release the page. There doesn't appear to be any valid
> reason why this should be banned, given that we're not calling this from
> a memory allocation context.
>  
> For this reason, change the gfp_mask argument of the call to GFP_KERNEL.
>     
> Note: I am less sure of what the callers of invalidate_complete_page()
> require, and so this patch does not touch that mask.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> diff --git a/mm/truncate.c b/mm/truncate.c
> index f4edbc1..49c1ffd 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -302,7 +302,7 @@ invalidate_complete_page2(struct address
>  	if (page->mapping != mapping)
>  		return 0;
>  
> -	if (PagePrivate(page) && !try_to_release_page(page, 0))
> +	if (PagePrivate(page) && !try_to_release_page(page, GFP_KERNEL))
>  		return 0;
>  
>  	write_lock_irq(&mapping->tree_lock);
Well I was using mapping_gfp_mask(mapping) as the argument to
try_to_release_page() which also worked... but isn't this
just plugging one of many holes? Meaning try_to_release_page is called
from a number of places with a zero gfp_mask so shouldn't those
also be fixed as well OR removed the gfp_mask as an argument as the
comment at the top of try_to_release_page() alludes to?

steved.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

On Fri, 2006-10-06 at 17:49 -0400, Steve Dickson wrote:
> Trond Myklebust wrote:
> > If try_to_release_page() is called with a zero gfp mask, then the
> > filesystem is effectively denied the possibility of sleeping while
> > attempting to release the page. There doesn't appear to be any valid
> > reason why this should be banned, given that we're not calling this from
> > a memory allocation context.
> >  
> > For this reason, change the gfp_mask argument of the call to GFP_KERNEL.
> >     
> > Note: I am less sure of what the callers of invalidate_complete_page()
> > require, and so this patch does not touch that mask.
> > 
> > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> > ---
> > diff --git a/mm/truncate.c b/mm/truncate.c
> > index f4edbc1..49c1ffd 100644
> > --- a/mm/truncate.c
> > +++ b/mm/truncate.c
> > @@ -302,7 +302,7 @@ invalidate_complete_page2(struct address
> >  	if (page->mapping != mapping)
> >  		return 0;
> >  
> > -	if (PagePrivate(page) && !try_to_release_page(page, 0))
> > +	if (PagePrivate(page) && !try_to_release_page(page, GFP_KERNEL))
> >  		return 0;
> >  
> >  	write_lock_irq(&mapping->tree_lock);
> Well I was using mapping_gfp_mask(mapping) as the argument to
> try_to_release_page() which also worked... but isn't this
> just plugging one of many holes?

Yeah using mapping_gfp_mask(mapping) sounds like a better option.

...and yes, there are other callers that need to be audited. I'm
particularly curious about the effect of the call in
block_invalidatepage() on XFS, which has a similar test to ours.

The call in fallback_migrate_page() should probably be using
mapping_gfp_mask() too.

> Meaning try_to_release_page is called
> from a number of places with a zero gfp_mask so shouldn't those
> also be fixed as well OR removed the gfp_mask as an argument as the
> comment at the top of try_to_release_page() alludes to?

Nope. In order to make it work correctly with NFS and XFS, all calls to
try_to_release_page() would have to be allowed to be blocking. The
problem is that shrink_page_list() still wants to call
try_to_release_page() from a memory allocation context.

Cheers,
  Trond

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 174 bytes --]

On Fri, 2006-10-06 at 18:16 -0400, Trond Myklebust wrote:
> Yeah using mapping_gfp_mask(mapping) sounds like a better option.

Revised patch is attached...

Cheers,
  Trond


[-- Attachment #2: linux-2.6.18-005-fix_gfp_mask_in_invalidate_complete_page2.dif --]
[-- Type: message/rfc822, Size: 362 bytes --]

From: Trond Myklebust <Trond.Myklebust@netapp.com>
Subject: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2
Date: Fri Oct 6 17:31:47 2006 -0400
Message-ID: <1160173167.12253.18.camel@lade.trondhjem.org>

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Andrew Morton]

On Fri, 06 Oct 2006 18:19:27 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2006-10-06 at 18:16 -0400, Trond Myklebust wrote:
> > Yeah using mapping_gfp_mask(mapping) sounds like a better option.
> 
> Revised patch is attached...

Well, it wasn't attached, but I can simulate it.

invalidate_complete_page() wants to be called from inside spinlocks by
drop_pagecache(), so if we wanted to pull the same trick there we'd need to
pass a new flag into invalidate_inode_pages().

It's not 100% clear what the gfp_t _means_ in the try_to_release_page()
context.  Callees will rarely want to allocate memory (true?).  So it
conveys two concepts: 

a) can sleep. (__GFP_WAIT).  That's fairly straightforward

b) can take fs locks (__GFP_FS).  This is less clear.  By passing down
   __GFP_FS we're telling the callee that it's OK to take i_mutex, even
   lock_page().  That sounds pretty unsafe in this context, particularly
   the latter, as we're already holding a page lock.

So perhaps the safer and more appropriate solution here is to pass in a
bare __GFP_WAIT.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Steve Dickson]

Andrew Morton wrote:
> On Fri, 06 Oct 2006 18:19:27 -0400
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> 
>>On Fri, 2006-10-06 at 18:16 -0400, Trond Myklebust wrote:
>>
>>>Yeah using mapping_gfp_mask(mapping) sounds like a better option.
>>
>>Revised patch is attached...
> 
> 
> Well, it wasn't attached, but I can simulate it.
> 
> invalidate_complete_page() wants to be called from inside spinlocks by
> drop_pagecache(), so if we wanted to pull the same trick there we'd need to
> pass a new flag into invalidate_inode_pages().
That seems abit broken (wrt performance) that drop_pagecache_sb() holds
the fairly popular inode_lock while it invalidate pages...
Nobody else seem to...

> 
> It's not 100% clear what the gfp_t _means_ in the try_to_release_page()
> context.  Callees will rarely want to allocate memory (true?).  So it
> conveys two concepts: 
> 
> a) can sleep. (__GFP_WAIT).  That's fairly straightforward
> 
> b) can take fs locks (__GFP_FS).  This is less clear.  By passing down
>    __GFP_FS we're telling the callee that it's OK to take i_mutex, even
>    lock_page().  That sounds pretty unsafe in this context, particularly
>    the latter, as we're already holding a page lock.
> 
> So perhaps the safer and more appropriate solution here is to pass in a
> bare __GFP_WAIT.
I agree... __GFP_WAIT does seem to be a bit more straightforward...
either way is find.. as long as it cause NFS to flush its pages...

steved.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Andrew Morton]

On Fri, 06 Oct 2006 19:09:29 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> > Well, it wasn't attached, but I can simulate it.
> > 
> > invalidate_complete_page() wants to be called from inside spinlocks by
> > drop_pagecache(), so if we wanted to pull the same trick there we'd need to
> > pass a new flag into invalidate_inode_pages().
> That seems abit broken (wrt performance) that drop_pagecache_sb() holds
> the fairly popular inode_lock while it invalidate pages...
> Nobody else seem to...

Yes, it was very rude of me.  But it's just a debugging thing, and is
privileged.

I removed a cond_resched() from in there in the process.  If we make the
invalidate_inode_pages() caller pass in a must_be_atomic flag then the
cond_resched() can be resuscitated.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Andrew Morton]

On Fri, 06 Oct 2006 19:09:29 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> > 
> > It's not 100% clear what the gfp_t _means_ in the try_to_release_page()
> > context.  Callees will rarely want to allocate memory (true?).  So it
> > conveys two concepts: 
> > 
> > a) can sleep. (__GFP_WAIT).  That's fairly straightforward
> > 
> > b) can take fs locks (__GFP_FS).  This is less clear.  By passing down
> >    __GFP_FS we're telling the callee that it's OK to take i_mutex, even
> >    lock_page().  That sounds pretty unsafe in this context, particularly
> >    the latter, as we're already holding a page lock.
> > 
> > So perhaps the safer and more appropriate solution here is to pass in a
> > bare __GFP_WAIT.
> I agree... __GFP_WAIT does seem to be a bit more straightforward...
> either way is find.. as long as it cause NFS to flush its pages...

Except NFS looks at __GFP_FS, so __GFP_WAIT won't help.

Oh well.  Passing __GFP_FS in here sort-of implies that it's OK to run
lock_page(), but if a ->releasepage() impementation tries to lock the page
it's passed then it needs its head read.

I made it GFP_KERNEL.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> -	if (PagePrivate(page) && !try_to_release_page(page, 0))
> +	if (PagePrivate(page) && !try_to_release_page(page, GFP_KERNEL))

This can't be the right way to fix things.  try_to_release_page() may fail
whatever GFP flags you give it.  If the page *must* be invalidated at this
point then you _must_ call the invalidatepage() op, not the releasepage() op.

What is currently there allows an instikill to take place on a page at this
point, but defers the kill on pages that are busy until all the pages have
been scanned once, thus giving I/O time to happen on busy pages whilst reaping
pages that can be made immediately available.

David

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

On Tue, 2006-10-10 at 10:43 +0100, David Howells wrote:
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > -	if (PagePrivate(page) && !try_to_release_page(page, 0))
> > +	if (PagePrivate(page) && !try_to_release_page(page, GFP_KERNEL))
> 
> This can't be the right way to fix things.  try_to_release_page() may fail
> whatever GFP flags you give it.  If the page *must* be invalidated at this
> point then you _must_ call the invalidatepage() op, not the releasepage() op.

No. Invalidatepage does precisely the wrong thing: it invalidates dirty
data instead of committing it to disk. If you need to have the data
invalidated, then you should call truncate_inode_pages().

Trond

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Steve Dickson]

Trond Myklebust wrote:
> 
> No. Invalidatepage does precisely the wrong thing: it invalidates dirty
> data instead of committing it to disk. If you need to have the data
> invalidated, then you should call truncate_inode_pages().
Just curious... would it make sense to call truncate_inode_pages()
to purge the the readdir cache? Meaning, in nfs_revalidate_mapping()
truncate_inode_pages() would be called for S_ISDIR inodes?

It seems to me that it would be more of a decisive why to ensure
the readdir cache is purged...

steved.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

On Tue, 2006-10-10 at 08:18 -0400, Steve Dickson wrote:
> 
> Trond Myklebust wrote:
> > 
> > No. Invalidatepage does precisely the wrong thing: it invalidates dirty
> > data instead of committing it to disk. If you need to have the data
> > invalidated, then you should call truncate_inode_pages().
> Just curious... would it make sense to call truncate_inode_pages()
> to purge the the readdir cache? Meaning, in nfs_revalidate_mapping()
> truncate_inode_pages() would be called for S_ISDIR inodes?

Why? If, as in the case of an NFS directory, there are no dirty pages
then the two are supposed to be 100% equivalent.

Trond

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Steve Dickson]

Trond Myklebust wrote:
> On Tue, 2006-10-10 at 08:18 -0400, Steve Dickson wrote:
>> Trond Myklebust wrote:
>>> No. Invalidatepage does precisely the wrong thing: it invalidates dirty
>>> data instead of committing it to disk. If you need to have the data
>>> invalidated, then you should call truncate_inode_pages().
>> Just curious... would it make sense to call truncate_inode_pages()
>> to purge the the readdir cache? Meaning, in nfs_revalidate_mapping()
>> truncate_inode_pages() would be called for S_ISDIR inodes?
> 
> Why? If, as in the case of an NFS directory, there are no dirty pages
> then the two are supposed to be 100% equivalent.
Well as you know, lately we've had problems with 
invalidate_inode_pages2() failing to invalidate pages (regardless of
their state). So I was thinking truncate_inode_pages() might be
better for directories since there seem to be more a guarantee that
the pages will be gone with truncate_inode_pages() than
invalidate_inode_pages2() (due to the fact there will not be any
dirty pages).

But since you have to call truncate_inode_pages under the
inode->i_mutex, there might be a performance hit...

steved.

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

On Tue, 2006-10-10 at 09:22 -0400, Steve Dickson wrote:
> 
> Trond Myklebust wrote: 
> > Why? If, as in the case of an NFS directory, there are no dirty pages
> > then the two are supposed to be 100% equivalent.
> Well as you know, lately we've had problems with 
> invalidate_inode_pages2() failing to invalidate pages (regardless of
> their state). So I was thinking truncate_inode_pages() might be
> better for directories since there seem to be more a guarantee that
> the pages will be gone with truncate_inode_pages() than
> invalidate_inode_pages2() (due to the fact there will not be any
> dirty pages).

truncate_inode_pages and invalidate_inode_pages2 are supposed to result
in exactly the same behaviour on NFS directories. If they don't then
that would be a bug.

  Trond

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> No. Invalidatepage does precisely the wrong thing: it invalidates dirty
> data instead of committing it to disk. If you need to have the data
> invalidated, then you should call truncate_inode_pages().

Hmmm... Good point, but you still need to handle try_to_release_page() failing,
but that only means checking the return value of invalidate_inode_pages2_range
(which you don't do, I notice).  Or is it defined that if must succeed if
__GFP_WAIT is set?

With the two-phase thing, I think I'm thinking of the wrong portion of that
file (I'm thinking of truncate_inode_pages_range()).

Should invalidate_inode_pages2_range() take a gfp_t argument to pass on down?

David

Re: [PATCH] VM: Fix the gfp_mask in invalidate_complete_page2

[Trond Myklebust]

On Tue, 2006-10-10 at 13:49 +0100, David Howells wrote:
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > No. Invalidatepage does precisely the wrong thing: it invalidates dirty
> > data instead of committing it to disk. If you need to have the data
> > invalidated, then you should call truncate_inode_pages().
> 
> Hmmm... Good point, but you still need to handle try_to_release_page() failing,
> but that only means checking the return value of invalidate_inode_pages2_range
> (which you don't do, I notice).  Or is it defined that if must succeed if
> __GFP_WAIT is set?

The only way for it to fail if __GFP_IO is set, is if someone kills the
process.
Note that since the NFS client itself will flush out all dirty data to
disk prior to calling invalidate_inode_pages2, the only thing we're
trying to do here is to avoid races while invalidating those pages.

> With the two-phase thing, I think I'm thinking of the wrong portion of that
> file (I'm thinking of truncate_inode_pages_range()).
> 
> Should invalidate_inode_pages2_range() take a gfp_t argument to pass on down?

Maybe. Do any of the current callers care? NFS should be quite happy
with the patch that is currently in Andrew's tree.

  Trond