Re: [PATCH v7 1/2] mm: store zero pages to be swapped out in a bitmap

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 1 Jul 2024 20:15:46 +0300 Usama Arif <usamaarif642@gmail.com> wrote:

> Changes from v7 (Johannes):
> - Give overview and explain how locking works in zeromap in comments
> - Add comment for why last word is checked first when checking if
>   folio is zero-filled
> - Merge is_folio_zero_filled and is_folio_page_zero_filled into
>   1 function.
> - Use folio_zero_range to fill a folio with zero at readtime.
> - Put swap_zeromap_folio_clear in an else branch (even if checkpatch
>   gives warning) and add comment to make it explicitly clear that it
>   needs to happen if folio is not zero filled.
> - add missing kvfree for zeromap incase swapon fails.

I queued the below as a delta against what was in mm-unstable.

Can we please get this nailed down?  It has been nearly three weeks and
this patch is getting in the way of the "mm/zsmalloc: change back to
per-size_class lock" series, at least.

 include/linux/swap.h |    2 -
 mm/page_io.c         |   64 +++++++++++++++++++++--------------------
 mm/swapfile.c        |    9 +++--
 3 files changed, 40 insertions(+), 35 deletions(-)

--- a/include/linux/swap.h~mm-store-zero-pages-to-be-swapped-out-in-a-bitmap-v8
+++ a/include/linux/swap.h
@@ -299,7 +299,7 @@ struct swap_info_struct {
 	signed char	type;		/* strange name for an index */
 	unsigned int	max;		/* extent of the swap_map */
 	unsigned char *swap_map;	/* vmalloc'ed array of usage counts */
-	unsigned long *zeromap;		/* vmalloc'ed bitmap to track zero pages */
+	unsigned long *zeromap;		/* kvmalloc'ed bitmap to track zero pages */
 	struct swap_cluster_info *cluster_info; /* cluster info. Only for SSD */
 	struct swap_cluster_list free_clusters; /* free clusters list */
 	unsigned int lowest_bit;	/* index of first free in swap_map */
--- a/mm/page_io.c~mm-store-zero-pages-to-be-swapped-out-in-a-bitmap-v8
+++ a/mm/page_io.c
@@ -172,42 +172,34 @@ bad_bmap:
 	goto out;
 }
 
-static bool is_folio_page_zero_filled(struct folio *folio, int i)
-{
-	unsigned long *data;
-	unsigned int pos, last_pos = PAGE_SIZE / sizeof(*data) - 1;
-	bool ret = false;
-
-	data = kmap_local_folio(folio, i * PAGE_SIZE);
-	if (data[last_pos])
-		goto out;
-	for (pos = 0; pos < PAGE_SIZE / sizeof(*data); pos++) {
-		if (data[pos])
-			goto out;
-	}
-	ret = true;
-out:
-	kunmap_local(data);
-	return ret;
-}
-
 static bool is_folio_zero_filled(struct folio *folio)
 {
+	unsigned int pos, last_pos;
+	unsigned long *data;
 	unsigned int i;
 
+	last_pos = PAGE_SIZE / sizeof(*data) - 1;
 	for (i = 0; i < folio_nr_pages(folio); i++) {
-		if (!is_folio_page_zero_filled(folio, i))
+		data = kmap_local_folio(folio, i * PAGE_SIZE);
+		/*
+		 * Check last word first, incase the page is zero-filled at
+		 * the start and has non-zero data at the end, which is common
+		 * in real-world workloads.
+		 */
+		if (data[last_pos]) {
+			kunmap_local(data);
 			return false;
+		}
+		for (pos = 0; pos < last_pos; pos++) {
+			if (data[pos]) {
+				kunmap_local(data);
+				return false;
+			}
+		}
+		kunmap_local(data);
 	}
-	return true;
-}
 
-static void folio_zero_fill(struct folio *folio)
-{
-	unsigned int i;
-
-	for (i = 0; i < folio_nr_pages(folio); i++)
-		clear_highpage(folio_page(folio, i));
+	return true;
 }
 
 static void swap_zeromap_folio_set(struct folio *folio)
@@ -278,12 +270,24 @@ int swap_writepage(struct page *page, st
 		return ret;
 	}
 
+	/*
+	 * Use a bitmap (zeromap) to avoid doing IO for zero-filled pages.
+	 * The bits in zeromap are protected by the locked swapcache folio
+	 * and atomic updates are used to protect against read-modify-write
+	 * corruption due to other zero swap entries seeing concurrent updates.
+	 */
 	if (is_folio_zero_filled(folio)) {
 		swap_zeromap_folio_set(folio);
 		folio_unlock(folio);
 		return 0;
+	} else {
+		/*
+		 * Clear bits this folio occupies in the zeromap to prevent
+		 * zero data being read in from any previous zero writes that
+		 * occupied the same swap entries.
+		 */
+		swap_zeromap_folio_clear(folio);
 	}
-	swap_zeromap_folio_clear(folio);
 	if (zswap_store(folio)) {
 		folio_unlock(folio);
 		return 0;
@@ -528,7 +532,7 @@ static bool swap_read_folio_zeromap(stru
 	if (WARN_ON_ONCE(idx < folio_nr_pages(folio)))
 		return true;
 
-	folio_zero_fill(folio);
+	folio_zero_range(folio, 0, folio_size(folio));
 	folio_mark_uptodate(folio);
 	return true;
 }
--- a/mm/swapfile.c~mm-store-zero-pages-to-be-swapped-out-in-a-bitmap-v8
+++ a/mm/swapfile.c
@@ -3181,11 +3181,11 @@ SYSCALL_DEFINE2(swapon, const char __use
 	}
 
 	/*
-	 * Use kvmalloc_array instead of bitmap_zalloc as the allocation order
-	 * might be above MAX_PAGE_ORDER incase of a large swap file.
+	 * Use kvmalloc_array instead of bitmap_zalloc as the allocation order might
+	 * be above MAX_PAGE_ORDER incase of a large swap file.
 	 */
-	p->zeromap = kvmalloc_array(BITS_TO_LONGS(maxpages),
-				sizeof(unsigned long), GFP_KERNEL | __GFP_ZERO);
+	p->zeromap = kvmalloc_array(BITS_TO_LONGS(maxpages), sizeof(long),
+				    GFP_KERNEL | __GFP_ZERO);
 	if (!p->zeromap) {
 		error = -ENOMEM;
 		goto bad_swap_unlock_inode;
@@ -3345,6 +3345,7 @@ bad_swap:
 	p->flags = 0;
 	spin_unlock(&swap_lock);
 	vfree(swap_map);
+	kvfree(p->zeromap);
 	kvfree(cluster_info);
 	if (inced_nr_rotate_swap)
 		atomic_dec(&nr_rotate_swap);
_