Read before you deploy btrfs + zstd

Read before you deploy btrfs + zstd

[David Sterba]

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.

Re: Read before you deploy btrfs + zstd

[Martin Steigerwald]

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

RE: Read before you deploy btrfs + zstd

[Paul Jones]

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

Re: Read before you deploy btrfs + zstd

[Austin S. Hemmelgarn]

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.

Re: Read before you deploy btrfs + zstd

[David Sterba]

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.

Re: Read before you deploy btrfs + zstd

[David Sterba]

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

Re: Read before you deploy btrfs + zstd

[Martin Steigerwald]

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

Re: Read before you deploy btrfs + zstd

[David Sterba]

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.

Re: Read before you deploy btrfs + zstd

[Martin Steigerwald]

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

Re: Read before you deploy btrfs + zstd

[Imran Geriskovan]

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.

Re: Read before you deploy btrfs + zstd

[Duncan]

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

Re: Read before you deploy btrfs + zstd

[Nick Terrell]

[-- 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~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: Read before you deploy btrfs + zstd

[Duncan]

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

Re: Read before you deploy btrfs + zstd

[Dmitrii Tcvetkov]

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.

Re: Read before you deploy btrfs + zstd

[David Sterba]

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.

Re: Read before you deploy btrfs + zstd

[Nick Terrell]

> 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());
```

Re: Read before you deploy btrfs + zstd

[David Sterba]

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.

Re: Read before you deploy btrfs + zstd

[Nick Terrell]

> 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

Re: Read before you deploy btrfs + zstd

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

Re: Read before you deploy btrfs + zstd

[Andrei Borzenkov]

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.

Re: Read before you deploy btrfs + zstd

[David Sterba]

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.

Re: Read before you deploy btrfs + zstd

[Nick Terrell]

> 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;
}
```