Received-SPF: pass (google.com: domain of linux-kernel+bounces-112071-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 22 Mar 2024 22:33:13 +0000
In-Reply-To: <CAGsJ_4yc-XB3+FkcZTy1aZ0n6ZKEkfWVYk_TjqqrdcROa5VYtA@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240322163939.17846-1-chengming.zhou@linux.dev>
 <CAJD7tkYuYEsKFvjKKRxOx3fCekA03jPpOpmV7T20q=9K=Jb2bA@mail.gmail.com> <CAGsJ_4yc-XB3+FkcZTy1aZ0n6ZKEkfWVYk_TjqqrdcROa5VYtA@mail.gmail.com>
Message-ID: <Zf4HKUpKpDWZygni@google.com>
Subject: Re: [RFC PATCH] mm: add folio in swapcache if swapin from zswap
From: Yosry Ahmed <yosryahmed@google.com>
To: Barry Song <21cnbao@gmail.com>
Cc: chengming.zhou@linux.dev, hannes@cmpxchg.org, nphamcs@gmail.com, 
	akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	Zhongkun He <hezhongkun.hzk@bytedance.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Mar 23, 2024 at 10:41:32AM +1300, Barry Song wrote:
> On Sat, Mar 23, 2024 at 8:38=E2=80=AFAM Yosry Ahmed <yosryahmed@google.co=
m> wrote:
> >
> > On Fri, Mar 22, 2024 at 9:40=E2=80=AFAM <chengming.zhou@linux.dev> wrot=
e:
> > >
> > > From: Chengming Zhou <chengming.zhou@linux.dev>
> > >
> > > There is a report of data corruption caused by double swapin, which i=
s
> > > only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backen=
ds.
> > >
> > > The root cause is that zswap is not like other "normal" swap backends=
,
> > > it won't keep the copy of data after the first time of swapin. So if
>=20
> I don't quite understand this, so once we load a page from zswap, zswap
> will free it even though do_swap_page might not set it to PTE?
>=20
> shouldn't zswap free the memory after notify_free just like zram?

It's an optimization that zswap has, exclusive loads. After a page is
swapped in it can stick around in the swapcache for a while. In this
case, there would be two copies in memory with zram (compressed and
uncompressed). Zswap implements exclusive loads to drop the compressed
copy. The folio is marked as dirty so that any attempts to reclaim it
cause a new write (compression) to zswap. It is also for a lot of
cleanups and straightforward entry lifetime tracking in zswap.

It is mostly fine, the problem here happens because we skip the
swapcache during swapin, so there is a possibility that we load the
folio from zswap then just drop it without stashing it anywhere.

>=20
> > > the folio in the first time of swapin can't be installed in the paget=
able
> > > successfully and we just free it directly. Then in the second time of
> > > swapin, we can't find anything in zswap and read wrong data from swap=
file,
> > > so this data corruption problem happened.
> > >
> > > We can fix it by always adding the folio into swapcache if we know th=
e
> > > pinned swap entry can be found in zswap, so it won't get freed even t=
hough
> > > it can't be installed successfully in the first time of swapin.
> >
> > A concurrent faulting thread could have already checked the swapcache
> > before we add the folio to it, right? In this case, that thread will
> > go ahead and call swap_read_folio() anyway.
> >
> > Also, I suspect the zswap lookup might hurt performance. Would it be
> > better to add the folio back to zswap upon failure? This should be
> > detectable by checking if the folio is dirty as I mentioned in the bug
> > report thread.
>=20
> I don't like the idea either as sync-io is the fast path for zram etc.
> or, can we use
> the way of zram to free compressed data?

I don't think we want to stop doing exclusive loads in zswap due to this
interaction with zram, which shouldn't be common.

I think we can solve this by just writing the folio back to zswap upon
failure as I mentioned.