On Fri, 2021-10-01 at 15:51 +0100, David Howells wrote:
> Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
