Message-ID: <CANeU7QkrXyv8f4JE+iUte2iTUg-dP_Q1==g2JwMjS4TEyiLovA@mail.gmail.com>
Date: Fri, 7 Jun 2024 14:13:54 -0700
From: Chris Li <chrisl@...nel.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: David Hildenbrand <david@...hat.com>, Barry Song <21cnbao@...il.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Johannes Weiner <hannes@...xchg.org>, 
	Nhat Pham <nphamcs@...il.com>, Chengming Zhou <chengming.zhou@...ux.dev>, 
	Baolin Wang <baolin.wang@...ux.alibaba.com>, Ryan Roberts <ryan.roberts@....com>, 
	Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted

On Fri, Jun 7, 2024 at 11:58 AM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> On Fri, Jun 7, 2024 at 11:52 AM David Hildenbrand <david@...hat.com> wrote:
> >
> > >> I have no strong opinion on this one, but likely a VM_WARN_ON would also
> > >> be sufficient to find such issues early during testing. No need to crash
> > >> the machine.
> > >
> > > I thought VM_BUG_ON() was less frowned-upon than BUG_ON(), but after
> > > some digging I found your patches to checkpatch and Linus clearly
> > > stating that it isn't.
> >
> > :) yes.
> >
> > VM_BUG_ON is not particularly helpful IMHO. If you want something to be
> > found early during testing, VM_WARN_ON is good enough.
> >
> > Ever since Fedora stopped enabling CONFIG_DEBUG_VM, VM_* friends are
> > primarily reported during early/development testing only. But maybe some
> > distro out there still sets it.
> >
> > >
> > > How about something like the following (untested), it is the minimal
> > > recovery we can do but should work for a lot of cases, and does
> > > nothing beyond a warning if we can swapin the large folio from disk:
> > >
> > > diff --git a/mm/page_io.c b/mm/page_io.c
> > > index f1a9cfab6e748..8f441dd8e109f 100644
> > > --- a/mm/page_io.c
> > > +++ b/mm/page_io.c
> > > @@ -517,7 +517,6 @@ void swap_read_folio(struct folio *folio, struct
> > > swap_iocb **plug)
> > >          delayacct_swapin_start();
> > >
> > >          if (zswap_load(folio)) {
> > > -               folio_mark_uptodate(folio);
> > >                  folio_unlock(folio);
> > >          } else if (data_race(sis->flags & SWP_FS_OPS)) {
> > >                  swap_read_folio_fs(folio, plug);
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 6007252429bb2..cc04db6bb217e 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1557,6 +1557,22 @@ bool zswap_load(struct folio *folio)
> > >
> > >          VM_WARN_ON_ONCE(!folio_test_locked(folio));
> > >
> > > +       /*
> > > +        * Large folios should not be swapped in while zswap is being used, as
> > > +        * they are not properly handled.
> > > +        *
> > > +        * If any of the subpages are in zswap, reading from disk would result
> > > +        * in data corruption, so return true without marking the folio uptodate
> > > +        * so that an IO error is emitted (e.g. do_swap_page() will sigfault).
> > > +        *
> > > +        * Otherwise, return false and read the folio from disk.
> > > +        */
> > > +       if (WARN_ON_ONCE(folio_test_large(folio))) {
> > > +               if (xa_find(tree, &offset, offset +
> > > folio_nr_pages(folio) - 1, 0))
> > > +                       return true;
> > > +               return false;
> > > +       }
> > > +
> > >          /*
> > >           * When reading into the swapcache, invalidate our entry. The
> > >           * swapcache can be the authoritative owner of the page and
> > > @@ -1593,7 +1609,7 @@ bool zswap_load(struct folio *folio)
> > >                  zswap_entry_free(entry);
> > >                  folio_mark_dirty(folio);
> > >          }
> > > -
> > > +       folio_mark_uptodate(folio);
> > >          return true;
> > >   }
> > >
> > > One problem is that even if zswap was never enabled, the warning will
> > > be emitted just if CONFIG_ZSWAP is on. Perhaps we need a variable or
> > > static key if zswap was "ever" enabled.
> >
> > We should use WARN_ON_ONCE() only for things that cannot happen. So if
> > there are cases where this could be triggered today, it would be
> > problematic -- especially if it can be triggered from unprivileged user
> > space. But if we're concerned of other code messing up our invariant in
> > the future (e.g., enabling large folios without taking proper care about
> > zswap etc), we're good to add it.
>
> Right now I can't see any paths allocating large folios for swapin, so
> I think it cannot happen. Once someone tries adding it, the warning
> will fire if CONFIG_ZSWAP is used, even if zswap is disabled.
> At this point we will have several options:

Here is my take on this:

> - Make large folios swapin depend on !CONFIG_ZSWAP for now.

I think a WARON or BUG_ON is better. I would need to revert this
change when I am working on 3).  It is a make up rule, not a real
dependency any way.

> - Keep track if zswap was ever enabled and make the warning
> conditional on it. We should also always fallback to order-0 if zswap
> was ever enabled.

IMHO, falling back to order-0 inside zswap is not desired because it
complicates the zswap code. We should not pass large folio to zswap if
zswap is not ready to handle large folio. The core swap already has
the fall back to order-0. If we get to 3), then this fall back in
zswap needs to be removed. It is a transitional thing then maybe not
introduce it in the first place.

> - Properly handle large folio swapin with zswap.
That obviously is ideal.

Chris

Powered by blists - more mailing lists