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@xxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> Cc: Nhat Pham <nphamcs@xxxxxxxxx>
>> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
>> Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
>> ---
>>  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
>>