Re: [PATCH 2/2] mm/zswap: classify compression dst overflow as poor compression

[Yosry Ahmed]

On Thu, Oct 22, 2026 at 8:51 AM Suseendran Bhaskaran
<suseendranb@xxxxxxxxxx> wrote:
>
> On Wed, Oct 21, 2026 at 02:47:46PM -0700, Yosry Ahmed wrote:
> > On Wed, Oct 21, 2026 at 1:57 PM Suseendran Bhaskaran
> > <suseendranb@xxxxxxxxxx> wrote:
> > >
> > > 1. When zswap writeback is disabled (e.g. memory.zswap.writeback = 0),
> > >    zswap_compress() sets 'comp_ret = comp_ret ? comp_ret : -EINVAL' and
> > >    jumps to unlock. Whether the compressor fails on destination overflow
> > >    with -EINVAL or succeeds with dlen == PAGE_SIZE, comp_ret becomes
> > >    -EINVAL. Consequently, incompressible pages are misaccounted in
> > >    debugfs under reject_compress_fail instead of reject_compress_poor
> > >    (leaving reject_compress_poor stuck at 0 and triggering false
> > >    compressor failure alerts on normal incompressible workloads).
> >
> > Hmm IIUC this is completely fixed by patch 1, so it doesn't belong here, right?
>
> You are right, thank you for pointing this out. For the destination
> buffer overflow case, Patch 1 alone fixes it since comp_ret is already
> -ENOSPC and 'comp_ret ? comp_ret : -EINVAL' preserves it. The only case
> not covered by Patch 1 when writeback is disabled is when compression
> succeeds (!comp_ret) with dlen >= PAGE_SIZE, where 'comp_ret ? comp_ret :
> -EINVAL' sets comp_ret to -EINVAL instead of -ENOSPC.
>
> > > 2. When zswap writeback is enabled, any non-zero comp_ret (such as
> > >    -EINVAL from an invalid compressor context, -ENOMEM, or a hardware
> > >    accelerator error) is treated identically to an incompressible page:
> > >    comp_ret is cleared to 0 and the raw page is stored uncompressed in
> > >    zsmalloc. This silently masks genuine compressor failures
> > >    (reject_compress_fail is never incremented) and miscounts pages that
> > >    encountered a compressor error under stored_incompressible_pages.
> >
> > Storing pages in zswap when compression fails to avoid LRU inversion
> > was intentional in commit dca4437a5861 ("mm/zswap: store <PAGE_SIZE
> > compression failed page as-is"). Unless we think this needs to be
> > changed, I think all we really need to do is make sure we account
> > debugfs correctly for -ENOSPC vs other error codes? Or am I missing
> > something?
>
> Thank you for the context. Keeping the writeback-enabled behavior from
> commit dca4437a5861 ("mm/zswap: store <PAGE_SIZE compression failed page
> as-is") as-is makes sense.
>
> With that unchanged, existing zswap_compress() already accounts -ENOSPC
> vs other error codes when writeback is disabled, except for the
> !comp_ret && dlen >= PAGE_SIZE case mentioned above:

Is this actually possible? When we changed the buffer size to
PAGE_SIZE, I thought we made sure all compressors wouldn't write
beyond the buffer size. dlen > PAGE_SIZE means buffer overflow, which
is a bigger problem than debugfs stats. Or maybe compressors can set
dlen > PAGE_SIZE but won't actually write to the buffer? I didn't
check.

As for the (!comp_ret && dlen == PAGE_SIZE) case specifically, yeah we
should probably use -ENOSPC to count it as poor compression.

The compt_ret and dlen checks are not super intuitive tbh, I wonder
how they should relate. What is the expected value of dlen if comp_ret
is an error? Is it always 0, or can the compressor set it as a
non-zero value if compression fails?

>
> -                       comp_ret = comp_ret ? comp_ret : -EINVAL;
> +                       comp_ret = comp_ret ? comp_ret :
> +                                  (dlen >= PAGE_SIZE ? -ENOSPC : -EINVAL);
>
> Would you prefer that I narrow Patch 2 to just that change in v2, or drop
> Patch 2 and only send Patch 1?

Let's figure out what needs to be done for patch 2 first, if anything.