Received-SPF: pass (google.com: domain of linux-kernel+bounces-205019-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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>
 <CAGsJ_4zTJgOgkXE=4a=w=ZsuC4WuwoBNHf0v0BwP7tatkkJZqA@mail.gmail.com>
In-Reply-To: <CAGsJ_4zTJgOgkXE=4a=w=ZsuC4WuwoBNHf0v0BwP7tatkkJZqA@mail.gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 6 Jun 2024 14:00:47 -0700
Message-ID: <CAJD7tkZru-hD4300-KHPrBFM3km3osXtaQf6AauUKhwALzXn3g@mail.gmail.com>
Subject: Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted
To: Barry Song <21cnbao@gmail.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 Thu, Jun 6, 2024 at 1:55=E2=80=AFPM Barry Song <21cnbao@gmail.com> wrote=
:
>
> On Fri, Jun 7, 2024 at 8:32=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com=
> wrote:
> >
> > On Thu, Jun 6, 2024 at 1:22=E2=80=AFPM David Hildenbrand <david@redhat.=
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 implementi=
ng
> > > > 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 folio=
. If
> > > > the first page is not in zswap, the folio will be read from disk, e=
ven
> > > > 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 zsw=
ap
> > > > can be enabled in a system that supports large folio swapin.
> > > >
> > > > Looking at callers of swap_read_folio(), it seems like they are eit=
her
> > > > allocated from __read_swap_cache_async() or do_swap_page() in the
> > > > SWP_SYNCHRONOUS_IO path. Both of which allocate order-0 folios, so =
we
> > > > are fine for now.
> > > >
> > > > Add a VM_BUG_ON() in zswap_load() to make sure that we detect chang=
es 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 la=
rge
> > > > folio is already in the swapcache and the rest of it is swapped out=
.
> > > >
> > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
>
> Acked-by: Barry Song <baohua@kernel.org>

Thanks!

>
> this has been observed by me[1], that's why you can find the below
> code in my swapin patch

Thanks for the pointer. I suppose if we add more generic swapin
support we'll have to add a similar check in
__read_swap_cache_async(), unless we are planning to reuse
alloc_swap_folio() there.

It would be nice if we can have a global check for this rather than
add it to all different swapin paths (although I suspect there are
only two allocations paths right now). We can always disable zswap if
mTHP swapin is enabled, or always disable mTHP swapin if zswap is
enabled. At least until proper support is added.

>
> +static struct folio *alloc_swap_folio(struct vm_fault *vmf)
> +{
> +      ...
> +      /*
> +      * a large folio being swapped-in could be partially in
> +      * zswap and partially in swap devices, zswap doesn't
> +      * support large folios yet, we might get corrupted
> +      * zero-filled data by reading all subpages from swap
> +      * devices while some of them are actually in zswap
> +      */
> +      if (is_zswap_enabled())
> +           goto fallback;
>
> [1] https://lore.kernel.org/linux-mm/20240304081348.197341-6-21cnbao@gmai=
l.com/