On Mon, Mar 3, 2025 at 2:34 PM Yosry Ahmed <yosry.ahmed@xxxxxxxxx> wrote:
>
> 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.
I think Johannes' suggestion accomplishes a similar effect (see below).
>
> 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?
I like this. It also moves the (much rarer) failure case to its own
corner, which we can skip (most of the time). :)
> >
> > > > + "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:
No objection from my end.
> >
> > 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.
That said, I'm not so sure about adding local variables here. We would
be cluttering the code for a bunch of single-use variables, that are
not even the "common" case. I mean, this seems fine to me?
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, dlen);
(with proper indentation, but you get the idea).
