On Wed, Feb 26, 2025 at 5:19 PM Yosry Ahmed <yosry.ahmed@xxxxxxxxx> wrote: > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > Currently, we crash the kernel when a decompression failure occurs in > > zswap (either because of memory corruption, or a bug in the compression > > algorithm). This is overkill. We should only SIGBUS the unfortunate > > process asking for the zswap entry on zswap load, and skip the corrupted > > entry in zswap writeback. The former is accomplished by returning true > > from zswap_load(), indicating that zswap owns the swapped out content, > > but without flagging the folio as up-to-date. The process trying to swap > > in the page will check for the uptodate folio flag and SIGBUS (see > > do_swap_page() in mm/memory.c for more details). > > We should call out the extra xarray walks and their perf impact (if > any). Lemme throw this in a quick and dirty test. I doubt there's any impact, but since I'm reworking this patch for a third version anyway might as well. > > > > > See [1] for a recent upstream discussion about this. > > > > [1]: https://lore.kernel.org/all/ZsiLElTykamcYZ6J@xxxxxxxxxxxxxxxxxxxx/ > > > > Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx> > > Suggested-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx> > > Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx> > > --- > > mm/zswap.c | 94 ++++++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 67 insertions(+), 27 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 6dbf31bd2218..e4a2157bbc64 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -62,6 +62,8 @@ static u64 zswap_reject_reclaim_fail; > > static u64 zswap_reject_compress_fail; > > /* Compressed page was too big for the allocator to (optimally) store */ > > static u64 zswap_reject_compress_poor; > > +/* Load or writeback failed due to decompression failure */ > > +static u64 zswap_decompress_fail; > > /* Store failed because underlying allocator could not get memory */ > > static u64 zswap_reject_alloc_fail; > > /* Store failed because the entry metadata could not be allocated (rare) */ > > @@ -996,11 +998,13 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, > > return comp_ret == 0 && alloc_ret == 0; > > } > > > > -static void zswap_decompress(struct zswap_entry *entry, struct folio *folio) > > +static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio) > > { > > struct zpool *zpool = entry->pool->zpool; > > struct scatterlist input, output; > > struct crypto_acomp_ctx *acomp_ctx; > > + int decomp_ret; > > + bool ret = true; > > u8 *src; > > > > acomp_ctx = acomp_ctx_get_cpu_lock(entry->pool); > > @@ -1025,12 +1029,25 @@ 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); > > + if (decomp_ret || acomp_ctx->req->dlen != PAGE_SIZE) { > > + ret = false; > > + zswap_decompress_fail++; > > + pr_alert_ratelimited( > > + "decompression failed with returned value %d on zswap entry with 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", > > This is a very long line. I think we should break it into multiple > lines. I know multiline strings are frowned upon by checkpatch, by this > exist (see the warning in mem_cgroup_oom_control_write() for example), > and they are definitely better than a very long line imo. My personal take is I prefer multi line strings, but was not sure what is the "preferred" or "official" style. Oh well. > > > + decomp_ret, > > + entry->swpentry.val, > > + swp_type(entry->swpentry), > > + swp_offset(entry->swpentry), > > + entry->pool->tfm_name, > > + entry->length, > > + acomp_ctx->req->dlen); > > + } > > > > if (src != acomp_ctx->buffer) > > zpool_unmap_handle(zpool, entry->handle); > > acomp_ctx_put_unlock(acomp_ctx); > > + return ret; > > Not a big deal but we could probably store the length in a local > variable and move the check here, and avoid needing 'ret'. Ah the suggestion you made in an older version right? But sounds like we're just trading some one local variable for another? That said, it *technically* move some work outside of the lock section. I'll just give it a try :) > > > } > > > > /********************************* > > @@ -1060,6 +1077,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > struct writeback_control wbc = { > > .sync_mode = WB_SYNC_NONE, > > }; > > + int ret = 0; > > > > /* try to allocate swap cache folio */ > > si = get_swap_device(swpentry); > > @@ -1081,8 +1099,8 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > * and freed when invalidated by the concurrent shrinker anyway. > > */ > > if (!folio_was_allocated) { > > - folio_put(folio); > > - return -EEXIST; > > + ret = -EEXIST; > > + goto put_folio; > > } > > > > /* > > @@ -1095,14 +1113,17 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > * be dereferenced. > > */ > > tree = swap_zswap_tree(swpentry); > > - if (entry != xa_cmpxchg(tree, offset, entry, NULL, GFP_KERNEL)) { > > - delete_from_swap_cache(folio); > > - folio_unlock(folio); > > - folio_put(folio); > > - return -ENOMEM; > > + if (entry != xa_load(tree, offset)) { > > + ret = -ENOMEM; > > + goto delete_unlock; > > + } > > + > > + if (!zswap_decompress(entry, folio)) { > > + ret = -EIO; > > + goto delete_unlock; > > } > > > > - zswap_decompress(entry, folio); > > + xa_erase(tree, offset); > > > > count_vm_event(ZSWPWB); > > if (entry->objcg) > > @@ -1118,9 +1139,14 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > > /* start writeback */ > > __swap_writepage(folio, &wbc); > > - folio_put(folio); > > > > - return 0; > > +put_folio: > > + folio_put(folio); > > + return ret; > > +delete_unlock: > > + delete_from_swap_cache(folio); > > + folio_unlock(folio); > > + goto put_folio; > > I think I suggested a way to avoid this goto in v1: > https://lore.kernel.org/lkml/Z782SPcJI8DFISRa@xxxxxxxxxx/. > > Did this not work out? Oh I thought your suggestion was the same as Johannes. Let me take a closer look... > > > } > > > > /********************************* > > @@ -1620,6 +1646,20 @@ bool zswap_store(struct folio *folio) > > return ret; > > } > > > > +/** > > + * zswap_load() - load a page from zswap > > + * @folio: folio to load > > + * > > + * Returns: true if zswap owns the swapped out contents, false otherwise. > > + * > > + * Note that the zswap_load() return value doesn't indicate success or failure, > > + * but whether zswap owns the swapped out contents. This MUST return true if > > + * zswap does own the swapped out contents, even if it fails to write the > > + * contents to the folio. Otherwise, the caller will try to read garbage from > > + * the backend. > > + * > > + * Success is signaled by marking the folio uptodate. > > + */ > > bool zswap_load(struct folio *folio) > > { > > swp_entry_t swp = folio->swap; > > @@ -1644,6 +1684,17 @@ bool zswap_load(struct folio *folio) > > The comment that exists here (not visible in the diff) should be > abbreviated now that we already explained the whole uptodate thing > above, right? Lemme take a stab at it :) > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > return true; > > > > + entry = xa_load(tree, offset); > > + if (!entry) > > + return false; > > + > > A small comment here pointing out that we are deliberatly not setting > uptodate because of the failure may make things more obvious, or do you > think that's not needed? I was thinking the comment above zswap_load() should be necessary, but a small comment here won't hurt. > > > + if (!zswap_decompress(entry, folio)) > > + return true; > > + > > + count_vm_event(ZSWPIN); > > + if (entry->objcg) > > + count_objcg_events(entry->objcg, ZSWPIN, 1); > > + > > /* > > * When reading into the swapcache, invalidate our entry. The > > * swapcache can be the authoritative owner of the page and > > @@ -1656,21 +1707,8 @@ bool zswap_load(struct folio *folio) > > * files, which reads into a private page and may free it if > > * the fault fails. We remain the primary owner of the entry.) > > */ > > - if (swapcache) > > - entry = xa_erase(tree, offset); > > - else > > - entry = xa_load(tree, offset); > > - > > - if (!entry) > > - return false; > > - > > - zswap_decompress(entry, folio); > > - > > - count_vm_event(ZSWPIN); > > - if (entry->objcg) > > - count_objcg_events(entry->objcg, ZSWPIN, 1); > > - > > if (swapcache) { > > + xa_erase(tree, offset); > > zswap_entry_free(entry); > > folio_mark_dirty(folio); > > } > > @@ -1771,6 +1809,8 @@ static int zswap_debugfs_init(void) > > zswap_debugfs_root, &zswap_reject_compress_fail); > > debugfs_create_u64("reject_compress_poor", 0444, > > zswap_debugfs_root, &zswap_reject_compress_poor); > > + debugfs_create_u64("decompress_fail", 0444, > > + zswap_debugfs_root, &zswap_decompress_fail); > > debugfs_create_u64("written_back_pages", 0444, > > zswap_debugfs_root, &zswap_written_back_pages); > > debugfs_create_file("pool_total_size", 0444, > > > > base-commit: 598d34afeca6bb10554846cf157a3ded8729516c > > -- > > 2.43.5
