Re: [PATCH] frontswap: allow multiple backends

[Andrew Morton]

On Thu, 28 May 2015 16:28:37 -0400 Dan Streetman <ddstreet@xxxxxxxx> wrote:

> Change frontswap single pointer to a singly linked list of frontswap
> implementations.  Update Xen tmem implementation as register no longer
> returns anything.
> 
> Frontswap only keeps track of a single implementation; any implementation
> that registers second (or later) will replace the previously registered
> implementation, and gets a pointer to the previous implementation that
> the new implementation is expected to pass all frontswap functions to
> if it can't handle the function itself.  However that method doesn't
> really make much sense, as passing that work on to every implementation
> adds unnecessary work to implementations; instead, frontswap should
> simply keep a list of all registered implementations and try each
> implementation for any function.  Most importantly, neither of the
> two currently existing frontswap implementations in the kernel actually
> do anything with any previous frontswap implementation that they
> replace when registering.
> 
> This allows frontswap to successfully manage multiple implementations
> by keeping a list of them all.

Looks OK to me.  The "you can never deregister" thing makes life
simpler.

But we need to have a fight over style issues.  Just because you *can*
do something doesn't mean you should.  Don't make you poor readers sit
there going crosseyed at elaborate `for' statements.  Try to keep the
code as simple and straightforward as possible.

> ...
>
>  /*
> - * Register operations for frontswap, returning previous thus allowing
> - * detection of multiple backends and possible nesting.
> + * Register operations for frontswap
>   */
> -struct frontswap_ops *frontswap_register_ops(struct frontswap_ops *ops)
> +void frontswap_register_ops(struct frontswap_ops *ops)
>  {
> -	struct frontswap_ops *old = frontswap_ops;
> -	int i;
> -
> -	for (i = 0; i < MAX_SWAPFILES; i++) {
> -		if (test_and_clear_bit(i, need_init)) {
> -			struct swap_info_struct *sis = swap_info[i];
> -			/* __frontswap_init _should_ have set it! */
> -			if (!sis->frontswap_map)
> -				return ERR_PTR(-EINVAL);
> -			ops->init(i);
> +	DECLARE_BITMAP(a, MAX_SWAPFILES);
> +	DECLARE_BITMAP(b, MAX_SWAPFILES);
> +	struct swap_info_struct *si;
> +	unsigned int i;
> +
> +	spin_lock(&swap_lock);
> +	plist_for_each_entry(si, &swap_active_head, list) {
> +		if (!WARN_ON(!si->frontswap_map))
> +			set_bit(si->type, a);
> +	}
> +	spin_unlock(&swap_lock);
> +
> +	for (i = find_first_bit(a, MAX_SWAPFILES);
> +	     i < MAX_SWAPFILES;
> +	     i = find_next_bit(a, MAX_SWAPFILES, i + 1))
> +		ops->init(i);

	i = find_first_bit(a, MAX_SWAPFILES);
	while (i < MAX_SWAPFILES) {
		ops->init(i);
		i = find_next_bit(a, MAX_SWAPFILES, i + 1);
	}

> +	do {
> +		ops->next = frontswap_ops;
> +	} while (cmpxchg(&frontswap_ops, ops->next, ops) != ops->next);
> +
> +	spin_lock(&swap_lock);
> +	plist_for_each_entry(si, &swap_active_head, list) {
> +		if (si->frontswap_map)
> +			set_bit(si->type, b);
> +	}
> +	spin_unlock(&swap_lock);
> +
> +	if (!bitmap_equal(a, b, MAX_SWAPFILES)) {
> +		for (i = 0; i < MAX_SWAPFILES; i++) {
> +			if (!test_bit(i, a) && test_bit(i, b))
> +				ops->init(i);
> +			else if (test_bit(i, a) && !test_bit(i, b))
> +				ops->invalidate_area(i);
>  		}
> ...
>
> @@ -215,24 +216,25 @@ static inline void __frontswap_clear(struct swap_info_struct *sis,
>   */
>  int __frontswap_store(struct page *page)
>  {
> -	int ret = -1, dup = 0;
> +	int ret, dup;
>  	swp_entry_t entry = { .val = page_private(page), };
>  	int type = swp_type(entry);
>  	struct swap_info_struct *sis = swap_info[type];
>  	pgoff_t offset = swp_offset(entry);
> +	struct frontswap_ops *ops;
>  
>  	/*
>  	 * Return if no backend registed.
>  	 * Don't need to inc frontswap_failed_stores here.
>  	 */
>  	if (!frontswap_ops)
> -		return ret;
> +		return -1;
>  
>  	BUG_ON(!PageLocked(page));
>  	BUG_ON(sis == NULL);
> -	if (__frontswap_test(sis, offset))
> -		dup = 1;
> -	ret = frontswap_ops->store(type, offset, page);
> +	dup = __frontswap_test(sis, offset);
> +	for (ops = frontswap_ops, ret = -1; ops && ret; ops = ops->next)
> +		ret = ops->store(type, offset, page);

	ret = -1;
	for (ops = frontswap_ops; ops; ops = ops->next) {
		ret = ops->store(type, offset, page);
		if (!ret)
			break;
	}

One advantage of doing it this way is that it leaves room for comments.
And this code would benefit from a comment above the "if (!ret)". 
What's going on here?  What could cause ->store to return zero and is
this an error?  We should explain this somewhere;  `struct
frontswap_ops' is cheerily undocumented, so where?


Is the `ret = -1' really needed?  Can this function ever be called if
there aren't any registered frontswap_ops?


Also, __frontswap_store() disturbs me:

: int __frontswap_store(struct page *page)
: {
: 	int ret, dup;
: 	swp_entry_t entry = { .val = page_private(page), };
: 	int type = swp_type(entry);
: 	struct swap_info_struct *sis = swap_info[type];
: 	pgoff_t offset = swp_offset(entry);
: 	struct frontswap_ops *ops;
: 
: 	/*
: 	 * Return if no backend registed.
: 	 * Don't need to inc frontswap_failed_stores here.
: 	 */
: 	if (!frontswap_ops)
: 		return -1;
: 
: 	BUG_ON(!PageLocked(page));
: 	BUG_ON(sis == NULL);
: 	dup = __frontswap_test(sis, offset);
: 	ret = -1;
: 	for (ops = frontswap_ops; ops; ops = ops->next) {
: 		ret = ops->store(type, offset, page);
: 		if (!ret)
: 			break;
: 	}

Here we've just iterated through all the registered operations.

: 	if (ret == 0) {
: 		set_bit(offset, sis->frontswap_map);
: 		inc_frontswap_succ_stores();
: 		if (!dup)
: 			atomic_inc(&sis->frontswap_pages);
: 	} else {
: 		/*
: 		  failed dup always results in automatic invalidate of
: 		  the (older) page from frontswap
: 		 */
: 		inc_frontswap_failed_stores();
: 		if (dup) {
: 			__frontswap_clear(sis, offset);
: 			frontswap_ops->invalidate_page(type, offset);

But here we call ->invalidate_page on just one of teh registered
operations.  Seems wrong.

Maybe some careful code commentary would clear this up.

: 		}
: 	}
: 	if (frontswap_writethrough_enabled)
: 		/* report failure so swap also writes to swap device */
: 		ret = -1;
: 	return ret;
: }

Please review:

--- a/mm/frontswap.c~frontswap-allow-multiple-backends-fix
+++ a/mm/frontswap.c
@@ -97,7 +97,7 @@ static inline void inc_frontswap_invalid
  *
  * Obviously the opposite (unloading the backend) must be done after all
  * the frontswap_[store|load|invalidate_area|invalidate_page] start
- * ignorning or failing the requests.  However, there is currently no way
+ * ignoring or failing the requests.  However, there is currently no way
  * to unload a backend once it is registered.
  */
 
@@ -118,10 +118,11 @@ void frontswap_register_ops(struct front
 	}
 	spin_unlock(&swap_lock);
 
-	for (i = find_first_bit(a, MAX_SWAPFILES);
-	     i < MAX_SWAPFILES;
-	     i = find_next_bit(a, MAX_SWAPFILES, i + 1))
+	i = find_first_bit(a, MAX_SWAPFILES);
+	while (i < MAX_SWAPFILES) {
 		ops->init(i);
+		i = find_next_bit(a, MAX_SWAPFILES, i + 1);
+	}
 
 	do {
 		ops->next = frontswap_ops;
@@ -233,8 +234,12 @@ int __frontswap_store(struct page *page)
 	BUG_ON(!PageLocked(page));
 	BUG_ON(sis == NULL);
 	dup = __frontswap_test(sis, offset);
-	for (ops = frontswap_ops, ret = -1; ops && ret; ops = ops->next)
+	ret = -1;
+	for (ops = frontswap_ops; ops; ops = ops->next) {
 		ret = ops->store(type, offset, page);
+		if (!ret)
+			break;
+	}
 	if (ret == 0) {
 		set_bit(offset, sis->frontswap_map);
 		inc_frontswap_succ_stores();
@@ -279,8 +284,12 @@ int __frontswap_load(struct page *page)
 	BUG_ON(sis == NULL);
 	if (!__frontswap_test(sis, offset))
 		return -1;
-	for (ops = frontswap_ops, ret = -1; ops && ret; ops = ops->next)
+	ret = -1;
+	for (ops = frontswap_ops; ops; ops = ops->next) {
 		ret = ops->load(type, offset, page);
+		if (!ret)
+			break;
+	}
 	if (ret == 0) {
 		inc_frontswap_loads();
 		if (frontswap_tmem_exclusive_gets_enabled) {
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/