All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "dhowells@redhat.com" <dhowells@redhat.com>,
	"linux-cachefs@redhat.com" <linux-cachefs@redhat.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
	"dwysocha@redhat.com" <dwysocha@redhat.com>
Subject: 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
Date: Fri, 1 Oct 2021 20:42:33 +0100	[thread overview]
Message-ID: <YVdkqUrwcPUqhF6d@casper.infradead.org> (raw)
In-Reply-To: <23033528036059af4633f60b8325e48eab95ac36.camel@hammerspace.com>

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.

  reply	other threads:[~2021-10-01 19:43 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-17 15:04 [RFC PATCH v2 0/8] fscache: Replace and remove old I/O API v2 David Howells
2021-09-17 15:04 ` [PATCH v2 1/8] fscache: Generalise the ->begin_read_operation method David Howells
2021-09-17 15:04 ` [PATCH v2 2/8] fscache: Implement a fallback I/O interface to replace the old API David Howells
2021-09-17 15:04 ` [PATCH v2 3/8] nfs: Move to using the alternate fallback fscache I/O API David Howells
2021-09-29 14:44   ` David Wysochanski
2021-10-01 14:42   ` David Howells
     [not found]   ` <CALF+zO=165sRYRaxPpDS7DaQCpTe-YOa4FamSoMy5FV2uuG5Yg@mail.gmail.com>
2021-10-01 14:29     ` Trond Myklebust
2021-10-01 14:51     ` Can the GFP flags to releasepage() be trusted? -- was " David Howells
2021-10-01 15:04       ` Trond Myklebust
2021-10-01 19:42         ` Matthew Wilcox [this message]
2021-10-05 13:15       ` David Howells
2021-09-17 15:04 ` [PATCH v2 4/8] 9p: (untested) Convert to using the netfs helper lib to do reads and caching David Howells
2021-09-17 15:05 ` [PATCH v2 5/8] cifs: (untested) Move to using the alternate fallback fscache I/O API David Howells
2021-09-17 15:05 ` [PATCH v2 6/8] fscache: Remove the old " David Howells
2021-09-17 15:05 ` [PATCH v2 7/8] fscache: Remove stats that are no longer used David Howells
2021-09-17 15:05 ` [PATCH v2 8/8] fscache: Update the documentation to reflect I/O API changes David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVdkqUrwcPUqhF6d@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=anna.schumaker@netapp.com \
    --cc=dhowells@redhat.com \
    --cc=dwysocha@redhat.com \
    --cc=linux-cachefs@redhat.com \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trondmy@hammerspace.com \
    --subject='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' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.