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