Re: [PATCH] page_io: return proper error codes for swap_read_folio_zeromap()

Yosry Ahmed
2025-03-06T23:42:21Z

On Thu, Mar 06, 2025 at 03:00:15PM -0800, Nhat Pham wrote:
> Similar to zswap_load(), also return proper error codes for
> swap_read_folio_zeromap():
>
> * 0 on success. The folio is unlocked and marked up-to-date.
> * -ENOENT, if the folio is entirely not zeromapped.
> * -EINVAL (with the follio unlocked but not marked to date), if the
> folio is partially zeromapped. This is not supported, and will SIGBUS
> the faulting process.
>
> This patch is purely a clean-up, and should not have any behavioral
> change. It is based on (and should be applied on top of) [1].
>
> [1]: https://lore.kernel.org/linux-mm/20250306205011.784787-1-nphamcs@gmail.com/
>
> Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> ---
> mm/page_io.c | 35 ++++++++++++++++++++++++++---------
> 1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/mm/page_io.c b/mm/page_io.c
> index 4bce19df557b..48ed1e810392 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -511,7 +511,23 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
> mempool_free(sio, sio_pool);
> }
>
> -static bool swap_read_folio_zeromap(struct folio *folio)
> +/**
> + * swap_read_folio_zeromap - check if the folio was zeromapped, and if so,
> + * zero-fill it.
> + * @folio: the folio.
> + *
> + * Return: 0 on success, with the folio zero-filled, unlocked, and marked
> + * up-to-date, or one of the following error codes:
> + *
> + * -ENOENT: the folio is entirely not zeromapped. The folio remains locked.
> + *
> + * -EINVAL: some of the subpages in the folio are zeromaped, but not all of
> + * them. This is an error because we don't currently support a large folio
> + * that is partially in the zeromap. The folio is unlocked, but NOT marked
> + * up-to-date, so that an IO error is emitted (e.g. do_swap_page() will
> + * sigbus).

This is a bit repetitive. Maybe:

* -EINVAL: The folio is partially in the zeromap, which is not
* currently supported. The folio is unlocked, but NOT marked
* up-to-date, so that an IO error is emitted (e.g. do_swap_page() will
* sigbus).

> + */
> +static int swap_read_folio_zeromap(struct folio *folio)
> {
> int nr_pages = folio_nr_pages(folio);
> struct obj_cgroup *objcg;
> @@ -519,15 +535,17 @@ static bool swap_read_folio_zeromap(struct folio *folio)
>
> /*
> * Swapping in a large folio that is partially in the zeromap is not
> - * currently handled. Return true without marking the folio uptodate so
> + * currently handled. Return -EINVAL without marking the folio uptodate so
> * that an IO error is emitted (e.g. do_swap_page() will sigbus).
> */

I would drop this whole comment now because it's mostly repeating what's
now documneted above.

With the comments fixed up:

Reviewed-by: Yosry Ahmed <yosry.ahmed@linux.dev>

> if (WARN_ON_ONCE(swap_zeromap_batch(folio->swap, nr_pages,
> - &is_zeromap) != nr_pages))
> - return true;
> + &is_zeromap) != nr_pages)) {
> + folio_unlock(folio);
> + return -EINVAL;
> + }
>
> if (!is_zeromap)
> - return false;
> + return -ENOENT;
>
> objcg = get_obj_cgroup_from_folio(folio);
> count_vm_events(SWPIN_ZERO, nr_pages);
> @@ -538,7 +556,8 @@ static bool swap_read_folio_zeromap(struct folio *folio)
>
> folio_zero_range(folio, 0, folio_size(folio));
> folio_mark_uptodate(folio);
> - return true;
> + folio_unlock(folio);
> + return 0;
> }
>
> static void swap_read_folio_fs(struct folio *folio, struct swap_iocb **plug)
> @@ -635,10 +654,8 @@ void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> }
> delayacct_swapin_start();
>
> - if (swap_read_folio_zeromap(folio)) {
> - folio_unlock(folio);
> + if (swap_read_folio_zeromap(folio) != -ENOENT)
> goto finish;
> - }
>
> if (zswap_load(folio) != -ENOENT)
> goto finish;
> --
> 2.43.5
>