[PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

My commit fc8580edbaa6 ("mm: zsmalloc: return -ENOSPC rather than -EINVAL
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")
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 = acomp_ctx->req->dlen;
 
 	if (ret) {
-		zswap_reject_compress_fail++;
+		if (ret == -ENOSPC)
+			zswap_reject_compress_poor++;
+		else
+			zswap_reject_compress_fail++;
 		goto put_dstmem;
 	}
 
-- 
2.34.1

Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[Yosry Ahmed]

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 -EINVAL
> 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.

I think the proper fixes should be 744e1885922a if it introduced the
first scenario where -ENOSPC can be returned from scomp without handling
it properly in zswap. If -ENOSPC was a possible return value before
that, then it should be cb61dad80fdc ("zswap: export compression failure
stats"), where the counter was introduced.

> 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 = acomp_ctx->req->dlen;
>  
>  	if (ret) {
> -		zswap_reject_compress_fail++;
> +		if (ret == -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 = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
 	dlen = acomp_ctx->req->dlen;
-	if (ret) {
-		zswap_reject_compress_fail++;
+	if (ret)
 		goto unlock;
-	}

 	zpool = zswap_find_zpool(entry);
 	gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
 	if (zpool_malloc_support_movable(zpool))
 		gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
 	ret = zpool_malloc(zpool, dlen, gfp, &handle);
-	if (ret == -ENOSPC) {
-		zswap_reject_compress_poor++;
-		goto unlock;
-	}
-	if (ret) {
-		zswap_reject_alloc_fail++;
+	if (ret)
 		goto unlock;
-	}

 	buf = 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 = dlen;

 unlock:
+	if (ret == -ENOSPC)
+		zswap_reject_compress_poor++;
+	else if (ret)
+		zswap_reject_alloc_fail++;
 	mutex_unlock(&acomp_ctx->mutex);
 	return ret == 0;
 }

>  		goto put_dstmem;
>  	}
>  
> -- 
> 2.34.1
> 

Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[Chengming Zhou]

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 -EINVAL
>> 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.
> 
> I think the proper fixes should be 744e1885922a if it introduced the
> first scenario where -ENOSPC can be returned from scomp without handling
> it properly in zswap. If -ENOSPC was a possible return value before
> that, then it should be cb61dad80fdc ("zswap: export compression failure
> stats"), where the counter was introduced.

Right, 744e1885922a maybe 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 = acomp_ctx->req->dlen;
>>  
>>  	if (ret) {
>> -		zswap_reject_compress_fail++;
>> +		if (ret == -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 = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
>  	dlen = acomp_ctx->req->dlen;
> -	if (ret) {
> -		zswap_reject_compress_fail++;
> +	if (ret)
>  		goto unlock;
> -	}
> 
>  	zpool = zswap_find_zpool(entry);
>  	gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
>  	if (zpool_malloc_support_movable(zpool))
>  		gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
>  	ret = zpool_malloc(zpool, dlen, gfp, &handle);
> -	if (ret == -ENOSPC) {
> -		zswap_reject_compress_poor++;
> -		goto unlock;
> -	}
> -	if (ret) {
> -		zswap_reject_alloc_fail++;
> +	if (ret)
>  		goto unlock;
> -	}
> 
>  	buf = 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 = dlen;
> 
>  unlock:
> +	if (ret == -ENOSPC)
> +		zswap_reject_compress_poor++;
> +	else if (ret)
> +		zswap_reject_alloc_fail++;

Here have two cases: zswap_reject_compress_fail, zswap_reject_alloc_fail.

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

Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[Barry Song]

On Fri, Feb 16, 2024 at 5:07 PM 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 -EINVAL
> >> 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 handling
> > it properly in zswap. If -ENOSPC was a possible return value before
> > that, then it should be cb61dad80fdc ("zswap: export compression failure
> > 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 = acomp_ctx->req->dlen;
> >>
> >>      if (ret) {
> >> -            zswap_reject_compress_fail++;
> >> +            if (ret == -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 = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
> >       dlen = acomp_ctx->req->dlen;
> > -     if (ret) {
> > -             zswap_reject_compress_fail++;
> > +     if (ret)
> >               goto unlock;
> > -     }
> >
> >       zpool = zswap_find_zpool(entry);
> >       gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> >       if (zpool_malloc_support_movable(zpool))
> >               gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
> >       ret = zpool_malloc(zpool, dlen, gfp, &handle);
> > -     if (ret == -ENOSPC) {
> > -             zswap_reject_compress_poor++;
> > -             goto unlock;
> > -     }
> > -     if (ret) {
> > -             zswap_reject_alloc_fail++;
> > +     if (ret)
> >               goto unlock;
> > -     }
> >
> >       buf = 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 = dlen;
> >
> >  unlock:
> > +     if (ret == -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 == -ENOMEM ?
otherwise, it seems the original patch still makes more sense?

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

Thanks
Barry

Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[Yosry Ahmed]

> > >> 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 = acomp_ctx->req->dlen;
> > >>
> > >>      if (ret) {
> > >> -            zswap_reject_compress_fail++;
> > >> +            if (ret == -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 = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
> > >       dlen = acomp_ctx->req->dlen;
> > > -     if (ret) {
> > > -             zswap_reject_compress_fail++;
> > > +     if (ret)
> > >               goto unlock;
> > > -     }
> > >
> > >       zpool = zswap_find_zpool(entry);
> > >       gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> > >       if (zpool_malloc_support_movable(zpool))
> > >               gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
> > >       ret = zpool_malloc(zpool, dlen, gfp, &handle);
> > > -     if (ret == -ENOSPC) {
> > > -             zswap_reject_compress_poor++;
> > > -             goto unlock;
> > > -     }
> > > -     if (ret) {
> > > -             zswap_reject_alloc_fail++;
> > > +     if (ret)
> > >               goto unlock;
> > > -     }
> > >
> > >       buf = 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 = dlen;
> > >
> > >  unlock:
> > > +     if (ret == -ENOSPC)
> > > +             zswap_reject_compress_poor++;
> > > +     else if (ret)
> > > +             zswap_reject_alloc_fail++;
> >
> > Here have two cases: zswap_reject_compress_fail, zswap_reject_alloc_fail.

Ah brain fart, sorry.

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

I don't think it is in all cases, some allocators return other error
codes. It seems unlikely that we'll get any of them, but it can be
missed in the future. How about we use different return codes to
differentiate failures, and still centralize the counters handling.

Something like the following (ideally that one is not a brain fart):

diff --git a/mm/zswap.c b/mm/zswap.c
index 62fe307521c93..20ba25b7601a7 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1021,12 +1021,12 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 {
 	struct crypto_acomp_ctx *acomp_ctx;
 	struct scatterlist input, output;
+	int comp_ret = 0, alloc_ret = 0;
 	unsigned int dlen = PAGE_SIZE;
 	unsigned long handle;
 	struct zpool *zpool;
 	char *buf;
 	gfp_t gfp;
-	int ret;
 	u8 *dst;

 	acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
@@ -1057,26 +1057,18 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 	 * but in different threads running on different cpu, we have different
 	 * acomp instance, so multiple threads can do (de)compression in parallel.
 	 */
-	ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
+	comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
 	dlen = acomp_ctx->req->dlen;
-	if (ret) {
-		zswap_reject_compress_fail++;
+	if (comp_ret)
 		goto unlock;
-	}

 	zpool = zswap_find_zpool(entry);
 	gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
 	if (zpool_malloc_support_movable(zpool))
 		gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
-	ret = zpool_malloc(zpool, dlen, gfp, &handle);
-	if (ret == -ENOSPC) {
-		zswap_reject_compress_poor++;
-		goto unlock;
-	}
-	if (ret) {
-		zswap_reject_alloc_fail++;
+	alloc_ret = zpool_malloc(zpool, dlen, gfp, &handle);
+	if (alloc_ret)
 		goto unlock;
-	}

 	buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO);
 	memcpy(buf, dst, dlen);
@@ -1086,6 +1078,13 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 	entry->length = dlen;

 unlock:
+	if (comp_ret == -ENOSPC || alloc_ret == -ENOSPC)
+		zswap_reject_compress_poor++;
+	else if (comp_ret)
+		zswap_reject_compress_fail++;
+	else if (alloc_ret)
+		zswap_reject_alloc_fail++;
+
 	mutex_unlock(&acomp_ctx->mutex);
 	return ret == 0;
 }

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

Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress_fail if compression returns ENOSPC

[Nhat Pham]

On Fri, Feb 16, 2024 at 12:23 AM Yosry Ahmed <yosryahmed@google.com> 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 -EINVAL
> > 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.
>
> I think the proper fixes should be 744e1885922a if it introduced the
> first scenario where -ENOSPC can be returned from scomp without handling
> it properly in zswap. If -ENOSPC was a possible return value before
> that, then it should be cb61dad80fdc ("zswap: export compression failure
> stats"), where the counter was introduced.

IIRC, the counter was introduced before the zsmalloc patch that
allowed for returning -ENOSPC, as well as the patch that allowed
crypto API to return -ENOSPC.

I think "Fixes: 744e1885922a" would be the closest, as it introduces
the -ENOSPC return value, without handling it in zswap_store().


>
> > 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 = acomp_ctx->req->dlen;
> >
> >       if (ret) {
> > -             zswap_reject_compress_fail++;
> > +             if (ret == -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 = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
>         dlen = acomp_ctx->req->dlen;
> -       if (ret) {
> -               zswap_reject_compress_fail++;
> +       if (ret)
>                 goto unlock;
> -       }
>
>         zpool = zswap_find_zpool(entry);
>         gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
>         if (zpool_malloc_support_movable(zpool))
>                 gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
>         ret = zpool_malloc(zpool, dlen, gfp, &handle);
> -       if (ret == -ENOSPC) {
> -               zswap_reject_compress_poor++;
> -               goto unlock;
> -       }
> -       if (ret) {
> -               zswap_reject_alloc_fail++;
> +       if (ret)
>                 goto unlock;
> -       }
>
>         buf = 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 = dlen;
>
>  unlock:
> +       if (ret == -ENOSPC)
> +               zswap_reject_compress_poor++;
> +       else if (ret)
> +               zswap_reject_alloc_fail++;

I'm eyeballing this, but we have 3 debug counters possible right?
zswap_reject_compress_poor, zswap_reject_compress_fail,
zswap_reject_alloc_fail. I think you remove 3 incrementations (is that
a word lol), and add only 2 cases here.

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