Precedence: list
MIME-Version: 1.0
References: <20261021205626.3286270-1-suseendranb@google.com>
 <20261021205626.3286270-3-suseendranb@google.com> <CAO9r8zNtuN=_BTV53oHqaTqE0Bo2qiDSrgTBzEZo5oyOnWyeYA@mail.gmail.com>
 <20261022155144.4177200-1-suseendranb@google.com> <CAO9r8zP+GUE-hcGWSGzhLNLcchFoZ38EMVCAtX5yZ78tZ0bbgA@mail.gmail.com>
 <20261022183319.163484-1-suseendranb@google.com>
In-Reply-To: <20261022183319.163484-1-suseendranb@google.com>
From: Yosry Ahmed <yosry@kernel.org>
Date: Thu, 22 Oct 2026 12:15:51 -0700
Message-ID: <CAO9r8zN6hN4h5m9pg4xQpirJj3o2nhGx5dxAuSddZ8+UenYnMg@mail.gmail.com>
Subject: Re: [PATCH 2/2] mm/zswap: classify compression dst overflow as poor compression
To: Suseendran Bhaskaran <suseendranb@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, Herbert Xu <herbert@gondor.apana.org.au>, 
	"David S . Miller" <davem@davemloft.net>, Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Nick Terrell <terrelln@fb.com>, 
	David Sterba <dsterba@suse.com>, SJ Park <sj@kernel.org>, Chris Li <chrisl@kernel.org>, 
	Giovanni Cabiddu <giovanni.cabiddu@intel.com>, 
	Suman Kumar Chakraborty <suman.kumar.chakraborty@intel.com>, linux-crypto@vger.kernel.org, 
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Oct 22, 2026 at 11:33=E2=80=AFAM Suseendran Bhaskaran
<suseendranb@google.com> 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@google.com> wrote:
> [...]
> > > With that unchanged, existing zswap_compress() already accounts -ENOS=
PC
> > > vs other error codes when writeback is disabled, except for the
> > > !comp_ret && dlen >=3D 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 <=3D PAGE_SIZE. The 'dlen >=3D 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 =3D=3D 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 !=3D 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 =3D 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 =3D comp_ret ? comp_ret : -EINVAL;
> > > +                       comp_ret =3D comp_ret ? comp_ret :
> > > +                                  (dlen >=3D 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, struc=
t zswap_entry *entry,
>          */
>         comp_ret =3D crypto_wait_req(crypto_acomp_compress(acomp_ctx->req=
), &acomp_ctx->wait);
>         dlen =3D acomp_ctx->req->dlen;
> +       if (!comp_ret) {
> +               if (dlen =3D=3D PAGE_SIZE)
> +                       comp_ret =3D -ENOSPC;
> +               else if (WARN_ON_ONCE(!dlen || dlen > PAGE_SIZE))
> +                       comp_ret =3D -EINVAL;
> +       }
>
>         /*
>          * If a page cannot be compressed into a size smaller than PAGE_S=
IZE,
>          * save the content as is without a compression, to keep the LRU =
order
>          * of writebacks.  If writeback is disabled, reject the page sinc=
e it
>          * only adds metadata overhead.  swap_writeout() will put the pag=
e back
>          * to the active LRU list in the case.
>          */
> -       if (comp_ret || !dlen || dlen >=3D PAGE_SIZE) {
> +       if (comp_ret) {
>                 rcu_read_lock();
>                 if (!mem_cgroup_zswap_writeback_enabled(
>                                         folio_memcg(page_folio(page)))) {
>                         rcu_read_unlock();
> -                       comp_ret =3D 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?