Received-SPF: pass (google.com: domain of linux-kernel+bounces-210175-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: <20240608023654.3513385-1-yosryahmed@google.com>
 <2821278f-bc94-c147-d0fe-8cc52dbdccb1@redhat.com> <CAJD7tkYepf6z1Q7HiSRAc7-S9OwYCphx=9NcyHxtW0YrkFv0sQ@mail.gmail.com>
 <811778e4-6af4-563a-757e-83fec207e79f@redhat.com>
In-Reply-To: <811778e4-6af4-563a-757e-83fec207e79f@redhat.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Tue, 11 Jun 2024 08:47:10 -0700
Message-ID: <CAJD7tkZ60Ru7b_BYZZy_hZtp=Tmz=KwQy28tqn-kHMkEKwbiFQ@mail.gmail.com>
Subject: Re: [PATCH v2] mm: zswap: handle incorrect attempts to load of large folios
To: =?UTF-8?Q?Mika_Penttil=C3=A4?= <mpenttil@redhat.com>
Cc: 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>, Barry Song <21cnbao@gmail.com>, 
	Chris Li <chrisl@kernel.org>, Ryan Roberts <ryan.roberts@arm.com>, 
	David Hildenbrand <david@redhat.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 Mon, Jun 10, 2024 at 9:14=E2=80=AFPM Mika Penttil=C3=A4 <mpenttil@redhat=
.com> wrote:
>
>
> On 6/10/24 20:35, Yosry Ahmed wrote:
> > On Fri, Jun 7, 2024 at 9:08=E2=80=AFPM Mika Penttil=C3=A4 <mpenttil@red=
hat.com> wrote:
> >> On 6/8/24 05:36, Yosry Ahmed wrote:
> >>> diff --git a/mm/zswap.c b/mm/zswap.c
> >>> index b9b35ef86d9be..ebb878d3e7865 100644
> >>> --- a/mm/zswap.c
> >>> +++ b/mm/zswap.c
> >>> @@ -1557,6 +1557,26 @@ 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 u=
sed, as
> >>> +      * they are not properly handled. Zswap does not properly load =
large
> >>> +      * folios, and a large folio may only be partially in zswap.
> >>> +      *
> >>> +      * 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 sig=
fault).
> >>> +      *
> >>> +      * Otherwise, return false and read the folio from disk.
> >>> +      */
> >>> +     if (folio_test_large(folio)) {
> >>> +             if (xa_find(tree, &offset,
> >>> +                         offset + folio_nr_pages(folio) - 1, XA_PRES=
ENT)) {
> >>> +                     WARN_ON_ONCE(1);
> >>> +                     return true;
> >>> +             }
> >> How does that work? Should it be xa_find_after() to not always find
> >> current entry?
> > By "current entry" I believe you mean the entry corresponding to
> > "offset" (i.e. the first subpage of the folio). At this point, we
> > haven't checked if that offset has a corresponding entry in zswap or
> > not. It may be on disk, or zwap may be disabled.
>
> Okay you test if there's any matching offset in zswap for the folio.
>
>
> >> And does it still mean those subsequent entries map to same folio?
> > If I understand correctly, a folio in the swapcache has contiguous
> > swap offsets for its subpages. So I am assuming that the large folio
> > swapin case will adhere to that (i.e. we only swapin a large folio if
> > the swap offsets are contiguous). Did I misunderstand something here?
>
> Yes I think that is fair assumption for now. But also saw your v3 which
> doesn't depend on this.

Yeah Barry pointed out that we want to warn if a large folio reaches
zswap and there is a chance that it is in zswap (i.e. zswap was
enabled), even if it happens that the folio is not in zswap during
swapin. This gives wider coverage and is cheaper when zswap is
disabled than the lookups.

We will also need to check if zswap was ever enabled in the large
folio swapin series anyway, so the helper introduced in v3 should be
helpful there as well.