| Message ID | 20221018020519.never.337-kees@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | pstore: Use zstd directly by default for compression | expand |
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 >
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
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