On Tue, Nov 28, 2023 at 09:36:56PM +0000, Nick Terrell wrote: > > > > 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.
You're right - the workspace sizes were swapped. Sorry for the fake bug report :)
