On Mon, Feb 24, 2025 at 1:14 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> 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.
