On Mon, Mar 03, 2025 at 06:16:54PM -0500, Johannes Weiner 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.
I don't disagree, especially after this was shortened. I wanted a helper
when this was a huge function call making zswap_decompress() more
annoying to parse.
I thought you wanted local variables to make the meaning clear after
shortening the message, so my stance was to add a helper iff we do that.
Otherwise what you have above LGTM.
>
> 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.
Good point about bitrotting if rules change.
