[PATCH v2] zram: introduce comp algorithm fallback functionality

[PATCH v2] zram: introduce comp algorithm fallback functionality

[Luis Henriques]

When the user supplies an unsupported compression algorithm, keep the
previously selected one (knowingly supported) or the default one (if the
compression algorithm hasn't been changed yet).

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.

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

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

[Sergey Senozhatsky]

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

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

[Minchan Kim]

On Tue, Sep 08, 2015 at 07:42:56PM +0100, Luis Henriques wrote:
> When the user supplies an unsupported compression algorithm, keep the
> previously selected one (knowingly supported) or the default one (if the
> compression algorithm hasn't been changed yet).
> 
> 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.

It seems it is hard for Andrew to parse so I will add more.

Before)

# echo "super-lz4" > /sys/block/zram0/comp_algorithm
bash: echo: write error: Invalid argument
# echo $((200<<20)) > /sys/block/zram0/disksize
bash: echo: write error: Invalid argument
# dmesg | grep zram
..
zram: Cannot initialise super-lz4 compressing backend
# cat /sys/block/zram0/initstate
0
# cat /sys/block/zram0/comp_algorithm
lzo lz4


After)

# echo "super-lz4" > /sys/block/zram0/comp_algorithm
bash: echo: write error: Invalid argument
# echo $((200<<20)) > /sys/block/zram0/disksize
# dmesg | grep zram
root@bboxv:/home/barrios# dmesg | grep zram
..
zram0: detected capacity change from 0 to 209715200
# cat /sys/block/zram0/initstate
1
# cat /sys/block/zram0/comp_algorithm
[lzo] lz4

IOW, with the patch, if user pass a unsupported algorithm,
zram will be initialized with default compressor while we didn't
allow it old.

As Sergey pointed out, it changes zRAM ABI slightly so if someone
doesn't have checked result of algorithm selection but go with that
to initialize, it could be initialized with default compressor rather
than failing.

Although it might be regression, I want to correct it in this chance.

For initializing zram, there are some preparation steps but they are
*optional* steps. Must step is only 'disksize setting' now.
It means you could initialize zram with below one line enoughly.

# echo $((200<<20)) > /sys/block/zram0/disksize

If you feed woring input, it could be ignored and initialized
with default values for optional parameters.

# echo "aaa" > /sys/block/zram0/max_comp_streams
bash: echo: write error: Invalid argument
# echo $((200<<20)) > /sys/block/zram0/disksize
# cat /sys/block/zram0/initstate
1
# cat /sys/block/zram0/max_comp_streams
1

Another example,

# echo "aaa" > /sys/block/zram0/mem_limit
bash: echo: write error: Invalid argument
# echo $((200<<20)) > /sys/block/zram0/disksize
# cat /sys/block/zram0/initstate
1
# cat /sys/block/zram0/mem_limit
0

But now only one exception with comp_algorithm.

# echo "super-lz4" > /sys/block/zram0/comp_algorithm
bash: echo: write error: Invalid argument
# echo $((200<<20)) > /sys/block/zram0/disksize
bash: echo: write error: Invalid argument
# cat /sys/block/zram0/initstate
0

Why should we handle comp_algorithm is so special?

With another approach:

We could remove every error return code for all optional steps
so user never fails to set option to zram and check them only
where MUST step(ie, disksize set).
It could be doable but the problem is as follows,

1. It makes hard for user to understand why it was failed.

Only thing kernel can return is -EINVAL, I think. What is invalid?

        algorithm? mem_limit? streams?

Maybe we should export some message via dmesg so user should rely on it.
But dmesg should be just helper, not ABI.

2. It would break more scripts. IOW, ABI regression would be more severe.

I guess most of scripts have checked result of his doing so if we
removes it, it will break them.

3. Weired interface

Although we could change ABI of optional parameters into no failure smoothly
without no regressoin, it looks like strange.
Currently, comp_algorithm couldn't be changed in runtime, at least.
Given that, every success of "echo zzz > /sys/block/zram0/comp_algorithm"
makes users to be confused that he might think to success to change algorithm
in runtime. IOW, it should return error which is more intuitive forme.

That's why I support this patch.

Acked-by: Minchan Kim <minchan@kernel.org>

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

[Sergey Senozhatsky]

Hello,

On (09/10/15 14:03), Minchan Kim wrote:
[..]
>
> I guess most of scripts have checked result of his doing so if we
> removes it, it will break them.

to be honest, we never documented or required any of those. the only source
of information for the user space -- zram.txt documentation -- simply says
to do 'echo 3 > /sys/block/zram0/max_comp_streams' or any other bunch of
'echo'-s. so, technically, a user is free to simply copy-paste what we do
in zram.txt to his zram.sh and call it a "recommended way of doing things
in zram".

besides, zram.txt is outdated. for example there is no mem_used_max
documentation.

we need to do better job documenting. I'll try to take a look on this later
this week.


> Given that, every success of "echo zzz > /sys/block/zram0/comp_algorithm"
> makes users to be confused that he might think to success to change algorithm
> in runtime. IOW, it should return error which is more intuitive forme.

well, not quite like that. we return -EINVAL on invalid output since
d93435c3fba4a47b773693b0c8992470d38510d5. this patch does not change
anything from this pov. it does, however, change the behaviour of
disksize store that follows.

I'm fine when the motivation for this patch is to introduce the
"fallback" mechanism (like zswap fallbacks to default compressor, f.e.),
but it wasn't the case -- I rewrote the patch slightly, reworded the
commit message and put some reasoning to this patch.

	-ss

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

[Minchan Kim]

On Thu, Sep 10, 2015 at 02:33:19PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (09/10/15 14:03), Minchan Kim wrote:
> [..]
> >
> > I guess most of scripts have checked result of his doing so if we
> > removes it, it will break them.
> 
> to be honest, we never documented or required any of those. the only source
> of information for the user space -- zram.txt documentation -- simply says
> to do 'echo 3 > /sys/block/zram0/max_comp_streams' or any other bunch of
> 'echo'-s. so, technically, a user is free to simply copy-paste what we do
> in zram.txt to his zram.sh and call it a "recommended way of doing things
> in zram".

Agree. That's why we spend a lot discussion for this small change.

> 
> besides, zram.txt is outdated. for example there is no mem_used_max
> documentation.
> 
> we need to do better job documenting. I'll try to take a look on this later
> this week.

Thanks.

> 
> 
> > Given that, every success of "echo zzz > /sys/block/zram0/comp_algorithm"
> > makes users to be confused that he might think to success to change algorithm
> > in runtime. IOW, it should return error which is more intuitive forme.
> 
> well, not quite like that. we return -EINVAL on invalid output since
> d93435c3fba4a47b773693b0c8992470d38510d5. this patch does not change
> anything from this pov. it does, however, change the behaviour of
> disksize store that follows.

I said like that you cut off.

"Although we could change ABI of optional parameters into no failure smoothly"
                                                          ^^^^^^^^^^

> 
> I'm fine when the motivation for this patch is to introduce the
> "fallback" mechanism (like zswap fallbacks to default compressor, f.e.),
> but it wasn't the case -- I rewrote the patch slightly, reworded the
> commit message and put some reasoning to this patch.
> 
> 	-ss

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

[Andrew Morton]

On Thu, 10 Sep 2015 14:03:51 +0900 Minchan Kim <minchan@kernel.org> wrote:

> On Tue, Sep 08, 2015 at 07:42:56PM +0100, Luis Henriques wrote:
> > When the user supplies an unsupported compression algorithm, keep the
> > previously selected one (knowingly supported) or the default one (if the
> > compression algorithm hasn't been changed yet).
> > 
> > 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.
> 
> It seems it is hard for Andrew to parse so I will add more.

Thanks ;)

What's missing here is an understandable-by-andrew *reason* for the
patch.  What's wrong with the old behaviour and why is the new
behaviour better?

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

[Minchan Kim]

Hello Andrew,

On Tue, Sep 15, 2015 at 04:07:00PM -0700, Andrew Morton wrote:
> On Thu, 10 Sep 2015 14:03:51 +0900 Minchan Kim <minchan@kernel.org> wrote:
> 
> > On Tue, Sep 08, 2015 at 07:42:56PM +0100, Luis Henriques wrote:
> > > When the user supplies an unsupported compression algorithm, keep the
> > > previously selected one (knowingly supported) or the default one (if the
> > > compression algorithm hasn't been changed yet).
> > > 
> > > 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.
> > 
> > It seems it is hard for Andrew to parse so I will add more.
> 
> Thanks ;)
> 
> What's missing here is an understandable-by-andrew *reason* for the
> patch.  What's wrong with the old behaviour and why is the new
> behaviour better?

Oops, I said it in detail but it seems I got failed.

For initializing zram, we need to set up 3 optional parameters in advance.

1. the number of compression streams
2. memory limitation
3. compression alrogithm

Although user pass completely wrong value to set up for 1 and 2 parameters,
it's okay because they have default value so zram will be initialized
with the default value(Of course, when user pass wrong value via *echo*,
sysfs returns -EINVAL so user can notice it).

But 3 is not consistent with other optional parameters.
IOW, If user pass wrong value to set up 3 parameter, zram's initialization
would be failed unlike other optional parameters.

So, this patch make them consistent.