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)
> ---
> 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:
if ((fgp_flags & FGP_NOWAIT) && (err == -ENOMEM))
> + if (fgp_flags & FGP_NOWAIT)
> + err = -EAGAIN;
> return ERR_PTR(err);
> + }
> /*
> * filemap_add_folio locks the page, and for mmap
> * we expect an unlocked page.
> --
> 2.48.1
>
