Re: [PATCH] mm: Fix error handling in __filemap_get_folio() with FGP_NOWAIT

["Raphael S. Carvalho" <raphaelsc@scylladb.com>]

On Mon, Feb 24, 2025 at 1:14 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Feb 23, 2025 at 08:57:19PM -0300, Raphael S. Carvalho wrote:
> > This is likely a regression caused by 66dabbb65d67 ("mm: return an ERR_PTR
> > from __filemap_get_folio"), which performed the following changes:
> >     --- a/fs/iomap/buffered-io.c
> >     +++ b/fs/iomap/buffered-io.c
> >     @@ -468,19 +468,12 @@ EXPORT_SYMBOL_GPL(iomap_is_partially_uptodate);
> >     struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos)
> >     {
> >             unsigned fgp = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE | FGP_NOFS;
> >     -       struct folio *folio;
> >
> >             if (iter->flags & IOMAP_NOWAIT)
> >                     fgp |= FGP_NOWAIT;
> >
> >     -       folio = __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
> >     +       return __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
> >                             fgp, mapping_gfp_mask(iter->inode->i_mapping));
> >     -       if (folio)
> >     -               return folio;
> >     -
> >     -       if (iter->flags & IOMAP_NOWAIT)
> >     -               return ERR_PTR(-EAGAIN);
> >     -       return ERR_PTR(-ENOMEM);
> >     }
>
> We don't usually put this in the changelog ...
>
> > Essentially, that patch is moving error picking decision to
> > __filemap_get_folio, but it missed proper FGP_NOWAIT handling, so ENOMEM
> > is being escaped to user space. Had it correctly returned -EAGAIN with NOWAIT,
> > either io_uring or user space itself would be able to retry the request.
> > It's not enough to patch io_uring since the iomap interface is the one
> > responsible for it, and pwritev2(RWF_NOWAIT) and AIO interfaces must return
> > the proper error too.
> >
> > The patch was tested with scylladb test suite (its original reproducer), and
> > the tests all pass now when memory is pressured.
> >
> > Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
>
> Instead, we add:
>
> Fixes: 66dabbb65d67 (mm: return an ERR_PTR from __filemap_get_folio)

Thanks, will fix it in v2.

>
> > ---
> >  mm/filemap.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 804d7365680c..b06bd6eedaf7 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1986,8 +1986,15 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
> >
> >               if (err == -EEXIST)
> >                       goto repeat;
> > -             if (err)
> > +             if (err) {
> > +                     /*
> > +                      * Presumably ENOMEM, either from when allocating or
> > +                      * adding folio (this one for xarray node)
> > +                      */
>
> I don't like the comment.  Better to do that in code:
>

Initially I was doing exactly what you proposed above, but after
reading do_read_cache_folio() and the patch the introduces the
regression, which transforms failure to get a folio (a NULL) with
FGP_NOWAIT into NOAGAIN, I decided to do this, but it's indeed better
to remove assumptions. Not ideal for the long run. Will change in v2.
thanks.