The __read_swap_cache_async() interface isn't more difficult to
understand than what the helper abstracts. Save the indirection and a
level of indentation for the primary work of the writeback func.
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
mm/zswap.c | 142 ++++++++++++++++++++---------------------------------
1 file changed, 53 insertions(+), 89 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index e34ac89e6098..bba4ead672be 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1016,43 +1016,6 @@ static int zswap_enabled_param_set(const char *val,
/*********************************
* writeback code
**********************************/
-/* return enum for zswap_get_swap_cache_page */
-enum zswap_get_swap_ret {
- ZSWAP_SWAPCACHE_NEW,
- ZSWAP_SWAPCACHE_EXIST,
- ZSWAP_SWAPCACHE_FAIL,
-};
-
-/*
- * zswap_get_swap_cache_page
- *
- * This is an adaption of read_swap_cache_async()
- *
- * This function tries to find a page with the given swap entry
- * in the swapper_space address space (the swap cache). If the page
- * is found, it is returned in retpage. Otherwise, a page is allocated,
- * added to the swap cache, and returned in retpage.
- *
- * If success, the swap cache page is returned in retpage
- * Returns ZSWAP_SWAPCACHE_EXIST if page was already in the swap cache
- * Returns ZSWAP_SWAPCACHE_NEW if the new page needs to be populated,
- * the new page is added to swapcache and locked
- * Returns ZSWAP_SWAPCACHE_FAIL on error
- */
-static int zswap_get_swap_cache_page(swp_entry_t entry,
- struct page **retpage)
-{
- bool page_was_allocated;
-
- *retpage = __read_swap_cache_async(entry, GFP_KERNEL,
- NULL, 0, &page_was_allocated);
- if (page_was_allocated)
- return ZSWAP_SWAPCACHE_NEW;
- if (!*retpage)
- return ZSWAP_SWAPCACHE_FAIL;
- return ZSWAP_SWAPCACHE_EXIST;
-}
-
/*
* Attempts to free an entry by adding a page to the swap cache,
* decompressing the entry data into the page, and issuing a
@@ -1073,7 +1036,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
struct scatterlist input, output;
struct crypto_acomp_ctx *acomp_ctx;
struct zpool *pool = entry->pool->zpool;
-
+ bool page_was_allocated;
u8 *src, *tmp = NULL;
unsigned int dlen;
int ret;
@@ -1088,65 +1051,66 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
}
/* try to allocate swap cache page */
- switch (zswap_get_swap_cache_page(swpentry, &page)) {
- case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
+ page = __read_swap_cache_async(swpentry, GFP_KERNEL, NULL, 0,
+ &page_was_allocated);
+ if (!page) {
ret = -ENOMEM;
goto fail;
+ }
- case ZSWAP_SWAPCACHE_EXIST:
- /* page is already in the swap cache, ignore for now */
+ /* Found an existing page, we raced with load/swapin */
+ if (!page_was_allocated) {
put_page(page);
ret = -EEXIST;
goto fail;
+ }
- case ZSWAP_SWAPCACHE_NEW: /* page is locked */
- /*
- * Having a local reference to the zswap entry doesn't exclude
- * swapping from invalidating and recycling the swap slot. Once
- * the swapcache is secured against concurrent swapping to and
- * from the slot, recheck that the entry is still current before
- * writing.
- */
- spin_lock(&tree->lock);
- if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
- spin_unlock(&tree->lock);
- delete_from_swap_cache(page_folio(page));
- ret = -ENOMEM;
- goto fail;
- }
+ /*
+ * Page is locked, and the swapcache is now secured against
+ * concurrent swapping to and from the slot. Verify that the
+ * swap entry hasn't been invalidated and recycled behind our
+ * backs (our zswap_entry reference doesn't prevent that), to
+ * avoid overwriting a new swap page with old compressed data.
+ */
+ spin_lock(&tree->lock);
+ if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
spin_unlock(&tree->lock);
+ delete_from_swap_cache(page_folio(page));
+ ret = -ENOMEM;
+ goto fail;
+ }
+ spin_unlock(&tree->lock);
- /* decompress */
- acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
- dlen = PAGE_SIZE;
+ /* decompress */
+ acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
+ dlen = PAGE_SIZE;
- src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
- if (!zpool_can_sleep_mapped(pool)) {
- memcpy(tmp, src, entry->length);
- src = tmp;
- zpool_unmap_handle(pool, entry->handle);
- }
+ src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
+ if (!zpool_can_sleep_mapped(pool)) {
+ memcpy(tmp, src, entry->length);
+ src = tmp;
+ zpool_unmap_handle(pool, entry->handle);
+ }
- mutex_lock(acomp_ctx->mutex);
- sg_init_one(&input, src, entry->length);
- sg_init_table(&output, 1);
- sg_set_page(&output, page, PAGE_SIZE, 0);
- acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);
- ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
- dlen = acomp_ctx->req->dlen;
- mutex_unlock(acomp_ctx->mutex);
-
- if (!zpool_can_sleep_mapped(pool))
- kfree(tmp);
- else
- zpool_unmap_handle(pool, entry->handle);
+ mutex_lock(acomp_ctx->mutex);
+ sg_init_one(&input, src, entry->length);
+ sg_init_table(&output, 1);
+ sg_set_page(&output, page, PAGE_SIZE, 0);
+ acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);
+ ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
+ dlen = acomp_ctx->req->dlen;
+ mutex_unlock(acomp_ctx->mutex);
+
+ if (!zpool_can_sleep_mapped(pool))
+ kfree(tmp);
+ else
+ zpool_unmap_handle(pool, entry->handle);
- BUG_ON(ret);
- BUG_ON(dlen != PAGE_SIZE);
+ BUG_ON(ret);
+ BUG_ON(dlen != PAGE_SIZE);
- /* page is up to date */
- SetPageUptodate(page);
- }
+ /* page is up to date */
+ SetPageUptodate(page);
/* move it to the tail of the inactive list after end_writeback */
SetPageReclaim(page);
@@ -1157,16 +1121,16 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
zswap_written_back_pages++;
return ret;
+
fail:
if (!zpool_can_sleep_mapped(pool))
kfree(tmp);
/*
- * if we get here due to ZSWAP_SWAPCACHE_EXIST
- * a load may be happening concurrently.
- * it is safe and okay to not free the entry.
- * it is also okay to return !0
- */
+ * If we get here because the page is already in swapcache, a
+ * load may be happening concurrently. It is safe and okay to
+ * not free the entry. It is also okay to return !0.
+ */
return ret;
}
--
2.41.0On Thu, Jul 27, 2023 at 9:23 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> The __read_swap_cache_async() interface isn't more difficult to
> understand than what the helper abstracts. Save the indirection and a
> level of indentation for the primary work of the writeback func.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Arguably any abstraction to the swap code is better than dealing with
the swap code, but I am not against this :P
The diffstat looks nice and the code looks correct to me.
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
> ---
> mm/zswap.c | 142 ++++++++++++++++++++---------------------------------
> 1 file changed, 53 insertions(+), 89 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index e34ac89e6098..bba4ead672be 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1016,43 +1016,6 @@ static int zswap_enabled_param_set(const char *val,
> /*********************************
> * writeback code
> **********************************/
> -/* return enum for zswap_get_swap_cache_page */
> -enum zswap_get_swap_ret {
> - ZSWAP_SWAPCACHE_NEW,
> - ZSWAP_SWAPCACHE_EXIST,
> - ZSWAP_SWAPCACHE_FAIL,
> -};
> -
> -/*
> - * zswap_get_swap_cache_page
> - *
> - * This is an adaption of read_swap_cache_async()
> - *
> - * This function tries to find a page with the given swap entry
> - * in the swapper_space address space (the swap cache). If the page
> - * is found, it is returned in retpage. Otherwise, a page is allocated,
> - * added to the swap cache, and returned in retpage.
> - *
> - * If success, the swap cache page is returned in retpage
> - * Returns ZSWAP_SWAPCACHE_EXIST if page was already in the swap cache
> - * Returns ZSWAP_SWAPCACHE_NEW if the new page needs to be populated,
> - * the new page is added to swapcache and locked
> - * Returns ZSWAP_SWAPCACHE_FAIL on error
> - */
> -static int zswap_get_swap_cache_page(swp_entry_t entry,
> - struct page **retpage)
> -{
> - bool page_was_allocated;
> -
> - *retpage = __read_swap_cache_async(entry, GFP_KERNEL,
> - NULL, 0, &page_was_allocated);
> - if (page_was_allocated)
> - return ZSWAP_SWAPCACHE_NEW;
> - if (!*retpage)
> - return ZSWAP_SWAPCACHE_FAIL;
> - return ZSWAP_SWAPCACHE_EXIST;
> -}
> -
> /*
> * Attempts to free an entry by adding a page to the swap cache,
> * decompressing the entry data into the page, and issuing a
> @@ -1073,7 +1036,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> struct scatterlist input, output;
> struct crypto_acomp_ctx *acomp_ctx;
> struct zpool *pool = entry->pool->zpool;
> -
> + bool page_was_allocated;
> u8 *src, *tmp = NULL;
> unsigned int dlen;
> int ret;
> @@ -1088,65 +1051,66 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> }
>
> /* try to allocate swap cache page */
> - switch (zswap_get_swap_cache_page(swpentry, &page)) {
> - case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
> + page = __read_swap_cache_async(swpentry, GFP_KERNEL, NULL, 0,
> + &page_was_allocated);
> + if (!page) {
> ret = -ENOMEM;
> goto fail;
> + }
>
> - case ZSWAP_SWAPCACHE_EXIST:
> - /* page is already in the swap cache, ignore for now */
> + /* Found an existing page, we raced with load/swapin */
> + if (!page_was_allocated) {
> put_page(page);
> ret = -EEXIST;
> goto fail;
> + }
>
> - case ZSWAP_SWAPCACHE_NEW: /* page is locked */
> - /*
> - * Having a local reference to the zswap entry doesn't exclude
> - * swapping from invalidating and recycling the swap slot. Once
> - * the swapcache is secured against concurrent swapping to and
> - * from the slot, recheck that the entry is still current before
> - * writing.
> - */
> - spin_lock(&tree->lock);
> - if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
> - spin_unlock(&tree->lock);
> - delete_from_swap_cache(page_folio(page));
> - ret = -ENOMEM;
> - goto fail;
> - }
> + /*
> + * Page is locked, and the swapcache is now secured against
> + * concurrent swapping to and from the slot. Verify that the
> + * swap entry hasn't been invalidated and recycled behind our
> + * backs (our zswap_entry reference doesn't prevent that), to
> + * avoid overwriting a new swap page with old compressed data.
> + */
> + spin_lock(&tree->lock);
> + if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
> spin_unlock(&tree->lock);
> + delete_from_swap_cache(page_folio(page));
> + ret = -ENOMEM;
> + goto fail;
> + }
> + spin_unlock(&tree->lock);
>
> - /* decompress */
> - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> - dlen = PAGE_SIZE;
> + /* decompress */
> + acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> + dlen = PAGE_SIZE;
>
> - src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> - if (!zpool_can_sleep_mapped(pool)) {
> - memcpy(tmp, src, entry->length);
> - src = tmp;
> - zpool_unmap_handle(pool, entry->handle);
> - }
> + src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> + if (!zpool_can_sleep_mapped(pool)) {
> + memcpy(tmp, src, entry->length);
> + src = tmp;
> + zpool_unmap_handle(pool, entry->handle);
> + }
>
> - mutex_lock(acomp_ctx->mutex);
> - sg_init_one(&input, src, entry->length);
> - sg_init_table(&output, 1);
> - sg_set_page(&output, page, PAGE_SIZE, 0);
> - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);
> - ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
> - dlen = acomp_ctx->req->dlen;
> - mutex_unlock(acomp_ctx->mutex);
> -
> - if (!zpool_can_sleep_mapped(pool))
> - kfree(tmp);
> - else
> - zpool_unmap_handle(pool, entry->handle);
> + mutex_lock(acomp_ctx->mutex);
> + sg_init_one(&input, src, entry->length);
> + sg_init_table(&output, 1);
> + sg_set_page(&output, page, PAGE_SIZE, 0);
> + acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);
> + ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
> + dlen = acomp_ctx->req->dlen;
> + mutex_unlock(acomp_ctx->mutex);
> +
> + if (!zpool_can_sleep_mapped(pool))
> + kfree(tmp);
> + else
> + zpool_unmap_handle(pool, entry->handle);
>
> - BUG_ON(ret);
> - BUG_ON(dlen != PAGE_SIZE);
> + BUG_ON(ret);
> + BUG_ON(dlen != PAGE_SIZE);
>
> - /* page is up to date */
> - SetPageUptodate(page);
> - }
> + /* page is up to date */
> + SetPageUptodate(page);
>
> /* move it to the tail of the inactive list after end_writeback */
> SetPageReclaim(page);
> @@ -1157,16 +1121,16 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> zswap_written_back_pages++;
>
> return ret;
> +
> fail:
> if (!zpool_can_sleep_mapped(pool))
> kfree(tmp);
>
> /*
> - * if we get here due to ZSWAP_SWAPCACHE_EXIST
> - * a load may be happening concurrently.
> - * it is safe and okay to not free the entry.
> - * it is also okay to return !0
> - */
> + * If we get here because the page is already in swapcache, a
> + * load may be happening concurrently. It is safe and okay to
> + * not free the entry. It is also okay to return !0.
> + */
> return ret;
> }
>
> --
> 2.41.0
>
© 2016 - 2026 Red Hat, Inc.