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

[Yosry Ahmed]

On Thu, Oct 22, 2026 at 11:33 AM Suseendran Bhaskaran
<suseendranb@xxxxxxxxxx> wrote:
>
> On Thu, Oct 22, 2026 at 09:01:02AM -0700, Yosry Ahmed wrote:
> > On Thu, Oct 22, 2026 at 8:51 AM Suseendran Bhaskaran
> > <suseendranb@xxxxxxxxxx> wrote:
> [...]
> > > 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.
>
> dlen > PAGE_SIZE isn't possible, since all compressors bound their
> output by the input req->dlen (PAGE_SIZE) as you mentioned, and only
> set req->dlen <= PAGE_SIZE. The 'dlen >= PAGE_SIZE' check in commit
> dca4437a5861 ("mm/zswap: store <PAGE_SIZE compression failed page
> as-is") was written when the buffer was still 2 * PAGE_SIZE prior to
> commit 0b1bf60c324a ("mm/zswap: reduce the size of the compression
> buffer to a single page").
>
> > As for the (!comp_ret && dlen == PAGE_SIZE) case specifically, yeah we
> > should probably use -ENOSPC to count it as poor compression.
>
> Ack.
>
> > 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?
>
> On error (comp_ret != 0), dlen is not guaranteed to be 0. The value
> left in dlen depends on the compressor used. For example:
> - zstd, 842, and qat set dlen = 0 on error.
> - lz4, lz4hc, lzo, lzo-rle (via scomp_acomp_comp_decomp()), deflate,
>   and iaa return the error code without modifying dlen, so dlen
>   remains at its input value (PAGE_SIZE).
> - hisi_zip sets dlen to the hardware-reported produced bytes even
>   on error.
>
> So dlen is only meaningful when !comp_ret. The !dlen check was
> added in commit dca4437a5861 ("mm/zswap: store <PAGE_SIZE
> compression failed page as-is") as a defensive check for the case when
> comp_ret is 0 with 0 dlen which shouldn't happen unless the compressor
> is faulty.
>
> > >
> > > -                       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.
>
> Since dlen is only valid when !comp_ret, we could normalize comp_ret
> first when !comp_ret and then just check 'if (comp_ret)':
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -874,19 +874,24 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>          */
>         comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
>         dlen = acomp_ctx->req->dlen;
> +       if (!comp_ret) {
> +               if (dlen == PAGE_SIZE)
> +                       comp_ret = -ENOSPC;
> +               else if (WARN_ON_ONCE(!dlen || dlen > PAGE_SIZE))
> +                       comp_ret = -EINVAL;
> +       }
>
>         /*
>          * If a page cannot be compressed into a size smaller than PAGE_SIZE,
>          * save the content as is without a compression, to keep the LRU order
>          * of writebacks.  If writeback is disabled, reject the page since it
>          * only adds metadata overhead.  swap_writeout() will put the page back
>          * to the active LRU list in the case.
>          */
> -       if (comp_ret || !dlen || dlen >= PAGE_SIZE) {
> +       if (comp_ret) {
>                 rcu_read_lock();
>                 if (!mem_cgroup_zswap_writeback_enabled(
>                                         folio_memcg(page_folio(page)))) {
>                         rcu_read_unlock();
> -                       comp_ret = comp_ret ? comp_ret : -EINVAL;
>                         goto unlock;
>                 }
>
> Does this look good to you?

Yes. I assume this is completely independent from patch 1, so we can
tell Herbert to pick up patch 1 and send this separately?