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

[Nhat Pham]

On Mon, Mar 3, 2025 at 3:17 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Mon, Mar 03, 2025 at 10:34:46PM +0000, Yosry Ahmed wrote:
> > On Mon, Mar 03, 2025 at 04:55:24PM -0500, Johannes Weiner wrote:
> > > 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.
>
> pr_alert_ratelimited("Decompression error from zswap (%d:%lu %s %u->%d)\n",
>                 swp_type(entry->swpentry), swp_offset(entry->swpentry),
>                 entry->pool->tfm_name, entry->length, acomp_ctx->req->dlen);
>
> Seriously, this does not warrant another function.
>
> It's also valuable to keep warnings inside the problem context instead
> of socking them away somewhere. It makes it clear that decompression
> failure is a serious situation. We also expect this to trigger almost
> never and it won't be tested routinely, so the best chance to fight
> bitrot is to keep all those derefs close by. Imagine if this triggers
> and the data is misleading or it crashes the system because some rules
> around entry, acomp_ctx, the pool or whatever changed. Or if the work
> involved in decompression changed and this is incomplete/unhelpful.


I was actually thinking along the line of Yosry's, but you make some
good points.

Anyway, let's just keep the printing in the OG function. Let's not
overthink one print function call. :)

(sorry for the duplicate email, Johannes. I accidentally sent this
email to you and not cc-ing the rest lol. Resending it)