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>
In-Reply-To: <20261022155144.4177200-1-suseendranb@google.com>
From: Yosry Ahmed <yosry@kernel.org>
Date: Thu, 22 Oct 2026 09:01:02 -0700
Message-ID: <CAO9r8zP+GUE-hcGWSGzhLNLcchFoZ38EMVCAtX5yZ78tZ0bbgA@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 8:51=E2=80=AFAM Suseendran Bhaskaran
<suseendranb@google.com> 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@google.com> wrote:
> > >
> > > 1. When zswap writeback is disabled (e.g. memory.zswap.writeback =3D =
0),
> > >    zswap_compress() sets 'comp_ret =3D comp_ret ? comp_ret : -EINVAL'=
 and
> > >    jumps to unlock. Whether the compressor fails on destination overf=
low
> > >    with -EINVAL or succeeds with dlen =3D=3D PAGE_SIZE, comp_ret beco=
mes
> > >    -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 >=3D 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 pag=
e:
> > >    comp_ret is cleared to 0 and the raw page is stored uncompressed i=
n
> > >    zsmalloc. This silently masks genuine compressor failures
> > >    (reject_compress_fail is never incremented) and miscounts pages th=
at
> > >    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 >=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.

As for the (!comp_ret && dlen =3D=3D 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 =3D comp_ret ? comp_ret : -EINVAL;
> +                       comp_ret =3D comp_ret ? comp_ret :
> +                                  (dlen >=3D PAGE_SIZE ? -ENOSPC : -EINV=
AL);
>
> 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.