* Read before you deploy btrfs + zstd @ 2017-11-13 22:50 David Sterba 2017-11-14 7:34 ` Martin Steigerwald 2017-11-14 18:53 ` David Sterba 0 siblings, 2 replies; 22+ messages in thread From: David Sterba @ 2017-11-13 22:50 UTC (permalink / raw) To: linux-btrfs Hi, while 4.14 is still fresh, let me address some concerns I've seen on linux forums already. The newly added ZSTD support is a feature that has broader impact than just the runtime compression. The btrfs-progs understand filesystem with ZSTD since 4.13. The remaining key part is the bootloader. Up to now, there are no bootloaders supporting ZSTD. This could lead to an unmountable filesystem if the critical files under /boot get accidentally or intentionally compressed by ZSTD. There are several ways how to get around that: - separate boot partition, no zstd there - reset potential compression of /boot/* files to something supported, eg. $ btrfs filesystem defrag -r -czlib /boot/* or $ btrfs filesystem defrag -r -clzo /boot/* To see if there are zstd files: $ find /boot -print -exec sudo btrfs prop get '{}' compression \; | grep -B1 zstd There might be other workarounds but I want to keep the advice simple. d. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-13 22:50 Read before you deploy btrfs + zstd David Sterba @ 2017-11-14 7:34 ` Martin Steigerwald 2017-11-14 9:45 ` Paul Jones ` (2 more replies) 2017-11-14 18:53 ` David Sterba 1 sibling, 3 replies; 22+ messages in thread From: Martin Steigerwald @ 2017-11-14 7:34 UTC (permalink / raw) To: dsterba, linux-btrfs Hello David. David Sterba - 13.11.17, 23:50: > while 4.14 is still fresh, let me address some concerns I've seen on linux > forums already. > > The newly added ZSTD support is a feature that has broader impact than > just the runtime compression. The btrfs-progs understand filesystem with > ZSTD since 4.13. The remaining key part is the bootloader. > > Up to now, there are no bootloaders supporting ZSTD. This could lead to an > unmountable filesystem if the critical files under /boot get accidentally > or intentionally compressed by ZSTD. But otherwise ZSTD is safe to use? Are you aware of any other issues? I consider switching from LZO to ZSTD on this ThinkPad T520 with Sandybridge. Thank you, -- Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: Read before you deploy btrfs + zstd 2017-11-14 7:34 ` Martin Steigerwald @ 2017-11-14 9:45 ` Paul Jones 2017-11-14 12:38 ` Austin S. Hemmelgarn 2017-11-14 18:49 ` David Sterba 2 siblings, 0 replies; 22+ messages in thread From: Paul Jones @ 2017-11-14 9:45 UTC (permalink / raw) To: Martin Steigerwald, dsterba, linux-btrfs > -----Original Message----- > From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs- > owner@vger.kernel.org] On Behalf Of Martin Steigerwald > Sent: Tuesday, 14 November 2017 6:35 PM > To: dsterba@suse.cz; linux-btrfs@vger.kernel.org > Subject: Re: Read before you deploy btrfs + zstd > > Hello David. > > David Sterba - 13.11.17, 23:50: > > while 4.14 is still fresh, let me address some concerns I've seen on > > linux forums already. > > > > The newly added ZSTD support is a feature that has broader impact than > > just the runtime compression. The btrfs-progs understand filesystem > > with ZSTD since 4.13. The remaining key part is the bootloader. > > > > Up to now, there are no bootloaders supporting ZSTD. This could lead > > to an unmountable filesystem if the critical files under /boot get > > accidentally or intentionally compressed by ZSTD. > > But otherwise ZSTD is safe to use? Are you aware of any other issues? > > I consider switching from LZO to ZSTD on this ThinkPad T520 with > Sandybridge. I've been using it since rc2 and had no trouble at all so far. The filesystem is running faster now (with zstd) than it did uncompressed on 4.13 Paul. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-14 7:34 ` Martin Steigerwald 2017-11-14 9:45 ` Paul Jones @ 2017-11-14 12:38 ` Austin S. Hemmelgarn 2017-11-15 20:23 ` Duncan 2017-11-14 18:49 ` David Sterba 2 siblings, 1 reply; 22+ messages in thread From: Austin S. Hemmelgarn @ 2017-11-14 12:38 UTC (permalink / raw) To: Martin Steigerwald, dsterba, linux-btrfs On 2017-11-14 02:34, Martin Steigerwald wrote: > Hello David. > > David Sterba - 13.11.17, 23:50: >> while 4.14 is still fresh, let me address some concerns I've seen on linux >> forums already. >> >> The newly added ZSTD support is a feature that has broader impact than >> just the runtime compression. The btrfs-progs understand filesystem with >> ZSTD since 4.13. The remaining key part is the bootloader. >> >> Up to now, there are no bootloaders supporting ZSTD. This could lead to an >> unmountable filesystem if the critical files under /boot get accidentally >> or intentionally compressed by ZSTD. > > But otherwise ZSTD is safe to use? Are you aware of any other issues? Aside from the obvious issue that recovery media like SystemRescueCD and the GParted LIveCD haven't caught up yet, and thus won't be able to do anything with the filesystem, my testing has not uncovered any issues, though it is by no means rigorous. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-14 12:38 ` Austin S. Hemmelgarn @ 2017-11-15 20:23 ` Duncan 2017-11-16 14:31 ` Dmitrii Tcvetkov 0 siblings, 1 reply; 22+ messages in thread From: Duncan @ 2017-11-15 20:23 UTC (permalink / raw) To: linux-btrfs Austin S. Hemmelgarn posted on Tue, 14 Nov 2017 07:38:03 -0500 as excerpted: > On 2017-11-14 02:34, Martin Steigerwald wrote: >> Hello David. >> >> David Sterba - 13.11.17, 23:50: >>> while 4.14 is still fresh, let me address some concerns I've seen on >>> linux forums already. >>> >>> The newly added ZSTD support is a feature that has broader impact than >>> just the runtime compression. The btrfs-progs understand filesystem >>> with ZSTD since 4.13. The remaining key part is the bootloader. >>> >>> Up to now, there are no bootloaders supporting ZSTD. This could lead >>> to an unmountable filesystem if the critical files under /boot get >>> accidentally or intentionally compressed by ZSTD. >> >> But otherwise ZSTD is safe to use? Are you aware of any other issues? > Aside from the obvious issue that recovery media like SystemRescueCD and > the GParted LIveCD haven't caught up yet, and thus won't be able to do > anything with the filesystem, my testing has not uncovered any issues, > though it is by no means rigorous. Tho from my understanding and last I read, btrfs restore (I believe it was) hadn't been updated to handle zstd yet, tho btrfs check and btrfs filesystem defrag had been, and of course btrfs balance if the kernel handles it since all balance does is call the kernel to do it. So just confirming, does btrfs restore handle zstd from -progs 4.13? Because once a filesystem goes unmountable, restore is what I've had best luck with, so if it doesn't understand zstd, no zstd for me. (Regardless, being the slightly cautious type I'll very likely wait a couple kernel cycles before switching from lzo to zstd here, just to see if any weird reports about it from the first-out testers hit the list in the mean time.) Meanwhile, it shouldn't need said but just in case, if you're using it, be sure you have backups /not/ using zstd, for at least a couple kernel cycles. =:^) -- Duncan - List replies preferred. No HTML msgs. "Every nonfree program has a lord, a master -- and if you use the program, he is your master." Richard Stallman ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-15 20:23 ` Duncan @ 2017-11-16 14:31 ` Dmitrii Tcvetkov 0 siblings, 0 replies; 22+ messages in thread From: Dmitrii Tcvetkov @ 2017-11-16 14:31 UTC (permalink / raw) To: linux-btrfs On Wed, 15 Nov 2017 20:23:44 +0000 (UTC) Duncan <1i5t5.duncan@cox.net> wrote: > Tho from my understanding and last I read, btrfs restore (I believe > it was) hadn't been updated to handle zstd yet, tho btrfs check and > btrfs filesystem defrag had been, and of course btrfs balance if the > kernel handles it since all balance does is call the kernel to do it. > > So just confirming, does btrfs restore handle zstd from -progs 4.13? > > Because once a filesystem goes unmountable, restore is what I've had > best luck with, so if it doesn't understand zstd, no zstd for me. > (Regardless, being the slightly cautious type I'll very likely wait a > couple kernel cycles before switching from lzo to zstd here, just to > see if any weird reports about it from the first-out testers hit the > list in the mean time.) > > Meanwhile, it shouldn't need said but just in case, if you're using > it, be sure you have backups /not/ using zstd, for at least a couple > kernel cycles. =:^) > Btrfs-progs 4.13 can be optionally built with libzstd, then btrfs restore can restore from zstd compressed file systems (I tested that right now with temporary fs using compress-force=zstd mount option). Btrfs-progs 4.14 will require libzstd by default during build. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-14 7:34 ` Martin Steigerwald 2017-11-14 9:45 ` Paul Jones 2017-11-14 12:38 ` Austin S. Hemmelgarn @ 2017-11-14 18:49 ` David Sterba 2017-11-14 19:43 ` Martin Steigerwald 2 siblings, 1 reply; 22+ messages in thread From: David Sterba @ 2017-11-14 18:49 UTC (permalink / raw) To: Martin Steigerwald; +Cc: dsterba, linux-btrfs On Tue, Nov 14, 2017 at 08:34:37AM +0100, Martin Steigerwald wrote: > Hello David. > > David Sterba - 13.11.17, 23:50: > > while 4.14 is still fresh, let me address some concerns I've seen on linux > > forums already. > > > > The newly added ZSTD support is a feature that has broader impact than > > just the runtime compression. The btrfs-progs understand filesystem with > > ZSTD since 4.13. The remaining key part is the bootloader. > > > > Up to now, there are no bootloaders supporting ZSTD. This could lead to an > > unmountable filesystem if the critical files under /boot get accidentally > > or intentionally compressed by ZSTD. > > But otherwise ZSTD is safe to use? Are you aware of any other issues? No issues from my own testing or reported by other users. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-14 18:49 ` David Sterba @ 2017-11-14 19:43 ` Martin Steigerwald 0 siblings, 0 replies; 22+ messages in thread From: Martin Steigerwald @ 2017-11-14 19:43 UTC (permalink / raw) To: dsterba, linux-btrfs David Sterba - 14.11.17, 19:49: > On Tue, Nov 14, 2017 at 08:34:37AM +0100, Martin Steigerwald wrote: > > Hello David. > > > > David Sterba - 13.11.17, 23:50: > > > while 4.14 is still fresh, let me address some concerns I've seen on > > > linux > > > forums already. > > > > > > The newly added ZSTD support is a feature that has broader impact than > > > just the runtime compression. The btrfs-progs understand filesystem with > > > ZSTD since 4.13. The remaining key part is the bootloader. > > > > > > Up to now, there are no bootloaders supporting ZSTD. This could lead to > > > an > > > unmountable filesystem if the critical files under /boot get > > > accidentally > > > or intentionally compressed by ZSTD. > > > > But otherwise ZSTD is safe to use? Are you aware of any other issues? > > No issues from my own testing or reported by other users. Thanks to you and the others. I think I try this soon. Thanks, -- Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-13 22:50 Read before you deploy btrfs + zstd David Sterba 2017-11-14 7:34 ` Martin Steigerwald @ 2017-11-14 18:53 ` David Sterba 2017-11-15 14:39 ` David Sterba 1 sibling, 1 reply; 22+ messages in thread From: David Sterba @ 2017-11-14 18:53 UTC (permalink / raw) To: dsterba, linux-btrfs On Mon, Nov 13, 2017 at 11:50:46PM +0100, David Sterba wrote: > Up to now, there are no bootloaders supporting ZSTD. I've tried to implement the support to GRUB, still incomplete and hacky but most of the code is there. The ZSTD implementation is copied from kernel. The allocators need to be properly set up, as it needs to use grub_malloc/grub_free for the workspace thats called from some ZSTD_* functions. https://github.com/kdave/grub/tree/btrfs-zstd ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-14 18:53 ` David Sterba @ 2017-11-15 14:39 ` David Sterba 2017-11-15 16:22 ` Martin Steigerwald 2017-11-15 20:09 ` Nick Terrell 0 siblings, 2 replies; 22+ messages in thread From: David Sterba @ 2017-11-15 14:39 UTC (permalink / raw) To: dsterba, linux-btrfs; +Cc: terrelln On Tue, Nov 14, 2017 at 07:53:31PM +0100, David Sterba wrote: > On Mon, Nov 13, 2017 at 11:50:46PM +0100, David Sterba wrote: > > Up to now, there are no bootloaders supporting ZSTD. > > I've tried to implement the support to GRUB, still incomplete and hacky > but most of the code is there. The ZSTD implementation is copied from > kernel. The allocators need to be properly set up, as it needs to use > grub_malloc/grub_free for the workspace thats called from some ZSTD_* > functions. > > https://github.com/kdave/grub/tree/btrfs-zstd The branch is now in a state that can be tested. Turns out the memory requirements are too much for grub, so the boot fails with "not enough memory". The calculated value ZSTD_BTRFS_MAX_INPUT: 131072 ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 This is not something I could fix easily, we'd probalby need a tuned version of ZSTD for grub constraints. Adding Nick to CC. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-15 14:39 ` David Sterba @ 2017-11-15 16:22 ` Martin Steigerwald 2017-11-15 18:16 ` Imran Geriskovan 2017-11-15 20:09 ` Nick Terrell 1 sibling, 1 reply; 22+ messages in thread From: Martin Steigerwald @ 2017-11-15 16:22 UTC (permalink / raw) To: dsterba, linux-btrfs, terrelln David Sterba - 15.11.17, 15:39: > On Tue, Nov 14, 2017 at 07:53:31PM +0100, David Sterba wrote: > > On Mon, Nov 13, 2017 at 11:50:46PM +0100, David Sterba wrote: > > > Up to now, there are no bootloaders supporting ZSTD. > > > > I've tried to implement the support to GRUB, still incomplete and hacky > > but most of the code is there. The ZSTD implementation is copied from > > kernel. The allocators need to be properly set up, as it needs to use > > grub_malloc/grub_free for the workspace thats called from some ZSTD_* > > functions. > > > > https://github.com/kdave/grub/tree/btrfs-zstd > > The branch is now in a state that can be tested. Turns out the memory > requirements are too much for grub, so the boot fails with "not enough > memory". The calculated value > > ZSTD_BTRFS_MAX_INPUT: 131072 > ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 > > This is not something I could fix easily, we'd probalby need a tuned > version of ZSTD for grub constraints. Adding Nick to CC. Somehow I am happy that I still have a plain Ext4 for /boot. :) Thanks for looking into Grub support anyway. Thanks, -- Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-15 16:22 ` Martin Steigerwald @ 2017-11-15 18:16 ` Imran Geriskovan 2017-11-15 20:06 ` Duncan 0 siblings, 1 reply; 22+ messages in thread From: Imran Geriskovan @ 2017-11-15 18:16 UTC (permalink / raw) To: Martin Steigerwald; +Cc: dsterba, linux-btrfs, terrelln On 11/15/17, Martin Steigerwald <martin@lichtvoll.de> wrote: > Somehow I am happy that I still have a plain Ext4 for /boot. :) You may use uncompressed btrfs for /boot. Both Syslinux (my choice) and Grub supports it. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-15 18:16 ` Imran Geriskovan @ 2017-11-15 20:06 ` Duncan 0 siblings, 0 replies; 22+ messages in thread From: Duncan @ 2017-11-15 20:06 UTC (permalink / raw) To: linux-btrfs Imran Geriskovan posted on Wed, 15 Nov 2017 20:16:56 +0200 as excerpted: > On 11/15/17, Martin Steigerwald <martin@lichtvoll.de> wrote: >> Somehow I am happy that I still have a plain Ext4 for /boot. :) > > You may use uncompressed btrfs for /boot. > Both Syslinux (my choice) and Grub supports it. And I've been running grub2 with compress=lzo on my /boot for years, now, without issue. -- Duncan - List replies preferred. No HTML msgs. "Every nonfree program has a lord, a master -- and if you use the program, he is your master." Richard Stallman ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-15 14:39 ` David Sterba 2017-11-15 16:22 ` Martin Steigerwald @ 2017-11-15 20:09 ` Nick Terrell 2017-11-21 16:22 ` David Sterba 1 sibling, 1 reply; 22+ messages in thread From: Nick Terrell @ 2017-11-15 20:09 UTC (permalink / raw) To: dsterba, linux-btrfs, Martin Steigerwald, Imran Geriskovan [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1979 bytes --] On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: > The branch is now in a state that can be tested. Turns out the memory > requirements are too much for grub, so the boot fails with "not enough > memory". The calculated value > > ZSTD_BTRFS_MAX_INPUT: 131072 > ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 > > This is not something I could fix easily, we'd probalby need a tuned > version of ZSTD for grub constraints. Adding Nick to CC. If I understand the grub code correctly, we only need to read, and we have the entire input and output buffer in one segment. In that case you can use ZSTD_initDCtx(), and ZSTD_decompressDCtx(). ZSTD_DCtxWorkspaceBound() is only 155984. See decompress_single() in https://patchwork.kernel.org/patch/9997909/ for an example. This (uncompiled and untested) code should work: ``` static grub_ssize_t grub_btrfs_zstd_decompress(char *ibuf, grub_size_t isize, grub_off_t off, char *obuf, grub_size_t osize) { grub_size_t ret = 0; ZSTD_DCtx *ctx; void *wmem; grub_size_t wsize; wsize = ZSTD_DCtxWorkspaceBound(); wmem = grub_malloc(wsize); if (!wmem) { return -1; } ctx = ZSTD_initDCtx(wmem, wsize); if (!ctx) { grub_free(wmem); return -1; } /* This is only necessary if there is junk after the end of the zstd * compressed data in the input buffer. Otherwise the return value * should always be exactly isize. If you delete this, and it always works, * then there isn't junk data at the end. */ isize = ZSTD_findFrameCompressedSize(in_buf, in_len); if (ZSTD_isError(isize)) { grub_free(wmem); return -1; } ret = ZSTD_decompressDCtx(ctx, obuf, osize, ibuf, isize); grub_free(wmem); if (ZSTD_isError(ret)) { return -1; } return ret; } ``` ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±ý»k~ÏâØ^nr¡ö¦zË\x1aëh¨èÚ&£ûàz¿äz¹Þú+Ê+zf£¢·h§~Ûiÿÿïêÿêçz_è®\x0fæj:+v¨þ)ߣøm ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-15 20:09 ` Nick Terrell @ 2017-11-21 16:22 ` David Sterba 2017-11-28 21:31 ` Nick Terrell 0 siblings, 1 reply; 22+ messages in thread From: David Sterba @ 2017-11-21 16:22 UTC (permalink / raw) To: Nick Terrell; +Cc: linux-btrfs, Martin Steigerwald, Imran Geriskovan On Wed, Nov 15, 2017 at 08:09:15PM +0000, Nick Terrell wrote: > On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: > > The branch is now in a state that can be tested. Turns out the memory > > requirements are too much for grub, so the boot fails with "not enough > > memory". The calculated value > > > > ZSTD_BTRFS_MAX_INPUT: 131072 > > ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 > > > > This is not something I could fix easily, we'd probalby need a tuned > > version of ZSTD for grub constraints. Adding Nick to CC. > > If I understand the grub code correctly, we only need to read, and we have > the entire input and output buffer in one segment. In that case you can use > ZSTD_initDCtx(), and ZSTD_decompressDCtx(). ZSTD_DCtxWorkspaceBound() is > only 155984. See decompress_single() in > https://patchwork.kernel.org/patch/9997909/ for an example. Does not help, still ENOMEM. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-21 16:22 ` David Sterba @ 2017-11-28 21:31 ` Nick Terrell 2017-11-28 23:49 ` David Sterba 0 siblings, 1 reply; 22+ messages in thread From: Nick Terrell @ 2017-11-28 21:31 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs, Martin Steigerwald, Imran Geriskovan > On Nov 21, 2017, at 8:22 AM, David Sterba <dsterba@suse.cz> wrote: > > On Wed, Nov 15, 2017 at 08:09:15PM +0000, Nick Terrell wrote: >> On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: >>> The branch is now in a state that can be tested. Turns out the memory >>> requirements are too much for grub, so the boot fails with "not enough >>> memory". The calculated value >>> >>> ZSTD_BTRFS_MAX_INPUT: 131072 >>> ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 >>> >>> This is not something I could fix easily, we'd probalby need a tuned >>> version of ZSTD for grub constraints. Adding Nick to CC. >> >> If I understand the grub code correctly, we only need to read, and we have >> the entire input and output buffer in one segment. In that case you can use >> ZSTD_initDCtx(), and ZSTD_decompressDCtx(). ZSTD_DCtxWorkspaceBound() is >> only 155984. See decompress_single() in >> https://patchwork.kernel.org/patch/9997909/ for an example. > > Does not help, still ENOMEM. It looks like XZ had the same issue, and they make the decompression context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could potentially do the same and statically allocate the workspace: ``` /* Could also be size_t */ #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t)) static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; /* ... */ assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); ``` ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-28 21:31 ` Nick Terrell @ 2017-11-28 23:49 ` David Sterba 2017-11-29 0:44 ` Nick Terrell 2017-11-29 13:24 ` Austin S. Hemmelgarn 0 siblings, 2 replies; 22+ messages in thread From: David Sterba @ 2017-11-28 23:49 UTC (permalink / raw) To: Nick Terrell; +Cc: linux-btrfs, Martin Steigerwald, Imran Geriskovan On Tue, Nov 28, 2017 at 09:31:57PM +0000, Nick Terrell wrote: > > > On Nov 21, 2017, at 8:22 AM, David Sterba <dsterba@suse.cz> wrote: > > > > On Wed, Nov 15, 2017 at 08:09:15PM +0000, Nick Terrell wrote: > >> On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: > >>> The branch is now in a state that can be tested. Turns out the memory > >>> requirements are too much for grub, so the boot fails with "not enough > >>> memory". The calculated value > >>> > >>> ZSTD_BTRFS_MAX_INPUT: 131072 > >>> ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 > >>> > >>> This is not something I could fix easily, we'd probalby need a tuned > >>> version of ZSTD for grub constraints. Adding Nick to CC. > >> > >> If I understand the grub code correctly, we only need to read, and we have > >> the entire input and output buffer in one segment. In that case you can use > >> ZSTD_initDCtx(), and ZSTD_decompressDCtx(). ZSTD_DCtxWorkspaceBound() is > >> only 155984. See decompress_single() in > >> https://patchwork.kernel.org/patch/9997909/ for an example. > > > > Does not help, still ENOMEM. > > It looks like XZ had the same issue, and they make the decompression > context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could > potentially do the same and statically allocate the workspace: > > ``` > /* Could also be size_t */ > #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t)) > static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; > > /* ... */ > > assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); > ``` Interesting, thanks for the tip, I'll try it next. I've meanwhile tried to tweak the numbers, the maximum block for zstd, that squeezed the DCtx somewhere under 48k, with block size 8k. Still enomem. I've tried to add some debugging prints to see what numbers get actually passed to the allocator, but did not see anything printed. I'm sure there is a more intelligent way to test the grub changes. So far each test loop takes quite some time, as I build the rpm package, test it in a VM and have to recreate the environmet each time. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-28 23:49 ` David Sterba @ 2017-11-29 0:44 ` Nick Terrell 2017-12-05 15:54 ` David Sterba 2017-11-29 13:24 ` Austin S. Hemmelgarn 1 sibling, 1 reply; 22+ messages in thread From: Nick Terrell @ 2017-11-29 0:44 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs, Martin Steigerwald, Imran Geriskovan > On Nov 28, 2017, at 3:49 PM, David Sterba <dsterba@suse.cz> wrote: > > On Tue, Nov 28, 2017 at 09:31:57PM +0000, Nick Terrell wrote: >> >>> On Nov 21, 2017, at 8:22 AM, David Sterba <dsterba@suse.cz> wrote: >>> >>> On Wed, Nov 15, 2017 at 08:09:15PM +0000, Nick Terrell wrote: >>>> On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: >>>>> The branch is now in a state that can be tested. Turns out the memory >>>>> requirements are too much for grub, so the boot fails with "not enough >>>>> memory". The calculated value >>>>> >>>>> ZSTD_BTRFS_MAX_INPUT: 131072 >>>>> ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 >>>>> >>>>> This is not something I could fix easily, we'd probalby need a tuned >>>>> version of ZSTD for grub constraints. Adding Nick to CC. >>>> >>>> If I understand the grub code correctly, we only need to read, and we have >>>> the entire input and output buffer in one segment. In that case you can use >>>> ZSTD_initDCtx(), and ZSTD_decompressDCtx(). ZSTD_DCtxWorkspaceBound() is >>>> only 155984. See decompress_single() in >>>> https://patchwork.kernel.org/patch/9997909/ for an example. >>> >>> Does not help, still ENOMEM. >> >> It looks like XZ had the same issue, and they make the decompression >> context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could >> potentially do the same and statically allocate the workspace: >> >> ``` >> /* Could also be size_t */ >> #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t)) >> static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; >> >> /* ... */ >> >> assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); >> ``` > > Interesting, thanks for the tip, I'll try it next. > > I've meanwhile tried to tweak the numbers, the maximum block for zstd, > that squeezed the DCtx somewhere under 48k, with block size 8k. Still > enomem. Sadly the block size has to stay 128 KiB, since that is the block size that is used for compression. > I've tried to add some debugging prints to see what numbers get actually > passed to the allocator, but did not see anything printed. I'm sure > there is a more intelligent way to test the grub changes. So far each > test loop takes quite some time, as I build the rpm package, test it in > a VM and have to recreate the environmet each time. Is the code in github.com/kdave/grub in the btrfs-zstd branch up to date? btrfs.c:1230 looks like it will error for zstd compression, but not with ENOMEM. btrfs.c:1272 [2] looks like a typo. Maybe the second is causing the compressed block to be interpreted as uncompressed, and causes a too-large allocation? [1] https://github.com/kdave/grub/blob/btrfs-zstd/grub-core/fs/btrfs.c#L1230 [2] https://github.com/kdave/grub/blob/btrfs-zstd/grub-core/fs/btrfs.c#L1272 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-29 0:44 ` Nick Terrell @ 2017-12-05 15:54 ` David Sterba 2017-12-05 20:36 ` Nick Terrell 0 siblings, 1 reply; 22+ messages in thread From: David Sterba @ 2017-12-05 15:54 UTC (permalink / raw) To: Nick Terrell; +Cc: dsterba, linux-btrfs, Martin Steigerwald, Imran Geriskovan On Wed, Nov 29, 2017 at 12:44:32AM +0000, Nick Terrell wrote: > >> It looks like XZ had the same issue, and they make the decompression > >> context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could > >> potentially do the same and statically allocate the workspace: > >> > >> ``` > >> /* Could also be size_t */ > >> #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t)) > >> static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; > >> > >> /* ... */ > >> > >> assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); > >> ``` > > > > Interesting, thanks for the tip, I'll try it next. And it worked. > > I've meanwhile tried to tweak the numbers, the maximum block for zstd, > > that squeezed the DCtx somewhere under 48k, with block size 8k. Still > > enomem. > > Sadly the block size has to stay 128 KiB, since that is the block size that > is used for compression. > > > I've tried to add some debugging prints to see what numbers get actually > > passed to the allocator, but did not see anything printed. I'm sure > > there is a more intelligent way to test the grub changes. So far each > > test loop takes quite some time, as I build the rpm package, test it in > > a VM and have to recreate the environmet each time. > > Is the code in github.com/kdave/grub in the btrfs-zstd branch up to date? > btrfs.c:1230 looks like it will error for zstd compression, but not with > ENOMEM. btrfs.c:1272 [2] looks like a typo. Maybe the second is causing > the compressed block to be interpreted as uncompressed, and causes a > too-large allocation? I had a different branch with patches from openSUSE, so the diffs apply with minimal efforts to the package. The branch btrfs-zstd has been synced up. The ENOMEM error was not from the file decompression but from the zstdio.c module, that does something different, now disabled. I got a bit further, with a few debugging messages, this check fails: 957 total_size = grub_le_to_cpu32 (grub_get_unaligned32 (ibuf)); 958 ibuf += sizeof (total_size); 959 960 if (isize < total_size) { 961 grub_error (GRUB_ERR_BAD_FS, "ASSERTION: isize < total_size: %ld < %ld", 962 isize, total_size); 963 return -1; 964 } with: "isize < total_size: 61440 < -47205080" total_size is u32, but wrngly printed as signed long so it's negative, but would be wrong anyway. This looks like the compressed format is not read correctly. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-12-05 15:54 ` David Sterba @ 2017-12-05 20:36 ` Nick Terrell 0 siblings, 0 replies; 22+ messages in thread From: Nick Terrell @ 2017-12-05 20:36 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs, Martin Steigerwald, Imran Geriskovan > On Dec 5, 2017, at 7:54 AM, David Sterba <dsterba@suse.cz> wrote: > > I had a different branch with patches from openSUSE, so the diffs apply with > minimal efforts to the package. The branch btrfs-zstd has been synced up. The > ENOMEM error was not from the file decompression but from the zstdio.c module, > that does something different, now disabled. > > I got a bit further, with a few debugging messages, this check fails: > > 957 total_size = grub_le_to_cpu32 (grub_get_unaligned32 (ibuf)); > 958 ibuf += sizeof (total_size); > 959 > 960 if (isize < total_size) { > 961 grub_error (GRUB_ERR_BAD_FS, "ASSERTION: isize < total_size: %ld < %ld", > 962 isize, total_size); > 963 return -1; > 964 } > > with: "isize < total_size: 61440 < -47205080" > > total_size is u32, but wrngly printed as signed long so it's negative, but > would be wrong anyway. This looks like the compressed format is not read > correctly. The blocks of 4 KB uncompressed data with a u32 containing the compressed size is part of the btrfs lzo format, so zstd doesn't follow that. The compressed data is just zstd, with nothing else. I think I mistook the semantics of grub_btrfs_zstd_decompress() earlier. If I understand the function correctly, grub_btrfs_zstd_decompress() is given a compressed block and is asked to read `osize` bytes at offset `off` of the uncompressed bytes. `osize` may be less than the uncompressed block size if `off > 0`. Is that correct? Zstd compressed data is called a "frame", and the frame is made up of multiple "blocks", each containing up to 128 KB on uncompressed data. Since btrfs currently passes blocks of up to 128 KB to the compressor, each frame will contain only one block. Zstd is only able to decompress at block granularity, so we have to decompress the whole thing. Since `osize` may be less than the uncompressed size of the zstd frame, we will need a secondary buffer to decompress into, and then copy the requested segment into the output buffer. The code would look something like this: ``` #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(grub_uint64_t)) static grub_uint64_t zstd_workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; #define ZSTD_BTRFS_MAX_WINDOWLOG 17 #define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG) static grub_uint8_t zstd_outbuf[ZSTD_BTRFS_MAX_INPUT]; static grub_ssize_t grub_btrfs_zstd_decompress(char *ibuf, grub_size_t isize, grub_off_t off, char *obuf, grub_size_t osize) { void *ubuf; grub_size_t usize; void *const wmem = zstd_workspace; grub_size_t const wsize = sizeof(zstd_workspace); ZSTD_DCtx *ctx; if (wsize < ZSTD_DCtxWorkspaceBound()) { /* Unexpected */ return -1; } ctx = ZSTD_initDCtx(wmem, wsize); if (!ctx) { /* Unexpected */ return -1; } /* Find the end of the zstd frame given a zstd compressed frame possibly * followed by extra data. The returned value will always be an error or * <= isize. * * This is only necessary if there is junk after the end of the zstd * compressed data in the input buffer. Otherwise the return value * should always be exactly isize. If you delete this, and it always works, * then there isn't junk data at the end. */ isize = ZSTD_findFrameCompressedSize(ibuf, isize); if (ZSTD_isError(isize)) { /* Corruption */ return -1; } /* We don't know the original uncompressed size here (osize might be smaller) * so we have to fall back to checking that osize >= ZSTD_BTRFS_MAX_INPUT. * If we knew the size, we could relax the check. * * This is only an optimization, not necessary for correctness. */ if (off == 0 && osize >= ZSTD_BTRFS_MAX_INPUT) { ubuf = obuf; usize = osize; } else { ubuf = zstd_outbuf; usize = sizeof(zstd_outbuf); } usize = ZSTD_decompressDCtx(ctx, ubuf, usize, ibuf, isize); if (ZSTD_isError(usize)) { /* Corruption */ return -1; } /* We need to read osize bytes at offset off. * Check that we have enough data. */ if (usize < off + osize) { /* Corruption */ return -1; } /* If we decompressed directly to the output buffer, off must be zero. */ if (ubuf == obuf) { assert(off == 0); return osize; } assert(ubuf == zstd_outbuf); grub_memcpy(obuf, ubuf + off, osize); return osize; } ``` ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-28 23:49 ` David Sterba 2017-11-29 0:44 ` Nick Terrell @ 2017-11-29 13:24 ` Austin S. Hemmelgarn 2017-11-29 19:29 ` Andrei Borzenkov 1 sibling, 1 reply; 22+ messages in thread From: Austin S. Hemmelgarn @ 2017-11-29 13:24 UTC (permalink / raw) To: dsterba, Nick Terrell, linux-btrfs, Martin Steigerwald, Imran Geriskovan On 2017-11-28 18:49, David Sterba wrote: > On Tue, Nov 28, 2017 at 09:31:57PM +0000, Nick Terrell wrote: >> >>> On Nov 21, 2017, at 8:22 AM, David Sterba <dsterba@suse.cz> wrote: >>> >>> On Wed, Nov 15, 2017 at 08:09:15PM +0000, Nick Terrell wrote: >>>> On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: >>>>> The branch is now in a state that can be tested. Turns out the memory >>>>> requirements are too much for grub, so the boot fails with "not enough >>>>> memory". The calculated value >>>>> >>>>> ZSTD_BTRFS_MAX_INPUT: 131072 >>>>> ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 >>>>> >>>>> This is not something I could fix easily, we'd probalby need a tuned >>>>> version of ZSTD for grub constraints. Adding Nick to CC. >>>> >>>> If I understand the grub code correctly, we only need to read, and we have >>>> the entire input and output buffer in one segment. In that case you can use >>>> ZSTD_initDCtx(), and ZSTD_decompressDCtx(). ZSTD_DCtxWorkspaceBound() is >>>> only 155984. See decompress_single() in >>>> https://patchwork.kernel.org/patch/9997909/ for an example. >>> >>> Does not help, still ENOMEM. >> >> It looks like XZ had the same issue, and they make the decompression >> context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could >> potentially do the same and statically allocate the workspace: >> >> ``` >> /* Could also be size_t */ >> #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t)) >> static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; >> >> /* ... */ >> >> assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); >> ``` > > Interesting, thanks for the tip, I'll try it next. > > I've meanwhile tried to tweak the numbers, the maximum block for zstd, > that squeezed the DCtx somewhere under 48k, with block size 8k. Still > enomem. > > I've tried to add some debugging prints to see what numbers get actually > passed to the allocator, but did not see anything printed. I'm sure > there is a more intelligent way to test the grub changes. So far each > test loop takes quite some time, as I build the rpm package, test it in > a VM and have to recreate the environmet each time. On the note of testing, have you tried writing up a module to just test the decompressor? If so, you could probably use the 'emu' platform to save the need to handle the RPM package and the VM until you get the decompressor working by itself, at which point the FUSE modules used to test the GRUB filesystem modules may be of some use (or you might be able to just use them directly). ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Read before you deploy btrfs + zstd 2017-11-29 13:24 ` Austin S. Hemmelgarn @ 2017-11-29 19:29 ` Andrei Borzenkov 0 siblings, 0 replies; 22+ messages in thread From: Andrei Borzenkov @ 2017-11-29 19:29 UTC (permalink / raw) To: Austin S. Hemmelgarn, dsterba, Nick Terrell, linux-btrfs, Martin Steigerwald, Imran Geriskovan 29.11.2017 16:24, Austin S. Hemmelgarn пишет: > On 2017-11-28 18:49, David Sterba wrote: >> On Tue, Nov 28, 2017 at 09:31:57PM +0000, Nick Terrell wrote: >>> >>>> On Nov 21, 2017, at 8:22 AM, David Sterba <dsterba@suse.cz> wrote: >>>> >>>> On Wed, Nov 15, 2017 at 08:09:15PM +0000, Nick Terrell wrote: >>>>> On 11/15/17, 6:41 AM, "David Sterba" <dsterba@suse.cz> wrote: >>>>>> The branch is now in a state that can be tested. Turns out the memory >>>>>> requirements are too much for grub, so the boot fails with "not >>>>>> enough >>>>>> memory". The calculated value >>>>>> >>>>>> ZSTD_BTRFS_MAX_INPUT: 131072 >>>>>> ZSTD_DStreamWorkspaceBound with ZSTD_BTRFS_MAX_INPUT: 549424 >>>>>> >>>>>> This is not something I could fix easily, we'd probalby need a tuned >>>>>> version of ZSTD for grub constraints. Adding Nick to CC. >>>>> >>>>> If I understand the grub code correctly, we only need to read, and >>>>> we have >>>>> the entire input and output buffer in one segment. In that case you >>>>> can use >>>>> ZSTD_initDCtx(), and ZSTD_decompressDCtx(). >>>>> ZSTD_DCtxWorkspaceBound() is >>>>> only 155984. See decompress_single() in >>>>> https://patchwork.kernel.org/patch/9997909/ for an example. >>>> >>>> Does not help, still ENOMEM. >>> >>> It looks like XZ had the same issue, and they make the decompression >>> context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could >>> potentially do the same and statically allocate the workspace: >>> >>> ``` >>> /* Could also be size_t */ >>> #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t)) >>> static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64]; >>> >>> /* ... */ >>> >>> assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); >>> ``` >> >> Interesting, thanks for the tip, I'll try it next. >> >> I've meanwhile tried to tweak the numbers, the maximum block for zstd, >> that squeezed the DCtx somewhere under 48k, with block size 8k. Still >> enomem. >> >> I've tried to add some debugging prints to see what numbers get actually >> passed to the allocator, but did not see anything printed. I'm sure >> there is a more intelligent way to test the grub changes. So far each >> test loop takes quite some time, as I build the rpm package, test it in >> a VM and have to recreate the environmet each time. > On the note of testing, have you tried writing up a module to just test > the decompressor? If so, you could probably use the 'emu' platform to > save the need to handle the RPM package and the VM until you get the > decompressor working by itself, at which point the FUSE modules used to > test the GRUB filesystem modules may be of some use (or you might be > able to just use them directly). There is also grub-fstest which directly calls filesystem drivers; usage is something like "grub-fstest /dev/sdb1 cat /foo". Replace /dev/sdb1 with any btrfs image. As this is user space it is easy to single step if needed. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-12-05 20:36 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-13 22:50 Read before you deploy btrfs + zstd David Sterba 2017-11-14 7:34 ` Martin Steigerwald 2017-11-14 9:45 ` Paul Jones 2017-11-14 12:38 ` Austin S. Hemmelgarn 2017-11-15 20:23 ` Duncan 2017-11-16 14:31 ` Dmitrii Tcvetkov 2017-11-14 18:49 ` David Sterba 2017-11-14 19:43 ` Martin Steigerwald 2017-11-14 18:53 ` David Sterba 2017-11-15 14:39 ` David Sterba 2017-11-15 16:22 ` Martin Steigerwald 2017-11-15 18:16 ` Imran Geriskovan 2017-11-15 20:06 ` Duncan 2017-11-15 20:09 ` Nick Terrell 2017-11-21 16:22 ` David Sterba 2017-11-28 21:31 ` Nick Terrell 2017-11-28 23:49 ` David Sterba 2017-11-29 0:44 ` Nick Terrell 2017-12-05 15:54 ` David Sterba 2017-12-05 20:36 ` Nick Terrell 2017-11-29 13:24 ` Austin S. Hemmelgarn 2017-11-29 19:29 ` Andrei Borzenkov
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.