Re: [PATCH v2] zsmalloc: return -EBUSY for zspage migration lock contention

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/03/19 11:07), Hui Zhu wrote:
> movable_operations::migrate_page() should return an appropriate error
> code for temporary migration failures so the migration core can handle
> them correctly.
> 
> zs_page_migrate() currently returns -EINVAL when zspage_write_trylock()
> fails. That path reflects transient lock contention, not invalid input,
> so -EINVAL is clearly wrong.
> 
> However, -EAGAIN is also inappropriate here: the zspage's reader-lock
> owner may hold the lock for an unbounded duration due to slow
> decompression. Since migration retries are bounded by
> NR_MAX_MIGRATE_PAGES_RETRY and performed with virtually no delay between
> attempts, there is no guarantee the lock will be released in time for a
> retry to succeed. -EAGAIN implies "try again soon", which does not hold
> in this case.
> 
> Return -EBUSY instead, which more accurately conveys that the resource
> is occupied and migration cannot proceed at this time.
> 
> Changelog:
> v2:
> According to the comments of Sergey Senozhatsky, change from -EAGAIN to
> -EBUSY and add comments.

I assume this Changelog was supposed to be under "---".

> +		/*
> +		 * Return -EBUSY but not -EAGAIN: the zspage's reader-lock
> +		 * owner may hold the lock for an unbounded duration due to a
> +		 * slow decompression.
		^^^ or reader-lock owner preemption

> +		 * Since migration retries are bounded by
> +		 * NR_MAX_MIGRATE_PAGES_RETRY and performed with virtually no
> +		 * delay between attempts, there is no guarantee the lock will
> +		 * be released in time for a retry to succeed.
> +		 * -EAGAIN implies "try again soon", which does not hold here.
> +		 * -EBUSY more accurately conveys "resource is occupied,
> +		 * migration cannot proceed".
> +		 */
> +		return -EBUSY;
>  	}

Otherwise,
Acked-by: Sergey Senozhatsky <senozhatsky@chromium.org>