ubifs: Add support for zstd compression.
diff mbox series

Message ID 20190515210202.21169-1-richard@nod.at
State New
Delegated to: Richard Weinberger
Headers show
Series
  • ubifs: Add support for zstd compression.
Related show

Commit Message

Richard Weinberger May 15, 2019, 9:02 p.m. UTC
From: Michele Dionisio <michele.dionisio@gmail.com>

zstd shows a good compression rate and is faster than lzo,
also on slow ARM cores.

Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Michele Dionisio <michele.dionisio@gmail.com>
[rw: rewrote commit message]
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/Kconfig       | 10 ++++++++++
 fs/ubifs/compress.c    | 27 ++++++++++++++++++++++++++-
 fs/ubifs/super.c       |  2 ++
 fs/ubifs/ubifs-media.h |  2 ++
 4 files changed, 40 insertions(+), 1 deletion(-)

Comments

Sebastian Andrzej Siewior May 16, 2019, 6:23 p.m. UTC | #1
On 2019-05-15 23:02:02 [+0200], Richard Weinberger wrote:

That is a lot less compared to what I needed for mkfs.ubifs. I just
tested this in nandsim and it works - I can read the image generated by
mkfs.

As for my compression level testing: Do we want to keep -L parameter or
rather drop it? The max level hardly makes a difference (and we don't
have it / use it for LZO/ZLIB)…

Sebastian
Emil Lenngren June 7, 2019, 3:34 p.m. UTC | #2
Hello,

Den ons 15 maj 2019 kl 23:03 skrev Richard Weinberger <richard@nod.at>:
>
> From: Michele Dionisio <michele.dionisio@gmail.com>
>
> zstd shows a good compression rate and is faster than lzo,
> also on slow ARM cores.
>
> Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Signed-off-by: Michele Dionisio <michele.dionisio@gmail.com>
> [rw: rewrote commit message]
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ubifs/Kconfig       | 10 ++++++++++
>  fs/ubifs/compress.c    | 27 ++++++++++++++++++++++++++-
>  fs/ubifs/super.c       |  2 ++
>  fs/ubifs/ubifs-media.h |  2 ++
>  4 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
> index 9da2f135121b..8d84d2ed096d 100644
> --- a/fs/ubifs/Kconfig
> +++ b/fs/ubifs/Kconfig
> @@ -5,8 +5,10 @@ config UBIFS_FS
>         select CRYPTO if UBIFS_FS_ADVANCED_COMPR
>         select CRYPTO if UBIFS_FS_LZO
>         select CRYPTO if UBIFS_FS_ZLIB
> +       select CRYPTO if UBIFS_FS_ZSTD
>         select CRYPTO_LZO if UBIFS_FS_LZO
>         select CRYPTO_DEFLATE if UBIFS_FS_ZLIB
> +       select CRYPTO_ZSTD if UBIFS_FS_ZSTD
>         select CRYPTO_HASH_INFO
>         select UBIFS_FS_XATTR if FS_ENCRYPTION
>         depends on MTD_UBI
> @@ -37,6 +39,14 @@ config UBIFS_FS_ZLIB
>         help
>           Zlib compresses better than LZO but it is slower. Say 'Y' if unsure.
>
> +config UBIFS_FS_ZSTD
> +       bool "ZSTD compression support" if UBIFS_FS_ADVANCED_COMPR
> +       depends on UBIFS_FS
> +       default y
> +       help
> +         ZSTD compresses is a big win in speed over Zlib and
> +         in compression ratio over LZO. Say 'Y' if unsure.
> +
>  config UBIFS_ATIME_SUPPORT
>         bool "Access time support"
>         default n
> diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
> index 565cb56d7225..89183aeeeb7a 100644
> --- a/fs/ubifs/compress.c
> +++ b/fs/ubifs/compress.c
> @@ -71,6 +71,24 @@ static struct ubifs_compressor zlib_compr = {
>  };
>  #endif
>
> +#ifdef CONFIG_UBIFS_FS_ZSTD
> +static DEFINE_MUTEX(zstd_enc_mutex);
> +static DEFINE_MUTEX(zstd_dec_mutex);
> +
> +static struct ubifs_compressor zstd_compr = {
> +       .compr_type = UBIFS_COMPR_ZSTD,
> +       .comp_mutex = &zstd_enc_mutex,
> +       .decomp_mutex = &zstd_dec_mutex,
> +       .name = "zstd",
> +       .capi_name = "zstd",
> +};
> +#else
> +static struct ubifs_compressor zstd_compr = {
> +       .compr_type = UBIFS_COMPR_ZSTD,
> +       .name = "zstd",
> +};
> +#endif
> +
>  /* All UBIFS compressors */
>  struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
>
> @@ -228,13 +246,19 @@ int __init ubifs_compressors_init(void)
>         if (err)
>                 return err;
>
> -       err = compr_init(&zlib_compr);
> +       err = compr_init(&zstd_compr);
>         if (err)
>                 goto out_lzo;
>
> +       err = compr_init(&zlib_compr);
> +       if (err)
> +               goto out_zstd;
> +
>         ubifs_compressors[UBIFS_COMPR_NONE] = &none_compr;
>         return 0;
>
> +out_zstd:
> +       compr_exit(&zstd_compr);
>  out_lzo:
>         compr_exit(&lzo_compr);
>         return err;
> @@ -247,4 +271,5 @@ void ubifs_compressors_exit(void)
>  {
>         compr_exit(&lzo_compr);
>         compr_exit(&zlib_compr);
> +       compr_exit(&zstd_compr);
>  }
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 04b8ecfd3470..ea8615261936 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1055,6 +1055,8 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>                                 c->mount_opts.compr_type = UBIFS_COMPR_LZO;
>                         else if (!strcmp(name, "zlib"))
>                                 c->mount_opts.compr_type = UBIFS_COMPR_ZLIB;
> +                       else if (!strcmp(name, "zstd"))
> +                               c->mount_opts.compr_type = UBIFS_COMPR_ZSTD;
>                         else {
>                                 ubifs_err(c, "unknown compressor \"%s\"", name); //FIXME: is c ready?
>                                 kfree(name);
> diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
> index 8b7c1844014f..697b1b89066a 100644
> --- a/fs/ubifs/ubifs-media.h
> +++ b/fs/ubifs/ubifs-media.h
> @@ -348,12 +348,14 @@ enum {
>   * UBIFS_COMPR_NONE: no compression
>   * UBIFS_COMPR_LZO: LZO compression
>   * UBIFS_COMPR_ZLIB: ZLIB compression
> + * UBIFS_COMPR_ZSTD: ZSTD compression
>   * UBIFS_COMPR_TYPES_CNT: count of supported compression types
>   */
>  enum {
>         UBIFS_COMPR_NONE,
>         UBIFS_COMPR_LZO,
>         UBIFS_COMPR_ZLIB,
> +       UBIFS_COMPR_ZSTD,
>         UBIFS_COMPR_TYPES_CNT,
>  };
>
> --
> 2.16.4

In fs/ubifs/sb.c we have

static int get_default_compressor(struct ubifs_info *c)
{
    if (ubifs_compr_present(c, UBIFS_COMPR_LZO))
        return UBIFS_COMPR_LZO;

    if (ubifs_compr_present(c, UBIFS_COMPR_ZLIB))
        return UBIFS_COMPR_ZLIB;

    return UBIFS_COMPR_NONE;
}

Maybe add an entry for zstd here as well?

/Emil
Richard Weinberger June 7, 2019, 8:09 p.m. UTC | #3
Emil,

----- Ursprüngliche Mail -----
> In fs/ubifs/sb.c we have
> 
> static int get_default_compressor(struct ubifs_info *c)
> {
>    if (ubifs_compr_present(c, UBIFS_COMPR_LZO))
>        return UBIFS_COMPR_LZO;
> 
>    if (ubifs_compr_present(c, UBIFS_COMPR_ZLIB))
>        return UBIFS_COMPR_ZLIB;
> 
>    return UBIFS_COMPR_NONE;
> }
> 
> Maybe add an entry for zstd here as well?

Where would you add it? If we add it after UBIFS_COMPR_ZLIB,
it will effectively never get selected, unless someone builds a kernel
without lzo and zlib but zstd.
If we add it before UBIFS_COMPR_ZLIB, it will be used always and users
end up with unreadable files if they reboot to an older kernel.
Please note that we didn't raise the UBIFS format version for zstd.

So I'm not sure what is the best choice for the default filesystem.

Thanks,
//richard
Emil Lenngren June 7, 2019, 8:27 p.m. UTC | #4
Hi,

Den fre 7 juni 2019 kl 22:09 skrev Richard Weinberger <richard@nod.at>:
>
> Emil,
>
> ----- Ursprüngliche Mail -----
> > In fs/ubifs/sb.c we have
> >
> > static int get_default_compressor(struct ubifs_info *c)
> > {
> >    if (ubifs_compr_present(c, UBIFS_COMPR_LZO))
> >        return UBIFS_COMPR_LZO;
> >
> >    if (ubifs_compr_present(c, UBIFS_COMPR_ZLIB))
> >        return UBIFS_COMPR_ZLIB;
> >
> >    return UBIFS_COMPR_NONE;
> > }
> >
> > Maybe add an entry for zstd here as well?
>
> Where would you add it? If we add it after UBIFS_COMPR_ZLIB,
> it will effectively never get selected, unless someone builds a kernel
> without lzo and zlib but zstd.
> If we add it before UBIFS_COMPR_ZLIB, it will be used always and users
> end up with unreadable files if they reboot to an older kernel.
> Please note that we didn't raise the UBIFS format version for zstd.
>
> So I'm not sure what is the best choice for the default filesystem.

My idea was at the end, i.e. it will only be used when LZO and ZLIB
are not selected to be included for UBIFS, for example when someone
compiles a minimal kernel who knows exactly which compression
algorithms will be used on that system. I guess that's the same reason
why zlib exists at all and is placed after lzo.
But of course the other positions also have their pros. If we perform
more benchmarks speed/size and conclude that zstd outperforms zlib for
all aspects, then maybe placing it between lzo and zlib could be an
option, but as you say we should avoid breaking compatibility with
older systems.
I did a single test today and compared lzo and zstd and on that test
lzo had faster decompression speed but resulted in larger space. I'll
do more tests later.

/Emil
Richard Weinberger June 7, 2019, 8:49 p.m. UTC | #5
----- Ursprüngliche Mail -----
> Von: "Emil Lenngren" <emil.lenngren@gmail.com>
> An: "richard" <richard@nod.at>
> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Sebastian Andrzej Siewior" <sebastian@breakpoint.cc>, "linux-kernel"
> <linux-kernel@vger.kernel.org>, "Michele Dionisio" <michele.dionisio@gmail.com>
> Gesendet: Freitag, 7. Juni 2019 22:27:09
> Betreff: Re: [PATCH] ubifs: Add support for zstd compression.
>> So I'm not sure what is the best choice for the default filesystem.
> 
> My idea was at the end, i.e. it will only be used when LZO and ZLIB
> are not selected to be included for UBIFS, for example when someone
> compiles a minimal kernel who knows exactly which compression
> algorithms will be used on that system.

BTW: you can always select the compressor using the compr= mount option.
Also for the default filesystem.

Putting it at the end does not harm but IMHO the use is little.
But for the sake of completes, I agree with you. Can you send a follow-up
patch? 

> I did a single test today and compared lzo and zstd and on that test
> lzo had faster decompression speed but resulted in larger space. I'll
> do more tests later.

Can you please share more details? I'm interested what CPU this was.

Thanks,
//richard
Emil Lenngren June 7, 2019, 11:40 p.m. UTC | #6
Hi,

Den fre 7 juni 2019 kl 22:49 skrev Richard Weinberger <richard@nod.at>:
>
> ----- Ursprüngliche Mail -----
> > Von: "Emil Lenngren" <emil.lenngren@gmail.com>
> > An: "richard" <richard@nod.at>
> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Sebastian Andrzej Siewior" <sebastian@breakpoint.cc>, "linux-kernel"
> > <linux-kernel@vger.kernel.org>, "Michele Dionisio" <michele.dionisio@gmail.com>
> > Gesendet: Freitag, 7. Juni 2019 22:27:09
> > Betreff: Re: [PATCH] ubifs: Add support for zstd compression.
> >> So I'm not sure what is the best choice for the default filesystem.
> >
> > My idea was at the end, i.e. it will only be used when LZO and ZLIB
> > are not selected to be included for UBIFS, for example when someone
> > compiles a minimal kernel who knows exactly which compression
> > algorithms will be used on that system.
>
> BTW: you can always select the compressor using the compr= mount option.
> Also for the default filesystem.

Yep that's what I'm using while I'm testing.

> Putting it at the end does not harm but IMHO the use is little.
> But for the sake of completes, I agree with you. Can you send a follow-up
> patch?

Ok

>
> > I did a single test today and compared lzo and zstd and on that test
> > lzo had faster decompression speed but resulted in larger space. I'll
> > do more tests later.
>
> Can you please share more details? I'm interested what CPU this was.

ARM Cortex-A7. The kernel is compiled with gcc 7.3.1. Next week I'll
test some more.
I have a question about how the decompression is done while reading.
When a large file is read from the filesystem (assuming not in any
cache), is it the case that first a chunk is read from flash, that
chunk is then decompressed, later next chunk is read from flash, that
one is then decompressed and so on, or can the decompression be done
in parallel while reading the next chunk from flash?

/Emil
Richard Weinberger June 8, 2019, 8:46 a.m. UTC | #7
----- Ursprüngliche Mail -----
> ARM Cortex-A7. The kernel is compiled with gcc 7.3.1. Next week I'll
> test some more.

Good to know!

> I have a question about how the decompression is done while reading.
> When a large file is read from the filesystem (assuming not in any
> cache), is it the case that first a chunk is read from flash, that
> chunk is then decompressed, later next chunk is read from flash, that
> one is then decompressed and so on, or can the decompression be done
> in parallel while reading the next chunk from flash?

No, this is a serial operation.

Thanks,
//richard

Patch
diff mbox series

diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index 9da2f135121b..8d84d2ed096d 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -5,8 +5,10 @@  config UBIFS_FS
 	select CRYPTO if UBIFS_FS_ADVANCED_COMPR
 	select CRYPTO if UBIFS_FS_LZO
 	select CRYPTO if UBIFS_FS_ZLIB
+	select CRYPTO if UBIFS_FS_ZSTD
 	select CRYPTO_LZO if UBIFS_FS_LZO
 	select CRYPTO_DEFLATE if UBIFS_FS_ZLIB
+	select CRYPTO_ZSTD if UBIFS_FS_ZSTD
 	select CRYPTO_HASH_INFO
 	select UBIFS_FS_XATTR if FS_ENCRYPTION
 	depends on MTD_UBI
@@ -37,6 +39,14 @@  config UBIFS_FS_ZLIB
 	help
 	  Zlib compresses better than LZO but it is slower. Say 'Y' if unsure.
 
+config UBIFS_FS_ZSTD
+	bool "ZSTD compression support" if UBIFS_FS_ADVANCED_COMPR
+	depends on UBIFS_FS
+	default y
+	help
+	  ZSTD compresses is a big win in speed over Zlib and
+	  in compression ratio over LZO. Say 'Y' if unsure.
+
 config UBIFS_ATIME_SUPPORT
 	bool "Access time support"
 	default n
diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
index 565cb56d7225..89183aeeeb7a 100644
--- a/fs/ubifs/compress.c
+++ b/fs/ubifs/compress.c
@@ -71,6 +71,24 @@  static struct ubifs_compressor zlib_compr = {
 };
 #endif
 
+#ifdef CONFIG_UBIFS_FS_ZSTD
+static DEFINE_MUTEX(zstd_enc_mutex);
+static DEFINE_MUTEX(zstd_dec_mutex);
+
+static struct ubifs_compressor zstd_compr = {
+	.compr_type = UBIFS_COMPR_ZSTD,
+	.comp_mutex = &zstd_enc_mutex,
+	.decomp_mutex = &zstd_dec_mutex,
+	.name = "zstd",
+	.capi_name = "zstd",
+};
+#else
+static struct ubifs_compressor zstd_compr = {
+	.compr_type = UBIFS_COMPR_ZSTD,
+	.name = "zstd",
+};
+#endif
+
 /* All UBIFS compressors */
 struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
 
@@ -228,13 +246,19 @@  int __init ubifs_compressors_init(void)
 	if (err)
 		return err;
 
-	err = compr_init(&zlib_compr);
+	err = compr_init(&zstd_compr);
 	if (err)
 		goto out_lzo;
 
+	err = compr_init(&zlib_compr);
+	if (err)
+		goto out_zstd;
+
 	ubifs_compressors[UBIFS_COMPR_NONE] = &none_compr;
 	return 0;
 
+out_zstd:
+	compr_exit(&zstd_compr);
 out_lzo:
 	compr_exit(&lzo_compr);
 	return err;
@@ -247,4 +271,5 @@  void ubifs_compressors_exit(void)
 {
 	compr_exit(&lzo_compr);
 	compr_exit(&zlib_compr);
+	compr_exit(&zstd_compr);
 }
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 04b8ecfd3470..ea8615261936 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1055,6 +1055,8 @@  static int ubifs_parse_options(struct ubifs_info *c, char *options,
 				c->mount_opts.compr_type = UBIFS_COMPR_LZO;
 			else if (!strcmp(name, "zlib"))
 				c->mount_opts.compr_type = UBIFS_COMPR_ZLIB;
+			else if (!strcmp(name, "zstd"))
+				c->mount_opts.compr_type = UBIFS_COMPR_ZSTD;
 			else {
 				ubifs_err(c, "unknown compressor \"%s\"", name); //FIXME: is c ready?
 				kfree(name);
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index 8b7c1844014f..697b1b89066a 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -348,12 +348,14 @@  enum {
  * UBIFS_COMPR_NONE: no compression
  * UBIFS_COMPR_LZO: LZO compression
  * UBIFS_COMPR_ZLIB: ZLIB compression
+ * UBIFS_COMPR_ZSTD: ZSTD compression
  * UBIFS_COMPR_TYPES_CNT: count of supported compression types
  */
 enum {
 	UBIFS_COMPR_NONE,
 	UBIFS_COMPR_LZO,
 	UBIFS_COMPR_ZLIB,
+	UBIFS_COMPR_ZSTD,
 	UBIFS_COMPR_TYPES_CNT,
 };