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

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

[Nhat Pham]

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).
+ */
+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).
 	 */
 	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

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

[Yosry Ahmed]

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
> 

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

[Nhat Pham]

On Thu, Mar 6, 2025 at 3:42 PM Yosry Ahmed <yosry.ahmed@linux.dev> wrote:
>
> > + *
> > + *  -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).
>
>
>
> 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>
>

Ah I like both suggestions :) Gimme one sec to send out a fixlet.

Thanks, Yosry!

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

[Johannes Weiner]

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>

Nice! With Yosry's two suggestions:

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

[Chengming Zhou]

On 2025/3/7 07:00, 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>

Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>

Thanks!

> ---
>   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).
> + */
> +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).
>   	 */
>   	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;

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

[Nhat Pham]

Fix the comments for swap_read_folio_zeromap().

Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev>
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 mm/page_io.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/mm/page_io.c b/mm/page_io.c
index 48ed1e810392..30ec132c801b 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -521,9 +521,8 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
  *
  *  -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
+ *  -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).
  */
@@ -533,11 +532,6 @@ static int swap_read_folio_zeromap(struct folio *folio)
 	struct obj_cgroup *objcg;
 	bool is_zeromap;
 
-	/*
-	 * Swapping in a large folio that is partially in the zeromap is not
-	 * currently handled. Return -EINVAL without marking the folio uptodate so
-	 * that an IO error is emitted (e.g. do_swap_page() will sigbus).
-	 */
 	if (WARN_ON_ONCE(swap_zeromap_batch(folio->swap, nr_pages,
 			&is_zeromap) != nr_pages)) {
 		folio_unlock(folio);
-- 
2.43.5

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

[Nhat Pham]

On Thu, Mar 6, 2025 at 3:00 PM Nhat Pham <nphamcs@gmail.com> 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>

Hi Andrew, I think Yosry and Johannes signed off on this patch (with
the fixlet). Looks like it was not merged for 6.15-rc1 - let me know
if there is something I need to do.

It's mostly a clean up with no (intended) behavioral change, so no
rush of course.

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

[Andrew Morton]

On Tue, 1 Apr 2025 11:55:31 -0700 Nhat Pham <nphamcs@gmail.com> wrote:

> On Thu, Mar 6, 2025 at 3:00 PM Nhat Pham <nphamcs@gmail.com> 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>
> 
> Hi Andrew, I think Yosry and Johannes signed off on this patch (with
> the fixlet). Looks like it was not merged for 6.15-rc1 - let me know
> if there is something I need to do.

Thanks, seems it fell through a crack.

> It's mostly a clean up with no (intended) behavioral change, so no
> rush of course.

Yup, I'll toss it onto the after-merge-window pile.