Re: [PATCH v2] zram: introduce comp algorithm fallback functionality

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

On (09/08/15 19:42), Luis Henriques wrote:
> 
> Note that previously this operation (i.e. setting an invalid algorithm)
> would result in no algorithm being selected, which means that this
> represents a small change in the default behaviour.
> 

previously it would result in guaranteed to fail
	echo xxx > /sys/block/zram<id>/disksize
and thus in no device, not just "in no algorithm being selected".

comp_algorithm historically wasn't returning anything back to user space
and it always copied in its input (what later would have been used as a
compression algorithm name). yes, "wasn't returning anything back" is not
entirely correct, there was (and still is) a chance to receive -EBUSY, but
that was (and still is) is absolutely equivalent to
	/sys/block/zram<id>/initstate != 0

(well, I never had a clear understanding of why comp_algorithm and other
attrs return -EBUSY instead of simply silently ignoring the input; is it
really an error? there is a initstate attr for that, but I never had enough
reasons to change it either). anyway, believe me or not, that's what my toy
scripts were doing for years (and hopefully it's not because of the fact that
I'm completely unaware of the trivial basics of software development, as we
found out yesterday, but because zram ABI was letting me to do so).

# head -17 create-zram
---
#!/bin/sh

rmmod zram
modprobe zram

if [ -e /sys/block/zram0/initstate ]; then
	initdone=`cat /sys/block/zram0/initstate`
	if [ $initdone = 1 ]; then
		echo "init done"
		exit 1
	fi
fi

echo 8 > /sys/block/zram0/max_comp_streams

echo $1 > /sys/block/zram0/comp_algorithm
cat /sys/block/zram0/comp_algorithm

---

So, "./create-zram LLZZOO 2G" will have different result now.


Other than that

Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
> Changes since v1:
>  * Moved the algorithm check further up, before mutex lock
>  * Dropped error path refactoring
>  * Updated commit text to explicitly refer to the ABI change
>  (changes suggested by Sergey and Minchan)
> 
> drivers/block/zram/zram_drv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 9c01f5bfa33f..1caa8f793e51 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	struct zram *zram = dev_to_zram(dev);
>  	size_t sz;
>  
> +	if (!zcomp_available_algorithm(buf))
> +		return -EINVAL;
> +
>  	down_write(&zram->init_lock);
>  	if (init_done(zram)) {
>  		up_write(&zram->init_lock);
> @@ -378,9 +381,6 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>  		zram->compressor[sz - 1] = 0x00;
>  
> -	if (!zcomp_available_algorithm(zram->compressor))
> -		len = -EINVAL;
> -
>  	up_write(&zram->init_lock);
>  	return len;
>  }
>