Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted

[Yosry Ahmed <yosryahmed@google.com>]

On Thu, Jun 6, 2024 at 1:55 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Fri, Jun 7, 2024 at 8:32 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Thu, Jun 6, 2024 at 1:22 PM 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 implementing
> > > > 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, 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 zswap
> > > > can be enabled in a system that supports large folio swapin.
> > > >
> > > > Looking at callers of swap_read_folio(), it seems like they are either
> > > > 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 changes 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 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@gmail.com/