Read before you deploy btrfs + zstd
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 zstdThere might be other workarounds but I want to keep the advice simple.
d.
Hello David.
David Sterba - 13.11.17, 23:50:
Quoted 10 lines
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,
Signature · 1 line
Martin
Quoted 17 lines
-----Original Message-----
From: [email protected] [mailto:linux-btrfs- [email protected]] On Behalf Of Martin Steigerwald
Sent: Tuesday, 14 November 2017 6:35 PM
To: [email protected]; [email protected]
Subject: Re: Read before you deploy btrfs + zstdHello David.
David Sterba - 13.11.17, 23:50:
Quoted 7 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 10 lines · Martin Steigerwald
Hello David.
David Sterba - 13.11.17, 23:50:
Quoted 7 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Austin S. Hemmelgarn posted on Tue, 14 Nov 2017 07:38:03 -0500 as excerpted:
Quoted 15 lines · Martin Steigerwald
Quoted 14 lines
Hello David.
David Sterba - 13.11.17, 23:50:
Quoted 10 lines
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.
Quoted 5 lines
Quoted 1 line
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. =:^)
Signature · 3 lines
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
Quoted 14 lines · On Wed, 15 Nov 2017 20:23:44 +0000 (UTC) Duncan <[email protected]> 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.)
Quoted 4 lines
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.
Quoted 10 lines · Martin Steigerwald
Hello David.
David Sterba - 13.11.17, 23:50:
Quoted 7 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
David Sterba - 14.11.17, 19:49:
Quoted 18 lines · Martin Steigerwald
Quoted 17 lines
Hello David.
David Sterba - 13.11.17, 23:50:
Quoted 13 lines
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.
Quoted 3 lines
Quoted 1 line
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,
Signature · 1 line
Martin
Quoted 1 line · David Sterba
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.
Quoted 10 lines · David Sterba
Quoted 1 line · David Sterba
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.
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.
David Sterba - 15.11.17, 15:39:
Quoted 16 lines · David Sterba
Quoted 10 lines · David Sterba
Quoted 1 line
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.
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
Quoted 5 lines
ZSTD_BTRFS_MAX_INPUT: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This 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,
Signature · 1 line
Martin
Quoted 1 line · Martin Steigerwald
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.
Imran Geriskovan posted on Wed, 15 Nov 2017 20:16:56 +0200 as excerpted:
Quoted 5 lines · Martin Steigerwald
Quoted 1 line
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.
Signature · 3 lines
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
Quoted 9 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This 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�+�������+%�����ݶ��w��{.n�+����{���k~���^n�r���z���h����&���z��z�ޗ�+��+zf���h���~����i��������z_��j:+v���)ߣ�m
Quoted 8 lines · Nick Terrell
Quoted 7 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424
Quoted 8 lines
Quoted 2 lines
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(), andZSTD_decompressDCtx().ZSTD_DCtxWorkspaceBound()is only 155984. Seedecompress_single()in
https://patchwork.kernel.org/patch/9997909/ for an example.
Does not help, still ENOMEM.
Quoted 15 lines · David Sterba
Quoted 11 lines · Nick Terrell
Quoted 9 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This is not something I could fix easily, we'd probalby need a tuned version of ZSTD for grub constraints. Adding Nick to CC.
Quoted 7 lines
Quoted 5 lines
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(), andZSTD_decompressDCtx().ZSTD_DCtxWorkspaceBound()is only 155984. Seedecompress_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()); ```
Quoted 27 lines · Nick Terrell
Quoted 15 lines · David Sterba
Quoted 11 lines · Nick Terrell
Quoted 9 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This is not something I could fix easily, we'd probalby need a tuned version of ZSTD for grub constraints. Adding Nick to CC.
Quoted 7 lines
Quoted 5 lines
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(), andZSTD_decompressDCtx().ZSTD_DCtxWorkspaceBound()is only 155984. Seedecompress_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:
Quoted 9 lines
``` /* 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.
Quoted 38 lines · David Sterba
Quoted 34 lines · Nick Terrell
Quoted 15 lines · David Sterba
Quoted 11 lines · Nick Terrell
Quoted 9 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This is not something I could fix easily, we'd probalby need a tuned version of ZSTD for grub constraints. Adding Nick to CC.
Quoted 7 lines
Quoted 5 lines
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(), andZSTD_decompressDCtx().ZSTD_DCtxWorkspaceBound()is only 155984. Seedecompress_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 besize_t*/
#defineBTRFS_ZSTD_WORKSPACE_SIZE_U64(155984 /sizeof(uint64_t)) staticuint64_tworkspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64];/* ... */
Quoted 8 lines
Quoted 2 lines
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.
Quoted 5 lines
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
Quoted 9 lines · Nick Terrell
Quoted 9 lines
Quoted 9 lines
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 besize_t*/
#defineBTRFS_ZSTD_WORKSPACE_SIZE_U64(155984 /sizeof(uint64_t)) staticuint64_tworkspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64];
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
/* ... */
assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound()); ```Interesting, thanks for the tip, I'll try it next.
And it worked.
Quoted 13 lines
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 19 lines · David Sterba
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.cmodule, 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) {
961grub_error(GRUB_ERR_BAD_FS, "ASSERTION: isize <total_size: %ld < %ld",
962 isize,total_size);
963 return -1;
964 }
Quoted 5 lines
with: "isize <
total_size: 61440 < -47205080"
total_sizeis 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;
}
```Quoted 44 lines · David Sterba
Quoted 27 lines · Nick Terrell
Quoted 15 lines · David Sterba
Quoted 11 lines · Nick Terrell
Quoted 9 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This is not something I could fix easily, we'd probalby need a tuned version of ZSTD for grub constraints. Adding Nick to CC.
Quoted 7 lines
Quoted 5 lines
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(), andZSTD_decompressDCtx().ZSTD_DCtxWorkspaceBound()is only 155984. Seedecompress_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:Quoted 9 lines
``` /* 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.
Quoted 5 lines
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).
29.11.2017 16:24, Austin S. Hemmelgarn пишет:
Quoted 54 lines · David Sterba
Quoted 48 lines · Nick Terrell
Quoted 31 lines
Quoted 25 lines · David Sterba
Quoted 12 lines · Nick Terrell
Quoted 10 lines · David Sterba
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: 131072ZSTD_DStreamWorkspaceBoundwithZSTD_BTRFS_MAX_INPUT: 549424This is not something I could fix easily, we'd probalby need a tuned version of ZSTD for grub constraints. Adding Nick to CC.
Quoted 8 lines
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 useZSTD_initDCtx(), andZSTD_decompressDCtx().ZSTD_DCtxWorkspaceBound()is
only 155984. Seedecompress_single()in
https://patchwork.kernel.org/patch/9997909/ for an example.Quoted 1 line
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:Quoted 9 lines
``` /* 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.
Quoted 5 lines
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.
Quoted 6 lines
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.