Re: [PATCH v3 2/2] btrfs: Add zstd support to grub btrfs

[Nick Terrell]

> On Oct 11, 2018, at 10:55 AM, Daniel Kiper <address@hidden> wrote:
> 
> On Tue, Oct 09, 2018 at 04:21:37PM -0700, Nick Terrell wrote:
>> Adds zstd support to the btrfs module.
>> 
>> Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
>> compression. A test case was also added to the test suite that fails before
>> the patch, and passes after.
>> 
>> Signed-off-by: Nick Terrell <address@hidden>
>> ---
>> v1 -> v2:
>> - Fix comments from Daniel Kiper.
>> 
>> v2 -> v3:
>> - Use grub_error() to set grub_errno in grub_btrfs_zstd_decompress().
>> - Fix style and formatting comments.
>> 
>> Makefile.util.def            |  10 +++-
>> grub-core/Makefile.core.def  |  10 +++-
>> grub-core/fs/btrfs.c         | 112 ++++++++++++++++++++++++++++++++++-
>> tests/btrfs_test.in          |   1 +
>> tests/util/grub-fs-tester.in |   2 +-
>> 5 files changed, 131 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Makefile.util.def b/Makefile.util.def
>> index f9caccb97..27c5e9903 100644
>> --- a/Makefile.util.def
>> +++ b/Makefile.util.def
>> @@ -54,7 +54,7 @@ library = {
>> library = {
>>   name = libgrubmods.a;
>>   cflags = '-fno-builtin -Wno-undef';
>> -  cppflags = '-I$(top_srcdir)/grub-core/lib/minilzo 
>> -I$(srcdir)/grub-core/lib/xzembed -DMINILZO_HAVE_CONFIG_H';
>> +  cppflags = '-I$(srcdir)/grub-core/lib/minilzo 
>> -I$(srcdir)/grub-core/lib/xzembed -I$(srcdir)/grub-core/lib/zstd 
>> -DMINILZO_HAVE_CONFIG_H';
>> 
>>   common_nodist = grub_script.tab.c;
>>   common_nodist = grub_script.yy.c;
>> @@ -164,6 +164,14 @@ library = {
>>   common = grub-core/lib/xzembed/xz_dec_bcj.c;
>>   common = grub-core/lib/xzembed/xz_dec_lzma2.c;
>>   common = grub-core/lib/xzembed/xz_dec_stream.c;
>> +  common = grub-core/lib/zstd/debug.c;
>> +  common = grub-core/lib/zstd/entropy_common.c;
>> +  common = grub-core/lib/zstd/error_private.c;
>> +  common = grub-core/lib/zstd/fse_decompress.c;
>> +  common = grub-core/lib/zstd/huf_decompress.c;
>> +  common = grub-core/lib/zstd/xxhash.c;
>> +  common = grub-core/lib/zstd/zstd_common.c;
>> +  common = grub-core/lib/zstd/zstd_decompress.c;
>> };
>> 
>> program = {
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 2c1d62cee..f818f58bc 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1265,8 +1265,16 @@ module = {
>>   name = btrfs;
>>   common = fs/btrfs.c;
>>   common = lib/crc.c;
>> +  common = lib/zstd/debug.c;
>> +  common = lib/zstd/entropy_common.c;
>> +  common = lib/zstd/error_private.c;
>> +  common = lib/zstd/fse_decompress.c;
>> +  common = lib/zstd/huf_decompress.c;
>> +  common = lib/zstd/xxhash.c;
>> +  common = lib/zstd/zstd_common.c;
>> +  common = lib/zstd/zstd_decompress.c;
>>   cflags = '$(CFLAGS_POSIX) -Wno-undef';
>> -  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo 
>> -DMINILZO_HAVE_CONFIG_H';
>> +  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo 
>> -I$(srcdir)/lib/zstd -DMINILZO_HAVE_CONFIG_H';
>> };
> 
> Please do not embed zstd lib into btrfs module here.
> I would like to see zstd lib as separate module if possible.

Sure, I'm not exactly sure how the build system works. I haven't been able to
find any documentation, if there is some can you point me to it? I think the 
zstd
module should look like:

module = {
  name = zstd;
  common = lib/zstd/debug.c;
  common = lib/zstd/entropy_common.c;
  common = lib/zstd/error_private.c;
  common = lib/zstd/fse_decompress.c;
  common = lib/zstd/huf_decompress.c;
  common = lib/zstd/xxhash.c;
  common = lib/zstd/zstd_common.c;
  common = lib/zstd/zstd_decompress.c;
  cflags = '$(CFLAGS_POSIX) -Wno-undef';
  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/zstd';
};

Then how do I add a dependency on it in btrfs?


>> +static grub_ssize_t
>> +grub_btrfs_zstd_decompress (char *ibuf, grub_size_t isize, grub_off_t off,
>> +                        char *obuf, grub_size_t osize)
>> +{
>> +    void *allocated = NULL;
>> +    char *otmpbuf = obuf;
>> +    grub_size_t otmpsize = osize;
>> +    ZSTD_DCtx *dctx = NULL;
>> +    grub_size_t zstd_ret;
>> +    grub_ssize_t ret = -1;
>> +
>> +    /*
>> +     * Zstd will fail if it can't fit the entire output in the destination
>> +     * buffer, so if osize isn't large enough, allocate a temporary buffer.
>> +     */
>> +    if (otmpsize < ZSTD_BTRFS_MAX_INPUT) {
>> +            allocated = grub_malloc (ZSTD_BTRFS_MAX_INPUT);
>> +            if (!allocated) {
> 
> Hmmm... Should not we say something here? Like grub_error() call below?
> It seems to me that failing silently is bad idea here.

grub_malloc() already calls grub_error with the message "Out of memory".

Thanks,
Nick