Message-ID: <20150910050351.GB9562@bbox>
Date:	Thu, 10 Sep 2015 14:03:51 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Luis Henriques <luis.henriques@...onical.com>
Cc:	Nitin Gupta <ngupta@...are.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] zram: introduce comp algorithm fallback functionality

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@...xv:/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@...nel.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists