mbox series

[0/5] pstore: Use zstd directly by default for compression

Message ID 20221018020519.never.337-kees@kernel.org
Headers
Series pstore: Use zstd directly by default for compression |

Message

Kees Cook Oct. 18, 2022, 2:08 a.m. UTC 
  Hi,

Okay, here is a very lightly tested version of using zstd directly, which
should save 128KB per CPU compare to using crypto API. This leaves the
crypto API available, though, if someone wants to use it instead. Even
supporting both, this is a net reduction in code, due to dropping all
the zbufsize logic.

How does this look?

-Kees

Kees Cook (5):
  pstore: Remove worse-case compression size logic
  pstore: Allow for arbitrary compression algorithm
  pstore: Use size_t for compress/decompression type widths
  pstore: Refactor compression initialization
  pstore: Use zstd directly by default for compression

 fs/pstore/Kconfig    | 131 +++++-----------
 fs/pstore/platform.c | 358 ++++++++++++++++++++-----------------------
 2 files changed, 209 insertions(+), 280 deletions(-)

Comments

Ard Biesheuvel Oct. 18, 2022, 8:20 a.m. UTC | #1 
On Tue, 18 Oct 2022 at 04:08, Kees Cook <keescook@chromium.org> wrote:
>
> Hi,
>
> Okay, here is a very lightly tested version of using zstd directly, which
> should save 128KB per CPU compare to using crypto API. This leaves the
> crypto API available, though, if someone wants to use it instead. Even
> supporting both, this is a net reduction in code, due to dropping all
> the zbufsize logic.
>
> How does this look?
>

The code changes all look correct and reasonable to me.

As for supporting both the library and the crypto API interface: I did
a little digging, and it turns out all additional compression modes
were added by the same contributor, with no justification other than
'this might be useful to some people' (see below)

However, I did a quick experiment with the output of dmesg on my
workstation, and these are the results I am getting

Input file:
-rw-r--r-- 1 ard ard 111792 Oct 18 09:23 uncompressed

Default compression
-rw-r--r-- 1 ard ard  21810 Oct 18 09:23 uncompressed.gz
-rw-r--r-- 1 ard ard  33923 Oct 18 09:23 uncompressed.lz4
-rw-r--r-- 1 ard ard  34238 Oct 18 09:23 uncompressed.lzo
-rw-r--r-- 1 ard ard  21599 Oct 18 09:23 uncompressed.zst

Max compression (-9)
-rw-r--r-- 1 ard ard  21589 Oct 18 09:23 uncompressed.gz
-rw-r--r-- 1 ard ard  28848 Oct 18 09:25 uncompressed.lz4 (== lz4hc?)
-rw-r--r-- 1 ard ard  26917 Oct 18 09:23 uncompressed.lzo
-rw-r--r-- 1 ard ard  19883 Oct 18 09:23 uncompressed.zst

So the patches in question don't actually fulfill their claim of
improving the compression ratio. Only zstd, which was added later,
improves upon zlib, but not significantly unless you override the
compression level (which we don't).

So I seriously doubt that those patches were inspired by the need to
solve an actual problem anyone was experiencing at the time, given
that they don't. It just seems that nobody bothered to ask the 'why?'
question.

So again, I suggest to simply drop this non-feature, and standardize
on either zlib or zstd using the library interface exclusively. If
someone present a compelling use case, we can always consider adding
it back in some form.

As for the choice of algorithm, given the equal performance using the
default compression level, and the difference in code size, I don't
see why zstd should be preferred here. If anything, it only increases
the likelihood of hitting another error if we are panicking due to
some memory corruption issue.

$ size  lib/zstd/zstd_compress.ko lib/zlib_deflate/zlib_deflate.ko
   text    data     bss     dec     hex filename
 218411     768       0 219179   3582b lib/zstd/zstd_compress.ko
  16231     876    2288   19395    4bc3 lib/zlib_deflate/zlib_deflate.ko






commit 8cfc8ddc99df9509a46043b14af81f5c6a223eab
Author:     Geliang Tang <geliangtang@163.com>
AuthorDate: Thu Feb 18 22:04:22 2016 +0800
Commit:     Kees Cook <keescook@chromium.org>
CommitDate: Thu Jun 2 10:59:31 2016 -0700

    pstore: add lzo/lz4 compression support

    Like zlib compression in pstore, this patch added lzo and lz4
    compression support so that users can have more options and better
    compression ratio.

commit 239b716199d9aff0d09444b0086e23aacd6bd445
Author:     Geliang Tang <geliangtang@gmail.com>
AuthorDate: Tue Feb 13 14:40:39 2018 +0800
Commit:     Kees Cook <keescook@chromium.org>
CommitDate: Tue Mar 6 15:06:11 2018 -0800

    pstore: Add lz4hc and 842 compression support


>
> Kees Cook (5):
>   pstore: Remove worse-case compression size logic
>   pstore: Allow for arbitrary compression algorithm
>   pstore: Use size_t for compress/decompression type widths
>   pstore: Refactor compression initialization
>   pstore: Use zstd directly by default for compression
>
>  fs/pstore/Kconfig    | 131 +++++-----------
>  fs/pstore/platform.c | 358 ++++++++++++++++++++-----------------------
>  2 files changed, 209 insertions(+), 280 deletions(-)
>
> --
> 2.34.1
>
Guilherme G. Piccoli Oct. 18, 2022, 2:10 p.m. UTC | #2 
On 18/10/2022 05:20, Ard Biesheuvel wrote:
> [...]
> So again, I suggest to simply drop this non-feature, and standardize
> on either zlib or zstd using the library interface exclusively. If
> someone present a compelling use case, we can always consider adding
> it back in some form.
> 
> As for the choice of algorithm, given the equal performance using the
> default compression level, and the difference in code size, I don't
> see why zstd should be preferred here. If anything, it only increases
> the likelihood of hitting another error if we are panicking due to
> some memory corruption issue.

I think it's a good argument - would zlib be simpler in code than zstd?
I've checked the zstd patch from Kees - not complex per se, but would be
great if we could have a simple mechanism, without the need of the ifdef
there for example...

Cheers,


Guilherme
Ard Biesheuvel Oct. 18, 2022, 2:38 p.m. UTC | #3 
On Tue, 18 Oct 2022 at 16:11, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 18/10/2022 05:20, Ard Biesheuvel wrote:
> > [...]
> > So again, I suggest to simply drop this non-feature, and standardize
> > on either zlib or zstd using the library interface exclusively. If
> > someone present a compelling use case, we can always consider adding
> > it back in some form.
> >
> > As for the choice of algorithm, given the equal performance using the
> > default compression level, and the difference in code size, I don't
> > see why zstd should be preferred here. If anything, it only increases
> > the likelihood of hitting another error if we are panicking due to
> > some memory corruption issue.
>
> I think it's a good argument - would zlib be simpler in code than zstd?

I think it should be rather straight-forward. Note that this is what
we had before 2016 when all the 'features' were starting to get added.

> I've checked the zstd patch from Kees - not complex per se, but would be
> great if we could have a simple mechanism, without the need of the ifdef
> there for example...
>
> Cheers,
>
>
> Guilherme