[PATCH v2] mm/zswap: change zswap to writethrough cache

[PATCH v2] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

Currently, zswap is writeback cache; stored pages are not sent
to swap disk, and when zswap wants to evict old pages it must
first write them back to swap cache/disk manually.  This avoids
swap out disk I/O up front, but only moves that disk I/O to
the writeback case (for pages that are evicted), and adds the
overhead of having to uncompress the evicted pages, and adds the
need for an additional free page (to store the uncompressed page)
at a time of likely high memory pressure.  Additionally, being
writeback adds complexity to zswap by having to perform the
writeback on page eviction.

This changes zswap to writethrough cache by enabling
frontswap_writethrough() before registering, so that any
successful page store will also be written to swap disk.  All the
writeback code is removed since it is no longer needed, and the
only operation during a page eviction is now to remove the entry
from the tree and free it.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---

This does require the patch just sent to the list
"mm/zswap: don't allow entry eviction if in use by load"
is applied.

Changes since v1:
update to apply to latest -tip, previous patch missed several recent
zswap patches.

 mm/zswap.c | 208 ++++++-------------------------------------------------------
 1 file changed, 18 insertions(+), 190 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index f4fbbd5..2d209a3 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -39,7 +39,6 @@
 #include <linux/mm_types.h>
 #include <linux/page-flags.h>
 #include <linux/swapops.h>
-#include <linux/writeback.h>
 #include <linux/pagemap.h>
 
 /*********************************
@@ -59,8 +58,8 @@ static atomic_t zswap_stored_pages = ATOMIC_INIT(0);
 
 /* Pool limit was hit (see zswap_max_pool_percent) */
 static u64 zswap_pool_limit_hit;
-/* Pages written back when pool limit was reached */
-static u64 zswap_written_back_pages;
+/* Pages evicted when pool limit was reached */
+static u64 zswap_evicted_pages;
 /* Store failed due to a reclaim failure after pool limit was reached */
 static u64 zswap_reject_reclaim_fail;
 /* Compressed page was too big for the allocator to (optimally) store */
@@ -160,7 +159,7 @@ static void zswap_comp_exit(void)
  * rbnode - links the entry into red-black tree for the appropriate swap type
  * refcount - the number of outstanding reference to the entry. This is needed
  *            to protect against premature freeing of the entry by code
- *            concurent calls to load, invalidate, and writeback.  The lock
+ *            concurent calls to load, invalidate, and evict.  The lock
  *            for the zswap_tree structure that contains the entry must
  *            be held while changing the refcount.  Since the lock must
  *            be held, there is no reason to also make refcount atomic.
@@ -412,132 +411,19 @@ static bool zswap_is_full(void)
 }
 
 /*********************************
-* writeback code
+* evict
 **********************************/
-/* 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)
-{
-	struct page *found_page, *new_page = NULL;
-	struct address_space *swapper_space = swap_address_space(entry);
-	int err;
-
-	*retpage = NULL;
-	do {
-		/*
-		 * First check the swap cache.  Since this is normally
-		 * called after lookup_swap_cache() failed, re-calling
-		 * that would confuse statistics.
-		 */
-		found_page = find_get_page(swapper_space, entry.val);
-		if (found_page)
-			break;
-
-		/*
-		 * Get a new page to read into from swap.
-		 */
-		if (!new_page) {
-			new_page = alloc_page(GFP_KERNEL);
-			if (!new_page)
-				break; /* Out of memory */
-		}
-
-		/*
-		 * call radix_tree_preload() while we can wait.
-		 */
-		err = radix_tree_preload(GFP_KERNEL);
-		if (err)
-			break;
-
-		/*
-		 * Swap entry may have been freed since our caller observed it.
-		 */
-		err = swapcache_prepare(entry);
-		if (err == -EEXIST) { /* seems racy */
-			radix_tree_preload_end();
-			continue;
-		}
-		if (err) { /* swp entry is obsolete ? */
-			radix_tree_preload_end();
-			break;
-		}
-
-		/* May fail (-ENOMEM) if radix-tree node allocation failed. */
-		__set_page_locked(new_page);
-		SetPageSwapBacked(new_page);
-		err = __add_to_swap_cache(new_page, entry);
-		if (likely(!err)) {
-			radix_tree_preload_end();
-			lru_cache_add_anon(new_page);
-			*retpage = new_page;
-			return ZSWAP_SWAPCACHE_NEW;
-		}
-		radix_tree_preload_end();
-		ClearPageSwapBacked(new_page);
-		__clear_page_locked(new_page);
-		/*
-		 * add_to_swap_cache() doesn't return -EEXIST, so we can safely
-		 * clear SWAP_HAS_CACHE flag.
-		 */
-		swapcache_free(entry, NULL);
-	} while (err != -ENOMEM);
-
-	if (new_page)
-		page_cache_release(new_page);
-	if (!found_page)
-		return ZSWAP_SWAPCACHE_FAIL;
-	*retpage = found_page;
-	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
- * bio write to write the page back to the swap device.
- *
- * This can be thought of as a "resumed writeback" of the page
- * to the swap device.  We are basically resuming the same swap
- * writeback path that was intercepted with the frontswap_store()
- * in the first place.  After the page has been decompressed into
- * the swap cache, the compressed version stored by zswap can be
- * freed.
+ * This is called from zbud to remove an entry that is being evicted.
  */
-static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
+static int zswap_evict_entry(struct zbud_pool *pool, unsigned long handle)
 {
 	struct zswap_header *zhdr;
 	swp_entry_t swpentry;
 	struct zswap_tree *tree;
 	pgoff_t offset;
 	struct zswap_entry *entry;
-	struct page *page;
-	u8 *src, *dst;
-	unsigned int dlen;
-	int ret;
-	struct writeback_control wbc = {
-		.sync_mode = WB_SYNC_NONE,
-	};
 
 	/* extract swpentry from data */
 	zhdr = zbud_map(pool, handle);
@@ -547,89 +433,30 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 	offset = swp_offset(swpentry);
 	BUG_ON(pool != tree->pool);
 
-	/* find and ref zswap entry */
+	/* find zswap entry */
 	spin_lock(&tree->lock);
-	entry = zswap_entry_find_get(&tree->rbroot, offset);
+	entry = zswap_rb_search(&tree->rbroot, offset);
 	if (!entry) {
 		/* entry was invalidated */
 		spin_unlock(&tree->lock);
 		return 0;
 	}
-	spin_unlock(&tree->lock);
 	BUG_ON(offset != entry->offset);
 
-	/* try to allocate swap cache page */
-	switch (zswap_get_swap_cache_page(swpentry, &page)) {
-	case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
-		ret = -ENOMEM;
-		goto fail;
-
-	case ZSWAP_SWAPCACHE_EXIST:
-		/* page is already in the swap cache, ignore for now */
-		page_cache_release(page);
-		ret = -EEXIST;
-		goto fail;
-
-	case ZSWAP_SWAPCACHE_NEW: /* page is locked */
-		/* decompress */
-		dlen = PAGE_SIZE;
-		src = (u8 *)zbud_map(tree->pool, entry->handle) +
-			sizeof(struct zswap_header);
-		dst = kmap_atomic(page);
-		ret = zswap_comp_op(ZSWAP_COMPOP_DECOMPRESS, src,
-				entry->length, dst, &dlen);
-		kunmap_atomic(dst);
-		zbud_unmap(tree->pool, entry->handle);
-		BUG_ON(ret);
-		BUG_ON(dlen != PAGE_SIZE);
-
-		/* page is up to date */
-		SetPageUptodate(page);
-	}
-
-	/* move it to the tail of the inactive list after end_writeback */
-	SetPageReclaim(page);
-
-	/* start writeback */
-	__swap_writepage(page, &wbc, end_swap_bio_write);
-	page_cache_release(page);
-	zswap_written_back_pages++;
-
-	spin_lock(&tree->lock);
-	/* drop local reference */
+	/* drop initial reference */
 	zswap_entry_put(tree, entry);
 
-	/*
-	* There are three possible situations for entry here:
-	* (1) refcount is 1(normal case),  entry is valid and on the tree
-	* (2) refcount is 0, entry is freed and not on the tree
-	*     because invalidate happened during writeback
-	* (3) refcount is 2, entry is in use by load, prevent eviction
-	*/
-	if (likely(entry->refcount > 0))
-		zswap_entry_put(tree, entry);
+	/* if still in use by load(), do not allow eviction */
 	if (unlikely(entry->refcount > 0)) {
 		spin_unlock(&tree->lock);
 		return -EAGAIN;
 	}
-	spin_unlock(&tree->lock);
 
-	goto end;
+	zswap_evicted_pages++;
 
-	/*
-	* if we get here due to ZSWAP_SWAPCACHE_EXIST
-	* a load may happening concurrently
-	* it is safe and okay to not free the entry
-	* if we free the entry in the following put
-	* it it either okay to return !0
-	*/
-fail:
-	spin_lock(&tree->lock);
-	zswap_entry_put(tree, entry);
 	spin_unlock(&tree->lock);
 
-end:
-	return ret;
+	return 0;
 }
 
 /*********************************
@@ -746,7 +573,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
 	spin_lock(&tree->lock);
 	entry = zswap_entry_find_get(&tree->rbroot, offset);
 	if (!entry) {
-		/* entry was written back */
+		/* entry was evicted */
 		spin_unlock(&tree->lock);
 		return -1;
 	}
@@ -780,7 +607,7 @@ static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t offset)
 	spin_lock(&tree->lock);
 	entry = zswap_rb_search(&tree->rbroot, offset);
 	if (!entry) {
-		/* entry was written back */
+		/* entry was evicted */
 		spin_unlock(&tree->lock);
 		return;
 	}
@@ -813,7 +640,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
 }
 
 static struct zbud_ops zswap_zbud_ops = {
-	.evict = zswap_writeback_entry
+	.evict = zswap_evict_entry
 };
 
 static void zswap_frontswap_init(unsigned type)
@@ -872,8 +699,8 @@ static int __init zswap_debugfs_init(void)
 			zswap_debugfs_root, &zswap_reject_kmemcache_fail);
 	debugfs_create_u64("reject_compress_poor", S_IRUGO,
 			zswap_debugfs_root, &zswap_reject_compress_poor);
-	debugfs_create_u64("written_back_pages", S_IRUGO,
-			zswap_debugfs_root, &zswap_written_back_pages);
+	debugfs_create_u64("evicted_pages", S_IRUGO,
+			zswap_debugfs_root, &zswap_evicted_pages);
 	debugfs_create_u64("duplicate_entry", S_IRUGO,
 			zswap_debugfs_root, &zswap_duplicate_entry);
 	debugfs_create_u64("pool_pages", S_IRUGO,
@@ -918,6 +745,7 @@ static int __init init_zswap(void)
 		pr_err("per-cpu initialization failed\n");
 		goto pcpufail;
 	}
+	frontswap_writethrough(true);
 	frontswap_register_ops(&zswap_frontswap_ops);
 	if (zswap_debugfs_init())
 		pr_warn("debugfs initialization failed\n");
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Bob Liu]

Hi Dan,

On 11/21/2013 03:49 AM, Dan Streetman wrote:
> Currently, zswap is writeback cache; stored pages are not sent
> to swap disk, and when zswap wants to evict old pages it must
> first write them back to swap cache/disk manually.  This avoids
> swap out disk I/O up front, but only moves that disk I/O to
> the writeback case (for pages that are evicted), and adds the
> overhead of having to uncompress the evicted pages, and adds the
> need for an additional free page (to store the uncompressed page)
> at a time of likely high memory pressure.  Additionally, being
> writeback adds complexity to zswap by having to perform the
> writeback on page eviction.
> 

Good work!

> This changes zswap to writethrough cache by enabling
> frontswap_writethrough() before registering, so that any
> successful page store will also be written to swap disk.  All the
> writeback code is removed since it is no longer needed, and the
> only operation during a page eviction is now to remove the entry
> from the tree and free it.
> 

Could you do some testing using eg. SPECjbb? And compare the result with
original zswap.

Thanks,
-Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Weijie Yang]

Hello Dan,

On Thu, Nov 21, 2013 at 8:42 AM, Bob Liu <bob.liu@oracle.com> wrote:
> Hi Dan,
>
> On 11/21/2013 03:49 AM, Dan Streetman wrote:
>> Currently, zswap is writeback cache; stored pages are not sent
>> to swap disk, and when zswap wants to evict old pages it must
>> first write them back to swap cache/disk manually.  This avoids
>> swap out disk I/O up front, but only moves that disk I/O to
>> the writeback case (for pages that are evicted), and adds the
>> overhead of having to uncompress the evicted pages, and adds the
>> need for an additional free page (to store the uncompressed page)
>> at a time of likely high memory pressure.  Additionally, being
>> writeback adds complexity to zswap by having to perform the
>> writeback on page eviction.
>>
>
> Good work!
>
>> This changes zswap to writethrough cache by enabling
>> frontswap_writethrough() before registering, so that any
>> successful page store will also be written to swap disk.  All the
>> writeback code is removed since it is no longer needed, and the
>> only operation during a page eviction is now to remove the entry
>> from the tree and free it.
>>

Thanks for your work. I reviewed this patch, and it is good to me.

However, I am skeptical about it because:
1. it will add more IO than original zswap, how does it result in a
performance improvement ?
2. most embedded device use NAND, more IO will reduce its working life

Regards

> Could you do some testing using eg. SPECjbb? And compare the result with
> original zswap.
>
> Thanks,
> -Bob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

On Wed, Nov 20, 2013 at 7:42 PM, Bob Liu <bob.liu@oracle.com> wrote:
> Hi Dan,
>
> On 11/21/2013 03:49 AM, Dan Streetman wrote:
>> Currently, zswap is writeback cache; stored pages are not sent
>> to swap disk, and when zswap wants to evict old pages it must
>> first write them back to swap cache/disk manually.  This avoids
>> swap out disk I/O up front, but only moves that disk I/O to
>> the writeback case (for pages that are evicted), and adds the
>> overhead of having to uncompress the evicted pages, and adds the
>> need for an additional free page (to store the uncompressed page)
>> at a time of likely high memory pressure.  Additionally, being
>> writeback adds complexity to zswap by having to perform the
>> writeback on page eviction.
>>
>
> Good work!
>
>> This changes zswap to writethrough cache by enabling
>> frontswap_writethrough() before registering, so that any
>> successful page store will also be written to swap disk.  All the
>> writeback code is removed since it is no longer needed, and the
>> only operation during a page eviction is now to remove the entry
>> from the tree and free it.
>>
>
> Could you do some testing using eg. SPECjbb? And compare the result with
> original zswap.

Sure, I have a small test program that I used for performance
comparisions, which I'll send and include some results, and I'll also
try to find a copy of SPECjbb to get results with.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

On Wed, Nov 20, 2013 at 10:50 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
> Hello Dan,
>
> On Thu, Nov 21, 2013 at 8:42 AM, Bob Liu <bob.liu@oracle.com> wrote:
>> Hi Dan,
>>
>> On 11/21/2013 03:49 AM, Dan Streetman wrote:
>>> Currently, zswap is writeback cache; stored pages are not sent
>>> to swap disk, and when zswap wants to evict old pages it must
>>> first write them back to swap cache/disk manually.  This avoids
>>> swap out disk I/O up front, but only moves that disk I/O to
>>> the writeback case (for pages that are evicted), and adds the
>>> overhead of having to uncompress the evicted pages, and adds the
>>> need for an additional free page (to store the uncompressed page)
>>> at a time of likely high memory pressure.  Additionally, being
>>> writeback adds complexity to zswap by having to perform the
>>> writeback on page eviction.
>>>
>>
>> Good work!
>>
>>> This changes zswap to writethrough cache by enabling
>>> frontswap_writethrough() before registering, so that any
>>> successful page store will also be written to swap disk.  All the
>>> writeback code is removed since it is no longer needed, and the
>>> only operation during a page eviction is now to remove the entry
>>> from the tree and free it.
>>>
>
> Thanks for your work. I reviewed this patch, and it is good to me.
>
> However, I am skeptical about it because:
> 1. it will add more IO than original zswap, how does it result in a
> performance improvement ?

I haven't used SPECjbb yet (I don't have it, I'll have to find someone
who has it that I can borrow), but my testing with a small test
program I wrote does show that CPU-bound performance is better with
writethrough over writeback, once zswap is full, which I think is
expected since writeback adds cycles for decompressing old pages while
writethrough simply drops the compressed page.  Now, the additional
CPU load may be more desirable depending on CPU speed, # of CPUs, swap
disk speed, etc., so maybe it would be better to make zswap writeback
or writethrough, with a param to select, depending on the specific hw
it's being run on.

> 2. most embedded device use NAND, more IO will reduce its working life

This is certainly true; but most embedded devices also have tiny cpus
that might get hit hard when zswap fills up and has to start
decompressing pages while simultaneously compressing newly swapped out
pages.  I'd expect for many embedded devices it would be better to
simply invoke the oom killer and/or reboot than try to run with
overcommitted memory.  But, having writeback vs writethrough
selectable by param would allow flexibility based on the specific
user's needs.


>
> Regards
>
>> Could you do some testing using eg. SPECjbb? And compare the result with
>> original zswap.
>>
>> Thanks,
>> -Bob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Seth Jennings]

On Wed, Nov 20, 2013 at 02:49:33PM -0500, Dan Streetman wrote:
> Currently, zswap is writeback cache; stored pages are not sent
> to swap disk, and when zswap wants to evict old pages it must
> first write them back to swap cache/disk manually.  This avoids
> swap out disk I/O up front, but only moves that disk I/O to
> the writeback case (for pages that are evicted), and adds the
> overhead of having to uncompress the evicted pages, and adds the
> need for an additional free page (to store the uncompressed page)
> at a time of likely high memory pressure.  Additionally, being
> writeback adds complexity to zswap by having to perform the
> writeback on page eviction.
> 
> This changes zswap to writethrough cache by enabling
> frontswap_writethrough() before registering, so that any
> successful page store will also be written to swap disk.  All the
> writeback code is removed since it is no longer needed, and the
> only operation during a page eviction is now to remove the entry
> from the tree and free it.

I like it.  It gets rid of a lot of nasty writeback code in zswap.

I'll have to test before I ack, hopefully by the end of the day.

Yes, this will increase writes to the swap device over the delayed
writeback approach.  I think it is a good thing though.  I think it
makes the difference between zswap and zram, both in operation and in
application, more apparent. Zram is the better choice for embedded where
write wear is a concern, and zswap being better if you need more
flexibility to dynamically manage the compressed pool.

Seth

> 
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
> 
> This does require the patch just sent to the list
> "mm/zswap: don't allow entry eviction if in use by load"
> is applied.
> 
> Changes since v1:
> update to apply to latest -tip, previous patch missed several recent
> zswap patches.
> 
>  mm/zswap.c | 208 ++++++-------------------------------------------------------
>  1 file changed, 18 insertions(+), 190 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index f4fbbd5..2d209a3 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -39,7 +39,6 @@
>  #include <linux/mm_types.h>
>  #include <linux/page-flags.h>
>  #include <linux/swapops.h>
> -#include <linux/writeback.h>
>  #include <linux/pagemap.h>
>  
>  /*********************************
> @@ -59,8 +58,8 @@ static atomic_t zswap_stored_pages = ATOMIC_INIT(0);
>  
>  /* Pool limit was hit (see zswap_max_pool_percent) */
>  static u64 zswap_pool_limit_hit;
> -/* Pages written back when pool limit was reached */
> -static u64 zswap_written_back_pages;
> +/* Pages evicted when pool limit was reached */
> +static u64 zswap_evicted_pages;
>  /* Store failed due to a reclaim failure after pool limit was reached */
>  static u64 zswap_reject_reclaim_fail;
>  /* Compressed page was too big for the allocator to (optimally) store */
> @@ -160,7 +159,7 @@ static void zswap_comp_exit(void)
>   * rbnode - links the entry into red-black tree for the appropriate swap type
>   * refcount - the number of outstanding reference to the entry. This is needed
>   *            to protect against premature freeing of the entry by code
> - *            concurent calls to load, invalidate, and writeback.  The lock
> + *            concurent calls to load, invalidate, and evict.  The lock
>   *            for the zswap_tree structure that contains the entry must
>   *            be held while changing the refcount.  Since the lock must
>   *            be held, there is no reason to also make refcount atomic.
> @@ -412,132 +411,19 @@ static bool zswap_is_full(void)
>  }
>  
>  /*********************************
> -* writeback code
> +* evict
>  **********************************/
> -/* 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)
> -{
> -	struct page *found_page, *new_page = NULL;
> -	struct address_space *swapper_space = swap_address_space(entry);
> -	int err;
> -
> -	*retpage = NULL;
> -	do {
> -		/*
> -		 * First check the swap cache.  Since this is normally
> -		 * called after lookup_swap_cache() failed, re-calling
> -		 * that would confuse statistics.
> -		 */
> -		found_page = find_get_page(swapper_space, entry.val);
> -		if (found_page)
> -			break;
> -
> -		/*
> -		 * Get a new page to read into from swap.
> -		 */
> -		if (!new_page) {
> -			new_page = alloc_page(GFP_KERNEL);
> -			if (!new_page)
> -				break; /* Out of memory */
> -		}
> -
> -		/*
> -		 * call radix_tree_preload() while we can wait.
> -		 */
> -		err = radix_tree_preload(GFP_KERNEL);
> -		if (err)
> -			break;
> -
> -		/*
> -		 * Swap entry may have been freed since our caller observed it.
> -		 */
> -		err = swapcache_prepare(entry);
> -		if (err == -EEXIST) { /* seems racy */
> -			radix_tree_preload_end();
> -			continue;
> -		}
> -		if (err) { /* swp entry is obsolete ? */
> -			radix_tree_preload_end();
> -			break;
> -		}
> -
> -		/* May fail (-ENOMEM) if radix-tree node allocation failed. */
> -		__set_page_locked(new_page);
> -		SetPageSwapBacked(new_page);
> -		err = __add_to_swap_cache(new_page, entry);
> -		if (likely(!err)) {
> -			radix_tree_preload_end();
> -			lru_cache_add_anon(new_page);
> -			*retpage = new_page;
> -			return ZSWAP_SWAPCACHE_NEW;
> -		}
> -		radix_tree_preload_end();
> -		ClearPageSwapBacked(new_page);
> -		__clear_page_locked(new_page);
> -		/*
> -		 * add_to_swap_cache() doesn't return -EEXIST, so we can safely
> -		 * clear SWAP_HAS_CACHE flag.
> -		 */
> -		swapcache_free(entry, NULL);
> -	} while (err != -ENOMEM);
> -
> -	if (new_page)
> -		page_cache_release(new_page);
> -	if (!found_page)
> -		return ZSWAP_SWAPCACHE_FAIL;
> -	*retpage = found_page;
> -	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
> - * bio write to write the page back to the swap device.
> - *
> - * This can be thought of as a "resumed writeback" of the page
> - * to the swap device.  We are basically resuming the same swap
> - * writeback path that was intercepted with the frontswap_store()
> - * in the first place.  After the page has been decompressed into
> - * the swap cache, the compressed version stored by zswap can be
> - * freed.
> + * This is called from zbud to remove an entry that is being evicted.
>   */
> -static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
> +static int zswap_evict_entry(struct zbud_pool *pool, unsigned long handle)
>  {
>  	struct zswap_header *zhdr;
>  	swp_entry_t swpentry;
>  	struct zswap_tree *tree;
>  	pgoff_t offset;
>  	struct zswap_entry *entry;
> -	struct page *page;
> -	u8 *src, *dst;
> -	unsigned int dlen;
> -	int ret;
> -	struct writeback_control wbc = {
> -		.sync_mode = WB_SYNC_NONE,
> -	};
>  
>  	/* extract swpentry from data */
>  	zhdr = zbud_map(pool, handle);
> @@ -547,89 +433,30 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>  	offset = swp_offset(swpentry);
>  	BUG_ON(pool != tree->pool);
>  
> -	/* find and ref zswap entry */
> +	/* find zswap entry */
>  	spin_lock(&tree->lock);
> -	entry = zswap_entry_find_get(&tree->rbroot, offset);
> +	entry = zswap_rb_search(&tree->rbroot, offset);
>  	if (!entry) {
>  		/* entry was invalidated */
>  		spin_unlock(&tree->lock);
>  		return 0;
>  	}
> -	spin_unlock(&tree->lock);
>  	BUG_ON(offset != entry->offset);
>  
> -	/* try to allocate swap cache page */
> -	switch (zswap_get_swap_cache_page(swpentry, &page)) {
> -	case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
> -		ret = -ENOMEM;
> -		goto fail;
> -
> -	case ZSWAP_SWAPCACHE_EXIST:
> -		/* page is already in the swap cache, ignore for now */
> -		page_cache_release(page);
> -		ret = -EEXIST;
> -		goto fail;
> -
> -	case ZSWAP_SWAPCACHE_NEW: /* page is locked */
> -		/* decompress */
> -		dlen = PAGE_SIZE;
> -		src = (u8 *)zbud_map(tree->pool, entry->handle) +
> -			sizeof(struct zswap_header);
> -		dst = kmap_atomic(page);
> -		ret = zswap_comp_op(ZSWAP_COMPOP_DECOMPRESS, src,
> -				entry->length, dst, &dlen);
> -		kunmap_atomic(dst);
> -		zbud_unmap(tree->pool, entry->handle);
> -		BUG_ON(ret);
> -		BUG_ON(dlen != PAGE_SIZE);
> -
> -		/* page is up to date */
> -		SetPageUptodate(page);
> -	}
> -
> -	/* move it to the tail of the inactive list after end_writeback */
> -	SetPageReclaim(page);
> -
> -	/* start writeback */
> -	__swap_writepage(page, &wbc, end_swap_bio_write);
> -	page_cache_release(page);
> -	zswap_written_back_pages++;
> -
> -	spin_lock(&tree->lock);
> -	/* drop local reference */
> +	/* drop initial reference */
>  	zswap_entry_put(tree, entry);
>  
> -	/*
> -	* There are three possible situations for entry here:
> -	* (1) refcount is 1(normal case),  entry is valid and on the tree
> -	* (2) refcount is 0, entry is freed and not on the tree
> -	*     because invalidate happened during writeback
> -	* (3) refcount is 2, entry is in use by load, prevent eviction
> -	*/
> -	if (likely(entry->refcount > 0))
> -		zswap_entry_put(tree, entry);
> +	/* if still in use by load(), do not allow eviction */
>  	if (unlikely(entry->refcount > 0)) {
>  		spin_unlock(&tree->lock);
>  		return -EAGAIN;
>  	}
> -	spin_unlock(&tree->lock);
>  
> -	goto end;
> +	zswap_evicted_pages++;
>  
> -	/*
> -	* if we get here due to ZSWAP_SWAPCACHE_EXIST
> -	* a load may happening concurrently
> -	* it is safe and okay to not free the entry
> -	* if we free the entry in the following put
> -	* it it either okay to return !0
> -	*/
> -fail:
> -	spin_lock(&tree->lock);
> -	zswap_entry_put(tree, entry);
>  	spin_unlock(&tree->lock);
>  
> -end:
> -	return ret;
> +	return 0;
>  }
>  
>  /*********************************
> @@ -746,7 +573,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>  	spin_lock(&tree->lock);
>  	entry = zswap_entry_find_get(&tree->rbroot, offset);
>  	if (!entry) {
> -		/* entry was written back */
> +		/* entry was evicted */
>  		spin_unlock(&tree->lock);
>  		return -1;
>  	}
> @@ -780,7 +607,7 @@ static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t offset)
>  	spin_lock(&tree->lock);
>  	entry = zswap_rb_search(&tree->rbroot, offset);
>  	if (!entry) {
> -		/* entry was written back */
> +		/* entry was evicted */
>  		spin_unlock(&tree->lock);
>  		return;
>  	}
> @@ -813,7 +640,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
>  }
>  
>  static struct zbud_ops zswap_zbud_ops = {
> -	.evict = zswap_writeback_entry
> +	.evict = zswap_evict_entry
>  };
>  
>  static void zswap_frontswap_init(unsigned type)
> @@ -872,8 +699,8 @@ static int __init zswap_debugfs_init(void)
>  			zswap_debugfs_root, &zswap_reject_kmemcache_fail);
>  	debugfs_create_u64("reject_compress_poor", S_IRUGO,
>  			zswap_debugfs_root, &zswap_reject_compress_poor);
> -	debugfs_create_u64("written_back_pages", S_IRUGO,
> -			zswap_debugfs_root, &zswap_written_back_pages);
> +	debugfs_create_u64("evicted_pages", S_IRUGO,
> +			zswap_debugfs_root, &zswap_evicted_pages);
>  	debugfs_create_u64("duplicate_entry", S_IRUGO,
>  			zswap_debugfs_root, &zswap_duplicate_entry);
>  	debugfs_create_u64("pool_pages", S_IRUGO,
> @@ -918,6 +745,7 @@ static int __init init_zswap(void)
>  		pr_err("per-cpu initialization failed\n");
>  		goto pcpufail;
>  	}
> +	frontswap_writethrough(true);
>  	frontswap_register_ops(&zswap_frontswap_ops);
>  	if (zswap_debugfs_init())
>  		pr_warn("debugfs initialization failed\n");
> -- 
> 1.8.3.1
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

On Fri, Nov 22, 2013 at 12:29 PM, Seth Jennings
<sjennings@variantweb.net> wrote:
> On Wed, Nov 20, 2013 at 02:49:33PM -0500, Dan Streetman wrote:
>> Currently, zswap is writeback cache; stored pages are not sent
>> to swap disk, and when zswap wants to evict old pages it must
>> first write them back to swap cache/disk manually.  This avoids
>> swap out disk I/O up front, but only moves that disk I/O to
>> the writeback case (for pages that are evicted), and adds the
>> overhead of having to uncompress the evicted pages, and adds the
>> need for an additional free page (to store the uncompressed page)
>> at a time of likely high memory pressure.  Additionally, being
>> writeback adds complexity to zswap by having to perform the
>> writeback on page eviction.
>>
>> This changes zswap to writethrough cache by enabling
>> frontswap_writethrough() before registering, so that any
>> successful page store will also be written to swap disk.  All the
>> writeback code is removed since it is no longer needed, and the
>> only operation during a page eviction is now to remove the entry
>> from the tree and free it.
>
> I like it.  It gets rid of a lot of nasty writeback code in zswap.
>
> I'll have to test before I ack, hopefully by the end of the day.

One note - when you test, change this section in the evict function:

        if (unlikely(entry->refcount > 0)) {
                spin_unlock(&tree->lock);
                return -EAGAIN;
        }

to a rb search instead:

        if (unlikely(entry == zswap_rb_search(&tree->rbroot, offset))) {
                spin_unlock(&tree->lock);
                return -EAGAIN;
        }

or, since even if load() is working on the entry concurrently, telling
zbud it's been evicted (by returning 0) shouldn't actually make any
difference (currently) since zbud doesn't actually do anything
significant with the return value, so that section could just be
removed.  (Although, thinking about it, I think the evict function
should additionally zswap_rb_erase() before the zswap_entry_put()
because if load() is using the entry, it'll remain in the rb tree and
if it's dup-detected or invalidated then it'll be zswap_entry_put()
down to -1 by the dup removal or invalidation, plus when load() is
done with it...I think this gets back to pulling zswap_rb_erase back
out of the zswap_entry_put function).

But it shouldn't stay as-is, since entry has likely been free'd
already and shouldn't be dereferenced to check the refcount.

I'll send an updated patch, but first I'd like to re-test myself as
well with SPECjbb and also I want to update my test program to check
the number of frontswap load hits also.


>
> Yes, this will increase writes to the swap device over the delayed
> writeback approach.  I think it is a good thing though.  I think it
> makes the difference between zswap and zram, both in operation and in
> application, more apparent. Zram is the better choice for embedded where
> write wear is a concern, and zswap being better if you need more
> flexibility to dynamically manage the compressed pool.
>
> Seth
>
>>
>> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
>> ---
>>
>> This does require the patch just sent to the list
>> "mm/zswap: don't allow entry eviction if in use by load"
>> is applied.
>>
>> Changes since v1:
>> update to apply to latest -tip, previous patch missed several recent
>> zswap patches.
>>
>>  mm/zswap.c | 208 ++++++-------------------------------------------------------
>>  1 file changed, 18 insertions(+), 190 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index f4fbbd5..2d209a3 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -39,7 +39,6 @@
>>  #include <linux/mm_types.h>
>>  #include <linux/page-flags.h>
>>  #include <linux/swapops.h>
>> -#include <linux/writeback.h>
>>  #include <linux/pagemap.h>
>>
>>  /*********************************
>> @@ -59,8 +58,8 @@ static atomic_t zswap_stored_pages = ATOMIC_INIT(0);
>>
>>  /* Pool limit was hit (see zswap_max_pool_percent) */
>>  static u64 zswap_pool_limit_hit;
>> -/* Pages written back when pool limit was reached */
>> -static u64 zswap_written_back_pages;
>> +/* Pages evicted when pool limit was reached */
>> +static u64 zswap_evicted_pages;
>>  /* Store failed due to a reclaim failure after pool limit was reached */
>>  static u64 zswap_reject_reclaim_fail;
>>  /* Compressed page was too big for the allocator to (optimally) store */
>> @@ -160,7 +159,7 @@ static void zswap_comp_exit(void)
>>   * rbnode - links the entry into red-black tree for the appropriate swap type
>>   * refcount - the number of outstanding reference to the entry. This is needed
>>   *            to protect against premature freeing of the entry by code
>> - *            concurent calls to load, invalidate, and writeback.  The lock
>> + *            concurent calls to load, invalidate, and evict.  The lock
>>   *            for the zswap_tree structure that contains the entry must
>>   *            be held while changing the refcount.  Since the lock must
>>   *            be held, there is no reason to also make refcount atomic.
>> @@ -412,132 +411,19 @@ static bool zswap_is_full(void)
>>  }
>>
>>  /*********************************
>> -* writeback code
>> +* evict
>>  **********************************/
>> -/* 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)
>> -{
>> -     struct page *found_page, *new_page = NULL;
>> -     struct address_space *swapper_space = swap_address_space(entry);
>> -     int err;
>> -
>> -     *retpage = NULL;
>> -     do {
>> -             /*
>> -              * First check the swap cache.  Since this is normally
>> -              * called after lookup_swap_cache() failed, re-calling
>> -              * that would confuse statistics.
>> -              */
>> -             found_page = find_get_page(swapper_space, entry.val);
>> -             if (found_page)
>> -                     break;
>> -
>> -             /*
>> -              * Get a new page to read into from swap.
>> -              */
>> -             if (!new_page) {
>> -                     new_page = alloc_page(GFP_KERNEL);
>> -                     if (!new_page)
>> -                             break; /* Out of memory */
>> -             }
>> -
>> -             /*
>> -              * call radix_tree_preload() while we can wait.
>> -              */
>> -             err = radix_tree_preload(GFP_KERNEL);
>> -             if (err)
>> -                     break;
>> -
>> -             /*
>> -              * Swap entry may have been freed since our caller observed it.
>> -              */
>> -             err = swapcache_prepare(entry);
>> -             if (err == -EEXIST) { /* seems racy */
>> -                     radix_tree_preload_end();
>> -                     continue;
>> -             }
>> -             if (err) { /* swp entry is obsolete ? */
>> -                     radix_tree_preload_end();
>> -                     break;
>> -             }
>> -
>> -             /* May fail (-ENOMEM) if radix-tree node allocation failed. */
>> -             __set_page_locked(new_page);
>> -             SetPageSwapBacked(new_page);
>> -             err = __add_to_swap_cache(new_page, entry);
>> -             if (likely(!err)) {
>> -                     radix_tree_preload_end();
>> -                     lru_cache_add_anon(new_page);
>> -                     *retpage = new_page;
>> -                     return ZSWAP_SWAPCACHE_NEW;
>> -             }
>> -             radix_tree_preload_end();
>> -             ClearPageSwapBacked(new_page);
>> -             __clear_page_locked(new_page);
>> -             /*
>> -              * add_to_swap_cache() doesn't return -EEXIST, so we can safely
>> -              * clear SWAP_HAS_CACHE flag.
>> -              */
>> -             swapcache_free(entry, NULL);
>> -     } while (err != -ENOMEM);
>> -
>> -     if (new_page)
>> -             page_cache_release(new_page);
>> -     if (!found_page)
>> -             return ZSWAP_SWAPCACHE_FAIL;
>> -     *retpage = found_page;
>> -     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
>> - * bio write to write the page back to the swap device.
>> - *
>> - * This can be thought of as a "resumed writeback" of the page
>> - * to the swap device.  We are basically resuming the same swap
>> - * writeback path that was intercepted with the frontswap_store()
>> - * in the first place.  After the page has been decompressed into
>> - * the swap cache, the compressed version stored by zswap can be
>> - * freed.
>> + * This is called from zbud to remove an entry that is being evicted.
>>   */
>> -static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>> +static int zswap_evict_entry(struct zbud_pool *pool, unsigned long handle)
>>  {
>>       struct zswap_header *zhdr;
>>       swp_entry_t swpentry;
>>       struct zswap_tree *tree;
>>       pgoff_t offset;
>>       struct zswap_entry *entry;
>> -     struct page *page;
>> -     u8 *src, *dst;
>> -     unsigned int dlen;
>> -     int ret;
>> -     struct writeback_control wbc = {
>> -             .sync_mode = WB_SYNC_NONE,
>> -     };
>>
>>       /* extract swpentry from data */
>>       zhdr = zbud_map(pool, handle);
>> @@ -547,89 +433,30 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>>       offset = swp_offset(swpentry);
>>       BUG_ON(pool != tree->pool);
>>
>> -     /* find and ref zswap entry */
>> +     /* find zswap entry */
>>       spin_lock(&tree->lock);
>> -     entry = zswap_entry_find_get(&tree->rbroot, offset);
>> +     entry = zswap_rb_search(&tree->rbroot, offset);
>>       if (!entry) {
>>               /* entry was invalidated */
>>               spin_unlock(&tree->lock);
>>               return 0;
>>       }
>> -     spin_unlock(&tree->lock);
>>       BUG_ON(offset != entry->offset);
>>
>> -     /* try to allocate swap cache page */
>> -     switch (zswap_get_swap_cache_page(swpentry, &page)) {
>> -     case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
>> -             ret = -ENOMEM;
>> -             goto fail;
>> -
>> -     case ZSWAP_SWAPCACHE_EXIST:
>> -             /* page is already in the swap cache, ignore for now */
>> -             page_cache_release(page);
>> -             ret = -EEXIST;
>> -             goto fail;
>> -
>> -     case ZSWAP_SWAPCACHE_NEW: /* page is locked */
>> -             /* decompress */
>> -             dlen = PAGE_SIZE;
>> -             src = (u8 *)zbud_map(tree->pool, entry->handle) +
>> -                     sizeof(struct zswap_header);
>> -             dst = kmap_atomic(page);
>> -             ret = zswap_comp_op(ZSWAP_COMPOP_DECOMPRESS, src,
>> -                             entry->length, dst, &dlen);
>> -             kunmap_atomic(dst);
>> -             zbud_unmap(tree->pool, entry->handle);
>> -             BUG_ON(ret);
>> -             BUG_ON(dlen != PAGE_SIZE);
>> -
>> -             /* page is up to date */
>> -             SetPageUptodate(page);
>> -     }
>> -
>> -     /* move it to the tail of the inactive list after end_writeback */
>> -     SetPageReclaim(page);
>> -
>> -     /* start writeback */
>> -     __swap_writepage(page, &wbc, end_swap_bio_write);
>> -     page_cache_release(page);
>> -     zswap_written_back_pages++;
>> -
>> -     spin_lock(&tree->lock);
>> -     /* drop local reference */
>> +     /* drop initial reference */
>>       zswap_entry_put(tree, entry);
>>
>> -     /*
>> -     * There are three possible situations for entry here:
>> -     * (1) refcount is 1(normal case),  entry is valid and on the tree
>> -     * (2) refcount is 0, entry is freed and not on the tree
>> -     *     because invalidate happened during writeback
>> -     * (3) refcount is 2, entry is in use by load, prevent eviction
>> -     */
>> -     if (likely(entry->refcount > 0))
>> -             zswap_entry_put(tree, entry);
>> +     /* if still in use by load(), do not allow eviction */
>>       if (unlikely(entry->refcount > 0)) {
>>               spin_unlock(&tree->lock);
>>               return -EAGAIN;
>>       }
>> -     spin_unlock(&tree->lock);
>>
>> -     goto end;
>> +     zswap_evicted_pages++;
>>
>> -     /*
>> -     * if we get here due to ZSWAP_SWAPCACHE_EXIST
>> -     * a load may happening concurrently
>> -     * it is safe and okay to not free the entry
>> -     * if we free the entry in the following put
>> -     * it it either okay to return !0
>> -     */
>> -fail:
>> -     spin_lock(&tree->lock);
>> -     zswap_entry_put(tree, entry);
>>       spin_unlock(&tree->lock);
>>
>> -end:
>> -     return ret;
>> +     return 0;
>>  }
>>
>>  /*********************************
>> @@ -746,7 +573,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>>       spin_lock(&tree->lock);
>>       entry = zswap_entry_find_get(&tree->rbroot, offset);
>>       if (!entry) {
>> -             /* entry was written back */
>> +             /* entry was evicted */
>>               spin_unlock(&tree->lock);
>>               return -1;
>>       }
>> @@ -780,7 +607,7 @@ static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t offset)
>>       spin_lock(&tree->lock);
>>       entry = zswap_rb_search(&tree->rbroot, offset);
>>       if (!entry) {
>> -             /* entry was written back */
>> +             /* entry was evicted */
>>               spin_unlock(&tree->lock);
>>               return;
>>       }
>> @@ -813,7 +640,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
>>  }
>>
>>  static struct zbud_ops zswap_zbud_ops = {
>> -     .evict = zswap_writeback_entry
>> +     .evict = zswap_evict_entry
>>  };
>>
>>  static void zswap_frontswap_init(unsigned type)
>> @@ -872,8 +699,8 @@ static int __init zswap_debugfs_init(void)
>>                       zswap_debugfs_root, &zswap_reject_kmemcache_fail);
>>       debugfs_create_u64("reject_compress_poor", S_IRUGO,
>>                       zswap_debugfs_root, &zswap_reject_compress_poor);
>> -     debugfs_create_u64("written_back_pages", S_IRUGO,
>> -                     zswap_debugfs_root, &zswap_written_back_pages);
>> +     debugfs_create_u64("evicted_pages", S_IRUGO,
>> +                     zswap_debugfs_root, &zswap_evicted_pages);
>>       debugfs_create_u64("duplicate_entry", S_IRUGO,
>>                       zswap_debugfs_root, &zswap_duplicate_entry);
>>       debugfs_create_u64("pool_pages", S_IRUGO,
>> @@ -918,6 +745,7 @@ static int __init init_zswap(void)
>>               pr_err("per-cpu initialization failed\n");
>>               goto pcpufail;
>>       }
>> +     frontswap_writethrough(true);
>>       frontswap_register_ops(&zswap_frontswap_ops);
>>       if (zswap_debugfs_init())
>>               pr_warn("debugfs initialization failed\n");
>> --
>> 1.8.3.1
>>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

Currently, zswap is writeback cache; stored pages are not sent
to swap disk, and when zswap wants to evict old pages it must
first write them back to swap cache/disk manually.  This avoids
swap out disk I/O up front, but only moves that disk I/O to
the writeback case (for pages that are evicted), and adds the
overhead of having to uncompress the evicted pages, and adds the
need for an additional free page (to store the uncompressed page)
at a time of likely high memory pressure.  Additionally, being
writeback adds complexity to zswap by having to perform the
writeback on page eviction.

This changes zswap to writethrough cache by enabling
frontswap_writethrough() before registering, so that any
successful page store will also be written to swap disk.  All the
writeback code is removed since it is no longer needed, and the
only operation during a page eviction is now to remove the entry
from the tree and free it.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---

I still owe the results of testing pre-patch and post-patch;
I don't think I'll have SPECjbb results today, but I do have a
small test program I can send with results of nozswap,
zswap writeback, and zswap writethrough; I'll send the SPECjbb
results also when I get them.

Changes since v2:
change the evict function to remove the entry from the tree,
and avoid checking if the entry is in use by load.  It is ok
to return success for the eviction even if load is using the entry
as zbud does not use the evict function return value in any significant
way (I'll submit a future patch to remove the return value entirely
from the evict function).

Changes since v1:
update to apply to latest -tip, previous patch missed several recent
zswap patches.


 mm/zswap.c | 209 ++++++-------------------------------------------------------
 1 file changed, 19 insertions(+), 190 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index e55bab9..fc35a7a 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -39,7 +39,6 @@
 #include <linux/mm_types.h>
 #include <linux/page-flags.h>
 #include <linux/swapops.h>
-#include <linux/writeback.h>
 #include <linux/pagemap.h>
 
 /*********************************
@@ -59,8 +58,8 @@ static atomic_t zswap_stored_pages = ATOMIC_INIT(0);
 
 /* Pool limit was hit (see zswap_max_pool_percent) */
 static u64 zswap_pool_limit_hit;
-/* Pages written back when pool limit was reached */
-static u64 zswap_written_back_pages;
+/* Pages evicted when pool limit was reached */
+static u64 zswap_evicted_pages;
 /* Store failed due to a reclaim failure after pool limit was reached */
 static u64 zswap_reject_reclaim_fail;
 /* Compressed page was too big for the allocator to (optimally) store */
@@ -160,7 +159,7 @@ static void zswap_comp_exit(void)
  * rbnode - links the entry into red-black tree for the appropriate swap type
  * refcount - the number of outstanding reference to the entry. This is needed
  *            to protect against premature freeing of the entry by code
- *            concurent calls to load, invalidate, and writeback.  The lock
+ *            concurent calls to load, invalidate, and evict.  The lock
  *            for the zswap_tree structure that contains the entry must
  *            be held while changing the refcount.  Since the lock must
  *            be held, there is no reason to also make refcount atomic.
@@ -412,132 +411,19 @@ static bool zswap_is_full(void)
 }
 
 /*********************************
-* writeback code
+* evict
 **********************************/
-/* 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
+ * This is called from zbud to remove an entry that is being evicted.
  */
-static int zswap_get_swap_cache_page(swp_entry_t entry,
-				struct page **retpage)
-{
-	struct page *found_page, *new_page = NULL;
-	struct address_space *swapper_space = swap_address_space(entry);
-	int err;
-
-	*retpage = NULL;
-	do {
-		/*
-		 * First check the swap cache.  Since this is normally
-		 * called after lookup_swap_cache() failed, re-calling
-		 * that would confuse statistics.
-		 */
-		found_page = find_get_page(swapper_space, entry.val);
-		if (found_page)
-			break;
-
-		/*
-		 * Get a new page to read into from swap.
-		 */
-		if (!new_page) {
-			new_page = alloc_page(GFP_KERNEL);
-			if (!new_page)
-				break; /* Out of memory */
-		}
-
-		/*
-		 * call radix_tree_preload() while we can wait.
-		 */
-		err = radix_tree_preload(GFP_KERNEL);
-		if (err)
-			break;
-
-		/*
-		 * Swap entry may have been freed since our caller observed it.
-		 */
-		err = swapcache_prepare(entry);
-		if (err == -EEXIST) { /* seems racy */
-			radix_tree_preload_end();
-			continue;
-		}
-		if (err) { /* swp entry is obsolete ? */
-			radix_tree_preload_end();
-			break;
-		}
-
-		/* May fail (-ENOMEM) if radix-tree node allocation failed. */
-		__set_page_locked(new_page);
-		SetPageSwapBacked(new_page);
-		err = __add_to_swap_cache(new_page, entry);
-		if (likely(!err)) {
-			radix_tree_preload_end();
-			lru_cache_add_anon(new_page);
-			*retpage = new_page;
-			return ZSWAP_SWAPCACHE_NEW;
-		}
-		radix_tree_preload_end();
-		ClearPageSwapBacked(new_page);
-		__clear_page_locked(new_page);
-		/*
-		 * add_to_swap_cache() doesn't return -EEXIST, so we can safely
-		 * clear SWAP_HAS_CACHE flag.
-		 */
-		swapcache_free(entry, NULL);
-	} while (err != -ENOMEM);
-
-	if (new_page)
-		page_cache_release(new_page);
-	if (!found_page)
-		return ZSWAP_SWAPCACHE_FAIL;
-	*retpage = found_page;
-	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
- * bio write to write the page back to the swap device.
- *
- * This can be thought of as a "resumed writeback" of the page
- * to the swap device.  We are basically resuming the same swap
- * writeback path that was intercepted with the frontswap_store()
- * in the first place.  After the page has been decompressed into
- * the swap cache, the compressed version stored by zswap can be
- * freed.
- */
-static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
+static int zswap_evict_entry(struct zbud_pool *pool, unsigned long handle)
 {
 	struct zswap_header *zhdr;
 	swp_entry_t swpentry;
 	struct zswap_tree *tree;
 	pgoff_t offset;
 	struct zswap_entry *entry;
-	struct page *page;
-	u8 *src, *dst;
-	unsigned int dlen;
-	int ret;
-	struct writeback_control wbc = {
-		.sync_mode = WB_SYNC_NONE,
-	};
 
 	/* extract swpentry from data */
 	zhdr = zbud_map(pool, handle);
@@ -547,85 +433,27 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 	offset = swp_offset(swpentry);
 	BUG_ON(pool != tree->pool);
 
-	/* find and ref zswap entry */
+	/* find zswap entry */
 	spin_lock(&tree->lock);
-	entry = zswap_entry_find_get(&tree->rbroot, offset);
+	entry = zswap_rb_search(&tree->rbroot, offset);
 	if (!entry) {
 		/* entry was invalidated */
 		spin_unlock(&tree->lock);
 		return 0;
 	}
-	spin_unlock(&tree->lock);
 	BUG_ON(offset != entry->offset);
 
-	/* try to allocate swap cache page */
-	switch (zswap_get_swap_cache_page(swpentry, &page)) {
-	case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
-		ret = -ENOMEM;
-		goto fail;
-
-	case ZSWAP_SWAPCACHE_EXIST:
-		/* page is already in the swap cache, ignore for now */
-		page_cache_release(page);
-		ret = -EEXIST;
-		goto fail;
-
-	case ZSWAP_SWAPCACHE_NEW: /* page is locked */
-		/* decompress */
-		dlen = PAGE_SIZE;
-		src = (u8 *)zbud_map(tree->pool, entry->handle) +
-			sizeof(struct zswap_header);
-		dst = kmap_atomic(page);
-		ret = zswap_comp_op(ZSWAP_COMPOP_DECOMPRESS, src,
-				entry->length, dst, &dlen);
-		kunmap_atomic(dst);
-		zbud_unmap(tree->pool, entry->handle);
-		BUG_ON(ret);
-		BUG_ON(dlen != PAGE_SIZE);
-
-		/* page is up to date */
-		SetPageUptodate(page);
-	}
-
-	/* move it to the tail of the inactive list after end_writeback */
-	SetPageReclaim(page);
-
-	/* start writeback */
-	__swap_writepage(page, &wbc, end_swap_bio_write);
-	page_cache_release(page);
-	zswap_written_back_pages++;
+	/* remove from rbtree */
+	zswap_rb_erase(&tree->rbroot, entry);
 
-	spin_lock(&tree->lock);
-	/* drop local reference */
+	/* drop initial reference */
 	zswap_entry_put(tree, entry);
 
-	/*
-	* There are two possible situations for entry here:
-	* (1) refcount is 1(normal case),  entry is valid and on the tree
-	* (2) refcount is 0, entry is freed and not on the tree
-	*     because invalidate happened during writeback
-	*  search the tree and free the entry if find entry
-	*/
-	if (entry == zswap_rb_search(&tree->rbroot, offset))
-		zswap_entry_put(tree, entry);
-	spin_unlock(&tree->lock);
-
-	goto end;
+	zswap_evicted_pages++;
 
-	/*
-	* if we get here due to ZSWAP_SWAPCACHE_EXIST
-	* a load may happening concurrently
-	* it is safe and okay to not free the entry
-	* if we free the entry in the following put
-	* it it either okay to return !0
-	*/
-fail:
-	spin_lock(&tree->lock);
-	zswap_entry_put(tree, entry);
 	spin_unlock(&tree->lock);
 
-end:
-	return ret;
+	return 0;
 }
 
 /*********************************
@@ -744,7 +572,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
 	spin_lock(&tree->lock);
 	entry = zswap_entry_find_get(&tree->rbroot, offset);
 	if (!entry) {
-		/* entry was written back */
+		/* entry was evicted */
 		spin_unlock(&tree->lock);
 		return -1;
 	}
@@ -778,7 +606,7 @@ static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t offset)
 	spin_lock(&tree->lock);
 	entry = zswap_rb_search(&tree->rbroot, offset);
 	if (!entry) {
-		/* entry was written back */
+		/* entry was evicted */
 		spin_unlock(&tree->lock);
 		return;
 	}
@@ -814,7 +642,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
 }
 
 static struct zbud_ops zswap_zbud_ops = {
-	.evict = zswap_writeback_entry
+	.evict = zswap_evict_entry
 };
 
 static void zswap_frontswap_init(unsigned type)
@@ -873,8 +701,8 @@ static int __init zswap_debugfs_init(void)
 			zswap_debugfs_root, &zswap_reject_kmemcache_fail);
 	debugfs_create_u64("reject_compress_poor", S_IRUGO,
 			zswap_debugfs_root, &zswap_reject_compress_poor);
-	debugfs_create_u64("written_back_pages", S_IRUGO,
-			zswap_debugfs_root, &zswap_written_back_pages);
+	debugfs_create_u64("evicted_pages", S_IRUGO,
+			zswap_debugfs_root, &zswap_evicted_pages);
 	debugfs_create_u64("duplicate_entry", S_IRUGO,
 			zswap_debugfs_root, &zswap_duplicate_entry);
 	debugfs_create_u64("pool_pages", S_IRUGO,
@@ -919,6 +747,7 @@ static int __init init_zswap(void)
 		pr_err("per-cpu initialization failed\n");
 		goto pcpufail;
 	}
+	frontswap_writethrough(true);
 	frontswap_register_ops(&zswap_frontswap_ops);
 	if (zswap_debugfs_init())
 		pr_warn("debugfs initialization failed\n");
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3] mm/zswap: change zswap to writethrough cache

[Weijie Yang]

Hello Dan,

On Sat, Nov 23, 2013 at 6:10 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> Currently, zswap is writeback cache; stored pages are not sent
> to swap disk, and when zswap wants to evict old pages it must
> first write them back to swap cache/disk manually.  This avoids
> swap out disk I/O up front, but only moves that disk I/O to
> the writeback case (for pages that are evicted), and adds the
> overhead of having to uncompress the evicted pages, and adds the
> need for an additional free page (to store the uncompressed page)
> at a time of likely high memory pressure.  Additionally, being
> writeback adds complexity to zswap by having to perform the
> writeback on page eviction.
>
> This changes zswap to writethrough cache by enabling
> frontswap_writethrough() before registering, so that any
> successful page store will also be written to swap disk.  All the
> writeback code is removed since it is no longer needed, and the
> only operation during a page eviction is now to remove the entry
> from the tree and free it.

I agree with Seth, It is not good to embedded device.
May be we can find its place in others like server.
I guess it is good to medium workload when swap io is not frequent.

My suggestion is would you please make it configurable so that user
can choice to use writethrough or writeback mode?

Regards,

> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
>
> I still owe the results of testing pre-patch and post-patch;
> I don't think I'll have SPECjbb results today, but I do have a
> small test program I can send with results of nozswap,
> zswap writeback, and zswap writethrough; I'll send the SPECjbb
> results also when I get them.
>
> Changes since v2:
> change the evict function to remove the entry from the tree,
> and avoid checking if the entry is in use by load.  It is ok
> to return success for the eviction even if load is using the entry
> as zbud does not use the evict function return value in any significant
> way (I'll submit a future patch to remove the return value entirely
> from the evict function).
>
> Changes since v1:
> update to apply to latest -tip, previous patch missed several recent
> zswap patches.
>
>
>  mm/zswap.c | 209 ++++++-------------------------------------------------------
>  1 file changed, 19 insertions(+), 190 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index e55bab9..fc35a7a 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -39,7 +39,6 @@
>  #include <linux/mm_types.h>
>  #include <linux/page-flags.h>
>  #include <linux/swapops.h>
> -#include <linux/writeback.h>
>  #include <linux/pagemap.h>
>
>  /*********************************
> @@ -59,8 +58,8 @@ static atomic_t zswap_stored_pages = ATOMIC_INIT(0);
>
>  /* Pool limit was hit (see zswap_max_pool_percent) */
>  static u64 zswap_pool_limit_hit;
> -/* Pages written back when pool limit was reached */
> -static u64 zswap_written_back_pages;
> +/* Pages evicted when pool limit was reached */
> +static u64 zswap_evicted_pages;
>  /* Store failed due to a reclaim failure after pool limit was reached */
>  static u64 zswap_reject_reclaim_fail;
>  /* Compressed page was too big for the allocator to (optimally) store */
> @@ -160,7 +159,7 @@ static void zswap_comp_exit(void)
>   * rbnode - links the entry into red-black tree for the appropriate swap type
>   * refcount - the number of outstanding reference to the entry. This is needed
>   *            to protect against premature freeing of the entry by code
> - *            concurent calls to load, invalidate, and writeback.  The lock
> + *            concurent calls to load, invalidate, and evict.  The lock
>   *            for the zswap_tree structure that contains the entry must
>   *            be held while changing the refcount.  Since the lock must
>   *            be held, there is no reason to also make refcount atomic.
> @@ -412,132 +411,19 @@ static bool zswap_is_full(void)
>  }
>
>  /*********************************
> -* writeback code
> +* evict
>  **********************************/
> -/* 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
> + * This is called from zbud to remove an entry that is being evicted.
>   */
> -static int zswap_get_swap_cache_page(swp_entry_t entry,
> -                               struct page **retpage)
> -{
> -       struct page *found_page, *new_page = NULL;
> -       struct address_space *swapper_space = swap_address_space(entry);
> -       int err;
> -
> -       *retpage = NULL;
> -       do {
> -               /*
> -                * First check the swap cache.  Since this is normally
> -                * called after lookup_swap_cache() failed, re-calling
> -                * that would confuse statistics.
> -                */
> -               found_page = find_get_page(swapper_space, entry.val);
> -               if (found_page)
> -                       break;
> -
> -               /*
> -                * Get a new page to read into from swap.
> -                */
> -               if (!new_page) {
> -                       new_page = alloc_page(GFP_KERNEL);
> -                       if (!new_page)
> -                               break; /* Out of memory */
> -               }
> -
> -               /*
> -                * call radix_tree_preload() while we can wait.
> -                */
> -               err = radix_tree_preload(GFP_KERNEL);
> -               if (err)
> -                       break;
> -
> -               /*
> -                * Swap entry may have been freed since our caller observed it.
> -                */
> -               err = swapcache_prepare(entry);
> -               if (err == -EEXIST) { /* seems racy */
> -                       radix_tree_preload_end();
> -                       continue;
> -               }
> -               if (err) { /* swp entry is obsolete ? */
> -                       radix_tree_preload_end();
> -                       break;
> -               }
> -
> -               /* May fail (-ENOMEM) if radix-tree node allocation failed. */
> -               __set_page_locked(new_page);
> -               SetPageSwapBacked(new_page);
> -               err = __add_to_swap_cache(new_page, entry);
> -               if (likely(!err)) {
> -                       radix_tree_preload_end();
> -                       lru_cache_add_anon(new_page);
> -                       *retpage = new_page;
> -                       return ZSWAP_SWAPCACHE_NEW;
> -               }
> -               radix_tree_preload_end();
> -               ClearPageSwapBacked(new_page);
> -               __clear_page_locked(new_page);
> -               /*
> -                * add_to_swap_cache() doesn't return -EEXIST, so we can safely
> -                * clear SWAP_HAS_CACHE flag.
> -                */
> -               swapcache_free(entry, NULL);
> -       } while (err != -ENOMEM);
> -
> -       if (new_page)
> -               page_cache_release(new_page);
> -       if (!found_page)
> -               return ZSWAP_SWAPCACHE_FAIL;
> -       *retpage = found_page;
> -       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
> - * bio write to write the page back to the swap device.
> - *
> - * This can be thought of as a "resumed writeback" of the page
> - * to the swap device.  We are basically resuming the same swap
> - * writeback path that was intercepted with the frontswap_store()
> - * in the first place.  After the page has been decompressed into
> - * the swap cache, the compressed version stored by zswap can be
> - * freed.
> - */
> -static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
> +static int zswap_evict_entry(struct zbud_pool *pool, unsigned long handle)
>  {
>         struct zswap_header *zhdr;
>         swp_entry_t swpentry;
>         struct zswap_tree *tree;
>         pgoff_t offset;
>         struct zswap_entry *entry;
> -       struct page *page;
> -       u8 *src, *dst;
> -       unsigned int dlen;
> -       int ret;
> -       struct writeback_control wbc = {
> -               .sync_mode = WB_SYNC_NONE,
> -       };
>
>         /* extract swpentry from data */
>         zhdr = zbud_map(pool, handle);
> @@ -547,85 +433,27 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>         offset = swp_offset(swpentry);
>         BUG_ON(pool != tree->pool);
>
> -       /* find and ref zswap entry */
> +       /* find zswap entry */
>         spin_lock(&tree->lock);
> -       entry = zswap_entry_find_get(&tree->rbroot, offset);
> +       entry = zswap_rb_search(&tree->rbroot, offset);
>         if (!entry) {
>                 /* entry was invalidated */
>                 spin_unlock(&tree->lock);
>                 return 0;
>         }
> -       spin_unlock(&tree->lock);
>         BUG_ON(offset != entry->offset);
>
> -       /* try to allocate swap cache page */
> -       switch (zswap_get_swap_cache_page(swpentry, &page)) {
> -       case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
> -               ret = -ENOMEM;
> -               goto fail;
> -
> -       case ZSWAP_SWAPCACHE_EXIST:
> -               /* page is already in the swap cache, ignore for now */
> -               page_cache_release(page);
> -               ret = -EEXIST;
> -               goto fail;
> -
> -       case ZSWAP_SWAPCACHE_NEW: /* page is locked */
> -               /* decompress */
> -               dlen = PAGE_SIZE;
> -               src = (u8 *)zbud_map(tree->pool, entry->handle) +
> -                       sizeof(struct zswap_header);
> -               dst = kmap_atomic(page);
> -               ret = zswap_comp_op(ZSWAP_COMPOP_DECOMPRESS, src,
> -                               entry->length, dst, &dlen);
> -               kunmap_atomic(dst);
> -               zbud_unmap(tree->pool, entry->handle);
> -               BUG_ON(ret);
> -               BUG_ON(dlen != PAGE_SIZE);
> -
> -               /* page is up to date */
> -               SetPageUptodate(page);
> -       }
> -
> -       /* move it to the tail of the inactive list after end_writeback */
> -       SetPageReclaim(page);
> -
> -       /* start writeback */
> -       __swap_writepage(page, &wbc, end_swap_bio_write);
> -       page_cache_release(page);
> -       zswap_written_back_pages++;
> +       /* remove from rbtree */
> +       zswap_rb_erase(&tree->rbroot, entry);
>
> -       spin_lock(&tree->lock);
> -       /* drop local reference */
> +       /* drop initial reference */
>         zswap_entry_put(tree, entry);
>
> -       /*
> -       * There are two possible situations for entry here:
> -       * (1) refcount is 1(normal case),  entry is valid and on the tree
> -       * (2) refcount is 0, entry is freed and not on the tree
> -       *     because invalidate happened during writeback
> -       *  search the tree and free the entry if find entry
> -       */
> -       if (entry == zswap_rb_search(&tree->rbroot, offset))
> -               zswap_entry_put(tree, entry);
> -       spin_unlock(&tree->lock);
> -
> -       goto end;
> +       zswap_evicted_pages++;
>
> -       /*
> -       * if we get here due to ZSWAP_SWAPCACHE_EXIST
> -       * a load may happening concurrently
> -       * it is safe and okay to not free the entry
> -       * if we free the entry in the following put
> -       * it it either okay to return !0
> -       */
> -fail:
> -       spin_lock(&tree->lock);
> -       zswap_entry_put(tree, entry);
>         spin_unlock(&tree->lock);
>
> -end:
> -       return ret;
> +       return 0;
>  }
>
>  /*********************************
> @@ -744,7 +572,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>         spin_lock(&tree->lock);
>         entry = zswap_entry_find_get(&tree->rbroot, offset);
>         if (!entry) {
> -               /* entry was written back */
> +               /* entry was evicted */
>                 spin_unlock(&tree->lock);
>                 return -1;
>         }
> @@ -778,7 +606,7 @@ static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t offset)
>         spin_lock(&tree->lock);
>         entry = zswap_rb_search(&tree->rbroot, offset);
>         if (!entry) {
> -               /* entry was written back */
> +               /* entry was evicted */
>                 spin_unlock(&tree->lock);
>                 return;
>         }
> @@ -814,7 +642,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
>  }
>
>  static struct zbud_ops zswap_zbud_ops = {
> -       .evict = zswap_writeback_entry
> +       .evict = zswap_evict_entry
>  };
>
>  static void zswap_frontswap_init(unsigned type)
> @@ -873,8 +701,8 @@ static int __init zswap_debugfs_init(void)
>                         zswap_debugfs_root, &zswap_reject_kmemcache_fail);
>         debugfs_create_u64("reject_compress_poor", S_IRUGO,
>                         zswap_debugfs_root, &zswap_reject_compress_poor);
> -       debugfs_create_u64("written_back_pages", S_IRUGO,
> -                       zswap_debugfs_root, &zswap_written_back_pages);
> +       debugfs_create_u64("evicted_pages", S_IRUGO,
> +                       zswap_debugfs_root, &zswap_evicted_pages);
>         debugfs_create_u64("duplicate_entry", S_IRUGO,
>                         zswap_debugfs_root, &zswap_duplicate_entry);
>         debugfs_create_u64("pool_pages", S_IRUGO,
> @@ -919,6 +747,7 @@ static int __init init_zswap(void)
>                 pr_err("per-cpu initialization failed\n");
>                 goto pcpufail;
>         }
> +       frontswap_writethrough(true);
>         frontswap_register_ops(&zswap_frontswap_ops);
>         if (zswap_debugfs_init())
>                 pr_warn("debugfs initialization failed\n");
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

On Fri, Nov 22, 2013 at 9:37 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
> Hello Dan,
>
> On Sat, Nov 23, 2013 at 6:10 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>> Currently, zswap is writeback cache; stored pages are not sent
>> to swap disk, and when zswap wants to evict old pages it must
>> first write them back to swap cache/disk manually.  This avoids
>> swap out disk I/O up front, but only moves that disk I/O to
>> the writeback case (for pages that are evicted), and adds the
>> overhead of having to uncompress the evicted pages, and adds the
>> need for an additional free page (to store the uncompressed page)
>> at a time of likely high memory pressure.  Additionally, being
>> writeback adds complexity to zswap by having to perform the
>> writeback on page eviction.
>>
>> This changes zswap to writethrough cache by enabling
>> frontswap_writethrough() before registering, so that any
>> successful page store will also be written to swap disk.  All the
>> writeback code is removed since it is no longer needed, and the
>> only operation during a page eviction is now to remove the entry
>> from the tree and free it.
>
> I agree with Seth, It is not good to embedded device.
> May be we can find its place in others like server.
> I guess it is good to medium workload when swap io is not frequent.
>
> My suggestion is would you please make it configurable so that user
> can choice to use writethrough or writeback mode?
>

Having to support both significantly increases complexity and I think
would make further improvements more difficult.  My opinion is the
writeback code should be removed.  Is there anyone else who thinks
both should be available by a param?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Seth Jennings]

On Fri, Nov 22, 2013 at 11:29:16AM -0600, Seth Jennings wrote:
> On Wed, Nov 20, 2013 at 02:49:33PM -0500, Dan Streetman wrote:
> > Currently, zswap is writeback cache; stored pages are not sent
> > to swap disk, and when zswap wants to evict old pages it must
> > first write them back to swap cache/disk manually.  This avoids
> > swap out disk I/O up front, but only moves that disk I/O to
> > the writeback case (for pages that are evicted), and adds the
> > overhead of having to uncompress the evicted pages, and adds the
> > need for an additional free page (to store the uncompressed page)
> > at a time of likely high memory pressure.  Additionally, being
> > writeback adds complexity to zswap by having to perform the
> > writeback on page eviction.
> > 
> > This changes zswap to writethrough cache by enabling
> > frontswap_writethrough() before registering, so that any
> > successful page store will also be written to swap disk.  All the
> > writeback code is removed since it is no longer needed, and the
> > only operation during a page eviction is now to remove the entry
> > from the tree and free it.
> 
> I like it.  It gets rid of a lot of nasty writeback code in zswap.
> 
> I'll have to test before I ack, hopefully by the end of the day.
> 
> Yes, this will increase writes to the swap device over the delayed
> writeback approach.  I think it is a good thing though.  I think it
> makes the difference between zswap and zram, both in operation and in
> application, more apparent. Zram is the better choice for embedded where
> write wear is a concern, and zswap being better if you need more
> flexibility to dynamically manage the compressed pool.

One thing I realized while doing my testing was that making zswap
writethrough also impacts synchronous reclaim.  Zswap, as it is now,
makes the swapcache page clean during swap_writepage() which allows
shrink_page_list() to immediately reclaim it.  Making zswap writethrough
eliminates this advantage and swapcache pages must be scanned again
before they can be reclaimed, as is the case with normal swapping.

Just something I am thinking about.

Seth

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

On Mon, Nov 25, 2013 at 1:00 PM, Seth Jennings <sjennings@variantweb.net> wrote:
> On Fri, Nov 22, 2013 at 11:29:16AM -0600, Seth Jennings wrote:
>> On Wed, Nov 20, 2013 at 02:49:33PM -0500, Dan Streetman wrote:
>> > Currently, zswap is writeback cache; stored pages are not sent
>> > to swap disk, and when zswap wants to evict old pages it must
>> > first write them back to swap cache/disk manually.  This avoids
>> > swap out disk I/O up front, but only moves that disk I/O to
>> > the writeback case (for pages that are evicted), and adds the
>> > overhead of having to uncompress the evicted pages, and adds the
>> > need for an additional free page (to store the uncompressed page)
>> > at a time of likely high memory pressure.  Additionally, being
>> > writeback adds complexity to zswap by having to perform the
>> > writeback on page eviction.
>> >
>> > This changes zswap to writethrough cache by enabling
>> > frontswap_writethrough() before registering, so that any
>> > successful page store will also be written to swap disk.  All the
>> > writeback code is removed since it is no longer needed, and the
>> > only operation during a page eviction is now to remove the entry
>> > from the tree and free it.
>>
>> I like it.  It gets rid of a lot of nasty writeback code in zswap.
>>
>> I'll have to test before I ack, hopefully by the end of the day.
>>
>> Yes, this will increase writes to the swap device over the delayed
>> writeback approach.  I think it is a good thing though.  I think it
>> makes the difference between zswap and zram, both in operation and in
>> application, more apparent. Zram is the better choice for embedded where
>> write wear is a concern, and zswap being better if you need more
>> flexibility to dynamically manage the compressed pool.
>
> One thing I realized while doing my testing was that making zswap
> writethrough also impacts synchronous reclaim.  Zswap, as it is now,
> makes the swapcache page clean during swap_writepage() which allows
> shrink_page_list() to immediately reclaim it.  Making zswap writethrough
> eliminates this advantage and swapcache pages must be scanned again
> before they can be reclaimed, as is the case with normal swapping.

Yep, I thought about that as well, and it is true, but only while
zswap is not full.  With writeback, once zswap fills up, page stores
will frequently have to reclaim pages by writing compressed pages to
disk.  With writethrough, the zbud reclaim should be quick, as it only
has to evict the pages, not write them to disk.  So I think basically
writeback should speed up (compared to no-zswap case) swap_writepage()
while zswap is not full, but (theoretically) slow it down (compared to
no-zswap case) while zswap is full, while writethrough should slow
down swap_writepage() slightly (the time it takes to compress/store
the page) but consistently, almost the same amount before it's full vs
when it's full.  Theoretically :-)  Definitely something to think
about and test for.

Another idea that I was going to bring up after/if writethrough was
added, was to move the page compression out of the store, maybe using
a mempool and worker thread (or something), so that the zswap store is
very fast.  Testing would of course be needed to see if that really
improved things or not...





>
> Just something I am thinking about.
>
> Seth

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Bob Liu]

Hi Dan & Seth,

On Wed, Nov 27, 2013 at 9:28 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Mon, Nov 25, 2013 at 1:00 PM, Seth Jennings <sjennings@variantweb.net> wrote:
>> On Fri, Nov 22, 2013 at 11:29:16AM -0600, Seth Jennings wrote:
>>> On Wed, Nov 20, 2013 at 02:49:33PM -0500, Dan Streetman wrote:
>>> > Currently, zswap is writeback cache; stored pages are not sent
>>> > to swap disk, and when zswap wants to evict old pages it must
>>> > first write them back to swap cache/disk manually.  This avoids
>>> > swap out disk I/O up front, but only moves that disk I/O to
>>> > the writeback case (for pages that are evicted), and adds the
>>> > overhead of having to uncompress the evicted pages, and adds the
>>> > need for an additional free page (to store the uncompressed page)
>>> > at a time of likely high memory pressure.  Additionally, being
>>> > writeback adds complexity to zswap by having to perform the
>>> > writeback on page eviction.
>>> >
>>> > This changes zswap to writethrough cache by enabling
>>> > frontswap_writethrough() before registering, so that any
>>> > successful page store will also be written to swap disk.  All the
>>> > writeback code is removed since it is no longer needed, and the
>>> > only operation during a page eviction is now to remove the entry
>>> > from the tree and free it.
>>>
>>> I like it.  It gets rid of a lot of nasty writeback code in zswap.
>>>
>>> I'll have to test before I ack, hopefully by the end of the day.
>>>
>>> Yes, this will increase writes to the swap device over the delayed
>>> writeback approach.  I think it is a good thing though.  I think it
>>> makes the difference between zswap and zram, both in operation and in
>>> application, more apparent. Zram is the better choice for embedded where
>>> write wear is a concern, and zswap being better if you need more
>>> flexibility to dynamically manage the compressed pool.
>>
>> One thing I realized while doing my testing was that making zswap
>> writethrough also impacts synchronous reclaim.  Zswap, as it is now,
>> makes the swapcache page clean during swap_writepage() which allows
>> shrink_page_list() to immediately reclaim it.  Making zswap writethrough
>> eliminates this advantage and swapcache pages must be scanned again
>> before they can be reclaimed, as is the case with normal swapping.
>
> Yep, I thought about that as well, and it is true, but only while
> zswap is not full.  With writeback, once zswap fills up, page stores
> will frequently have to reclaim pages by writing compressed pages to
> disk.  With writethrough, the zbud reclaim should be quick, as it only
> has to evict the pages, not write them to disk.  So I think basically
> writeback should speed up (compared to no-zswap case) swap_writepage()
> while zswap is not full, but (theoretically) slow it down (compared to
> no-zswap case) while zswap is full, while writethrough should slow
> down swap_writepage() slightly (the time it takes to compress/store
> the page) but consistently, almost the same amount before it's full vs
> when it's full.  Theoretically :-)  Definitely something to think
> about and test for.
>

Have you gotten any further benchmark result?

-- 
Thanks,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm/zswap: change zswap to writethrough cache

[Dan Streetman]

On Wed, Dec 11, 2013 at 4:02 AM, Bob Liu <lliubbo@gmail.com> wrote:
> Hi Dan & Seth,
>
> On Wed, Nov 27, 2013 at 9:28 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>> On Mon, Nov 25, 2013 at 1:00 PM, Seth Jennings <sjennings@variantweb.net> wrote:
>>> On Fri, Nov 22, 2013 at 11:29:16AM -0600, Seth Jennings wrote:
>>>> On Wed, Nov 20, 2013 at 02:49:33PM -0500, Dan Streetman wrote:
>>>> > Currently, zswap is writeback cache; stored pages are not sent
>>>> > to swap disk, and when zswap wants to evict old pages it must
>>>> > first write them back to swap cache/disk manually.  This avoids
>>>> > swap out disk I/O up front, but only moves that disk I/O to
>>>> > the writeback case (for pages that are evicted), and adds the
>>>> > overhead of having to uncompress the evicted pages, and adds the
>>>> > need for an additional free page (to store the uncompressed page)
>>>> > at a time of likely high memory pressure.  Additionally, being
>>>> > writeback adds complexity to zswap by having to perform the
>>>> > writeback on page eviction.
>>>> >
>>>> > This changes zswap to writethrough cache by enabling
>>>> > frontswap_writethrough() before registering, so that any
>>>> > successful page store will also be written to swap disk.  All the
>>>> > writeback code is removed since it is no longer needed, and the
>>>> > only operation during a page eviction is now to remove the entry
>>>> > from the tree and free it.
>>>>
>>>> I like it.  It gets rid of a lot of nasty writeback code in zswap.
>>>>
>>>> I'll have to test before I ack, hopefully by the end of the day.
>>>>
>>>> Yes, this will increase writes to the swap device over the delayed
>>>> writeback approach.  I think it is a good thing though.  I think it
>>>> makes the difference between zswap and zram, both in operation and in
>>>> application, more apparent. Zram is the better choice for embedded where
>>>> write wear is a concern, and zswap being better if you need more
>>>> flexibility to dynamically manage the compressed pool.
>>>
>>> One thing I realized while doing my testing was that making zswap
>>> writethrough also impacts synchronous reclaim.  Zswap, as it is now,
>>> makes the swapcache page clean during swap_writepage() which allows
>>> shrink_page_list() to immediately reclaim it.  Making zswap writethrough
>>> eliminates this advantage and swapcache pages must be scanned again
>>> before they can be reclaimed, as is the case with normal swapping.
>>
>> Yep, I thought about that as well, and it is true, but only while
>> zswap is not full.  With writeback, once zswap fills up, page stores
>> will frequently have to reclaim pages by writing compressed pages to
>> disk.  With writethrough, the zbud reclaim should be quick, as it only
>> has to evict the pages, not write them to disk.  So I think basically
>> writeback should speed up (compared to no-zswap case) swap_writepage()
>> while zswap is not full, but (theoretically) slow it down (compared to
>> no-zswap case) while zswap is full, while writethrough should slow
>> down swap_writepage() slightly (the time it takes to compress/store
>> the page) but consistently, almost the same amount before it's full vs
>> when it's full.  Theoretically :-)  Definitely something to think
>> about and test for.
>>
>
> Have you gotten any further benchmark result?

Yes, and sorry for the delay.

The initial numbers I got on a relatively low-end (laptop) system seem
to indicate that writethrough does reduce performance in the beginning
when zswap isn't full, but also improves performance once zswap fills
up.  I'm working on getting a higher-end server class system set up to
test as well, and getting a larger sample size of test runs (but
specjbb takes quite a long time each run).

At this point, I'm thinking that based on those results and Weijie's
suggestion to make it configurable, it probably is better to keep the
writeback code and allow selection of writeback or writethrough.  That
would allow *possibly* changing from writeback to writethrough based
on how full zswap is; but also at the least it would allow users to
select which to use.  I still think keeping both makes zswap more
complex, but moving completely to writethrough may not be best for all
situations.  I suspect that especially systems with relatively slow
disc I/O and fast processors would benefit from writeback, while
systems with relatively fast disc I/O (e.g. SSD swap) and slower
processors would benefit from writethrough.

So, any opinions on keeping both writeback and writethrough, with (for
now) a module param to select?  I'll send an updated patch if that
sounds agreeable to all...

The specific specjbb numbers (on a dual core 2.4GHz with 4G ram) I got were:

   writeback
heap in mb      bops
3000             38887
3500             39260
4000             38113
4500             15686
5000             10978
5500              1445
6000              1827

   writethrough
heap in mb      bops
3000             39021
3500             35998
4000             36223
4500              7222
5000              7717
5500              2304
6000              2455

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>