Re: [RFC 1/4] mm/zswap: skip swapcache for swapping in zswap pages

[Yosry Ahmed <yosryahmed@google.com>]

On Fri, Oct 25, 2024 at 11:19 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 5:46 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > [..]
> > > >> @@ -1576,6 +1576,52 @@ bool zswap_store(struct folio *folio)
> > > >>         return ret;
> > > >>  }
> > > >>
> > > >> +static bool swp_offset_in_zswap(unsigned int type, pgoff_t offset)
> > > >> +{
> > > >> +       return (offset >> SWAP_ADDRESS_SPACE_SHIFT) <  nr_zswap_trees[type];
> > > >
> > > > I am not sure I understand what we are looking for here. When does
> > > > this return false? Aren't the zswap trees always allocated during
> > > > swapon?
> > > >
> > >
> > > Hi Yosry,
> > >
> > > Thanks for the review!
> > >
> > > It becomes useful in patch 3 when trying to determine if a large folio can be allocated.
> > >
> > > For e.g. if the swap entry is the last entry of the last tree, and 1M folios are enabled
> > > (nr_pages = 256), then the while loop in zswap_present_test will try to access a tree
> > > that doesn't exist from the 2nd 4K page onwards if we dont have this check in
> > > zswap_present_test.
> >
> > Doesn't swap_pte_batch() make sure that the range of swap entries
> > passed here all corresponds to existing swap entries, and those
> > entries should always have a corresponding zswap tree? How can the
> > passed in range contain an entry that is not in any zswap tree?
> >
> > I feel like I am missing something.
> >
> > >
> > > >> +}
> > > >> +
> > > >> +/* Returns true if the entire folio is in zswap */
> > > >
> > > > There isn't really a folio at this point, maybe "Returns true if the
> > > > entire range is in zswap"?
> > >
> > > Will change, Thanks!
> > >
> > > >
> > > > Also, this is racy because an exclusive load, invalidation, or
> > > > writeback can cause an entry to be removed from zswap. Under what
> > > > conditions is this safe? The caller can probably guarantee we don't
> > > > race against invalidation, but can we guarantee that concurrent
> > > > exclusive loads or writebacks don't happen?
> > > >
> > > > If the answer is yes, this needs to be properly documented.
> > >
> > > swapcache_prepare should stop things from becoming racy.
> > >
> > > lets say trying to swapin a mTHP of size 32 pages:
> > > - T1 is doing do_swap_page, T2 is doing zswap_writeback.
> > > - T1 - Check if the entire 32 pages is in zswap, swapcache_prepare(entry, nr_pages) in do_swap_page is not yet called.
> > > - T2 - zswap_writeback_entry starts and lets say writes page 2 to swap. it calls __read_swap_cache_async -> swapcache_prepare increments swap_map count, writes page to swap.
> >
> > Can the folio be dropped from the swapcache at this point (e.g. by
> > reclaim)? If yes, it seems like swapcache_prepare() can succeed and
> > try to read it from zswap.
> >
>
> I think you're onto something.
>
> Can this happen: say T2 writebacks a couple of tail pages, but not all
> of them, then drops everything from swap cache. Then T1 can definitely
> proceed. It would then check again in zswap_load(), which returns
> false (thanks to zswap_present()) test.
>
> All fine and good so far, but then in swap_read_folio(), it would try
> to fall back to reading the entire large folio from swapfile, which
> will contain bogus data in pages that have not been written back by
> T2.
>
> I think the problem is swap_read_folio() assumes it always succeeds,
> and a precondition for successful reading is the large folio must have
> no mixed backing state for its subpages, which we fail to guarantee
> before entering swap_read_folio(). This can lead to memory corruption.
>
> Buuut, I think all we need to do is just check the backing state again
> after T1's swapcache_prepare() call. At this point, we are guaranteed
> to have a stable backing state. If it fails here, then we can just
> exit and fall back to individual page swapping in.

I think this should work, but we need to take a closer look for other
things that can go wrong along this path.