Received-SPF: pass (google.com: domain of linux-kernel+bounces-68317-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240216030539.110404-1-21cnbao@gmail.com> <Zc8bjZFZAZneObQG@google.com>
 <b717ae0c-ed4f-4ebc-90cf-51a8da0a6583@bytedance.com>
In-Reply-To: <b717ae0c-ed4f-4ebc-90cf-51a8da0a6583@bytedance.com>
From: Barry Song <21cnbao@gmail.com>
Date: Fri, 16 Feb 2024 17:16:50 +0800
Message-ID: <CAGsJ_4zSDP_A32jUPrYPwZ=uwU4o0a6x-GW2HO3vu8yUh0qYjA@mail.gmail.com>
Subject: Re: [PATCH] mm: zswap: increase reject_compress_poor but not
 reject_compress_fail if compression returns ENOSPC
To: Chengming Zhou <zhouchengming@bytedance.com>, Yosry Ahmed <yosryahmed@google.com>
Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, Barry Song <v-songbaohua@oppo.com>, 
	Nhat Pham <nphamcs@gmail.com>, Sergey Senozhatsky <senozhatsky@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 16, 2024 at 5:07=E2=80=AFPM Chengming Zhou
<zhouchengming@bytedance.com> wrote:
>
> On 2024/2/16 16:23, Yosry Ahmed wrote:
> > On Fri, Feb 16, 2024 at 04:05:39PM +1300, Barry Song wrote:
> >> From: Barry Song <v-songbaohua@oppo.com>
> >>
> >> My commit fc8580edbaa6 ("mm: zsmalloc: return -ENOSPC rather than -EIN=
VAL
> >> in zs_malloc while size is too large") wanted to depend on zs_malloc's
> >> returned ENOSPC to distinguish the case that compressed data is larger
> >> than the original data from normal compression cases. The commit, for
> >> sure, was correct and worked as expected but the code wouldn't run to
> >> there after commit 744e1885922a ("crypto: scomp - fix req->dst buffer
> >> overflow") as Chengming's this patch makes zswap_store() goto out
> >> immediately after the special compression case happens. So there is
> >> no chance to execute zs_malloc() now. We need to fix the count right
> >> after compressions return ENOSPC.
> >>
> >> Fixes: fc8580edbaa6 ("mm: zsmalloc: return -ENOSPC rather than -EINVAL=
 in zs_malloc while size is too large")
> >
> > I don't see how this is a fix for that commit. Commit fc8580edbaa6 made
> > sure zsmalloc returns a correct errno when the compressed size is too
> > large. The fact that zswap stores were failing before calling into
> > zsmalloc and not reporting the error correctly in debug counters is not
> > that commits fault.
> >

Hi Yosry, Chengming,

Thanks for your quick responses.

> > I think the proper fixes should be 744e1885922a if it introduced the
> > first scenario where -ENOSPC can be returned from scomp without handlin=
g
> > it properly in zswap. If -ENOSPC was a possible return value before
> > that, then it should be cb61dad80fdc ("zswap: export compression failur=
e
> > stats"), where the counter was introduced.
>
> Right, 744e1885922a maybe a better fixes target.

I agree 744e1885922a is a better fixes target.

>
> >
> >> Cc: Chengming Zhou <zhouchengming@bytedance.com>
> >> Cc: Nhat Pham <nphamcs@gmail.com>
> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> >> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >> ---
> >>  mm/zswap.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/zswap.c b/mm/zswap.c
> >> index 6319d2281020..9a21dbe8c056 100644
> >> --- a/mm/zswap.c
> >> +++ b/mm/zswap.c
> >> @@ -1627,7 +1627,10 @@ bool zswap_store(struct folio *folio)
> >>      dlen =3D acomp_ctx->req->dlen;
> >>
> >>      if (ret) {
> >> -            zswap_reject_compress_fail++;
> >> +            if (ret =3D=3D -ENOSPC)
> >> +                    zswap_reject_compress_poor++;
> >> +            else
> >> +                    zswap_reject_compress_fail++;
> >
> > With this diff, we have four locations in zswap_store() where we
> > increment zswap_reject_compress_{poor/fail}.
> >
> > How about the following instead?A
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 62fe307521c93..3a7e8ba7f6116 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1059,24 +1059,16 @@ static bool zswap_compress(struct folio *folio,=
 struct zswap_entry *entry)
> >        */
> >       ret =3D crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &a=
comp_ctx->wait);
> >       dlen =3D acomp_ctx->req->dlen;
> > -     if (ret) {
> > -             zswap_reject_compress_fail++;
> > +     if (ret)
> >               goto unlock;
> > -     }
> >
> >       zpool =3D zswap_find_zpool(entry);
> >       gfp =3D __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> >       if (zpool_malloc_support_movable(zpool))
> >               gfp |=3D __GFP_HIGHMEM | __GFP_MOVABLE;
> >       ret =3D zpool_malloc(zpool, dlen, gfp, &handle);
> > -     if (ret =3D=3D -ENOSPC) {
> > -             zswap_reject_compress_poor++;
> > -             goto unlock;
> > -     }
> > -     if (ret) {
> > -             zswap_reject_alloc_fail++;
> > +     if (ret)
> >               goto unlock;
> > -     }
> >
> >       buf =3D zpool_map_handle(zpool, handle, ZPOOL_MM_WO);
> >       memcpy(buf, dst, dlen);
> > @@ -1086,6 +1078,10 @@ static bool zswap_compress(struct folio *folio, =
struct zswap_entry *entry)
> >       entry->length =3D dlen;
> >
> >  unlock:
> > +     if (ret =3D=3D -ENOSPC)
> > +             zswap_reject_compress_poor++;
> > +     else if (ret)
> > +             zswap_reject_alloc_fail++;
>
> Here have two cases: zswap_reject_compress_fail, zswap_reject_alloc_fail.

Is it safe to differentiate these two cases by checking ret =3D=3D -ENOMEM =
?
otherwise, it seems the original patch still makes more sense?

>
> >       mutex_unlock(&acomp_ctx->mutex);
> >       return ret =3D=3D 0;
> >  }
> >
> >>              goto put_dstmem;
> >>      }
> >>
> >> --
> >> 2.34.1
> >>

Thanks
Barry