From: Chengming Zhou <[email protected]>
There is a report of data corruption caused by double swapin, which is
only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
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
the folio in the first time of swapin can't be installed in the pagetable
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 swapfile,
so this data corruption problem happened.
We can fix it by always adding the folio into swapcache if we know the
pinned swap entry can be found in zswap, so it won't get freed even though
it can't be installed successfully in the first time of swapin.
And we have to check if the swap entry is in zswap after entry pinned,
only then we can make sure the check result is stable.
Reported-by: Zhongkun He <[email protected]>
Closes: https://lore.kernel.org/all/CACSyD1N+dUvsu8=zV9P691B9bVq33erwOXNTmEaUbi9DrDeJzw@mail.gmail.com
Signed-off-by: Chengming Zhou <[email protected]>
---
include/linux/zswap.h | 6 ++++++
mm/memory.c | 28 ++++++++++++++++++++++++----
mm/zswap.c | 10 ++++++++++
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/include/linux/zswap.h b/include/linux/zswap.h
index 2a85b941db97..180d0b1f0886 100644
--- a/include/linux/zswap.h
+++ b/include/linux/zswap.h
@@ -36,6 +36,7 @@ void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
void zswap_lruvec_state_init(struct lruvec *lruvec);
void zswap_folio_swapin(struct folio *folio);
bool is_zswap_enabled(void);
+bool zswap_find(swp_entry_t swp);
#else
struct zswap_lruvec_state {};
@@ -65,6 +66,11 @@ static inline bool is_zswap_enabled(void)
return false;
}
+static inline bool zswap_find(swp_entry_t swp)
+{
+ return false;
+}
+
#endif
#endif /* _LINUX_ZSWAP_H */
diff --git a/mm/memory.c b/mm/memory.c
index 4f2caf1c3c4d..a564b2b8faca 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4031,18 +4031,38 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
ret = VM_FAULT_OOM;
goto out_page;
}
+
+ /*
+ * We have to add the folio into swapcache if
+ * this pinned swap entry is found in zswap,
+ * which won't keep copy of data after swapin.
+ * Or data will just get lost if later folio
+ * can't be installed successfully in pagetable.
+ */
+ if (zswap_find(entry)) {
+ if (add_to_swap_cache(folio, entry,
+ GFP_KERNEL, &shadow)) {
+ ret = VM_FAULT_OOM;
+ goto out_page;
+ }
+ swapcache = folio;
+ need_clear_cache = false;
+ } else {
+ shadow = get_shadow_from_swap_cache(entry);
+ /* To provide entry to swap_read_folio() */
+ folio->swap = entry;
+ }
+
mem_cgroup_swapin_uncharge_swap(entry);
- shadow = get_shadow_from_swap_cache(entry);
if (shadow)
workingset_refault(folio, shadow);
folio_add_lru(folio);
- /* To provide entry to swap_read_folio() */
- folio->swap = entry;
swap_read_folio(folio, true, NULL);
- folio->private = NULL;
+ if (need_clear_cache)
+ folio->private = NULL;
}
} else {
page = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE,
diff --git a/mm/zswap.c b/mm/zswap.c
index c4979c76d58e..84a904a788a3 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1601,6 +1601,16 @@ void zswap_invalidate(swp_entry_t swp)
zswap_entry_free(entry);
}
+bool zswap_find(swp_entry_t swp)
+{
+ pgoff_t offset = swp_offset(swp);
+ struct xarray *tree = swap_zswap_tree(swp);
+ struct zswap_entry *entry;
+
+ entry = xa_find(tree, &offset, offset, XA_PRESENT);
+ return entry != NULL;
+}
+
int zswap_swapon(int type, unsigned long nr_pages)
{
struct xarray *trees, *tree;
--
2.20.1
On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
>
> From: Chengming Zhou <[email protected]>
>
> There is a report of data corruption caused by double swapin, which is
> only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
>
> 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
> the folio in the first time of swapin can't be installed in the pagetable
> 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 swapfile,
> so this data corruption problem happened.
>
> We can fix it by always adding the folio into swapcache if we know the
> pinned swap entry can be found in zswap, so it won't get freed even though
> 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.
On Sat, Mar 23, 2024 at 8:38 AM Yosry Ahmed <[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
> >
> > From: Chengming Zhou <[email protected]>
> >
> > There is a report of data corruption caused by double swapin, which is
> > only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
> >
> > 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
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?
shouldn't zswap free the memory after notify_free just like zram?
> > the folio in the first time of swapin can't be installed in the pagetable
> > 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 swapfile,
> > so this data corruption problem happened.
> >
> > We can fix it by always adding the folio into swapcache if we know the
> > pinned swap entry can be found in zswap, so it won't get freed even though
> > 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.
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?
>
On Sat, Mar 23, 2024 at 10:41:32AM +1300, Barry Song wrote:
> On Sat, Mar 23, 2024 at 8:38 AM Yosry Ahmed <[email protected]> wrote:
> >
> > On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
> > >
> > > From: Chengming Zhou <[email protected]>
> > >
> > > There is a report of data corruption caused by double swapin, which is
> > > only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
> > >
> > > 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
>
> 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?
>
> 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.
>
> > > the folio in the first time of swapin can't be installed in the pagetable
> > > 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 swapfile,
> > > so this data corruption problem happened.
> > >
> > > We can fix it by always adding the folio into swapcache if we know the
> > > pinned swap entry can be found in zswap, so it won't get freed even though
> > > 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.
>
> 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.
On Fri, Mar 22, 2024 at 10:33:13PM +0000, Yosry Ahmed wrote:
> On Sat, Mar 23, 2024 at 10:41:32AM +1300, Barry Song wrote:
> > On Sat, Mar 23, 2024 at 8:38 AM Yosry Ahmed <[email protected]> wrote:
> > >
> > > On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
> > > >
> > > > From: Chengming Zhou <[email protected]>
> > > >
> > > > There is a report of data corruption caused by double swapin, which is
> > > > only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
> > > >
> > > > 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
> >
> > 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?
> >
> > 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.
>
> >
> > > > the folio in the first time of swapin can't be installed in the pagetable
> > > > 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 swapfile,
> > > > so this data corruption problem happened.
> > > >
> > > > We can fix it by always adding the folio into swapcache if we know the
> > > > pinned swap entry can be found in zswap, so it won't get freed even though
> > > > 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.
> >
> > 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.
Instead of storing again, can we avoid invalidating the entry in the
first place if the load is not "exclusive"?
The reason for exclusive loads is that the ownership is transferred to
the swapcache, so there is no point in keeping our copy. With an
optimistic read that doesn't transfer ownership, this doesn't
apply. And we can easily tell inside zswap_load() if we're dealing
with a swapcache read or not by testing the folio.
The synchronous read already has to pin the swp_entry_t to be safe,
using swapcache_prepare(). That blocks __read_swap_cache_async() which
means no other (exclusive) loads and no invalidates can occur.
The zswap entry is freed during the regular swap_free() path, which
the sync fault calls on success. Otherwise we keep it.
diff --git a/mm/zswap.c b/mm/zswap.c
index 535c907345e0..686364a6dd86 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
swp_entry_t swp = folio->swap;
pgoff_t offset = swp_offset(swp);
struct page *page = &folio->page;
+ bool swapcache = folio_test_swapcache(folio);
struct zswap_tree *tree = swap_zswap_tree(swp);
struct zswap_entry *entry;
u8 *dst;
@@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
spin_unlock(&tree->lock);
return false;
}
- zswap_rb_erase(&tree->rbroot, entry);
+ if (swapcache)
+ zswap_rb_erase(&tree->rbroot, entry);
spin_unlock(&tree->lock);
if (entry->length)
@@ -1649,9 +1651,10 @@ bool zswap_load(struct folio *folio)
if (entry->objcg)
count_objcg_event(entry->objcg, ZSWPIN);
- zswap_entry_free(entry);
-
- folio_mark_dirty(folio);
+ if (swapcache) {
+ zswap_entry_free(entry);
+ folio_mark_dirty(folio);
+ }
return true;
}
On Fri, Mar 22, 2024 at 4:48 PM Johannes Weiner <[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 10:33:13PM +0000, Yosry Ahmed wrote:
> > On Sat, Mar 23, 2024 at 10:41:32AM +1300, Barry Song wrote:
> > > On Sat, Mar 23, 2024 at 8:38 AM Yosry Ahmed <[email protected]> wrote:
> > > >
> > > > On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
> > > > >
> > > > > From: Chengming Zhou <[email protected]>
> > > > >
> > > > > There is a report of data corruption caused by double swapin, which is
> > > > > only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
> > > > >
> > > > > 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
> > >
> > > 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?
> > >
> > > 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.
> >
> > >
> > > > > the folio in the first time of swapin can't be installed in the pagetable
> > > > > 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 swapfile,
> > > > > so this data corruption problem happened.
> > > > >
> > > > > We can fix it by always adding the folio into swapcache if we know the
> > > > > pinned swap entry can be found in zswap, so it won't get freed even though
> > > > > 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.
> > >
> > > 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.
>
> Instead of storing again, can we avoid invalidating the entry in the
> first place if the load is not "exclusive"?
>
> The reason for exclusive loads is that the ownership is transferred to
> the swapcache, so there is no point in keeping our copy. With an
> optimistic read that doesn't transfer ownership, this doesn't
> apply. And we can easily tell inside zswap_load() if we're dealing
> with a swapcache read or not by testing the folio.
>
> The synchronous read already has to pin the swp_entry_t to be safe,
> using swapcache_prepare(). That blocks __read_swap_cache_async() which
> means no other (exclusive) loads and no invalidates can occur.
>
> The zswap entry is freed during the regular swap_free() path, which
> the sync fault calls on success. Otherwise we keep it.
I thought about this, but I was particularly worried about the need to
bring back the refcount that was removed when we switched to only
supporting exclusive loads:
https://lore.kernel.org/lkml/20240201-b4-zswap-invalidate-entry-v2-6-99d4084260a0@bytedance.com/
It seems to be that we don't need it, because swap_free() will free
the entry as you mentioned before anyone else has the chance to load
it or invalidate it. Writeback used to grab a reference as well, but
it removes the entry from the tree anyway and takes full ownership of
it then frees it, so that should be okay.
It makes me nervous though to be honest. For example, not long ago
swap_free() didn't call zswap_invalidate() directly (used to happen to
swap slots cache draining). Without it, a subsequent load could race
with writeback without refcount protection, right? We would need to
make sure to backport 0827a1fb143f ("mm/zswap: invalidate zswap entry
when swap entry free") with the fix to stable for instance.
I can't find a problem with your diff, but it just makes me nervous to
have non-exclusive loads without a refcount.
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..686364a6dd86 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
> swp_entry_t swp = folio->swap;
> pgoff_t offset = swp_offset(swp);
> struct page *page = &folio->page;
> + bool swapcache = folio_test_swapcache(folio);
> struct zswap_tree *tree = swap_zswap_tree(swp);
> struct zswap_entry *entry;
> u8 *dst;
> @@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
> spin_unlock(&tree->lock);
> return false;
> }
> - zswap_rb_erase(&tree->rbroot, entry);
> + if (swapcache)
> + zswap_rb_erase(&tree->rbroot, entry);
> spin_unlock(&tree->lock);
>
> if (entry->length)
> @@ -1649,9 +1651,10 @@ bool zswap_load(struct folio *folio)
> if (entry->objcg)
> count_objcg_event(entry->objcg, ZSWPIN);
>
> - zswap_entry_free(entry);
> -
> - folio_mark_dirty(folio);
> + if (swapcache) {
> + zswap_entry_free(entry);
> + folio_mark_dirty(folio);
> + }
>
> return true;
> }
[..]
> > > 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.
> >
> > Instead of storing again, can we avoid invalidating the entry in the
> > first place if the load is not "exclusive"?
> >
> > The reason for exclusive loads is that the ownership is transferred to
> > the swapcache, so there is no point in keeping our copy. With an
> > optimistic read that doesn't transfer ownership, this doesn't
> > apply. And we can easily tell inside zswap_load() if we're dealing
> > with a swapcache read or not by testing the folio.
> >
> > The synchronous read already has to pin the swp_entry_t to be safe,
> > using swapcache_prepare(). That blocks __read_swap_cache_async() which
> > means no other (exclusive) loads and no invalidates can occur.
> >
> > The zswap entry is freed during the regular swap_free() path, which
> > the sync fault calls on success. Otherwise we keep it.
>
> I thought about this, but I was particularly worried about the need to
> bring back the refcount that was removed when we switched to only
> supporting exclusive loads:
> https://lore.kernel.org/lkml/20240201-b4-zswap-invalidate-entry-v2-6-99d4084260a0@bytedance.com/
>
> It seems to be that we don't need it, because swap_free() will free
> the entry as you mentioned before anyone else has the chance to load
> it or invalidate it. Writeback used to grab a reference as well, but
> it removes the entry from the tree anyway and takes full ownership of
> it then frees it, so that should be okay.
>
> It makes me nervous though to be honest. For example, not long ago
> swap_free() didn't call zswap_invalidate() directly (used to happen to
> swap slots cache draining). Without it, a subsequent load could race
> with writeback without refcount protection, right? We would need to
> make sure to backport 0827a1fb143f ("mm/zswap: invalidate zswap entry
> when swap entry free") with the fix to stable for instance.
>
> I can't find a problem with your diff, but it just makes me nervous to
> have non-exclusive loads without a refcount.
>
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 535c907345e0..686364a6dd86 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
> > swp_entry_t swp = folio->swap;
> > pgoff_t offset = swp_offset(swp);
> > struct page *page = &folio->page;
> > + bool swapcache = folio_test_swapcache(folio);
> > struct zswap_tree *tree = swap_zswap_tree(swp);
> > struct zswap_entry *entry;
> > u8 *dst;
> > @@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
> > spin_unlock(&tree->lock);
> > return false;
> > }
> > - zswap_rb_erase(&tree->rbroot, entry);
> > + if (swapcache)
> > + zswap_rb_erase(&tree->rbroot, entry);
On second thought, if we don't remove the entry from the tree here,
writeback could free the entry from under us after we drop the lock
here, right?
> > spin_unlock(&tree->lock);
> >
> > if (entry->length)
> > @@ -1649,9 +1651,10 @@ bool zswap_load(struct folio *folio)
> > if (entry->objcg)
> > count_objcg_event(entry->objcg, ZSWPIN);
> >
> > - zswap_entry_free(entry);
> > -
> > - folio_mark_dirty(folio);
> > + if (swapcache) {
> > + zswap_entry_free(entry);
> > + folio_mark_dirty(folio);
> > + }
> >
> > return true;
> > }
On Fri, Mar 22, 2024 at 05:14:37PM -0700, Yosry Ahmed wrote:
> [..]
> > > > 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.
> > >
> > > Instead of storing again, can we avoid invalidating the entry in the
> > > first place if the load is not "exclusive"?
> > >
> > > The reason for exclusive loads is that the ownership is transferred to
> > > the swapcache, so there is no point in keeping our copy. With an
> > > optimistic read that doesn't transfer ownership, this doesn't
> > > apply. And we can easily tell inside zswap_load() if we're dealing
> > > with a swapcache read or not by testing the folio.
> > >
> > > The synchronous read already has to pin the swp_entry_t to be safe,
> > > using swapcache_prepare(). That blocks __read_swap_cache_async() which
> > > means no other (exclusive) loads and no invalidates can occur.
> > >
> > > The zswap entry is freed during the regular swap_free() path, which
> > > the sync fault calls on success. Otherwise we keep it.
> >
> > I thought about this, but I was particularly worried about the need to
> > bring back the refcount that was removed when we switched to only
> > supporting exclusive loads:
> > https://lore.kernel.org/lkml/20240201-b4-zswap-invalidate-entry-v2-6-99d4084260a0@bytedance.com/
> >
> > It seems to be that we don't need it, because swap_free() will free
> > the entry as you mentioned before anyone else has the chance to load
> > it or invalidate it. Writeback used to grab a reference as well, but
> > it removes the entry from the tree anyway and takes full ownership of
> > it then frees it, so that should be okay.
> >
> > It makes me nervous though to be honest. For example, not long ago
> > swap_free() didn't call zswap_invalidate() directly (used to happen to
> > swap slots cache draining). Without it, a subsequent load could race
> > with writeback without refcount protection, right? We would need to
> > make sure to backport 0827a1fb143f ("mm/zswap: invalidate zswap entry
> > when swap entry free") with the fix to stable for instance.
> >
> > I can't find a problem with your diff, but it just makes me nervous to
> > have non-exclusive loads without a refcount.
> >
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 535c907345e0..686364a6dd86 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
> > > swp_entry_t swp = folio->swap;
> > > pgoff_t offset = swp_offset(swp);
> > > struct page *page = &folio->page;
> > > + bool swapcache = folio_test_swapcache(folio);
> > > struct zswap_tree *tree = swap_zswap_tree(swp);
> > > struct zswap_entry *entry;
> > > u8 *dst;
> > > @@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
> > > spin_unlock(&tree->lock);
> > > return false;
> > > }
> > > - zswap_rb_erase(&tree->rbroot, entry);
> > > + if (swapcache)
> > > + zswap_rb_erase(&tree->rbroot, entry);
>
> On second thought, if we don't remove the entry from the tree here,
> writeback could free the entry from under us after we drop the lock
> here, right?
The sync-swapin does swapcache_prepare() and holds SWAP_HAS_CACHE, so
racing writeback would loop on the -EEXIST in __read_swap_cache_async().
(Or, if writeback wins the race, sync-swapin fails on swapcache_prepare()
instead and bails on the fault.)
This isn't coincidental. The sync-swapin needs to, and does, serialize
against the swap entry moving into swapcache or being invalidated for
it to be safe. Which is the same requirement that zswap ops have.
On Fri, Mar 22, 2024 at 6:55 PM Johannes Weiner <[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 05:14:37PM -0700, Yosry Ahmed wrote:
> > [..]
> > > > > 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.
> > > >
> > > > Instead of storing again, can we avoid invalidating the entry in the
> > > > first place if the load is not "exclusive"?
> > > >
> > > > The reason for exclusive loads is that the ownership is transferred to
> > > > the swapcache, so there is no point in keeping our copy. With an
> > > > optimistic read that doesn't transfer ownership, this doesn't
> > > > apply. And we can easily tell inside zswap_load() if we're dealing
> > > > with a swapcache read or not by testing the folio.
> > > >
> > > > The synchronous read already has to pin the swp_entry_t to be safe,
> > > > using swapcache_prepare(). That blocks __read_swap_cache_async() which
> > > > means no other (exclusive) loads and no invalidates can occur.
> > > >
> > > > The zswap entry is freed during the regular swap_free() path, which
> > > > the sync fault calls on success. Otherwise we keep it.
> > >
> > > I thought about this, but I was particularly worried about the need to
> > > bring back the refcount that was removed when we switched to only
> > > supporting exclusive loads:
> > > https://lore.kernel.org/lkml/20240201-b4-zswap-invalidate-entry-v2-6-99d4084260a0@bytedance.com/
> > >
> > > It seems to be that we don't need it, because swap_free() will free
> > > the entry as you mentioned before anyone else has the chance to load
> > > it or invalidate it. Writeback used to grab a reference as well, but
> > > it removes the entry from the tree anyway and takes full ownership of
> > > it then frees it, so that should be okay.
> > >
> > > It makes me nervous though to be honest. For example, not long ago
> > > swap_free() didn't call zswap_invalidate() directly (used to happen to
> > > swap slots cache draining). Without it, a subsequent load could race
> > > with writeback without refcount protection, right? We would need to
> > > make sure to backport 0827a1fb143f ("mm/zswap: invalidate zswap entry
> > > when swap entry free") with the fix to stable for instance.
> > >
> > > I can't find a problem with your diff, but it just makes me nervous to
> > > have non-exclusive loads without a refcount.
> > >
> > > >
> > > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > > index 535c907345e0..686364a6dd86 100644
> > > > --- a/mm/zswap.c
> > > > +++ b/mm/zswap.c
> > > > @@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
> > > > swp_entry_t swp = folio->swap;
> > > > pgoff_t offset = swp_offset(swp);
> > > > struct page *page = &folio->page;
> > > > + bool swapcache = folio_test_swapcache(folio);
> > > > struct zswap_tree *tree = swap_zswap_tree(swp);
> > > > struct zswap_entry *entry;
> > > > u8 *dst;
> > > > @@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
> > > > spin_unlock(&tree->lock);
> > > > return false;
> > > > }
> > > > - zswap_rb_erase(&tree->rbroot, entry);
> > > > + if (swapcache)
> > > > + zswap_rb_erase(&tree->rbroot, entry);
> >
> > On second thought, if we don't remove the entry from the tree here,
> > writeback could free the entry from under us after we drop the lock
> > here, right?
>
> The sync-swapin does swapcache_prepare() and holds SWAP_HAS_CACHE, so
> racing writeback would loop on the -EEXIST in __read_swap_cache_async().
> (Or, if writeback wins the race, sync-swapin fails on swapcache_prepare()
> instead and bails on the fault.)
>
> This isn't coincidental. The sync-swapin needs to, and does, serialize
> against the swap entry moving into swapcache or being invalidated for
> it to be safe. Which is the same requirement that zswap ops have.
You are right. Even if swap_free() isn't called under SWAP_HAS_CACHE's
protection, a subsequent load will also be protected by SWAP_HAS_CACHE
(whether it's swapped in with sync swapin or throught the swapcache)
-- so it would be protected against writeback as well. Now it seems
like we may have been able to drop the refcount even without exclusive
loads..?
Anyway, I think your fix is sound. Zhongkun, do you mind confirming
that the diff Johannes sent fixes the problem for you?
On Sat, Mar 23, 2024 at 10:03 AM Yosry Ahmed <[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 6:55 PM Johannes Weiner <[email protected]> wrote:
> >
> > On Fri, Mar 22, 2024 at 05:14:37PM -0700, Yosry Ahmed wrote:
> > > [..]
> > > > > > 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.
> > > > >
> > > > > Instead of storing again, can we avoid invalidating the entry in the
> > > > > first place if the load is not "exclusive"?
> > > > >
> > > > > The reason for exclusive loads is that the ownership is transferred to
> > > > > the swapcache, so there is no point in keeping our copy. With an
> > > > > optimistic read that doesn't transfer ownership, this doesn't
> > > > > apply. And we can easily tell inside zswap_load() if we're dealing
> > > > > with a swapcache read or not by testing the folio.
> > > > >
> > > > > The synchronous read already has to pin the swp_entry_t to be safe,
> > > > > using swapcache_prepare(). That blocks __read_swap_cache_async() which
> > > > > means no other (exclusive) loads and no invalidates can occur.
> > > > >
> > > > > The zswap entry is freed during the regular swap_free() path, which
> > > > > the sync fault calls on success. Otherwise we keep it.
> > > >
> > > > I thought about this, but I was particularly worried about the need to
> > > > bring back the refcount that was removed when we switched to only
> > > > supporting exclusive loads:
> > > > https://lore.kernel.org/lkml/20240201-b4-zswap-invalidate-entry-v2-6-99d4084260a0@bytedance.com/
> > > >
> > > > It seems to be that we don't need it, because swap_free() will free
> > > > the entry as you mentioned before anyone else has the chance to load
> > > > it or invalidate it. Writeback used to grab a reference as well, but
> > > > it removes the entry from the tree anyway and takes full ownership of
> > > > it then frees it, so that should be okay.
> > > >
> > > > It makes me nervous though to be honest. For example, not long ago
> > > > swap_free() didn't call zswap_invalidate() directly (used to happen to
> > > > swap slots cache draining). Without it, a subsequent load could race
> > > > with writeback without refcount protection, right? We would need to
> > > > make sure to backport 0827a1fb143f ("mm/zswap: invalidate zswap entry
> > > > when swap entry free") with the fix to stable for instance.
> > > >
> > > > I can't find a problem with your diff, but it just makes me nervous to
> > > > have non-exclusive loads without a refcount.
> > > >
> > > > >
> > > > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > > > index 535c907345e0..686364a6dd86 100644
> > > > > --- a/mm/zswap.c
> > > > > +++ b/mm/zswap.c
> > > > > @@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
> > > > > swp_entry_t swp = folio->swap;
> > > > > pgoff_t offset = swp_offset(swp);
> > > > > struct page *page = &folio->page;
> > > > > + bool swapcache = folio_test_swapcache(folio);
> > > > > struct zswap_tree *tree = swap_zswap_tree(swp);
> > > > > struct zswap_entry *entry;
> > > > > u8 *dst;
> > > > > @@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
> > > > > spin_unlock(&tree->lock);
> > > > > return false;
> > > > > }
> > > > > - zswap_rb_erase(&tree->rbroot, entry);
> > > > > + if (swapcache)
> > > > > + zswap_rb_erase(&tree->rbroot, entry);
> > >
> > > On second thought, if we don't remove the entry from the tree here,
> > > writeback could free the entry from under us after we drop the lock
> > > here, right?
> >
> > The sync-swapin does swapcache_prepare() and holds SWAP_HAS_CACHE, so
> > racing writeback would loop on the -EEXIST in __read_swap_cache_async().
> > (Or, if writeback wins the race, sync-swapin fails on swapcache_prepare()
> > instead and bails on the fault.)
> >
> > This isn't coincidental. The sync-swapin needs to, and does, serialize
> > against the swap entry moving into swapcache or being invalidated for
> > it to be safe. Which is the same requirement that zswap ops have.
>
> You are right. Even if swap_free() isn't called under SWAP_HAS_CACHE's
> protection, a subsequent load will also be protected by SWAP_HAS_CACHE
> (whether it's swapped in with sync swapin or throught the swapcache)
> -- so it would be protected against writeback as well. Now it seems
> like we may have been able to drop the refcount even without exclusive
> loads..?
>
> Anyway, I think your fix is sound. Zhongkun, do you mind confirming
> that the diff Johannes sent fixes the problem for you?
OK， I will try it and come back in a few hours.
Thanks for the solution, it sounds great.
On 2024/3/23 03:37, Yosry Ahmed wrote:
> On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
>>
>> From: Chengming Zhou <[email protected]>
>>
>> There is a report of data corruption caused by double swapin, which is
>> only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
>>
>> 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
>> the folio in the first time of swapin can't be installed in the pagetable
>> 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 swapfile,
>> so this data corruption problem happened.
>>
>> We can fix it by always adding the folio into swapcache if we know the
>> pinned swap entry can be found in zswap, so it won't get freed even though
>> 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.
Right, but it has to lock the folio to proceed.
>
> 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.
Yes, may hurt performance. As for adding back upon failure, the problem
is that adding may fail too... and I don't know how to handle that.
Anyway, I think the fix of Johannes is much better, we should take that way.
Thanks.
On Sat, Mar 23, 2024 at 7:48 AM Johannes Weiner <[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 10:33:13PM +0000, Yosry Ahmed wrote:
> > On Sat, Mar 23, 2024 at 10:41:32AM +1300, Barry Song wrote:
> > > On Sat, Mar 23, 2024 at 8:38 AM Yosry Ahmed <[email protected]> wrote:
> > > >
> > > > On Fri, Mar 22, 2024 at 9:40 AM <[email protected]> wrote:
> > > > >
> > > > > From: Chengming Zhou <[email protected]>
> > > > >
> > > > > There is a report of data corruption caused by double swapin, which is
> > > > > only possible in the skip swapcache path on SWP_SYNCHRONOUS_IO backends.
> > > > >
> > > > > 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
> > >
> > > 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?
> > >
> > > 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.
> >
> > >
> > > > > the folio in the first time of swapin can't be installed in the pagetable
> > > > > 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 swapfile,
> > > > > so this data corruption problem happened.
> > > > >
> > > > > We can fix it by always adding the folio into swapcache if we know the
> > > > > pinned swap entry can be found in zswap, so it won't get freed even though
> > > > > 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.
> > >
> > > 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.
>
> Instead of storing again, can we avoid invalidating the entry in the
> first place if the load is not "exclusive"?
>
> The reason for exclusive loads is that the ownership is transferred to
> the swapcache, so there is no point in keeping our copy. With an
> optimistic read that doesn't transfer ownership, this doesn't
> apply. And we can easily tell inside zswap_load() if we're dealing
> with a swapcache read or not by testing the folio.
>
> The synchronous read already has to pin the swp_entry_t to be safe,
> using swapcache_prepare(). That blocks __read_swap_cache_async() which
> means no other (exclusive) loads and no invalidates can occur.
>
> The zswap entry is freed during the regular swap_free() path, which
> the sync fault calls on success. Otherwise we keep it.
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..686364a6dd86 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1622,6 +1622,7 @@ bool zswap_load(struct folio *folio)
> swp_entry_t swp = folio->swap;
> pgoff_t offset = swp_offset(swp);
> struct page *page = &folio->page;
> + bool swapcache = folio_test_swapcache(folio);
> struct zswap_tree *tree = swap_zswap_tree(swp);
> struct zswap_entry *entry;
> u8 *dst;
> @@ -1634,7 +1635,8 @@ bool zswap_load(struct folio *folio)
> spin_unlock(&tree->lock);
> return false;
> }
> - zswap_rb_erase(&tree->rbroot, entry);
> + if (swapcache)
> + zswap_rb_erase(&tree->rbroot, entry);
> spin_unlock(&tree->lock);
>
> if (entry->length)
> @@ -1649,9 +1651,10 @@ bool zswap_load(struct folio *folio)
> if (entry->objcg)
> count_objcg_event(entry->objcg, ZSWPIN);
>
> - zswap_entry_free(entry);
> -
> - folio_mark_dirty(folio);
> + if (swapcache) {
> + zswap_entry_free(entry);
> + folio_mark_dirty(folio);
> + }
>
> return true;
> }
Hi Johannes,
After a few hours I haven't seen any problems.
If you don't mind，please add the following tag:
Tested-by:Zhongkun He <[email protected]>
Reported-by: Zhongkun He <[email protected]>
Thanks.