Re: [PATCH 2/2] mm/zswap: classify compression dst overflow as poor compression

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/zswap: zswap_compress() mishandles -E2BIG overflow from hw compressors
--

commit 6e958455f7e6f806b4bd7b7afc39fe7c9fb8cd1f
Author: Suseendran Bhaskaran <suseendranb@google.com>

mm/zswap: classify compression dst overflow as poor compression

This commit updates zswap_compress() to treat only -ENOSPC or an output
length of at least PAGE_SIZE as an incompressible page, while routing other
non-zero return values from crypto_acomp_compress() to the compressor failure
path.

> diff --git a/mm/zswap.c b/mm/zswap.c
> index 37f34e406c8e3..5f2a941fcaf1e 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -881,12 +881,12 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>  	 * only adds metadata overhead.  swap_writeout() will put the page back
>  	 * to the active LRU list in the case.
>  	 */
> -	if (comp_ret || !dlen || dlen >= PAGE_SIZE) {
> +	if (comp_ret == -ENOSPC || (!comp_ret && dlen >= PAGE_SIZE)) {
>  		rcu_read_lock();
>  		if (!mem_cgroup_zswap_writeback_enabled(
>  					folio_memcg(page_folio(page)))) {
>  			rcu_read_unlock();
> -			comp_ret = comp_ret ? comp_ret : -EINVAL;
> +			comp_ret = -ENOSPC;
>  			goto unlock;
>  		}
>  		rcu_read_unlock();
> @@ -894,6 +894,9 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>  		dlen = PAGE_SIZE;
>  		dst = kmap_local_page(page);
>  		mapped = true;
> +	} else if (comp_ret || !dlen) {
> +		comp_ret = comp_ret ? comp_ret : -EINVAL;
> +		goto unlock;
>  	}
>  
>  	gfp = GFP_NOWAIT | __GFP_NORETRY | __GFP_HIGHMEM | __GFP_MOVABLE;

[Severity: Medium]
Does restricting the incompressible page check to comp_ret == -ENOSPC cause a
regression when using hardware acomp drivers that return -E2BIG or -EOVERFLOW
on destination buffer overflow?

When zswap_store() -> zswap_store_page() -> zswap_compress() compresses an
incompressible page into a PAGE_SIZE destination buffer, hardware acomp
drivers in drivers/crypto/ do not return -ENOSPC when the compressed output
exceeds the destination buffer:

drivers/crypto/intel/iaa/iaa_crypto_main.c:check_completion() {
	if (comp->status == IAA_ANALYTICS_ERROR &&
	    comp->error_code == IAA_ERROR_COMP_BUF_OVERFLOW && compress) {
		ret = -E2BIG;
		dev_dbg(dev, "compressed > uncompressed size,"
			" not compressing, size=0x%x\n",
			comp->output_size);
		update_completion_comp_buf_overflow_errs();
		goto out;
	}
}

Similarly, the QAT acomp driver (which registers deflate and zstd with
priority 4001) completes destination buffer overflows with -E2BIG in
qat_comp_generic_callback() (and -E2BIG or -EOVERFLOW in
qat_comp_lz4s_zstd_callback()):

drivers/crypto/intel/qat/qat_common/qat_comp_algs.c:qat_comp_generic_callback() {
	if (unlikely(produced > qat_req->actual_dlen)) {
		memset(inst->dc_data->ovf_buff, 0,
		       inst->dc_data->ovf_buff_sz);
		dev_dbg(&GET_DEV(accel_dev),
			"Actual buffer overflow: produced=%d, dlen=%d\n",
			produced, qat_req->actual_dlen);

		res = -E2BIG;
		areq->dlen = 0;
		goto end;
	}
}

With this change, -E2BIG and -EOVERFLOW fall into the else if (comp_ret ||
!dlen) branch and jump to unlock. When zswap writeback is enabled,
incompressible pages will be rejected instead of being stored uncompressed in
zsmalloc, and zswap_reject_compress_fail will be incremented instead of
zswap_reject_compress_poor.

Should those hardware acomp drivers be updated to return -ENOSPC on
destination overflow, or should zswap_compress() also handle -E2BIG and
-EOVERFLOW as poor compression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021205626.3286270-1-suseendranb@google.com?part=2