Received-SPF: pass (google.com: domain of linux-kernel+bounces-205103-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240606184818.1566920-1-yosryahmed@google.com>
 <84d78362-e75c-40c8-b6c2-56d5d5292aa7@redhat.com> <CAJD7tkZH9C21nx75W9Erun=oUvmad5ujmDyGYWRRHEwPCCizUw@mail.gmail.com>
 <7507d075-9f4d-4a9b-836c-1fbb2fbd2257@redhat.com> <CAGsJ_4w_dEyLsPhdJCtRMReXDD116p+U979Yk-8EBu=zx5FWgQ@mail.gmail.com>
 <CAJD7tkYnZCvwUSTAx-N-08b+mVF4yHEdDTbp9oqYrVQXX8M1Cw@mail.gmail.com>
In-Reply-To: <CAJD7tkYnZCvwUSTAx-N-08b+mVF4yHEdDTbp9oqYrVQXX8M1Cw@mail.gmail.com>
From: Barry Song <21cnbao@gmail.com>
Date: Fri, 7 Jun 2024 09:53:52 +1200
Message-ID: <CAGsJ_4wAYbofjg99iEr7cEAptH9Ve9WcrSXho8A-Sw54vb8Msw@mail.gmail.com>
Subject: Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted
To: Yosry Ahmed <yosryahmed@google.com>
Cc: David Hildenbrand <david@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, 
	Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Baolin Wang <baolin.wang@linux.alibaba.com>, 
	Chris Li <chrisl@kernel.org>, Ryan Roberts <ryan.roberts@arm.com>, 
	Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 7, 2024 at 9:37=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com> =
wrote:
>
> On Thu, Jun 6, 2024 at 2:30=E2=80=AFPM Barry Song <21cnbao@gmail.com> wro=
te:
> >
> > On Fri, Jun 7, 2024 at 9:17=E2=80=AFAM David Hildenbrand <david@redhat.=
com> wrote:
> > >
> > > On 06.06.24 22:31, Yosry Ahmed wrote:
> > > > On Thu, Jun 6, 2024 at 1:22=E2=80=AFPM David Hildenbrand <david@red=
hat.com> wrote:
> > > >>
> > > >> On 06.06.24 20:48, Yosry Ahmed wrote:
> > > >>> With ongoing work to support large folio swapin, it is important =
to make
> > > >>> sure we do not pass large folios to zswap_load() without implemen=
ting
> > > >>> proper support.
> > > >>>
> > > >>> For example, if a swapin fault observes that contiguous PTEs are
> > > >>> pointing to contiguous swap entries and tries to swap them in as =
a large
> > > >>> folio, swap_read_folio() will pass in a large folio to zswap_load=
(), but
> > > >>> zswap_load() will only effectively load the first page in the fol=
io. If
> > > >>> the first page is not in zswap, the folio will be read from disk,=
 even
> > > >>> though other pages may be in zswap.
> > > >>>
> > > >>> In both cases, this will lead to silent data corruption.
> > > >>>
> > > >>> Proper large folio swapin support needs to go into zswap before z=
swap
> > > >>> can be enabled in a system that supports large folio swapin.
> > > >>>
> > > >>> Looking at callers of swap_read_folio(), it seems like they are e=
ither
> > > >>> allocated from __read_swap_cache_async() or do_swap_page() in the
> > > >>> SWP_SYNCHRONOUS_IO path. Both of which allocate order-0 folios, s=
o we
> > > >>> are fine for now.
> > > >>>
> > > >>> Add a VM_BUG_ON() in zswap_load() to make sure that we detect cha=
nges in
> > > >>> the order of those allocations without proper handling of zswap.
> > > >>>
> > > >>> Alternatively, swap_read_folio() (or its callers) can be updated =
to have
> > > >>> a fallback mechanism that splits large folios or reads subpages
> > > >>> separately. Similar logic may be needed anyway in case part of a =
large
> > > >>> folio is already in the swapcache and the rest of it is swapped o=
ut.
> > > >>>
> > > >>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> > > >>> ---
> > > >>>
> > > >>> Sorry for the long CC list, I just found myself repeatedly lookin=
g at
> > > >>> new series that add swap support for mTHPs / large folios, making=
 sure
> > > >>> they do not break with zswap or make incorrect assumptions. This =
debug
> > > >>> check should give us some peace of mind. Hopefully this patch wil=
l also
> > > >>> raise awareness among people who are working on this.
> > > >>>
> > > >>> ---
> > > >>>    mm/zswap.c | 3 +++
> > > >>>    1 file changed, 3 insertions(+)
> > > >>>
> > > >>> diff --git a/mm/zswap.c b/mm/zswap.c
> > > >>> index b9b35ef86d9be..6007252429bb2 100644
> > > >>> --- a/mm/zswap.c
> > > >>> +++ b/mm/zswap.c
> > > >>> @@ -1577,6 +1577,9 @@ bool zswap_load(struct folio *folio)
> > > >>>        if (!entry)
> > > >>>                return false;
> > > >>>
> > > >>> +     /* Zswap loads do not handle large folio swapins correctly =
yet */
> > > >>> +     VM_BUG_ON(folio_test_large(folio));
> > > >>> +
> > > >>
> > > >> There is no way we could have a WARN_ON_ONCE() and recover, right?
> > > >
> > > > Not without making more fundamental changes to the surrounding swap
> > > > code. Currently zswap_load() returns either true (folio was loaded
> > > > from zswap) or false (folio is not in zswap).
> > > >
> > > > To handle this correctly zswap_load() would need to tell
> > > > swap_read_folio() which subpages are in zswap and have been loaded,
> > > > and then swap_read_folio() would need to read the remaining subpage=
s
> > > > from disk. This of course assumes that the caller of swap_read_foli=
o()
> > > > made sure that the entire folio is swapped out and protected agains=
t
> > > > races with other swapins.
> > > >
> > > > Also, because swap_read_folio() cannot split the folio itself, othe=
r
> > > > swap_read_folio_*() functions that are called from it should be
> > > > updated to handle swapping in tail subpages, which may be questiona=
ble
> > > > in its own right.
> > > >
> > > > An alternative would be that zswap_load() (or a separate interface)
> > > > could tell swap_read_folio() that the folio is partially in zswap,
> > > > then we can just bail and tell the caller that it cannot read the
> > > > large folio and that it should be split.
> > > >
> > > > There may be other options as well, but the bottom line is that it =
is
> > > > possible, but probably not something that we want to do right now.
> > > >
> > > > A stronger protection method would be to introduce a config option =
or
> > > > boot parameter for large folio swapin, and then make CONFIG_ZSWAP
> > > > depend on it being disabled, or have zswap check it at boot and ref=
use
> > > > to be enabled if it is on.
> > >
> > > Right, sounds like the VM_BUG_ON() really is not that easily avoidabl=
e.
> > >
> > > I was wondering, if we could WARN_ON_ONCE and make the swap code dete=
ct
> > > this like a read-error from disk.
> > >
> > > I think do_swap_page() detects that by checking if the folio is not
> > > uptodate:
> > >
> > > if (unlikely(!folio_test_uptodate(folio))) {
> > >         ret =3D VM_FAULT_SIGBUS;
> > >         goto out_nomap;
> > > }
> > >
> > > So maybe WARN_ON_ONCE() + triggering that might be a bit nicer to the
> > > system (but the app would crash either way, there is no way around it=
).
> > >
> >
> > I'd rather fallback to small folios swapin instead crashing apps till w=
e fix
> > the large folio swapin in zswap :-)
>
> I think David is referring to catching the buggy cases that do not
> properly fallback to small folios with zswap, not as an alternative to
> the fallback. This is at least what I had in mind with the patch.

Cool. Thanks for the clarification. I'm fine with keeping the fallback,
whether it's the current VM_BUG_ON or David's recommended
SIGBUS.

Currently, mainline doesn't support large folios swap-in, so I see
your patch as a helpful warning for those attempting large folio
swap-in to avoid making mistakes like loading large folios from
zswap.

In fact, I spent a week trying to figure out why my app was crashing
before I added 'if (zswap_is_enabled()) goto fallback'. If your patch
had come earlier, it would have saved me that week of work :-)

To me, a direct crash seems like a more straightforward way to
prompt people to fallback when attempting large folio swap-ins.
So, I am slightly in favor of your current patch.

Thanks
Barry