Re: [PATCH] i386-pc: build btrfs zstd support into separate module

[Michael Chang <mchang@suse.com>]

On Wed, Sep 01, 2021 at 06:38:22PM +0200, Daniel Kiper wrote:
> On Tue, Aug 31, 2021 at 03:12:28PM +0800, Michael Chang via Grub-devel wrote:
> > The zstd support in btrfs brings significant size increment to the
> > on-disk image that it can no longer fit into btrfs bootloader area and
> > short mbr gap.
> >
> > In order to support grub update on outstanding i386-pc setup with these
> > size constraints remain in place, here we build the zstd suppprt of
> > btrfs into a separate module, named btrfs_zstd, to alleviate the size
> > change. Please note this only makes it's way to i386-pc, other
> > architecture is not affected.
> 
> I am OK with extracting zstd code from btrfs code. However, I want that be
> done for all architectures and platforms. No exceptions.

May I ask for more background about this decision ?

Given that btrfs compression is per file property and can be
recompressed on the fly, there is no way to detect it beforehand thus
the safest bet is to assume that it is always needed. In other words if
the platform has no known limitation on the size of image, the zstd code
should be indivisible to btrfs.mo.

> 
> [...]
> 
> > diff --git a/grub-core/fs/btrfs_zstd.c b/grub-core/fs/btrfs_zstd.c
> > new file mode 100644
> > index 000000000..d5d1e013c
> > --- /dev/null
> > +++ b/grub-core/fs/btrfs_zstd.c
> > @@ -0,0 +1,36 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2008  Free Software Foundation, Inc.
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <grub/types.h>
> > +#include <grub/dl.h>
> > +/* For NULL.  */
> > +#include <grub/mm.h>
> > +#include <grub/btrfs.h>
> > +#include <grub/lib/zstd.h>
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +
> > +GRUB_MOD_INIT (btrfs_zstd)
> > +{
> > +  grub_btrfs_zstd_decompress_func = grub_zstd_decompress;
> 
> Please create a function in btrfs module which will do this for you.

Sure. np.

> 
> > +}
> > +
> > +GRUB_MOD_FINI (btrfs_zstd)
> > +{
> > +  grub_btrfs_zstd_decompress_func = NULL;
> 
> Ditto.

OK.

> 
> Last but not least, please CC original author of zstd support next time.

OK. I will CC Nick Terrell next time.

Thank you very much for the review.

Regards,
Michael

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel