[PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Sami Kerola]

Earlier the --algorithm and --streams had to be combined with --size.  To
user requirement to combine with --size was indirectly told using
following message.

$ zramctl --stream 3 zram3
zramctl: options --algorithm, --find and --streams are mutually exclusive

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
index afbc0e8..43b7dcb 100644
--- a/sys-utils/zramctl.c
+++ b/sys-utils/zramctl.c
@@ -388,7 +388,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 	fputs(_("     --raw                 use raw status output format\n"), out);
 	fputs(_(" -r, --reset               reset all specified devices\n"), out);
 	fputs(_(" -s, --size <size>         device size\n"), out);
-	fputs(_(" -t, --streams <number>    number of compressoin streams\n\n"), out);
+	fputs(_(" -t, --streams <number>    number of compressoin streams\n"), out);
 
 	fputs(USAGE_SEPARATOR, out);
 	fputs(USAGE_HELP, out);
@@ -408,6 +408,7 @@ enum {
 	A_STATUS,
 	A_CREATE,
 	A_FINDONLY,
+	A_SETTINGS,
 	A_RESET
 };
 
@@ -502,8 +503,12 @@ int main(int argc, char **argv)
 	if (find && optind < argc)
 		errx(EXIT_FAILURE, _("option --find is mutually exclusive "
 				     "with <device>"));
-	if (act == A_NONE)
-		act = find ? A_FINDONLY : A_STATUS;
+	if (act == A_NONE) {
+		if (algorithm || nstreams)
+			act = A_SETTINGS;
+		else
+			act = find ? A_FINDONLY : A_STATUS;
+	}
 
 	if (act != A_RESET && optind + 1 < argc)
 		errx(EXIT_FAILURE, _("only one <device> at a time is allowed"));
@@ -552,6 +557,7 @@ int main(int argc, char **argv)
 		free_zram(zram);
 		break;
 	case A_CREATE:
+	case A_SETTINGS:
 		if (find) {
 			zram = find_free_zram();
 			if (!zram)
@@ -561,8 +567,9 @@ int main(int argc, char **argv)
 		else
 			zram = new_zram(argv[optind]);
 
-		if (zram_set_u64parm(zram, "reset", 1))
-			err(EXIT_FAILURE, _("%s: failed to reset"), zram->devname);
+		if (act == A_CREATE)
+			if (zram_set_u64parm(zram, "reset", 1))
+				err(EXIT_FAILURE, _("%s: failed to reset"), zram->devname);
 
 		if (nstreams &&
 		    zram_set_u64parm(zram, "max_comp_streams", nstreams))
@@ -572,9 +579,10 @@ int main(int argc, char **argv)
 		    zram_set_strparm(zram, "comp_algorithm", algorithm))
 			err(EXIT_FAILURE, _("%s: failed to set algorithm"), zram->devname);
 
-		if (zram_set_u64parm(zram, "disksize", size))
-			err(EXIT_FAILURE, _("%s: failed to set disksize (%ju bytes)"),
-				zram->devname, size);
+		if (act == A_CREATE)
+			if (zram_set_u64parm(zram, "disksize", size))
+				err(EXIT_FAILURE, _("%s: failed to set disksize (%ju bytes)"),
+				    zram->devname, size);
 		if (find)
 			printf("%s\n", zram->devname);
 		free_zram(zram);
-- 
2.0.3

Re: [PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Bernhard Voelker]

On 08/04/2014 01:08 AM, Sami Kerola wrote:
> Earlier the --algorithm and --streams had to be combined with --size.  To
> user requirement to combine with --size was indirectly told using
> following message.

Sorry, I can't parse this. What do you mean?

> $ zramctl --stream 3 zram3
> zramctl: options --algorithm, --find and --streams are mutually exclusive
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  sys-utils/zramctl.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
> index afbc0e8..43b7dcb 100644
> --- a/sys-utils/zramctl.c
> +++ b/sys-utils/zramctl.c
> @@ -388,7 +388,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
>  	fputs(_("     --raw                 use raw status output format\n"), out);
>  	fputs(_(" -r, --reset               reset all specified devices\n"), out);
>  	fputs(_(" -s, --size <size>         device size\n"), out);
> -	fputs(_(" -t, --streams <number>    number of compressoin streams\n\n"), out);
> +	fputs(_(" -t, --streams <number>    number of compressoin streams\n"), out);

s/compressoin/compression/

Have a nice day,
Berny

Re: [PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Karel Zak]

On Mon, Aug 04, 2014 at 12:08:10AM +0100, Sami Kerola wrote:
> Earlier the --algorithm and --streams had to be combined with --size.  To
> user requirement to combine with --size was indirectly told using
> following message.

And is it really supported by kernel? I see in
Documentation/blockdev/zram.txt:

 In order to enable compression backend's multi stream support
 max_comp_streams must be initially set to desired concurrency 
 level before ZRAM device initialisation.

I guess that when you set disksize the zram device is "locked" for
setting changes. It means that modify already initialized (created)
zram devices is impossible. You have to reset the device to "unlock"
the device setting.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Sami Kerola]

On 4 August 2014 12:55, Karel Zak <kzak@redhat.com> wrote:
> On Mon, Aug 04, 2014 at 12:08:10AM +0100, Sami Kerola wrote:
>> Earlier the --algorithm and --streams had to be combined with --size.  To
>> user requirement to combine with --size was indirectly told using
>> following message.
>
> And is it really supported by kernel? I see in
> Documentation/blockdev/zram.txt:
>
>  In order to enable compression backend's multi stream support
>  max_comp_streams must be initially set to desired concurrency
>  level before ZRAM device initialisation.
>
> I guess that when you set disksize the zram device is "locked" for
> setting changes. It means that modify already initialized (created)
> zram devices is impossible. You have to reset the device to "unlock"
> the device setting.

After playing with zramctl I found out --algorithm must be set before
--size. The --streams can be set after --size, but not if device is in use.
When device is in use none of the settings are allowed.

$ ./zramctl
NAME       ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
/dev/zram0 lzo           100K   4K   76B    4K       2 [SWAP]
/dev/zram2 lzo            44K   0B    0B    0B       1
/dev/zram3 lzo             1M  40K  796B   40K       1 /mnt
$ ./zramctl zram1
NAME  ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
zram1 lzo             0B   0B    0B    0B       1

$ ./zramctl --algorithm lz4 zram1 ; echo $?
0
$ ./zramctl --algorithm lz4 zram2 ; echo $?
zramctl: zram2: failed to set algorithm: Device or resource busy
1
$ ./zramctl --algorithm lz4 zram3 ; echo $?
zramctl: zram3: failed to set algorithm: Device or resource busy
1

$ ./zramctl --stream 2 zram1 ; echo $?
0
$ ./zramctl --stream 2 zram2 ; echo $?
0
$ ./zramctl --stream 2 zram3 ; echo $?
zramctl: zram3: failed to set number of streams: Invalid argument
1

Whether the case --streams 2 zram2 in above example is correct or a
bug is a question to kernel developers (Sergey is CC'd). Looking the
commit message

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=fe8eb122c82b2049c460fc6df6e8583a2f935cff

the device behavior might be bug.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Karel Zak]

On Tue, Aug 05, 2014 at 11:36:59PM +0100, Sami Kerola wrote:
> On 4 August 2014 12:55, Karel Zak <kzak@redhat.com> wrote:
> > On Mon, Aug 04, 2014 at 12:08:10AM +0100, Sami Kerola wrote:
> >> Earlier the --algorithm and --streams had to be combined with --size.  To
> >> user requirement to combine with --size was indirectly told using
> >> following message.
> >
> > And is it really supported by kernel? I see in
> > Documentation/blockdev/zram.txt:
> >
> >  In order to enable compression backend's multi stream support
> >  max_comp_streams must be initially set to desired concurrency
> >  level before ZRAM device initialisation.
> >
> > I guess that when you set disksize the zram device is "locked" for
> > setting changes. It means that modify already initialized (created)
> > zram devices is impossible. You have to reset the device to "unlock"
> > the device setting.
> 
> After playing with zramctl I found out --algorithm must be set before
> --size. The --streams can be set after --size, but not if device is in use.
> When device is in use none of the settings are allowed.

 I'd like to keep the command line semantic simple and stupid and
 support --streams and --algorithm for device initialization (it means
 with --size) only. We don't have to export to command line all the
 crazy kernel implementation details.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Sergey Senozhatsky]

Cc Minchan Kim, Jerome Marchand, Nitin Gupta, linux-kernel

On (08/05/14 23:36), Sami Kerola wrote:
> On 4 August 2014 12:55, Karel Zak <kzak@redhat.com> wrote:
> > On Mon, Aug 04, 2014 at 12:08:10AM +0100, Sami Kerola wrote:
> >> Earlier the --algorithm and --streams had to be combined with --size.  To
> >> user requirement to combine with --size was indirectly told using
> >> following message.
> >
> > And is it really supported by kernel? I see in
> > Documentation/blockdev/zram.txt:
> >
> >  In order to enable compression backend's multi stream support
> >  max_comp_streams must be initially set to desired concurrency
> >  level before ZRAM device initialisation.
> >
> > I guess that when you set disksize the zram device is "locked" for
> > setting changes. It means that modify already initialized (created)
> > zram devices is impossible. You have to reset the device to "unlock"
> > the device setting.
> 
> After playing with zramctl I found out --algorithm must be set before
> --size

yes. historically, storing the `size' == full device initialisation.
I believe, it used to be so since the beginning of zram. compression
algorithm selection was just plugged in into the existing scheme of
things.

> The --streams can be set after --size, but not if device is in use.
> When device is in use none of the settings are allowed.

you can't change compression algorithm on initialised/being used
device. obviously. if zram has X pages compressed with lz0 then
zram is supposed to use lz0 for decompression.

'streams' options is a little bit tricky. we have two different
implementations for a single and multi stream backends. and there are
reasons for that, to keep it short.

if you create device with a single stream backend -- you can't change
it any more.
if you create device with a multi stream backend -- you can change the
number of streams (though, it doesn't make a lot of sense to set streams
to 1 in that case).

iow, you can downgrade from multi stream to a multi stream device with
only 1 stream (which is not identical to a single stream), but you can't
upgrade a single stream device to a multi stream device.

> $ ./zramctl
> NAME       ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
> /dev/zram0 lzo           100K   4K   76B    4K       2 [SWAP]
> /dev/zram2 lzo            44K   0B    0B    0B       1
> /dev/zram3 lzo             1M  40K  796B   40K       1 /mnt
> $ ./zramctl zram1
> NAME  ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
> zram1 lzo             0B   0B    0B    0B       1
> 
> $ ./zramctl --algorithm lz4 zram1 ; echo $?
> 0
> $ ./zramctl --algorithm lz4 zram2 ; echo $?
> zramctl: zram2: failed to set algorithm: Device or resource busy
> 1
> $ ./zramctl --algorithm lz4 zram3 ; echo $?
> zramctl: zram3: failed to set algorithm: Device or resource busy
> 1
> 
> $ ./zramctl --stream 2 zram1 ; echo $?
> 0
> $ ./zramctl --stream 2 zram2 ; echo $?
> 0
> $ ./zramctl --stream 2 zram3 ; echo $?
> zramctl: zram3: failed to set number of streams: Invalid argument
> 1
> 
> Whether the case --streams 2 zram2 in above example is correct or a
> bug is a question to kernel developers (Sergey is CC'd). Looking the
> commit message
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=fe8eb122c82b2049c460fc6df6e8583a2f935cff
> 
> the device behavior might be bug.
>

where?

	-ss

Re: [PATCH 8/8] zramctl: allow use of --algorithm and --streams alone

[Sami Kerola]

On 6 August 2014 13:19, Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:
> Cc Minchan Kim, Jerome Marchand, Nitin Gupta, linux-kernel
>
> On (08/05/14 23:36), Sami Kerola wrote:
>> On 4 August 2014 12:55, Karel Zak <kzak@redhat.com> wrote:
>> > On Mon, Aug 04, 2014 at 12:08:10AM +0100, Sami Kerola wrote:
>> >> Earlier the --algorithm and --streams had to be combined with --size.  To
>> >> user requirement to combine with --size was indirectly told using
>> >> following message.
>> >
>> > And is it really supported by kernel? I see in
>> > Documentation/blockdev/zram.txt:
>> >
>> >  In order to enable compression backend's multi stream support
>> >  max_comp_streams must be initially set to desired concurrency
>> >  level before ZRAM device initialisation.
>> >
>> > I guess that when you set disksize the zram device is "locked" for
>> > setting changes. It means that modify already initialized (created)
>> > zram devices is impossible. You have to reset the device to "unlock"
>> > the device setting.
>>
>> After playing with zramctl I found out --algorithm must be set before
>> --size
>
> yes. historically, storing the `size' == full device initialisation.
> I believe, it used to be so since the beginning of zram. compression
> algorithm selection was just plugged in into the existing scheme of
> things.
>
>> The --streams can be set after --size, but not if device is in use.
>> When device is in use none of the settings are allowed.
>
> you can't change compression algorithm on initialised/being used
> device. obviously. if zram has X pages compressed with lz0 then
> zram is supposed to use lz0 for decompression.

Hi Sergey, and the others,

I expected this to be the case.

> 'streams' options is a little bit tricky. we have two different
> implementations for a single and multi stream backends. and there are
> reasons for that, to keep it short.
>
> if you create device with a single stream backend -- you can't change
> it any more.
> if you create device with a multi stream backend -- you can change the
> number of streams (though, it doesn't make a lot of sense to set streams
> to 1 in that case).
>
> iow, you can downgrade from multi stream to a multi stream device with
> only 1 stream (which is not identical to a single stream), but you can't
> upgrade a single stream device to a multi stream device.

Thank you for explanation, I understand.  But as Karel mentioned in the
previous message the zramctl will be kept simple, at least for now, which
means support to change number of streams in cases when device is multi
streaming capable will not be present.  At least for now.

Secondly the few updates to zramctl I wrote couple days ago are now ready
for final review.  Most importantly the last one of the changes is
modified to be a message change.  It was earlier the patch 8/8 in first
message of this thread, that allowed algorithm and streams changes
without reset.  Now it this modest improvement.

https://github.com/kerolasa/lelux-utiliteetit/commit/ba7340bd093828feac127c09e8c36e01640f6dcb

And here are there reset.

The following changes since commit cd8414f7a13aca0b3ea150b473b83f00819b312f:

  cfdisk: move curs_set(1) to ui_end() (2014-08-06 15:39:27 +0200)

are available in the git repository at:

  git://github.com/kerolasa/lelux-utiliteetit.git zram

for you to fetch changes up to ba7340bd093828feac127c09e8c36e01640f6dcb:

  zramctl: improve option combination error messaging (2014-08-06
20:46:23 +0100)

----------------------------------------------------------------
Sami Kerola (7):
      docs: add details to zramctl --size option documentation
      zramctl: mark --reset mutually exclusive with --algorithm and --streams
      zramctl: fail status printout when device does not exist
      zramctl: improve error message
      zramctl: fix mount point printout
      zramctl: add bash completion script
      zramctl: improve option combination error messaging

 bash-completion/Makemodule.am |  3 +++
 bash-completion/zramctl       | 51
+++++++++++++++++++++++++++++++++++++++++++++++++++
 sys-utils/zramctl.8           | 11 +++++++++--
 sys-utils/zramctl.c           | 37 +++++++++++++++++++++++--------------
 4 files changed, 86 insertions(+), 16 deletions(-)
 create mode 100644 bash-completion/zramctl

>> $ ./zramctl
>> NAME       ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
>> /dev/zram0 lzo           100K   4K   76B    4K       2 [SWAP]
>> /dev/zram2 lzo            44K   0B    0B    0B       1
>> /dev/zram3 lzo             1M  40K  796B   40K       1 /mnt
>> $ ./zramctl zram1
>> NAME  ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
>> zram1 lzo             0B   0B    0B    0B       1
>>
>> $ ./zramctl --algorithm lz4 zram1 ; echo $?
>> 0
>> $ ./zramctl --algorithm lz4 zram2 ; echo $?
>> zramctl: zram2: failed to set algorithm: Device or resource busy
>> 1
>> $ ./zramctl --algorithm lz4 zram3 ; echo $?
>> zramctl: zram3: failed to set algorithm: Device or resource busy
>> 1
>>
>> $ ./zramctl --stream 2 zram1 ; echo $?
>> 0
>> $ ./zramctl --stream 2 zram2 ; echo $?
>> 0
>> $ ./zramctl --stream 2 zram3 ; echo $?
>> zramctl: zram3: failed to set number of streams: Invalid argument
>> 1
>>
>> Whether the case --streams 2 zram2 in above example is correct or a
>> bug is a question to kernel developers (Sergey is CC'd). Looking the
>> commit message
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=fe8eb122c82b2049c460fc6df6e8583a2f935cff
>>
>> the device behavior might be bug.
>
> where?

Reading your earlier message it is now clear to me the there never was
a bug, but just lack of understanding how the zram is meant to work.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/