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

[Matthew Wilcox]

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
>