* [PATCH] btrfs: disable zstd support for i386-pc @ 2019-11-05 9:19 Michael Chang 2019-11-05 10:52 ` Paul Menzel ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Michael Chang @ 2019-11-05 9:19 UTC (permalink / raw) To: grub-devel 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 <mchang@suse.com> --- 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-05 9:19 [PATCH] btrfs: disable zstd support for i386-pc Michael Chang @ 2019-11-05 10:52 ` Paul Menzel 2019-11-07 4:22 ` Michael Chang 2019-11-06 12:40 ` David Sterba 2019-11-06 19:15 ` Vladimir 'phcoder' Serbinenko 2 siblings, 1 reply; 24+ messages in thread From: Paul Menzel @ 2019-11-05 10:52 UTC (permalink / raw) To: Michael Chang; +Cc: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 5220 bytes --] Dear Michael, Thank you for your patch. On 2019-11-05 10:19, Michael Chang wrote: > The zstd support in btrfs has dependenciy to zstd module and core.img dependency > 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 disables (present tense) Maybe: So, disable the zstd support of btrfs for the 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 is *a* simple > landed upstream. lands Is there a feature request/bug report for this? Maybe reference it heare? > Signed-off-by: Michael Chang <mchang@suse.com> > --- > 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 Why not just guard the ZSTD line by switching the order of LZO and ZSTD for the closing bracket? > { > 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; Does some documentation need to be updated? Kind regards, Paul [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 5174 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-05 10:52 ` Paul Menzel @ 2019-11-07 4:22 ` Michael Chang 0 siblings, 0 replies; 24+ messages in thread From: Michael Chang @ 2019-11-07 4:22 UTC (permalink / raw) To: Paul Menzel; +Cc: The development of GNU GRUB On Tue, Nov 05, 2019 at 11:52:44AM +0100, Paul Menzel wrote: > Dear Michael, > > > Thank you for your patch. > > On 2019-11-05 10:19, Michael Chang wrote: > > The zstd support in btrfs has dependenciy to zstd module and core.img > > dependency OK. The typo will be corrected next version. > > > 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 > > disables (present tense) OK. It will be corrected next version. > > Maybe: > > So, disable the zstd support of btrfs for the 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 > > is *a* simple OK. Next version will correct this. > > > landed upstream. > > lands OK. > > Is there a feature request/bug report for this? Maybe reference it heare? Yes, it is reported on openSUSE bugzilla. http://bugzilla.opensuse.org/show_bug.cgi?id=1154809 I will add the bz reference next version. > > > Signed-off-by: Michael Chang <mchang@suse.com> > > --- > > 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 > > Why not just guard the ZSTD line by switching the order of LZO and ZSTD for > the closing bracket? Yes. Indeed it is more readable and also makes my vim editor's syntax highlighting happy. I will do it in next version. > > > { > > 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; > > Does some documentation need to be updated? I didn't know which document is appropriate for this, if there's any I'd like to add. Please let me know if you have any idea about this. Thanks for your review. Regards, Michael > > > Kind regards, > > Paul > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-05 9:19 [PATCH] btrfs: disable zstd support for i386-pc Michael Chang 2019-11-05 10:52 ` Paul Menzel @ 2019-11-06 12:40 ` David Sterba 2019-11-06 18:53 ` Nick Terrell 2019-11-07 6:34 ` Michael Chang 2019-11-06 19:15 ` Vladimir 'phcoder' Serbinenko 2 siblings, 2 replies; 24+ messages in thread From: David Sterba @ 2019-11-06 12:40 UTC (permalink / raw) To: The development of GNU GRUB On Tue, Nov 05, 2019 at 09:19:59AM +0000, Michael Chang 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. So combination zstd+btrfs+i386-pc never worked? Removing support for zstd could lead to unbootable system, but if that has never worked before it'd be ok to make the build conditional. Looking at zstd code, does not seem to be easy to squeeze the asm to something like 64-56=8K. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-06 12:40 ` David Sterba @ 2019-11-06 18:53 ` Nick Terrell 2019-11-07 6:34 ` Michael Chang 1 sibling, 0 replies; 24+ messages in thread From: Nick Terrell @ 2019-11-06 18:53 UTC (permalink / raw) To: The development of GNU GRUB > On Nov 6, 2019, at 4:40 AM, David Sterba <dave@jikos.cz> wrote: > > On Tue, Nov 05, 2019 at 09:19:59AM +0000, Michael Chang 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. > > So combination zstd+btrfs+i386-pc never worked? Removing support for > zstd could lead to unbootable system, but if that has never worked > before it'd be ok to make the build conditional. > > Looking at zstd code, does not seem to be easy to squeeze the asm to > something like 64-56=8K. Yeah, upstream zstd won’t get down to 8K. We recently did some work to make the decompressor smaller for mobile reducing inlining and excluding variants of some functions. On x86_64 the upstream library went from 84KB -> 63KB uncompressed, and 31KB -> 25KB compressed. If we update GRUB to the latest zstd and define a few macros we can get that gain. But it won’t be 8KB. A custom zstd decompress optimized for size could be made much smaller But that would be a large maintenance burden. For example, we have an “educational decoder” that is 18 KB uncompressed and 8 KB compressed, but it can’t be used in production, since it isn’t hardened against invalid inputs. But, it shows the zstd format can be decompressed with a small library. If we (upstream) add a smaller zstd decompressor, we will put a patch up to use it in GRUB. -Nick ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-06 12:40 ` David Sterba 2019-11-06 18:53 ` Nick Terrell @ 2019-11-07 6:34 ` Michael Chang 1 sibling, 0 replies; 24+ messages in thread From: Michael Chang @ 2019-11-07 6:34 UTC (permalink / raw) To: The development of GNU GRUB On Wed, Nov 06, 2019 at 01:40:30PM +0100, David Sterba wrote: > On Tue, Nov 05, 2019 at 09:19:59AM +0000, Michael Chang 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. > > So combination zstd+btrfs+i386-pc never worked? It works for mbr gap, however we used to encounter some system shipped by Microsoft Windows still having its mbr gap aligned to cylinder boundary, which is usually 63 sectors. Therefore we could only fallback to btrfs bootloader area if mbr gap is found to be too small and now that the solution is no longer appropriate. > Removing support for > zstd could lead to unbootable system, but if that has never worked > before it'd be ok to make the build conditional. So far didn't have the request to enable btrfs zstd compression for the root filesystem. But with new grub supporting it, the request might come up in the future and hope we could have solution for i386-pc at that time. > > Looking at zstd code, does not seem to be easy to squeeze the asm to > something like 64-56=8K. Another solution from Vladmir is making the zstd support a separate module and loading on demand. I personally think it is better than reducing the size as the same situation could still come up in the future. Thanks, Michael > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-05 9:19 [PATCH] btrfs: disable zstd support for i386-pc Michael Chang 2019-11-05 10:52 ` Paul Menzel 2019-11-06 12:40 ` David Sterba @ 2019-11-06 19:15 ` Vladimir 'phcoder' Serbinenko 2019-11-07 4:55 ` Michael Chang 2019-11-07 11:52 ` Daniel Kiper 2 siblings, 2 replies; 24+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2019-11-06 19:15 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 5400 bytes --] Please don't do it this way. The right solution is to move it to separate module and include zstd module when needed. Not everybody uses btrfs embedded area. I recommend not to use it. Using mbr gap or BBP is the recommended way. On Tue, 5 Nov 2019, 01:25 Michael Chang, <MChang@suse.com> 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 <mchang@suse.com> > --- > 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 > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 6916 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-06 19:15 ` Vladimir 'phcoder' Serbinenko @ 2019-11-07 4:55 ` Michael Chang 2019-11-07 5:08 ` Vladimir 'phcoder' Serbinenko 2019-11-07 11:52 ` Daniel Kiper 1 sibling, 1 reply; 24+ messages in thread From: Michael Chang @ 2019-11-07 4:55 UTC (permalink / raw) To: The development of GNU GRUB 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. > 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, <MChang@suse.com> 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 <mchang@suse.com> > > --- > > 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 > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-07 4:55 ` Michael Chang @ 2019-11-07 5:08 ` Vladimir 'phcoder' Serbinenko 2019-11-07 6:59 ` Michael Chang 2020-06-11 22:58 ` Eli Schwartz 0 siblings, 2 replies; 24+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2019-11-07 5:08 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 7339 bytes --] On Wed, 6 Nov 2019, 20:55 Michael Chang, <MChang@suse.com> 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. 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. > > > 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, <MChang@suse.com> 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 <mchang@suse.com> > > > --- > > > 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 > > > Grub-devel@gnu.org > > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 10819 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-07 5:08 ` Vladimir 'phcoder' Serbinenko @ 2019-11-07 6:59 ` Michael Chang 2020-06-11 22:58 ` Eli Schwartz 1 sibling, 0 replies; 24+ messages in thread From: Michael Chang @ 2019-11-07 6:59 UTC (permalink / raw) To: The development of GNU GRUB On Wed, Nov 06, 2019 at 09:08:41PM -0800, Vladimir 'phcoder' Serbinenko wrote: > On Wed, 6 Nov 2019, 20:55 Michael Chang, <MChang@suse.com> 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, <MChang@suse.com> 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 <mchang@suse.com> > > > > --- > > > > 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 > > > > Grub-devel@gnu.org > > > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > > > > > _______________________________________________ > > > Grub-devel mailing list > > > Grub-devel@gnu.org > > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-07 5:08 ` Vladimir 'phcoder' Serbinenko 2019-11-07 6:59 ` Michael Chang @ 2020-06-11 22:58 ` Eli Schwartz 2020-06-21 18:26 ` Mike Gilbert 1 sibling, 1 reply; 24+ messages in thread From: Eli Schwartz @ 2020-06-11 22:58 UTC (permalink / raw) To: grub-devel [-- Attachment #1.1: Type: text/plain, Size: 1151 bytes --] On 11/7/19 12:08 AM, Vladimir 'phcoder' Serbinenko wrote: > On Wed, 6 Nov 2019, 20:55 Michael Chang, <MChang@suse.com> 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. 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. Given grub 2.06 is on the horizon, is it time to re-evaluate this and disable something known to be badly broken? Failing that, is anyone working on making it a separate module? :) -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 1601 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-06-11 22:58 ` Eli Schwartz @ 2020-06-21 18:26 ` Mike Gilbert 2020-06-21 18:56 ` Eli Schwartz 0 siblings, 1 reply; 24+ messages in thread From: Mike Gilbert @ 2020-06-21 18:26 UTC (permalink / raw) To: The development of GNU GRUB On Thu, Jun 11, 2020 at 6:58 PM Eli Schwartz <eschwartz@archlinux.org> wrote: > > On 11/7/19 12:08 AM, Vladimir 'phcoder' Serbinenko wrote: > > On Wed, 6 Nov 2019, 20:55 Michael Chang, <MChang@suse.com> 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. 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. > > Given grub 2.06 is on the horizon, is it time to re-evaluate this and > disable something known to be badly broken? If I have read the thread correctly, this only breaks if you try to install grub in an undersized embedding area. This would really only affect new grub installs; existing ones can keep using the currently installed code. I think it would be better to hold out for a real fix than to disable a feature for an entire platform where only a subset of configurations would not work. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-06-21 18:26 ` Mike Gilbert @ 2020-06-21 18:56 ` Eli Schwartz 2020-06-23 1:59 ` Mike Gilbert 2020-06-23 2:16 ` Mike Gilbert 0 siblings, 2 replies; 24+ messages in thread From: Eli Schwartz @ 2020-06-21 18:56 UTC (permalink / raw) To: grub-devel [-- Attachment #1.1: Type: text/plain, Size: 3259 bytes --] On 6/21/20 2:26 PM, Mike Gilbert wrote: > On Thu, Jun 11, 2020 at 6:58 PM Eli Schwartz <eschwartz@archlinux.org> wrote: >> >> On 11/7/19 12:08 AM, Vladimir 'phcoder' Serbinenko wrote: >>> On Wed, 6 Nov 2019, 20:55 Michael Chang, <MChang@suse.com> 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. 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. >> >> Given grub 2.06 is on the horizon, is it time to re-evaluate this and >> disable something known to be badly broken? > > If I have read the thread correctly, this only breaks if you try to > install grub in an undersized embedding area. This would really only > affect new grub installs; existing ones can keep using the currently > installed code. "changing grub only affects people who newly run the grub-install command" therefore it's not worth considering whether to make sure grub-install completes without errors? I don't comprehend this logic. Who ever suggested we care about people who aren't the target audience of new grub versions to begin with? But anyway this isn't true. There are valid reasons to reinstall grub on old systems, in which case you are most likely not benefiting from zstd support one way or another, but in this case, rerunning grub-install destroys the working bootloader code and fails to replace. https://bugs.archlinux.org/task/63656 our infrastructure apparently somehow ended up mismatching core.img and /boot modules, and had to rerun grub-install (it's not clear what happened exactly, possibly grub-install accidentally got rerun in a broken manner?) and then this remote server would not boot, and could not be fixed without hunting for the right old version of grub via the rescue system, reinstalling it, and using it to restore the boring (working) old pre-zstd bootloader code, after a lot of painful debugging. > I think it would be better to hold out for a real fix than to disable > a feature for an entire platform where only a subset of configurations > would not work. I'm astonished that "it's better to hold out for a real fix than to ensure users' systems actually work". Why is the benefit of enabling zstd support in situations where it very often breaks everything, worth more than the disadvantage of, in fact, breaking everything? The feature was broken out of the gate, no proper fix seems imminent, let's bandage the wound before more people get hurt. Better to not have a hot new feature than to have it, but be unable to boot. -- Eli Schwartz Bug Wrangler and Trusted User [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 1601 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-06-21 18:56 ` Eli Schwartz @ 2020-06-23 1:59 ` Mike Gilbert 2020-06-23 2:16 ` Mike Gilbert 1 sibling, 0 replies; 24+ messages in thread From: Mike Gilbert @ 2020-06-23 1:59 UTC (permalink / raw) To: The development of GNU GRUB On Sun, Jun 21, 2020 at 2:56 PM Eli Schwartz <eschwartz@archlinux.org> wrote: > > On 6/21/20 2:26 PM, Mike Gilbert wrote: > > On Thu, Jun 11, 2020 at 6:58 PM Eli Schwartz <eschwartz@archlinux.org> wrote: > >> > >> On 11/7/19 12:08 AM, Vladimir 'phcoder' Serbinenko wrote: > >>> On Wed, 6 Nov 2019, 20:55 Michael Chang, <MChang@suse.com> 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. 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. > >> > >> Given grub 2.06 is on the horizon, is it time to re-evaluate this and > >> disable something known to be badly broken? > > > > If I have read the thread correctly, this only breaks if you try to > > install grub in an undersized embedding area. This would really only > > affect new grub installs; existing ones can keep using the currently > > installed code. > > "changing grub only affects people who newly run the grub-install > command" therefore it's not worth considering whether to make sure > grub-install completes without errors? I don't comprehend this logic. > Who ever suggested we care about people who aren't the target audience > of new grub versions to begin with? > > But anyway this isn't true. There are valid reasons to reinstall grub on > old systems, in which case you are most likely not benefiting from zstd > support one way or another, but in this case, rerunning grub-install > destroys the working bootloader code and fails to replace. > > https://bugs.archlinux.org/task/63656 our infrastructure apparently > somehow ended up mismatching core.img and /boot modules, and had to > rerun grub-install (it's not clear what happened exactly, possibly > grub-install accidentally got rerun in a broken manner?) and then this > remote server would not boot, and could not be fixed without hunting for > the right old version of grub via the rescue system, reinstalling it, > and using it to restore the boring (working) old pre-zstd bootloader > code, after a lot of painful debugging. > > > I think it would be better to hold out for a real fix than to disable > > a feature for an entire platform where only a subset of configurations > > would not work. > > I'm astonished that "it's better to hold out for a real fix than to > ensure users' systems actually work". > > Why is the benefit of enabling zstd support in situations where it very > often breaks everything, worth more than the disadvantage of, in fact, > breaking everything? The feature was broken out of the gate, no proper > fix seems imminent, let's bandage the wound before more people get hurt. If the feature is "broken", it should be reverted fully, not disabled on a single platform. That might encourage the original contributor to implement it properly next time. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-06-21 18:56 ` Eli Schwartz 2020-06-23 1:59 ` Mike Gilbert @ 2020-06-23 2:16 ` Mike Gilbert 2020-06-23 6:32 ` Michael Chang 1 sibling, 1 reply; 24+ messages in thread From: Mike Gilbert @ 2020-06-23 2:16 UTC (permalink / raw) To: The development of GNU GRUB On Sun, Jun 21, 2020 at 2:56 PM Eli Schwartz <eschwartz@archlinux.org> wrote: > But anyway this isn't true. There are valid reasons to reinstall grub on > old systems, in which case you are most likely not benefiting from zstd > support one way or another, but in this case, rerunning grub-install > destroys the working bootloader code and fails to replace. This sounds like a bug/defect in grub-install. Ideally, it would look at the size of the embedding area, and abort early if it is too small. This should happen before files are copied to /boot/grub, but currently it is done afterward. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-06-23 2:16 ` Mike Gilbert @ 2020-06-23 6:32 ` Michael Chang 2020-06-23 17:50 ` Mike Gilbert 0 siblings, 1 reply; 24+ messages in thread From: Michael Chang @ 2020-06-23 6:32 UTC (permalink / raw) To: The development of GNU GRUB On Mon, Jun 22, 2020 at 10:16:49PM -0400, Mike Gilbert wrote: > On Sun, Jun 21, 2020 at 2:56 PM Eli Schwartz <eschwartz@archlinux.org> wrote: > > But anyway this isn't true. There are valid reasons to reinstall grub on > > old systems, in which case you are most likely not benefiting from zstd > > support one way or another, but in this case, rerunning grub-install > > destroys the working bootloader code and fails to replace. > > This sounds like a bug/defect in grub-install. Ideally, it would look > at the size of the embedding area, and abort early if it is too small. > This should happen before files are copied to /boot/grub, but > currently it is done afterward. Even though the system can stay bootable, it cannot receive any grub updates afterward, thus breaking the principle of backward compatibilty that new build couldn't run at all on an existing system running old grub. Thanks, Michael > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-06-23 6:32 ` Michael Chang @ 2020-06-23 17:50 ` Mike Gilbert 0 siblings, 0 replies; 24+ messages in thread From: Mike Gilbert @ 2020-06-23 17:50 UTC (permalink / raw) To: The development of GNU GRUB On Tue, Jun 23, 2020 at 2:32 AM Michael Chang <mchang@suse.com> wrote: > > On Mon, Jun 22, 2020 at 10:16:49PM -0400, Mike Gilbert wrote: > > On Sun, Jun 21, 2020 at 2:56 PM Eli Schwartz <eschwartz@archlinux.org> wrote: > > > But anyway this isn't true. There are valid reasons to reinstall grub on > > > old systems, in which case you are most likely not benefiting from zstd > > > support one way or another, but in this case, rerunning grub-install > > > destroys the working bootloader code and fails to replace. > > > > This sounds like a bug/defect in grub-install. Ideally, it would look > > at the size of the embedding area, and abort early if it is too small. > > This should happen before files are copied to /boot/grub, but > > currently it is done afterward. > > Even though the system can stay bootable, it cannot receive any grub > updates afterward, thus breaking the principle of backward compatibilty > that new build couldn't run at all on an existing system running old > grub. There was some discussion earlier in the thread that proposed the documentation be updated to indicate that using a small embedding area is not well-supported. I don't think 100% backward compatibility is a reasonable target, especially for configurations that have never been well-supported in the first place. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-06 19:15 ` Vladimir 'phcoder' Serbinenko 2019-11-07 4:55 ` Michael Chang @ 2019-11-07 11:52 ` Daniel Kiper 2019-11-13 11:00 ` Daniel Kiper 1 sibling, 1 reply; 24+ messages in thread From: Daniel Kiper @ 2019-11-07 11:52 UTC (permalink / raw) To: Vladimir 'phcoder' Serbinenko Cc: The development of GNU GRUB, dave, mchang, pmenzel, terrelln 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. Not everybody uses btrfs > embedded area. I recommend not to use it. Using mbr gap or BBP is the > recommended way. I will put the cat among the pigeons. Maybe we should finally stop pretending that the GRUB supports small MBR gaps. Otherwise we will be fighting with such issues endlessly. Daniel > On Tue, 5 Nov 2019, 01:25 Michael Chang, <MChang@suse.com> 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 <mchang@suse.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-07 11:52 ` Daniel Kiper @ 2019-11-13 11:00 ` Daniel Kiper 2019-11-14 9:53 ` Michael Chang 0 siblings, 1 reply; 24+ messages in thread From: Daniel Kiper @ 2019-11-13 11:00 UTC (permalink / raw) To: Vladimir 'phcoder' Serbinenko Cc: The development of GNU GRUB, dave, mchang, pmenzel, terrelln On Thu, Nov 07, 2019 at 12:52:35PM +0100, Daniel Kiper 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. Not everybody uses btrfs > > embedded area. I recommend not to use it. Using mbr gap or BBP is the > > recommended way. > > I will put the cat among the pigeons. Maybe we should finally stop > pretending that the GRUB supports small MBR gaps. Otherwise we will > be fighting with such issues endlessly. Bumping the thread... Nobody objects? Hmmm... > Daniel > > > On Tue, 5 Nov 2019, 01:25 Michael Chang, <MChang@suse.com> 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 <mchang@suse.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-13 11:00 ` Daniel Kiper @ 2019-11-14 9:53 ` Michael Chang 2019-11-15 11:42 ` Daniel Kiper 0 siblings, 1 reply; 24+ messages in thread From: Michael Chang @ 2019-11-14 9:53 UTC (permalink / raw) To: The development of GNU GRUB Cc: Vladimir 'phcoder' Serbinenko, dave, pmenzel, terrelln On Wed, Nov 13, 2019 at 12:00:50PM +0100, Daniel Kiper wrote: > On Thu, Nov 07, 2019 at 12:52:35PM +0100, Daniel Kiper 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. Not everybody uses btrfs > > > embedded area. I recommend not to use it. Using mbr gap or BBP is the > > > recommended way. > > > > I will put the cat among the pigeons. Maybe we should finally stop > > pretending that the GRUB supports small MBR gaps. Otherwise we will > > be fighting with such issues endlessly. > > Bumping the thread... Nobody objects? Hmmm... I don't think we are able to give up MBR gap support, simply because no other way out in creating the area for bootloader embedding. However we should consider to correct this claim in manual. "You must ensure that the first partition starts at least 31 KiB (63 sectors) from the start of the disk" to reflect the fact that 31 KiB is no longer applicable and the ideal size should be above 1MB (or such). You should go check with your disk tools to find the suitable parameter to fulfill the requirement, for eg, the partition alignment would mostly affect this. Thanks, Michael > > > Daniel > > > > > On Tue, 5 Nov 2019, 01:25 Michael Chang, <MChang@suse.com> 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 <mchang@suse.com> > > > > --- > > > > 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 > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-14 9:53 ` Michael Chang @ 2019-11-15 11:42 ` Daniel Kiper 2019-11-19 8:34 ` Michael Chang 0 siblings, 1 reply; 24+ messages in thread From: Daniel Kiper @ 2019-11-15 11:42 UTC (permalink / raw) To: Michael Chang Cc: The development of GNU GRUB, Vladimir 'phcoder' Serbinenko, dave, pmenzel, terrelln On Thu, Nov 14, 2019 at 09:53:54AM +0000, Michael Chang wrote: > On Wed, Nov 13, 2019 at 12:00:50PM +0100, Daniel Kiper wrote: > > On Thu, Nov 07, 2019 at 12:52:35PM +0100, Daniel Kiper 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. Not everybody uses btrfs > > > > embedded area. I recommend not to use it. Using mbr gap or BBP is the > > > > recommended way. > > > > > > I will put the cat among the pigeons. Maybe we should finally stop > > > pretending that the GRUB supports small MBR gaps. Otherwise we will > > > be fighting with such issues endlessly. > > > > Bumping the thread... Nobody objects? Hmmm... > > I don't think we are able to give up MBR gap support, simply because no > other way out in creating the area for bootloader embedding. However we Nope, I do not propose that... > should consider to correct this claim in manual. > > "You must ensure that the first partition starts at least 31 KiB (63 > sectors) from the start of the disk" > > to reflect the fact that 31 KiB is no longer applicable and the ideal > size should be above 1MB (or such). You should go check with your disk > tools to find the suitable parameter to fulfill the requirement, for eg, > the partition alignment would mostly affect this. Exactly! However, there are some legacy systems which do not boot if MBR gap does not end at 63 sectors boundary. Hence, maybe we should suggest chainloading, using e.g. SYSLINUX, in a such cases. Anyway, may I ask you to prepare a patch for GRUB manual which describes the problem? Daniel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-15 11:42 ` Daniel Kiper @ 2019-11-19 8:34 ` Michael Chang 2020-03-03 16:59 ` Daniel Kiper 0 siblings, 1 reply; 24+ messages in thread From: Michael Chang @ 2019-11-19 8:34 UTC (permalink / raw) To: Daniel Kiper Cc: The development of GNU GRUB, Vladimir 'phcoder' Serbinenko, dave, pmenzel, terrelln On Fri, Nov 15, 2019 at 12:42:52PM +0100, Daniel Kiper wrote: > On Thu, Nov 14, 2019 at 09:53:54AM +0000, Michael Chang wrote: > > On Wed, Nov 13, 2019 at 12:00:50PM +0100, Daniel Kiper wrote: > > > On Thu, Nov 07, 2019 at 12:52:35PM +0100, Daniel Kiper 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. Not everybody uses btrfs > > > > > embedded area. I recommend not to use it. Using mbr gap or BBP is the > > > > > recommended way. > > > > > > > > I will put the cat among the pigeons. Maybe we should finally stop > > > > pretending that the GRUB supports small MBR gaps. Otherwise we will > > > > be fighting with such issues endlessly. > > > > > > Bumping the thread... Nobody objects? Hmmm... > > > > I don't think we are able to give up MBR gap support, simply because no > > other way out in creating the area for bootloader embedding. However we > > Nope, I do not propose that... > > > should consider to correct this claim in manual. > > > > "You must ensure that the first partition starts at least 31 KiB (63 > > sectors) from the start of the disk" > > > > to reflect the fact that 31 KiB is no longer applicable and the ideal > > size should be above 1MB (or such). You should go check with your disk > > tools to find the suitable parameter to fulfill the requirement, for eg, > > the partition alignment would mostly affect this. > > Exactly! However, there are some legacy systems which do not boot if MBR > gap does not end at 63 sectors boundary. Hence, maybe we should suggest > chainloading, using e.g. SYSLINUX, in a such cases. Anyway, may I ask > you to prepare a patch for GRUB manual which describes the problem? Alright, sounds good to me. :) Regards, Michael > > Daniel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2019-11-19 8:34 ` Michael Chang @ 2020-03-03 16:59 ` Daniel Kiper 2020-03-04 7:58 ` Michael Chang 0 siblings, 1 reply; 24+ messages in thread From: Daniel Kiper @ 2020-03-03 16:59 UTC (permalink / raw) To: Michael Chang Cc: The development of GNU GRUB, Vladimir 'phcoder' Serbinenko, dave, pmenzel, terrelln On Tue, Nov 19, 2019 at 08:34:12AM +0000, Michael Chang wrote: > On Fri, Nov 15, 2019 at 12:42:52PM +0100, Daniel Kiper wrote: > > On Thu, Nov 14, 2019 at 09:53:54AM +0000, Michael Chang wrote: > > > On Wed, Nov 13, 2019 at 12:00:50PM +0100, Daniel Kiper wrote: > > > > On Thu, Nov 07, 2019 at 12:52:35PM +0100, Daniel Kiper 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. Not everybody uses btrfs > > > > > > embedded area. I recommend not to use it. Using mbr gap or BBP is the > > > > > > recommended way. > > > > > > > > > > I will put the cat among the pigeons. Maybe we should finally stop > > > > > pretending that the GRUB supports small MBR gaps. Otherwise we will > > > > > be fighting with such issues endlessly. > > > > > > > > Bumping the thread... Nobody objects? Hmmm... > > > > > > I don't think we are able to give up MBR gap support, simply because no > > > other way out in creating the area for bootloader embedding. However we > > > > Nope, I do not propose that... > > > > > should consider to correct this claim in manual. > > > > > > "You must ensure that the first partition starts at least 31 KiB (63 > > > sectors) from the start of the disk" > > > > > > to reflect the fact that 31 KiB is no longer applicable and the ideal > > > size should be above 1MB (or such). You should go check with your disk > > > tools to find the suitable parameter to fulfill the requirement, for eg, > > > the partition alignment would mostly affect this. > > > > Exactly! However, there are some legacy systems which do not boot if MBR > > gap does not end at 63 sectors boundary. Hence, maybe we should suggest > > chainloading, using e.g. SYSLINUX, in a such cases. Anyway, may I ask > > you to prepare a patch for GRUB manual which describes the problem? > > Alright, sounds good to me. :) Michael, ping? I would like to see this limitation documented in 2.06. Could you do that? Daniel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] btrfs: disable zstd support for i386-pc 2020-03-03 16:59 ` Daniel Kiper @ 2020-03-04 7:58 ` Michael Chang 0 siblings, 0 replies; 24+ messages in thread From: Michael Chang @ 2020-03-04 7:58 UTC (permalink / raw) To: The development of GNU GRUB Cc: Vladimir 'phcoder' Serbinenko, dave, pmenzel, terrelln On Tue, Mar 03, 2020 at 05:59:12PM +0100, Daniel Kiper wrote: > On Tue, Nov 19, 2019 at 08:34:12AM +0000, Michael Chang wrote: > > On Fri, Nov 15, 2019 at 12:42:52PM +0100, Daniel Kiper wrote: > > > On Thu, Nov 14, 2019 at 09:53:54AM +0000, Michael Chang wrote: > > > > On Wed, Nov 13, 2019 at 12:00:50PM +0100, Daniel Kiper wrote: > > > > > On Thu, Nov 07, 2019 at 12:52:35PM +0100, Daniel Kiper 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. Not everybody uses btrfs > > > > > > > embedded area. I recommend not to use it. Using mbr gap or BBP is the > > > > > > > recommended way. > > > > > > > > > > > > I will put the cat among the pigeons. Maybe we should finally stop > > > > > > pretending that the GRUB supports small MBR gaps. Otherwise we will > > > > > > be fighting with such issues endlessly. > > > > > > > > > > Bumping the thread... Nobody objects? Hmmm... > > > > > > > > I don't think we are able to give up MBR gap support, simply because no > > > > other way out in creating the area for bootloader embedding. However we > > > > > > Nope, I do not propose that... > > > > > > > should consider to correct this claim in manual. > > > > > > > > "You must ensure that the first partition starts at least 31 KiB (63 > > > > sectors) from the start of the disk" > > > > > > > > to reflect the fact that 31 KiB is no longer applicable and the ideal > > > > size should be above 1MB (or such). You should go check with your disk > > > > tools to find the suitable parameter to fulfill the requirement, for eg, > > > > the partition alignment would mostly affect this. > > > > > > Exactly! However, there are some legacy systems which do not boot if MBR > > > gap does not end at 63 sectors boundary. Hence, maybe we should suggest > > > chainloading, using e.g. SYSLINUX, in a such cases. Anyway, may I ask > > > you to prepare a patch for GRUB manual which describes the problem? > > > > Alright, sounds good to me. :) > > Michael, ping? I would like to see this limitation documented in 2.06. > Could you do that? Hi Daniel, OK. I'll come up with a patch later this week. I am sorry for the delay. Regards, Michael > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-06-23 17:50 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-05 9:19 [PATCH] btrfs: disable zstd support for i386-pc Michael Chang 2019-11-05 10:52 ` Paul Menzel 2019-11-07 4:22 ` Michael Chang 2019-11-06 12:40 ` David Sterba 2019-11-06 18:53 ` Nick Terrell 2019-11-07 6:34 ` Michael Chang 2019-11-06 19:15 ` Vladimir 'phcoder' Serbinenko 2019-11-07 4:55 ` Michael Chang 2019-11-07 5:08 ` Vladimir 'phcoder' Serbinenko 2019-11-07 6:59 ` Michael Chang 2020-06-11 22:58 ` Eli Schwartz 2020-06-21 18:26 ` Mike Gilbert 2020-06-21 18:56 ` Eli Schwartz 2020-06-23 1:59 ` Mike Gilbert 2020-06-23 2:16 ` Mike Gilbert 2020-06-23 6:32 ` Michael Chang 2020-06-23 17:50 ` Mike Gilbert 2019-11-07 11:52 ` Daniel Kiper 2019-11-13 11:00 ` Daniel Kiper 2019-11-14 9:53 ` Michael Chang 2019-11-15 11:42 ` Daniel Kiper 2019-11-19 8:34 ` Michael Chang 2020-03-03 16:59 ` Daniel Kiper 2020-03-04 7:58 ` Michael Chang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.