Can the GFP flags to releasepage() be trusted? -- was Re: [PATCH v2 3/8] nfs: Move to using the alternate fallback fscache I/O API

Can the GFP flags to releasepage() be trusted? -- was Re: [PATCH v2 3/8] nfs: Move to using the alternate fallback fscache I/O API

[David Howells]

Trond Myklebust <trondmy@hammerspace.com> wrote:

> > > @@ -432,7 +432,12 @@ static int nfs_release_page(struct page *page, gfp_t gfp)
> > >  	/* If PagePrivate() is set, then the page is not freeable */
> > >  	if (PagePrivate(page))
> > >  		return 0;
> > > -	return nfs_fscache_release_page(page, gfp);
> > > +	if (PageFsCache(page)) {
> > > +		if (!(gfp & __GFP_DIRECT_RECLAIM) || !(gfp & __GFP_FS))
> > > +			return false;
> > > +		wait_on_page_fscache(page);
> > > +	}
> > > +	return true;
> > >  }
> 
> I've found this generally not to be safe. The VM calls ->release_page()
> from a variety of contexts, and often fails to report it correctly in
> the gfp flags. That's particularly true of the stuff in mm/vmscan.c.
> This is why we have the check above that vetos page removal upon
> PagePrivate() being set.

[Adding Willy and the mm crew to the cc list]

I wonder if that matters in this case.  In the worst case, we'll wait for the
page to cease being DMA'd - but we won't return true if it is.

But if vmscan is generating the wrong VM flags, we should look at fixing that.

David

Re: Can the GFP flags to releasepage() be trusted? -- was Re: [PATCH v2 3/8] nfs: Move to using the alternate fallback fscache I/O API

[Trond Myklebust]

On Fri, 2021-10-01 at 15:51 +0100, David Howells wrote:
> Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> > > > @@ -432,7 +432,12 @@ static int nfs_release_page(struct page
> > > > *page, gfp_t gfp)
> > > >         /* If PagePrivate() is set, then the page is not
> > > > freeable */
> > > >         if (PagePrivate(page))
> > > >                 return 0;
> > > > -       return nfs_fscache_release_page(page, gfp);
> > > > +       if (PageFsCache(page)) {
> > > > +               if (!(gfp & __GFP_DIRECT_RECLAIM) || !(gfp &
> > > > __GFP_FS))
> > > > +                       return false;
> > > > +               wait_on_page_fscache(page);
> > > > +       }
> > > > +       return true;
> > > >  }
> > 
> > I've found this generally not to be safe. The VM calls -
> > >release_page()
> > from a variety of contexts, and often fails to report it correctly
> > in
> > the gfp flags. That's particularly true of the stuff in
> > mm/vmscan.c.
> > This is why we have the check above that vetos page removal upon
> > PagePrivate() being set.
> 
> [Adding Willy and the mm crew to the cc list]
> 
> I wonder if that matters in this case.  In the worst case, we'll wait
> for the
> page to cease being DMA'd - but we won't return true if it is.
> 
> But if vmscan is generating the wrong VM flags, we should look at
> fixing that.
> 
> 

To elaborate a bit: we used to have code here that would check whether
the page had been cleaned but was unstable, and if an argument of
GFP_KERNEL or above was set, we'd try to call COMMIT to ensure the page
was synched to disk on the server (and we'd wait for that call to
complete).

That code would end up deadlocking in all sorts of horrible ways, so we
ended up having to pull it.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: Can the GFP flags to releasepage() be trusted? -- was Re: [PATCH v2 3/8] nfs: Move to using the alternate fallback fscache I/O API

[Matthew Wilcox]

On Fri, Oct 01, 2021 at 03:04:08PM +0000, Trond Myklebust wrote:
> On Fri, 2021-10-01 at 15:51 +0100, David Howells wrote:
> > Trond Myklebust <trondmy@hammerspace.com> wrote:
> > 
> > > > > @@ -432,7 +432,12 @@ static int nfs_release_page(struct page
> > > > > *page, gfp_t gfp)
> > > > >         /* If PagePrivate() is set, then the page is not
> > > > > freeable */
> > > > >         if (PagePrivate(page))
> > > > >                 return 0;
> > > > > -       return nfs_fscache_release_page(page, gfp);
> > > > > +       if (PageFsCache(page)) {
> > > > > +               if (!(gfp & __GFP_DIRECT_RECLAIM) || !(gfp &
> > > > > __GFP_FS))
> > > > > +                       return false;
> > > > > +               wait_on_page_fscache(page);
> > > > > +       }
> > > > > +       return true;
> > > > >  }
> > > 
> > > I've found this generally not to be safe. The VM calls -
> > > >release_page()
> > > from a variety of contexts, and often fails to report it correctly
> > > in
> > > the gfp flags. That's particularly true of the stuff in
> > > mm/vmscan.c.
> > > This is why we have the check above that vetos page removal upon
> > > PagePrivate() being set.
> > 
> > [Adding Willy and the mm crew to the cc list]
> > 
> > I wonder if that matters in this case.  In the worst case, we'll wait
> > for the
> > page to cease being DMA'd - but we won't return true if it is.
> > 
> > But if vmscan is generating the wrong VM flags, we should look at
> > fixing that.
> > 
> > 
> 
> To elaborate a bit: we used to have code here that would check whether
> the page had been cleaned but was unstable, and if an argument of
> GFP_KERNEL or above was set, we'd try to call COMMIT to ensure the page
> was synched to disk on the server (and we'd wait for that call to
> complete).
> 
> That code would end up deadlocking in all sorts of horrible ways, so we
> ended up having to pull it.

Based on having read zero code at all in this area ...

Is it possible that you can wait for an existing operation to finish,
but starting a new operation will take a lock that is already being
held somewhere in your call chain?  So it's not that the gfp flags are
being set incorrectly, it's just that you're not in a context where you
can start a new operation.

Re: Can the GFP flags to releasepage() be trusted? -- was Re: [PATCH v2 3/8] nfs: Move to using the alternate fallback fscache I/O API

[David Howells]

Trond Myklebust <trondmy@hammerspace.com> wrote:

> To elaborate a bit: we used to have code here that would check whether
> the page had been cleaned but was unstable, and if an argument of
> GFP_KERNEL or above was set, we'd try to call COMMIT to ensure the page
> was synched to disk on the server (and we'd wait for that call to
> complete).
> 
> That code would end up deadlocking in all sorts of horrible ways, so we
> ended up having to pull it.

I don't think that a deadlock should be possible with this.  PG_fscache is now
only being used to indicate that a DIO write to the cache is in progress on
the page.  It will complete and remove the mark at some point.

David