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

[Daniel Kiper]

On Fri, Sep 03, 2021 at 09:21:39AM +0800, Michael Chang via Grub-devel wrote:
> 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

Cool! Thanks!

> 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.

Huh! The "btrfs partition" sounds like not resizable MBR gap! I am not
happy with it just at the beginning. Anyway, could you explain your use
case in more details including example commands and why core.img seems
landing in the "btrfs partition". I am especially curious about the
latter because I think the "btrfs partition" and things like that was
designed for something else, e.g., storing grubenv data. And why this
solution is i386 specific?

Daniel