[PATCH] mm/zswap: don't allow entry eviction if in use by load

[PATCH] mm/zswap: don't allow entry eviction if in use by load

[Dan Streetman]

The changes in commit 0ab0abcf511545d1fddbe72a36b3ca73388ac937
introduce a bug in writeback, if an entry is in use by load
it will be evicted anyway, which isn't correct (technically,
the code currently in zbud doesn't actually care much what the
zswap evict function returns, but that could change).

This changes the check in the writeback function to prevent eviction
if the entry is still in use (with a nonzero refcount).  The
refcount is used instead of searching the rb tree beacuse we're
holding the tree lock (which is required for any changes to refcount)
and it's faster than a tree search.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 mm/zswap.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index e55bab9..e154f1e 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -600,14 +600,18 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 	zswap_entry_put(tree, entry);
 
 	/*
-	* There are two possible situations for entry here:
+	* 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
-	*  search the tree and free the entry if find entry
+	* (3) refcount is 2, entry is in use by load, prevent eviction
 	*/
-	if (entry == zswap_rb_search(&tree->rbroot, offset))
+	if (likely(entry->refcount > 0))
 		zswap_entry_put(tree, entry);
+	if (unlikely(entry->refcount > 0)) {
+		spin_unlock(&tree->lock);
+		return -EAGAIN;
+	}
 	spin_unlock(&tree->lock);
 
 	goto end;
-- 
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] mm/zswap: don't allow entry eviction if in use by load

[Weijie Yang]

Hello Dan

On Thu, Nov 21, 2013 at 3:48 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> The changes in commit 0ab0abcf511545d1fddbe72a36b3ca73388ac937
> introduce a bug in writeback, if an entry is in use by load
> it will be evicted anyway, which isn't correct (technically,
> the code currently in zbud doesn't actually care much what the
> zswap evict function returns, but that could change).

Thanks for your work. Howerver it is not a bug.

I have thought about this situation, and it will never happen.
If entry is being loaded, its corresponding page must be in swapcache
so zswap_get_swap_cache_page() will return ZSWAP_SWAPCACHE_EXIST

If I miss something, please let me know.

Thanks!

> This changes the check in the writeback function to prevent eviction
> if the entry is still in use (with a nonzero refcount).  The
> refcount is used instead of searching the rb tree beacuse we're
> holding the tree lock (which is required for any changes to refcount)
> and it's faster than a tree search.
>
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
>  mm/zswap.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index e55bab9..e154f1e 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -600,14 +600,18 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>         zswap_entry_put(tree, entry);
>
>         /*
> -       * There are two possible situations for entry here:
> +       * 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
> -       *  search the tree and free the entry if find entry
> +       * (3) refcount is 2, entry is in use by load, prevent eviction
>         */
> -       if (entry == zswap_rb_search(&tree->rbroot, offset))
> +       if (likely(entry->refcount > 0))
>                 zswap_entry_put(tree, entry);
> +       if (unlikely(entry->refcount > 0)) {
> +               spin_unlock(&tree->lock);
> +               return -EAGAIN;
> +       }
>         spin_unlock(&tree->lock);
>
>         goto end;
> --
> 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] mm/zswap: don't allow entry eviction if in use by load

[Dan Streetman]

On Wed, Nov 20, 2013 at 8:59 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
> Hello Dan
>
> On Thu, Nov 21, 2013 at 3:48 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>> The changes in commit 0ab0abcf511545d1fddbe72a36b3ca73388ac937
>> introduce a bug in writeback, if an entry is in use by load
>> it will be evicted anyway, which isn't correct (technically,
>> the code currently in zbud doesn't actually care much what the
>> zswap evict function returns, but that could change).
>
> Thanks for your work. Howerver it is not a bug.
>
> I have thought about this situation, and it will never happen.
> If entry is being loaded, its corresponding page must be in swapcache
> so zswap_get_swap_cache_page() will return ZSWAP_SWAPCACHE_EXIST

ah, ok.

While you do imply that with the fail: comment, I personally think it
should also be stated in the refcount check comment; a comment
indicating failure can happen due to concurrent load does not make
clear that it will *always* fail in cases of concurrent load and so
that case doesn't need to be checked for in the success path.
Additionally, the lack of a check here is assuming that zswap won't be
updated to ever inc the refcount anywhere besides the load function,
which might cause unexpected breakage later; i.e., this is coding to
the current implementation, not to the entry->refcount api.

Can I also ask why you do a rb_search instead of just checking the
entry->refcount?  Doing the search is going to take longer than just
checking the refcount; is there some case where the entry will not be
in the rb but will have a nonzero refcount?

--
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] mm/zswap: don't allow entry eviction if in use by load

[Dan Streetman]

On Thu, Nov 21, 2013 at 4:44 PM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Wed, Nov 20, 2013 at 8:59 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
>> Hello Dan
>>
>> On Thu, Nov 21, 2013 at 3:48 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>>> The changes in commit 0ab0abcf511545d1fddbe72a36b3ca73388ac937
>>> introduce a bug in writeback, if an entry is in use by load
>>> it will be evicted anyway, which isn't correct (technically,
>>> the code currently in zbud doesn't actually care much what the
>>> zswap evict function returns, but that could change).
>>
>> Thanks for your work. Howerver it is not a bug.
>>
>> I have thought about this situation, and it will never happen.
>> If entry is being loaded, its corresponding page must be in swapcache
>> so zswap_get_swap_cache_page() will return ZSWAP_SWAPCACHE_EXIST
>
>
> Can I also ask why you do a rb_search instead of just checking the
> entry->refcount?  Doing the search is going to take longer than just
> checking the refcount; is there some case where the entry will not be
> in the rb but will have a nonzero refcount?

Never mind; I realized the entry will have been free'd once it's
refcount is 0 so that can't be checked.

--
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>