[PATCH 0/3] ubifs: add lz4hc compression

[PATCH 0/3] ubifs: add lz4hc compression

[Hyunchul Lee]

From: Hyunchul Lee <cheol.lee@lge.com>

This patch set is for adding support for lz4hc compression.  lz4hc's
compression ratio is comparable to zlib, but its compression/decompression
speed is faster than zlib[1].

Another compression type cannot be added any more in respect that type of
ubifs_inode's compr_type is "unsigned:2" and there are 4 compression
types.  There may be overflow in assignment to compr_type, and prevision 
version of ubifs is trying to decompress file with wrong compression type.

[1] https://github.com/lz4/lz4

Brent Taylor (1):
  ubifs: add lz4hc compressor

Hyunchul Lee (2):
  ubifs: change type of ubifs_inode's compr_type to unsigend:16
  ubifs: add "compr=lz4hc" into ubifs.txt

 Documentation/filesystems/ubifs.txt |  1 +
 fs/ubifs/Kconfig                    | 10 ++++++++++
 fs/ubifs/compress.c                 | 25 ++++++++++++++++++++++++-
 fs/ubifs/super.c                    |  2 ++
 fs/ubifs/ubifs-media.h              |  2 ++
 fs/ubifs/ubifs.h                    |  2 +-
 6 files changed, 40 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/3] ubifs: add lz4hc compressor

[Hyunchul Lee]

From: Brent Taylor <motobud@gmail.com>

This patch adds lz4hc compressor into ubifs. with "compr=lz4hc" mount
option, default compression type is changed to "lz4hc"

This was submited by Brent Taylor[1] and not merged.

[1] http://lists.infradead.org/pipermail/linux-mtd/2013-October/048993.html

Signed-off-by: Brent Taylor <motobud@gmail.com>
Signed-off-by: Hyunchul Lee <cheol.lee@lge.com>
---
 fs/ubifs/Kconfig       | 10 ++++++++++
 fs/ubifs/compress.c    | 25 ++++++++++++++++++++++++-
 fs/ubifs/super.c       |  2 ++
 fs/ubifs/ubifs-media.h |  2 ++
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index b0d0623..e038123 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -29,6 +29,16 @@ config UBIFS_FS_LZO
 	   LZO compressor is generally faster than zlib but compresses worse.
 	   Say 'Y' if unsure.
 
+config UBIFS_FS_LZ4HC
+       bool "LZ4HC compression support" if UBIFS_FS_ADVANCED_COMPR
+       depends on UBIFS_FS && CRYPTO_LZ4HC
+       default y
+       help
+          LZ4HC generally beats LZO on decompression speed while provides a lot
+	  better compression ratio (comparable to zlib). Compression speed is
+	  generally slower compared to LZO, but faster compared to zlib.
+	  Say 'Y' if unsure.
+
 config UBIFS_FS_ZLIB
 	bool "ZLIB compression support" if UBIFS_FS_ADVANCED_COMPR
 	depends on UBIFS_FS
diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
index 565cb56..00f62f8 100644
--- a/fs/ubifs/compress.c
+++ b/fs/ubifs/compress.c
@@ -71,6 +71,22 @@
 };
 #endif
 
+#ifdef CONFIG_UBIFS_FS_LZ4HC
+static DEFINE_MUTEX(lz4hc_mutex);
+
+static struct ubifs_compressor lz4hc_compr = {
+	.compr_type = UBIFS_COMPR_LZ4HC,
+	.comp_mutex = &lz4hc_mutex,
+	.name = "lz4hc",
+	.capi_name = "lz4hc",
+};
+#else
+static struct ubifs_compressor lz4hc_compr = {
+	.compr_type = UBIFS_COMPR_LZ4HC,
+	.name = "lz4hc",
+};
+#endif
+
 /* All UBIFS compressors */
 struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
 
@@ -224,10 +240,14 @@ int __init ubifs_compressors_init(void)
 {
 	int err;
 
-	err = compr_init(&lzo_compr);
+	err = compr_init(&lz4hc_compr);
 	if (err)
 		return err;
 
+	err = compr_init(&lzo_compr);
+	if (err)
+		goto out_lz4hc;
+
 	err = compr_init(&zlib_compr);
 	if (err)
 		goto out_lzo;
@@ -237,6 +257,8 @@ int __init ubifs_compressors_init(void)
 
 out_lzo:
 	compr_exit(&lzo_compr);
+out_lz4hc:
+	compr_exit(&lz4hc_compr);
 	return err;
 }
 
@@ -245,6 +267,7 @@ int __init ubifs_compressors_init(void)
  */
 void ubifs_compressors_exit(void)
 {
+	compr_exit(&lz4hc_compr);
 	compr_exit(&lzo_compr);
 	compr_exit(&zlib_compr);
 }
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index e08aa04..6615985 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1028,6 +1028,8 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
 				return -ENOMEM;
 			if (!strcmp(name, "none"))
 				c->mount_opts.compr_type = UBIFS_COMPR_NONE;
+			else if (!strcmp(name, "lz4hc"))
+				c->mount_opts.compr_type = UBIFS_COMPR_LZ4HC;
 			else if (!strcmp(name, "lzo"))
 				c->mount_opts.compr_type = UBIFS_COMPR_LZO;
 			else if (!strcmp(name, "zlib"))
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index e8c23c9..2f2fe45 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -341,12 +341,14 @@ enum {
  * UBIFS_COMPR_NONE: no compression
  * UBIFS_COMPR_LZO: LZO compression
  * UBIFS_COMPR_ZLIB: ZLIB compression
+ * UBIFS_COMPR_LZ4HC: LZ4HC compression
  * UBIFS_COMPR_TYPES_CNT: count of supported compression types
  */
 enum {
 	UBIFS_COMPR_NONE,
 	UBIFS_COMPR_LZO,
 	UBIFS_COMPR_ZLIB,
+	UBIFS_COMPR_LZ4HC,
 	UBIFS_COMPR_TYPES_CNT,
 };
 
-- 
1.9.1

[PATCH 2/3] ubifs: change type of ubifs_inode's compr_type to unsigend:16

[Hyunchul Lee]

From: Hyunchul Lee <cheol.lee@lge.com>

if new compression is added, UBIFS_COMPR_TYPES_CNT becomes greater than 3.
so type "unsigned:2" of compr_type should be changed.

Signed-off-by: Hyunchul Lee <cheol.lee@lge.com>
---
 fs/ubifs/ubifs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index ca72382..2a1e857 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -394,7 +394,7 @@ struct ubifs_inode {
 	unsigned int dirty:1;
 	unsigned int xattr:1;
 	unsigned int bulk_read:1;
-	unsigned int compr_type:2;
+	unsigned int compr_type:16;
 	struct mutex ui_mutex;
 	spinlock_t ui_lock;
 	loff_t synced_i_size;
-- 
1.9.1

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Artem Bityutskiy]

On Tue, 2017-02-28 at 14:16 +0900, Hyunchul Lee wrote:
> Another compression type cannot be added any more in respect that
> type of
> ubifs_inode's compr_type is "unsigned:2" and there are 4 compression
> types.  There may be overflow in assignment to compr_type, and
> prevision 
> version of ubifs is trying to decompress file with wrong compression
> type.

But this is just an in-memory inode structure, and you can change it,
just increase the size of 'compr_type'.

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Hyunchul Lee]

On Tue, Feb 28, 2017 at 12:17:05PM +0200, Artem Bityutskiy wrote:
> On Tue, 2017-02-28 at 14:16 +0900, Hyunchul Lee wrote:
> > Another compression type cannot be added any more in respect that
> > type of
> > ubifs_inode's compr_type is "unsigned:2" and there are 4 compression
> > types.  There may be overflow in assignment to compr_type, and
> > prevision 
> > version of ubifs is trying to decompress file with wrong compression
> > type.
> 
> But this is just an in-memory inode structure, and you can change it,
> just increase the size of 'compr_type'.

yes, it is an in-memory inode. but i think that there can be problem of 
backward compatibility. for example, if ubifs_ino_node's compr_type is
4 in flash, the previous version of ubifs could misinterpret it as 0,
because ubifs_inode's compr_type(2-bit) is assigned
ubifs_ino_nodes' compr_type at ubifs_iget.

thank you for comment.
Hyunchul

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Artem Bityutskiy]

On Wed, 2017-03-01 at 10:12 +0900, Hyunchul Lee wrote:
> yes, it is an in-memory inode. but i think that there can be problem
> of 
> backward compatibility. for example, if ubifs_ino_node's compr_type
> is
> 4 in flash, the previous version of ubifs could misinterpret it as 0,
> because ubifs_inode's compr_type(2-bit) is assigned
> ubifs_ino_nodes' compr_type at ubifs_iget.

Good point. I guess changing the in-memory compr_type to something
wider right away would be a good idea, even if no new compressors are
added.

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Richard Weinberger]

Hyunchul Lee,

Am 28.02.2017 um 06:16 schrieb Hyunchul Lee:
> From: Hyunchul Lee <cheol.lee@lge.com>
> 
> This patch set is for adding support for lz4hc compression.  lz4hc's
> compression ratio is comparable to zlib, but its compression/decompression
> speed is faster than zlib[1].

Do you have performance numbers that show the speedup on a common embedded
system with NAND?

> Another compression type cannot be added any more in respect that type of
> ubifs_inode's compr_type is "unsigned:2" and there are 4 compression
> types.  There may be overflow in assignment to compr_type, and prevision 
> version of ubifs is trying to decompress file with wrong compression type.

As Artem noted this is only an in-memory struct.

What happens when you compress a file with lz4hc and try to access this
UBIFS file with an older kernel?
AFAICT ubifs_iget() will notice and return -EINVAL but please double check.

Thanks,
//richard

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Hyunchul Lee]

Hi, Ricard

On 03/03/2017 05:08 PM, Richard Weinberger wrote:
> Hyunchul Lee,
> 
> Am 28.02.2017 um 06:16 schrieb Hyunchul Lee:
>> From: Hyunchul Lee <cheol.lee@lge.com>
>>
>> This patch set is for adding support for lz4hc compression.  lz4hc's
>> compression ratio is comparable to zlib, but its compression/decompression
>> speed is faster than zlib[1].
> 
> Do you have performance numbers that show the speedup on a common embedded
> system with NAND?

I am trying to build environment for a common embedded system with NAND. 
if I am done, I will report the performance result. 

> 
>> Another compression type cannot be added any more in respect that type of
>> ubifs_inode's compr_type is "unsigned:2" and there are 4 compression
>> types.  There may be overflow in assignment to compr_type, and prevision 
>> version of ubifs is trying to decompress file with wrong compression type.
> 
> As Artem noted this is only an in-memory struct.
> 
> What happens when you compress a file with lz4hc and try to access this
> UBIFS file with an older kernel?
> AFAICT ubifs_iget() will notice and return -EINVAL but please double check.
> 

yes, ubifs detects this. but I mean that if another compression type except lz4hc
is added, ubifs cannot. because ubifs_iget() assigns ubifs_ino_node's compr_type(16-bit) 
to ubifs_inode's compr_type(2-bit).
I guess that the disk format version should be incremented.

> Thanks,
> //richard
> 

Thanks,
Hyunchul

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Artem Bityutskiy]

On Fri, 2017-03-03 at 20:07 +0900, Hyunchul Lee wrote:
> yes, ubifs detects this. but I mean that if another compression type
> except lz4hc
> is added, ubifs cannot. because ubifs_iget() assigns ubifs_ino_node's
> compr_type(16-bit) 
> to ubifs_inode's compr_type(2-bit).
> I guess that the disk format version should be incremented.

Let's say that some day in the future you add the new XYZ compression
support, and its type is 5. Let's say the new compressor is supported
in kernel version 5.0.

Now I get your device which is running kernel 5.0 and uses XYZ
compression. I downgrade the kernel to version 4.0 on this device, v4.0
does not support XYZ. What happens?

Well, not a very good thing. UBIFS will read the inode, truncate
compr_type in ubifs_iget():

        ui->compr_type  = le16_to_cpu(ino->compr_type);

and happily continue, and later on failing to decompress.

Is this ideal? No. But we can lessen the future problem by making
compr_type to be full u16 already today.

Should we change on-flash format? I think no, because UBIFS will still
fail when it tries to decompress.

Artem.

Re: [PATCH 0/3] ubifs: add lz4hc compression

[Richard Weinberger]

Artem,

Am 03.03.2017 um 13:00 schrieb Artem Bityutskiy:
> On Fri, 2017-03-03 at 20:07 +0900, Hyunchul Lee wrote:
>> yes, ubifs detects this. but I mean that if another compression type
>> except lz4hc
>> is added, ubifs cannot. because ubifs_iget() assigns ubifs_ino_node's
>> compr_type(16-bit) 
>> to ubifs_inode's compr_type(2-bit).
>> I guess that the disk format version should be incremented.
> 
> Let's say that some day in the future you add the new XYZ compression
> support, and its type is 5. Let's say the new compressor is supported
> in kernel version 5.0.
> 
> Now I get your device which is running kernel 5.0 and uses XYZ
> compression. I downgrade the kernel to version 4.0 on this device, v4.0
> does not support XYZ. What happens?
> 
> Well, not a very good thing. UBIFS will read the inode, truncate
> compr_type in ubifs_iget():
> 
>         ui->compr_type  = le16_to_cpu(ino->compr_type);
> 
> and happily continue, and later on failing to decompress.
> 
> Is this ideal? No. But we can lessen the future problem by making
> compr_type to be full u16 already today.
> 
> Should we change on-flash format? I think no, because UBIFS will still
> fail when it tries to decompress.

We don't need to change the on-flash format.
I'd vote to add a new super block flag.

See: commit fc4b891bbefa73b496bb44b076bb5274b6bfba68
Author: Richard Weinberger <richard@nod.at>
Date:   Wed Oct 19 23:46:39 2016 +0200

    ubifs: Raise write version to 5

    Starting with version 5 the following properties change:
     - UBIFS_FLG_DOUBLE_HASH is mandatory
     - UBIFS_FLG_ENCRYPTION is optional but depdens on UBIFS_FLG_DOUBLE_HASH
     - Filesystems with unknown super block flags will be rejected, this
       allows us in future to add new features without raising the UBIFS
       write version.

    Signed-off-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard