> On Nov 27, 2023, at 6:18 PM, Kent Overstreet <[email protected]>
> wrote:
>
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
>
> |-------------------------------------------------------------------!
>
> On Mon, Nov 27, 2023 at 07:52:04PM +0000, Nick Terrell wrote:
>>
>>
>>> On Nov 22, 2023, at 5:42 PM, Kent Overstreet <[email protected]>
>>> wrote:
>>>
>>> !-------------------------------------------------------------------|
>>> This Message Is From an External Sender
>>>
>>> |-------------------------------------------------------------------!
>>>
>>> I just got a report of data being marked as incompressible when written
>>> with particular blocksizes.
>>
>> Is this on linux-next? If not, which revision?
>>
>>> Investigating a bit further, when attempting to compress >= 32k (of all
>>> zeroes), I get:
>>> [ 8.214090] bcachefs (vdb): zstd error: (efault) -64, src_len 32768
>>> dst_len 32768
>>> [ 8.214713] bcachefs (vdb): zstd error: (efault) -64, src_len 65536
>>> dst_len 65536
>>> [ 8.215349] bcachefs (vdb): zstd error: (efault) -64, src_len 65536
>>> dst_len 65536
>>> [ 8.215432] bcachefs (vdb): zstd error: (efault) -64, src_len 65536
>>> dst_len 65536
>>>
>>> bcachefs code that calls zstd:
>>> https://evilpiepirate.org/git/bcachefs.git/tree/fs/bcachefs/compress.c?h=bcachefs-testing#n350
>>>
>>
>> I’ll look into this tomorrow when I get back to my computer.
>>
>>> -64 appears to be ZSTD_error_memory_allocation, but I get no hits for
>>> that grepping, so I'm a bit stuck.
>
> It appears that the workspace size given by zstd_cctx_workspace_bound()
> is wrong - if I double the workspace size it works.
>
> I traced it to the workspaceTooSmall check in zstd_compress.c.
Hi Kent,
It looks like there was a bug in how bcachefs called zstd that triggered this
error.
I’m looking at the code at the tag v6.7-rc3.
In the function __bch2_fs_compress_init I see this [0]:
ZSTD_parameters params = zstd_get_params(zstd_max_clevel(),
c->opts.encoded_extent_max);
// …
c->zstd_params = params;
Then later in attempt_compress I see this [1]:
unsigned level = min((compression.level * 3) / 2, zstd_max_clevel());
ZSTD_parameters params = zstd_get_params(level,
c->opts.encoded_extent_max);
ZSTD_CCtx *ctx = zstd_init_cctx(workspace,
zstd_cctx_workspace_bound(¶ms.cParams));
size_t len = zstd_compress_cctx(ctx, dst + 4, dst_len - 4 - 7, src,
src_len, &c->zstd_params);
The workspace is initialized correctly, in that it is sized for the maximum
possible level.
However, zstd_init_cctx() is called with a workspace that is sized using the
level chosen by
compression.level, so It only believes that it has that much memory available.
Then
zstd_compress_cctx is called with &c->zstd_params, which are the params for the
maximum
level, which needs more memory than Zstd was told is available.
The fix is to call zstd_compress_cctx with ¶ms instead of &c->zstd_params.
You shouldn’t need
to store the zstd_params in the bch_fs struct after that.
Best,
Nick Terrell
[0]
https://github.com/torvalds/linux/blob/v6.7-rc3/fs/bcachefs/compress.c#L573-L593
[1]
https://github.com/torvalds/linux/blob/v6.7-rc3/fs/bcachefs/compress.c#L350-L374
