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

[Michael Chang]

On Thu, Sep 02, 2021 at 02:12:52PM +0200, Daniel Kiper wrote:
> On Thu, Sep 02, 2021 at 01:48:30PM +0800, Michael Chang via Grub-devel wrote:
> > 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 ?
> 
> Yes, I want the same code and behavior for all architectures and
> platforms if possible.
> 
> > 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.
> 
> Wait, you said this in the commit message too:
> 
>   Therefore if the system has enough space of embedding area for grub then
>   zstd support for btrfs will be enabled automatically in the process of
>   running grub-install through inserting btrfs_zstd module to the on-disk
>   image, otherwise a warning will be logged on screen to indicate user
>   that zstd support for btrfs is disabled due to the size limit.
> 
> So, I thought this may work in the same way in all cases. If this is not
> true you have to fix this. Otherwise I cannot take this patch.

I literally don't like to propagate this (somewhat awkward) fix to other
architectures that do not have sensible requirement to it, though it
looks possible to do so.

If this doesn't meet the expection then I'm sorry.

> 
> TBH, I should reject this patch immediately because this is "small MBR
> gap stuff". And as I said a few months ago, I want to stop to pay
> attention to "small MBR gap stuff" in upstream. Sorry for being blunt
> but it is full can of worms, i.e., each patch like that one adds more
> cruft which we have to take care because a few folks in the wild could
> not spend a few hours to repartition their systems. I think distros
> should start putting more effort in educating their users WRT to small
> MBR gap and problems related to it instead of pushing again and again
> upstream patches which make the GRUB code more complicated.

Well, I think the short mbr gap issue is all well received been
discussed several times. The reason I'm poking this beehive again is
just that I can't resist to fix problem from our users who opted to use
"btrfs partition" which differs to "short mbr gap" with a lot more user
base and sensible usecases.

FWIW we want to address this problem, because mbr gap is adjustable via
re-partitioning but btrfs bootloader area is not.

Thanks,
Michael

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