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

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Mon, Mar 03, 2025 at 04:55:24PM -0500, Johannes Weiner wrote:
> 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.

My rationale was that no one reading zswap_decompress() will feel the need
to read a function called zswap_warn_decompress_failure() in the error
path, so it will save people parsing this huge thing.

FWIW it would only need to take 3 parameters: decomp_ret, dlen, entry.

> 
> 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);

Yeah this looks much more concise. It's a bit harder to parser the dmesg
as you have to cross check the code, but hopefully this is something
that people rarely have to do.

I don't feel strongly about adding a helper in this case, unless we want
to add local variables (like Johannes did above), in which case a helper
would be a good way to hide them.

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