On Wed, Jun 21, 2023 at 2:06 AM Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> wrote: > > On Wed, Jun 21, 2023 at 01:05:56AM -0700, Yosry Ahmed wrote: > > On Wed, Jun 21, 2023 at 12:01 AM Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> wrote: > > > Hi Yosry, I was testing the latest mm-unstable and encountered a bug. > > > It was bisectable and this is the first bad commit. > > > > > > > > > Attached config file and bisect log. > > > The oops message is available at: > > > > > > https://social.kernel.org/media/eace06d71655b3cc76411366573e4a8ce240ad65b8fd20977d7c73eec9dc2253.jpg > > > > > > (the head commit is b9c91c43412f2e07 "mm: zswap: support exclusive loads") > > > (it's an image because I tested it on real machine) > > > > > > > > > This is what I have as swap space: > > > > > > $ cat /proc/swaps > > > Filename Type Size Used Priority > > > /var/swap file 134217724 0 -2 > > > /dev/zram0 partition 8388604 0 100 > > > > > > Hi Hyeonggon, > > > > Thanks for reporting this! I think I know what went wrong. Could you > > please verify if the below fix works if possible? > > > > Works fine and I was not able to reproduce the bug with the patch > applied. > > Not sure Andrew would prefer squashing it into original one or applying it > as separate patch, though (I'm totally fine with both way). I think it already landed in mm-stable so it cannot be squashed at this point. > > Anyway: > > Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> Thanks a lot for reporting and testing this! I will wait for Domenico to also respond then send the fix to Andrew. Hopefully it's not too late for this rc. > > > Domenico, I believe the below fix would also fix a problem with the > > recent writeback series. If the entry is invalidated before we grab the > > lock to put the local ref in zswap_frontswap_load(), then the entry > > will be freed once we call zswap_entry_put(), and the movement to the > > beginning LRU will be operating on a freed entry. It also modifies > > your recently added commit 418fd29d9de5 ("mm: zswap: invaldiate entry > > after writeback"). I would appreciate it if you also take a look. > > > > If this works as intended, I can send a formal patch (applies on top > > of fd247f029cd0 ("mm/gup: do not return 0 from pin_user_pages_fast() > > for bad args")): > > > > From 4b7f949b3ffb42d969d525d5b576fad474f55276 Mon Sep 17 00:00:00 2001 > > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > Date: Wed, 21 Jun 2023 07:43:51 +0000 > > Subject: [PATCH] mm: zswap: fix double invalidate with exclusive loads > > > > If exclusive loads are enabled for zswap, we invalidate the entry before > > returning from zswap_frontswap_load(), after dropping the local > > reference. However, the tree lock is dropped during decompression after > > the local reference is acquired, so the entry could be invalidated > > before we drop the local ref. If this happens, the entry is freed once > > we drop the local ref, and zswap_invalidate_entry() tries to invalidate > > an already freed entry. > > > > Fix this by: > > (a) Making sure zswap_invalidate_entry() is always called with a local > > ref held, to avoid being called on a freed entry. > > (b) Making sure zswap_invalidate_entry() only drops the ref if the entry > > was actually on the rbtree. Otherwise, another invalidation could > > have already happened, and the initial ref is already dropped. > > > > With these changes, there is no need to check that there is no need to > > make sure the entry still exists in the tree in zswap_reclaim_entry() > > before invalidating it, as zswap_reclaim_entry() will make this check > > internally. > > > > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads") > > Reported-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > <...snip...> > > -- > Hyeonggon Yoo > > Undergraduate | Chungnam National University
