mm: zswap: invaldiate entry after writeback

Commit Message

Domenico Cerasuolo June 14, 2023, 2:31 p.m. UTC

When an entry started writeback, it used to be invalidated with ref
count logic alone, meaning that it would stay on the tree until all
references were put. The problem with this behavior is that as soon as
the writeback started, the ownership of the data held by the entry is
passed to the swapcache and should not be left in zswap too.
Currently there are no known issues because of this, but this change
explicitly invalidates an entry that started writeback to reduce
opportunities for future bugs.

This patch is a follow up on the series titled "mm: zswap: move
writeback LRU from zpool to zswap" with message id
<[e-mail]> in mm-unstable +
commit f090b7949768("mm: zswap: support exclusive loads") that's in
mm-unstable as well.

Suggested-by: Johannes Weiner <[e-mail]>
Signed-off-by: Domenico Cerasuolo <[e-mail]>
---
 mm/zswap.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Comments

Johannes Weiner June 14, 2023, 2:35 p.m. UTC | #1

On Wed, Jun 14, 2023 at 04:31:22PM +0200, Domenico Cerasuolo wrote:
> When an entry started writeback, it used to be invalidated with ref
> count logic alone, meaning that it would stay on the tree until all
> references were put. The problem with this behavior is that as soon as
> the writeback started, the ownership of the data held by the entry is
> passed to the swapcache and should not be left in zswap too.
> Currently there are no known issues because of this, but this change
> explicitly invalidates an entry that started writeback to reduce
> opportunities for future bugs.
> 
> This patch is a follow up on the series titled "mm: zswap: move
> writeback LRU from zpool to zswap" with message id
> <[e-mail]> in mm-unstable +
> commit f090b7949768("mm: zswap: support exclusive loads") that's in
> mm-unstable as well.
> 
> Suggested-by: Johannes Weiner <[e-mail]>
> Signed-off-by: Domenico Cerasuolo <[e-mail]>

Acked-by: Johannes Weiner <[e-mail]>

Thanks Domenico!

Patch

diff --git a/mm/zswap.c b/mm/zswap.c
index 02d0a6f46f4a..c122f042a49d 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -599,6 +599,16 @@  static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
 	return NULL;
 }
 
+static void zswap_invalidate_entry(struct zswap_tree *tree,
+				   struct zswap_entry *entry)
+{
+	/* remove from rbtree */
+	zswap_rb_erase(&tree->rbroot, entry);
+
+	/* drop the initial reference from entry creation */
+	zswap_entry_put(tree, entry);
+}
+
 static int zswap_reclaim_entry(struct zswap_pool *pool)
 {
 	struct zswap_entry *entry;
@@ -644,12 +654,13 @@  static int zswap_reclaim_entry(struct zswap_pool *pool)
 		goto put_unlock;
 	}
 
-	/* Check for invalidate() race */
-	if (entry != zswap_rb_search(&tree->rbroot, swpoffset))
-		goto put_unlock;
-
-	/* Drop base reference */
-	zswap_entry_put(tree, entry);
+	/*
+	 * Writeback started successfully, the page now belongs to the
+	 * swapcache. Drop the entry from zswap - unless invalidate already
+	 * took it out while we had the tree->lock released for IO.
+	 */
+	if (entry == zswap_rb_search(&tree->rbroot, swpoffset))
+		zswap_invalidate_entry(tree, entry);
 
 put_unlock:
 	/* Drop local reference */
@@ -1376,16 +1387,6 @@  static int zswap_frontswap_store(unsigned type, pgoff_t offset,
 	goto reject;
 }
 
-static void zswap_invalidate_entry(struct zswap_tree *tree,
-				   struct zswap_entry *entry)
-{
-	/* remove from rbtree */
-	zswap_rb_erase(&tree->rbroot, entry);
-
-	/* drop the initial reference from entry creation */
-	zswap_entry_put(tree, entry);
-}
-
 /*
  * returns 0 if the page was successfully decompressed
  * return -1 on entry not found or error