[PATCH v3] zram: break the strict dependency from lzo

[PATCH v3] zram: break the strict dependency from lzo

[Rui Salvaterra]

There's nothing special about zram and lzo. It works just fine without it, so
as long as at least one of the other supported compression algorithms is
selected.

Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
---
v3: fix the default selection when lzo isn't present. Rebase against 5.10-rc1.
v2: fix the dependency on CRYPTO.

 drivers/block/zram/Kconfig    |  6 +++++-
 drivers/block/zram/zcomp.c    |  2 ++
 drivers/block/zram/zram_drv.c | 14 +++++++++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index fe7a4b7d30cf..14b2b098d662 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -2,7 +2,6 @@
 config ZRAM
 	tristate "Compressed RAM block device support"
 	depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO
-	select CRYPTO_LZO
 	help
 	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
 	  Pages written to these disks are compressed and stored in memory
@@ -37,3 +36,8 @@ config ZRAM_MEMORY_TRACKING
 	  /sys/kernel/debug/zram/zramX/block_state.
 
 	  See Documentation/admin-guide/blockdev/zram.rst for more information.
+
+config ZRAM_AUTOSEL_ALGO
+	def_bool y
+	depends on ZRAM && !(CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)
+	select CRYPTO_LZO
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 33e3b76c4fa9..052aa3f65514 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -15,8 +15,10 @@
 #include "zcomp.h"
 
 static const char * const backends[] = {
+#if IS_ENABLED(CONFIG_CRYPTO_LZO)
 	"lzo",
 	"lzo-rle",
+#endif
 #if IS_ENABLED(CONFIG_CRYPTO_LZ4)
 	"lz4",
 #endif
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 1b697208d661..2ae09561ba79 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,7 +42,19 @@ static DEFINE_IDR(zram_index_idr);
 static DEFINE_MUTEX(zram_index_mutex);
 
 static int zram_major;
-static const char *default_compressor = "lzo-rle";
+
+static const char *default_compressor =
+#if IS_ENABLED(CONFIG_CRYPTO_LZO)
+	"lzo-rle";
+#elif IS_ENABLED(CONFIG_CRYPTO_LZ4)
+	"lz4";
+#elif IS_ENABLED(CONFIG_CRYPTO_LZ4HC)
+	"lz4hc";
+#elif IS_ENABLED(CONFIG_CRYPTO_842)
+	"842";
+#elif IS_ENABLED(CONFIG_CRYPTO_ZSTD)
+	"zstd";
+#endif
 
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
-- 
2.28.0

Re: [PATCH v3] zram: break the strict dependency from lzo

[Sergey Senozhatsky]

On (20/10/26 08:51), Rui Salvaterra wrote:
>  static const char * const backends[] = {
> +#if IS_ENABLED(CONFIG_CRYPTO_LZO)
>  	"lzo",
>  	"lzo-rle",
> +#endif
>  #if IS_ENABLED(CONFIG_CRYPTO_LZ4)
>  	"lz4",
>  #endif
[..]
> +static const char *default_compressor =
> +#if IS_ENABLED(CONFIG_CRYPTO_LZO)
> +	"lzo-rle";
> +#elif IS_ENABLED(CONFIG_CRYPTO_LZ4)
> +	"lz4";
> +#elif IS_ENABLED(CONFIG_CRYPTO_LZ4HC)
> +	"lz4hc";
> +#elif IS_ENABLED(CONFIG_CRYPTO_842)
> +	"842";
> +#elif IS_ENABLED(CONFIG_CRYPTO_ZSTD)
> +	"zstd";
> +#endif

Honestly, I'm not entirely excited. lzo is a fallback compression
algorithm. If you want to use zram with something else thenconfigure
zram to use something else. What do all these #if/#elif buy us?

	-ss

Re: [PATCH v3] zram: break the strict dependency from lzo

[Rui Salvaterra]

Hi, Sergey,

On Tue, 27 Oct 2020 at 01:22, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> Honestly, I'm not entirely excited. lzo is a fallback compression
> algorithm. If you want to use zram with something else thenconfigure
> zram to use something else. What do all these #if/#elif buy us?

The idea is to allow us to select a single compression algorithm at
build time, if we're sure to use something other than lzo. The status
quo only allows us to select additional algorithms, as lzo is a hard
dependency. I dislike the "iffery" as much as the next guy, but in
this case the default selection stops being static (as lzo may not be
available at run time), so we have to fall back to an algorithm which
is enabled, otherwise zram won't work out of the box (we'd always need
to choose the algorithm manually in sysfs).
Personally, I always use zram with zstd, and the only lzo dependency I
have is zram. Disabling lzo saves me about 3 kiB in the final
(xz-compressed) vmlinuz image. It's not much, for sure, but when your
total storage is 4 MiB (and your RAM is 32 MiB), every bit counts. :)

Thanks,
Rui

Re: [PATCH v3] zram: break the strict dependency from lzo

[Sergey Senozhatsky]

Hi,

On (20/10/27 08:39), Rui Salvaterra wrote:
> Personally, I always use zram with zstd, and the only lzo dependency I
> have is zram. Disabling lzo saves me about 3 kiB in the final
> (xz-compressed) vmlinuz image. It's not much, for sure, but when your
> total storage is 4 MiB (and your RAM is 32 MiB), every bit counts. :)

Can the following work then?

Completely untested.

===8<===

diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index fe7a4b7d30cf..f93eed40e155 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -2,7 +2,7 @@
 config ZRAM
 	tristate "Compressed RAM block device support"
 	depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO
-	select CRYPTO_LZO
+	depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)
 	help
 	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
 	  Pages written to these disks are compressed and stored in memory
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 33e3b76c4fa9..98c7c46c9c3a 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -15,8 +15,10 @@
 #include "zcomp.h"
 
 static const char * const backends[] = {
+#if IS_ENABLED(CONFIG_CRYPTO_LZO)
 	"lzo",
 	"lzo-rle",
+#endif
 #if IS_ENABLED(CONFIG_CRYPTO_LZ4)
 	"lz4",
 #endif
@@ -202,6 +204,21 @@ void zcomp_destroy(struct zcomp *comp)
 	kfree(comp);
 }
 
+const char *default_compressor(void)
+{
+	/*
+	 * Pick the first available one (there should be at least one).
+	 *
+	 * In theory, we can drop all the ifdefs from backends[] and
+	 * just iterate backends array doing crypto_has_comp(comp, 0, 0)
+	 * for each entry and return the first one which is recognized by
+	 * crypto. But crypto_has_comp() modprobes compression drivers,
+	 * so we may endup with extra loaded drivers, when the 'default'
+	 * compressor is not what zram is configured to use.
+	 */
+	return backends[0];
+}
+
 /*
  * search available compressors for requested algorithm.
  * allocate new zcomp and initialize it. return compressing
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 40f6420f4b2e..f104be9eae9c 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -27,6 +27,7 @@ int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node);
 ssize_t zcomp_available_show(const char *comp, char *buf);
 bool zcomp_available_algorithm(const char *comp);
 
+const char *default_compressor(void);
 struct zcomp *zcomp_create(const char *comp);
 void zcomp_destroy(struct zcomp *comp);
 
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 1b697208d661..f02ee050c7bf 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,7 +42,6 @@ static DEFINE_IDR(zram_index_idr);
 static DEFINE_MUTEX(zram_index_mutex);
 
 static int zram_major;
-static const char *default_compressor = "lzo-rle";
 
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
@@ -1960,7 +1959,9 @@ static int zram_add(void)
 	blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, zram->disk->queue);
 	device_add_disk(NULL, zram->disk, zram_disk_attr_groups);
 
-	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
+	strlcpy(zram->compressor,
+		default_compressor(),
+		sizeof(zram->compressor));
 
 	zram_debugfs_register(zram);
 	pr_info("Added device: %s\n", zram->disk->disk_name);

Re: [PATCH v3] zram: break the strict dependency from lzo

[Rui Salvaterra]

Hi, Sergey,

On Wed, 28 Oct 2020 at 10:19, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> Can the following work then?

Almost! See below. :)

> Completely untested.
>
> ===8<===
>
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index fe7a4b7d30cf..f93eed40e155 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -2,7 +2,7 @@
>  config ZRAM
>         tristate "Compressed RAM block device support"
>         depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO
> -       select CRYPTO_LZO
> +       depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)

This reverses the dependency order, as now we have to select a
supported compression algorithm in order for zram to be visible in the
block device drivers list. This is why I wrote the ZRAM_AUTOSEL_ALGO
kconfig symbol, which automatically selects lzo as a fallback. If the
user chooses to select another supported algorithm, he will then be
allowed to deselect lzo. We thus follow the principle of least
surprise, IMHO.

>         help
>           Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
>           Pages written to these disks are compressed and stored in memory
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 33e3b76c4fa9..98c7c46c9c3a 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -15,8 +15,10 @@
>  #include "zcomp.h"
>
>  static const char * const backends[] = {
> +#if IS_ENABLED(CONFIG_CRYPTO_LZO)
>         "lzo",
>         "lzo-rle",
> +#endif
>  #if IS_ENABLED(CONFIG_CRYPTO_LZ4)
>         "lz4",
>  #endif
> @@ -202,6 +204,21 @@ void zcomp_destroy(struct zcomp *comp)
>         kfree(comp);
>  }
>
> +const char *default_compressor(void)
> +{
> +       /*
> +        * Pick the first available one (there should be at least one).
> +        *
> +        * In theory, we can drop all the ifdefs from backends[] and
> +        * just iterate backends array doing crypto_has_comp(comp, 0, 0)
> +        * for each entry and return the first one which is recognized by
> +        * crypto. But crypto_has_comp() modprobes compression drivers,
> +        * so we may endup with extra loaded drivers, when the 'default'
> +        * compressor is not what zram is configured to use.
> +        */
> +       return backends[0];
> +}

This is much more elegant, indeed, and I completely agree with just
returning the first one. I'll incorporate your feedback and send a v4
soon.

Thanks a lot,
Rui

Re: [PATCH v3] zram: break the strict dependency from lzo

[Sergey Senozhatsky]

Hi,

On (20/10/28 11:25), Rui Salvaterra wrote:
> > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > index fe7a4b7d30cf..f93eed40e155 100644
> > --- a/drivers/block/zram/Kconfig
> > +++ b/drivers/block/zram/Kconfig
> > @@ -2,7 +2,7 @@
> >  config ZRAM
> >         tristate "Compressed RAM block device support"
> >         depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO
> > -       select CRYPTO_LZO
> > +       depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)
> 
> This reverses the dependency order, as now we have to select a
> supported compression algorithm in order for zram to be visible in the
> block device drivers list.

Right, but well we also need to select ZSMALLOC and CRYPTO for
zram to become visible (the thing that I found out recently is
that you can always check the hidden/blocked items by hitting
'z' in menuconfig).

> This is why I wrote the ZRAM_AUTOSEL_ALGO
> kconfig symbol, which automatically selects lzo as a fallback. If the
> user chooses to select another supported algorithm, he will then be
> allowed to deselect lzo. We thus follow the principle of least
> surprise, IMHO.

OK, makes sense.

	-ss

Re: [PATCH v3] zram: break the strict dependency from lzo

[Rui Salvaterra]

Hi, again,

On Wed, 28 Oct 2020 at 18:22, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> Right, but well we also need to select ZSMALLOC and CRYPTO for
> zram to become visible (the thing that I found out recently is
> that you can always check the hidden/blocked items by hitting
> 'z' in menuconfig).

Sure, I can fix that too. Should I do it now, or wait for
Andrew's/Minchan's feedback?
And that 'z' key… wow. Did you read the kconfig code? I have no idea
how you found that one out, but it sure deserves to be documented
somewhere, it's too useful to be true.

Thanks,
Rui

Re: [PATCH v3] zram: break the strict dependency from lzo

[Sergey Senozhatsky]

Hi,

On (20/10/28 23:11), Rui Salvaterra wrote:
> Hi, again,
> 
> On Wed, 28 Oct 2020 at 18:22, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > Right, but well we also need to select ZSMALLOC and CRYPTO for
> > zram to become visible (the thing that I found out recently is
> > that you can always check the hidden/blocked items by hitting
> > 'z' in menuconfig).
> 
> Sure, I can fix that too. Should I do it now, or wait for
> Andrew's/Minchan's feedback?

Let's give Minchan and Andrew some time. It's still -rc1, so we have
plenty of time to land this patch.

> And that 'z' key… wow. Did you read the kconfig code?

By accident, "fat fingers" lol

	-ss