grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] btrfs: disable zstd support for i386-pc


From: Michael Chang
Subject: Re: [PATCH] btrfs: disable zstd support for i386-pc
Date: Thu, 7 Nov 2019 06:59:14 +0000

On Wed, Nov 06, 2019 at 09:08:41PM -0800, Vladimir 'phcoder' Serbinenko wrote:
> On Wed, 6 Nov 2019, 20:55 Michael Chang, <address@hidden> wrote:
> 
> > On Wed, Nov 06, 2019 at 11:15:04AM -0800, Vladimir 'phcoder' Serbinenko
> > wrote:
> > > Please don't do it this way. The right solution is to move it to separate
> > > module and include zstd module when needed.
> >
> > I fully agree to work out a proper solution, but before that I think it
> > is necessary to stop it from spread out going forward, as the running
> > system upgrading to new version could be affected and the consequence is
> > fatal - an unbootable system, qualified to be show stopper imho.
> >
> We don't do a release right now, so I don't think it's justified.

Alright, then I'll keep it as downstream patch as it was found while we
were doing a new release to latest stable grub.

> Also
> increase in size can easily come from a compiler. If an increase in size
> can make system unbootable on upgrade, I'd rather be fixing this than just
> pepper over it.


Here I'd like to share my thoughts to the unbootable issue.

The increased size will casue the new boot image failure to install thus
old/obseleted one still remains in place. We should detect such failure
and abort the grub-install process earlier before new grub modules
copied to preboot directory and avoiding the symbol lookup error as the
consequence of mixture of old and new grub versions.

Thanks,
Michael

> 
> >
> > > Not everybody uses btrfs
> > > embedded area. I recommend not to use it. Using mbr gap or BBP is the
> > > recommended way.
> >
> > The problem here is existing installation would fail to update to new
> > grub version if it is using btrfs bootloader area. For new installation
> > certainly we could recommend them to do so. or btrfs zstd support will
> > be disabled if using a btrfs bootloader area.
> >
> 
> 
> > Thanks,
> > Michael
> >
> > >
> > > On Tue, 5 Nov 2019, 01:25 Michael Chang, <address@hidden> wrote:
> > >
> > > > The zstd support in btrfs has dependenciy to zstd module and core.img
> > > > grows its size significantly to 75KB on my system. The resulted image
> > > > cannot be installed into btrfs bootloader area in the size of 64KB and
> > > > eventually fails with following message.
> > > >
> > > > /usr/sbin/grub-install: warning: your core.img is unusually large.  It
> > > > won't fit in the embedding area.
> > > > /usr/sbin/grub-install: error: filesystem `btrfs' doesn't support
> > > > blocklists.
> > > >
> > > > The patch disabled the zstd support of btrfs in pc-bios platform to
> > > > avoid the regression. The resulting size is 56KB, albeit a bit too
> > close
> > > > to the 64KB but works. This is simple workaround until a proper fix
> > > > landed upstream.
> > > >
> > > > Signed-off-by: Michael Chang <address@hidden>
> > > > ---
> > > >  grub-core/fs/btrfs.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> > > > index 48bd3d04a..8f98892d3 100644
> > > > --- a/grub-core/fs/btrfs.c
> > > > +++ b/grub-core/fs/btrfs.c
> > > > @@ -17,6 +17,7 @@
> > > >   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > > >   */
> > > >
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >  /*
> > > >   * Tell zstd to expose functions that aren't part of the stable API,
> > which
> > > >   * aren't safe to use when linking against a dynamic library. We
> > vendor
> > > > in a
> > > > @@ -24,6 +25,7 @@
> > > >   * functions to provide our own allocator, which uses grub_malloc(),
> > to
> > > > zstd.
> > > >   */
> > > >  #define ZSTD_STATIC_LINKING_ONLY
> > > > +#endif
> > > >
> > > >  #include <grub/err.h>
> > > >  #include <grub/file.h>
> > > > @@ -35,7 +37,9 @@
> > > >  #include <grub/lib/crc.h>
> > > >  #include <grub/deflate.h>
> > > >  #include <minilzo.h>
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >  #include <zstd.h>
> > > > +#endif
> > > >  #include <grub/i18n.h>
> > > >  #include <grub/btrfs.h>
> > > >  #include <grub/crypto.h>
> > > > @@ -56,8 +60,10 @@ GRUB_MOD_LICENSE ("GPLv3+");
> > > >  #define GRUB_BTRFS_LZO_BLOCK_MAX_CSIZE (GRUB_BTRFS_LZO_BLOCK_SIZE + \
> > > >                                      (GRUB_BTRFS_LZO_BLOCK_SIZE / 16)
> > + 64
> > > > + 3)
> > > >
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >  #define ZSTD_BTRFS_MAX_WINDOWLOG 17
> > > >  #define ZSTD_BTRFS_MAX_INPUT     (1 << ZSTD_BTRFS_MAX_WINDOWLOG)
> > > > +#endif
> > > >
> > > >  typedef grub_uint8_t grub_btrfs_checksum_t[0x20];
> > > >  typedef grub_uint16_t grub_btrfs_uuid_t[8];
> > > > @@ -229,7 +235,9 @@ struct grub_btrfs_extent_data
> > > >  #define GRUB_BTRFS_COMPRESSION_NONE 0
> > > >  #define GRUB_BTRFS_COMPRESSION_ZLIB 1
> > > >  #define GRUB_BTRFS_COMPRESSION_LZO  2
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >  #define GRUB_BTRFS_COMPRESSION_ZSTD 3
> > > > +#endif
> > > >
> > > >  #define GRUB_BTRFS_OBJECT_ID_CHUNK 0x100
> > > >
> > > > @@ -1229,6 +1237,7 @@ grub_btrfs_read_inode (struct grub_btrfs_data
> > *data,
> > > >    return grub_btrfs_read_logical (data, elemaddr, inode, sizeof
> > (*inode),
> > > > 0);
> > > >  }
> > > >
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >  static void *grub_zstd_malloc (void *state __attribute__((unused)),
> > > > size_t size)
> > > >  {
> > > >    return grub_malloc (size);
> > > > @@ -1318,6 +1327,7 @@ err:
> > > >
> > > >    return ret;
> > > >  }
> > > > +#endif
> > > >
> > > >  static grub_ssize_t
> > > >  grub_btrfs_lzo_decompress(char *ibuf, grub_size_t isize, grub_off_t
> > off,
> > > > @@ -1494,8 +1504,12 @@ grub_btrfs_extent_read (struct grub_btrfs_data
> > > > *data,
> > > >
> > > >        if (data->extent->compression != GRUB_BTRFS_COMPRESSION_NONE
> > > >           && data->extent->compression != GRUB_BTRFS_COMPRESSION_ZLIB
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >           && data->extent->compression != GRUB_BTRFS_COMPRESSION_LZO
> > > >           && data->extent->compression != GRUB_BTRFS_COMPRESSION_ZSTD)
> > > > +#else
> > > > +         && data->extent->compression != GRUB_BTRFS_COMPRESSION_LZO)
> > > > +#endif
> > > >         {
> > > >           grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > > >                       "compression type 0x%x not supported",
> > > > @@ -1535,6 +1549,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data
> > *data,
> > > >                   != (grub_ssize_t) csize)
> > > >                 return -1;
> > > >             }
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >           else if (data->extent->compression ==
> > > > GRUB_BTRFS_COMPRESSION_ZSTD)
> > > >             {
> > > >               if (grub_btrfs_zstd_decompress (data->extent->inl,
> > > > data->extsize -
> > > > @@ -1544,6 +1559,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data
> > *data,
> > > >                   != (grub_ssize_t) csize)
> > > >                 return -1;
> > > >             }
> > > > +#endif
> > > >           else
> > > >             grub_memcpy (buf, data->extent->inl + extoff, csize);
> > > >           break;
> > > > @@ -1581,10 +1597,12 @@ grub_btrfs_extent_read (struct grub_btrfs_data
> > > > *data,
> > > >                 ret = grub_btrfs_lzo_decompress (tmp, zsize, extoff
> > > >                                     + grub_le_to_cpu64
> > > > (data->extent->offset),
> > > >                                     buf, csize);
> > > > +#ifndef GRUB_MACHINE_PCBIOS
> > > >               else if (data->extent->compression ==
> > > > GRUB_BTRFS_COMPRESSION_ZSTD)
> > > >                 ret = grub_btrfs_zstd_decompress (tmp, zsize, extoff
> > > >                                     + grub_le_to_cpu64
> > > > (data->extent->offset),
> > > >                                     buf, csize);
> > > > +#endif
> > > >               else
> > > >                 ret = -1;
> > > >
> > > > --
> > > > 2.16.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Grub-devel mailing list
> > > > address@hidden
> > > > https://lists.gnu.org/mailman/listinfo/grub-devel
> > > >
> >
> > > _______________________________________________
> > > Grub-devel mailing list
> > > address@hidden
> > > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]