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

From: Rui Salvaterra
Date: Wed Oct 28 2020 - 18:40:32 EST


Hi, Sergey,

On Wed, 28 Oct 2020 at 10:19, Sergey Senozhatsky
<sergey.senozhatsky.work@xxxxxxxxx> 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