Re: [PATCH v3] page_io: zswap: do not crash the kernel on decompression failure

[Johannes Weiner <hannes@cmpxchg.org>]

On Mon, Mar 03, 2025 at 09:21:27PM +0000, Yosry Ahmed wrote:
> On Mon, Mar 03, 2025 at 12:06:27PM -0800, Nhat Pham wrote:
> > @@ -635,13 +652,11 @@ void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> >  	}
> >  	delayacct_swapin_start();
> >  
> > -	if (swap_read_folio_zeromap(folio)) {
> > -		folio_unlock(folio);
> > +	if (swap_read_folio_zeromap(folio) != -ENOENT)
> >  		goto finish;
> 
> I would split the zeromap change into a separate patch, but it's
> probably fine either way.

+1

> > @@ -1025,12 +1028,31 @@ static void zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> >  	sg_init_table(&output, 1);
> >  	sg_set_folio(&output, folio, PAGE_SIZE, 0);
> >  	acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, PAGE_SIZE);
> > -	BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait));
> > -	BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE);
> > +	decomp_ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
> > +	dlen = acomp_ctx->req->dlen;
> >  
> >  	if (src != acomp_ctx->buffer)
> >  		zpool_unmap_handle(zpool, entry->handle);
> >  	acomp_ctx_put_unlock(acomp_ctx);
> > +
> > +	if (decomp_ret || dlen != PAGE_SIZE) {
> > +		zswap_decompress_fail++;
> > +		pr_alert_ratelimited(
> > +			"decompression failed with returned value %d on zswap entry with "
> 
> nit: Decompression*
> 
> I am also wondering how this looks like in dmesg? Is the line too long
> to be read? Should we add some line breaks (e.g. like
> warn_sysctl_write()), we could probably also put this in a helper to
> keep this function visually easy to follow.

If it were more interwoven, I would agree. But it's only followed by
the return true, false. Moving it out of line would need another name
in the zswap namespace and also take an awkward amount of parameters,
so IMO more taxing on the reader.

But maybe do if (!decomp_ret && dlen == PAGE_SIZE) return true, and
then save an indentation for the error part?

> > +			"swap entry value %08lx, swap type %d, and swap offset %lu. "
> > +			"compression algorithm is %s. compressed size is %u bytes, and "
> > +			"decompressed size is %u bytes.\n",

Any objections to shortening it and avoiding the line length issue?
Even with \n's, this is still a lot of characters to dump 10x/5s. And
it's not like the debug info is super useful to anyone but kernel
developers, who in turn wouldn't have an issue interpreting this:

pr_alert_ratelimited("Decompression error from zswap (%d:%lu %s %u->%d)\n",
		     swptype, swpoffset, name, clen, dlen);

> > +			decomp_ret,
> > +			entry->swpentry.val,
> > +			swp_type(entry->swpentry),
> > +			swp_offset(entry->swpentry),
> > +			entry->pool->tfm_name,
> > +			entry->length,
> > +			acomp_ctx->req->dlen);