mm: zswap: fix data loss on SWP_SYNCHRONOUS_IO devices

Commit Message

Johannes Weiner March 24, 2024, 9:04 p.m. UTC

Zhongkun He reports data corruption when combining zswap with zram.

The issue is the exclusive loads we're doing in zswap. They assume
that all reads are going into the swapcache, which can assume
authoritative ownership of the data and so the zswap copy can go.

However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
to bypass the swapcache. This results in an optimistic read of the
swap data into a page that will be dismissed if the fault fails due to
races. In this case, zswap mustn't drop its authoritative copy.

Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
Reported-by: Zhongkun He <[e-mail]>
Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
Cc: [e-mail]	[6.5+]
Signed-off-by: Johannes Weiner <[e-mail]>
Tested-by: Zhongkun He <[e-mail]>
---
 mm/zswap.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Yosry Ahmed March 24, 2024, 9:22 p.m. UTC | #1

On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
>
> Zhongkun He reports data corruption when combining zswap with zram.
>
> The issue is the exclusive loads we're doing in zswap. They assume
> that all reads are going into the swapcache, which can assume
> authoritative ownership of the data and so the zswap copy can go.
>
> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> to bypass the swapcache. This results in an optimistic read of the
> swap data into a page that will be dismissed if the fault fails due to
> races. In this case, zswap mustn't drop its authoritative copy.
>
> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> Reported-by: Zhongkun He <[e-mail]>
> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> Cc: [e-mail]      [6.5+]
> Signed-off-by: Johannes Weiner <[e-mail]>
> Tested-by: Zhongkun He <[e-mail]>

Do we also want to mention somewhere (commit log or comment) that
keeping the entry in the tree is fine because we are still protected
from concurrent loads/invalidations/writeback by swapcache_prepare()
setting SWAP_HAS_CACHE or so?

Anyway, this LGTM.

Acked-by: Yosry Ahmed <[e-mail]>

> ---
>  mm/zswap.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..41a1170f7cfe 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,20 @@ bool zswap_load(struct folio *folio)
>                 spin_unlock(&tree->lock);
>                 return false;
>         }
> -       zswap_rb_erase(&tree->rbroot, entry);
> +       /*
> +        * When reading into the swapcache, invalidate our entry. The
> +        * swapcache can be the authoritative owner of the page and
> +        * its mappings, and the pressure that results from having two
> +        * in-memory copies outweighs any benefits of caching the
> +        * compression work.
> +        *
> +        * (Most swapins go through the swapcache. The notable
> +        * exception is the singleton fault on SWP_SYNCHRONOUS_IO
> +        * files, which reads into a private page and may free it if
> +        * the fault fails. We remain the primary owner of the entry.)
> +        */
> +       if (swapcache)
> +               zswap_rb_erase(&tree->rbroot, entry);
>         spin_unlock(&tree->lock);
>
>         if (entry->length)
> @@ -1649,9 +1663,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;
>  }
> --
> 2.44.0
>

Chengming Zhou March 25, 2024, 12:01 a.m. UTC | #2

On 2024/3/25 05:04, Johannes Weiner wrote:
> Zhongkun He reports data corruption when combining zswap with zram.
> 
> The issue is the exclusive loads we're doing in zswap. They assume
> that all reads are going into the swapcache, which can assume
> authoritative ownership of the data and so the zswap copy can go.
> 
> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> to bypass the swapcache. This results in an optimistic read of the
> swap data into a page that will be dismissed if the fault fails due to
> races. In this case, zswap mustn't drop its authoritative copy.
> 
> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> Reported-by: Zhongkun He <[e-mail]>
> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> Cc: [e-mail]	[6.5+]
> Signed-off-by: Johannes Weiner <[e-mail]>
> Tested-by: Zhongkun He <[e-mail]>

Very nice solution!

Reviewed-by: Chengming Zhou <[e-mail]>

Thanks.

> ---
>  mm/zswap.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..41a1170f7cfe 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,20 @@ bool zswap_load(struct folio *folio)
>  		spin_unlock(&tree->lock);
>  		return false;
>  	}
> -	zswap_rb_erase(&tree->rbroot, entry);
> +	/*
> +	 * When reading into the swapcache, invalidate our entry. The
> +	 * swapcache can be the authoritative owner of the page and
> +	 * its mappings, and the pressure that results from having two
> +	 * in-memory copies outweighs any benefits of caching the
> +	 * compression work.
> +	 *
> +	 * (Most swapins go through the swapcache. The notable
> +	 * exception is the singleton fault on SWP_SYNCHRONOUS_IO
> +	 * files, which reads into a private page and may free it if
> +	 * the fault fails. We remain the primary owner of the entry.)
> +	 */
> +	if (swapcache)
> +		zswap_rb_erase(&tree->rbroot, entry);
>  	spin_unlock(&tree->lock);
>  
>  	if (entry->length)
> @@ -1649,9 +1663,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;
>  }

Zhongkun He March 25, 2024, 3:01 a.m. UTC | #3

On Mon, Mar 25, 2024 at 5:05 AM Johannes Weiner <[e-mail]> wrote:
>
> Zhongkun He reports data corruption when combining zswap with zram.
>
> The issue is the exclusive loads we're doing in zswap. They assume
> that all reads are going into the swapcache, which can assume
> authoritative ownership of the data and so the zswap copy can go.
>
> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> to bypass the swapcache. This results in an optimistic read of the
> swap data into a page that will be dismissed if the fault fails due to
> races. In this case, zswap mustn't drop its authoritative copy.
>
> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> Reported-by: Zhongkun He <[e-mail]>
> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> Cc: [e-mail]      [6.5+]
> Signed-off-by: Johannes Weiner <[e-mail]>
> Tested-by: Zhongkun He <[e-mail]>
> ---
>  mm/zswap.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..41a1170f7cfe 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,20 @@ bool zswap_load(struct folio *folio)
>                 spin_unlock(&tree->lock);
>                 return false;
>         }
> -       zswap_rb_erase(&tree->rbroot, entry);
> +       /*
> +        * When reading into the swapcache, invalidate our entry. The
> +        * swapcache can be the authoritative owner of the page and
> +        * its mappings, and the pressure that results from having two
> +        * in-memory copies outweighs any benefits of caching the
> +        * compression work.
> +        *
> +        * (Most swapins go through the swapcache. The notable
> +        * exception is the singleton fault on SWP_SYNCHRONOUS_IO
> +        * files, which reads into a private page and may free it if
> +        * the fault fails. We remain the primary owner of the entry.)
> +        */
> +       if (swapcache)
> +               zswap_rb_erase(&tree->rbroot, entry);
>         spin_unlock(&tree->lock);
>
>         if (entry->length)
> @@ -1649,9 +1663,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;
>  }
> --
> 2.44.0
>

Good solution and makes great sense to me.

Thanks.

Barry Song March 25, 2024, 4:54 a.m. UTC | #4

On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
>
> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> >
> > Zhongkun He reports data corruption when combining zswap with zram.
> >
> > The issue is the exclusive loads we're doing in zswap. They assume
> > that all reads are going into the swapcache, which can assume
> > authoritative ownership of the data and so the zswap copy can go.
> >
> > However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> > to bypass the swapcache. This results in an optimistic read of the
> > swap data into a page that will be dismissed if the fault fails due to
> > races. In this case, zswap mustn't drop its authoritative copy.
> >
> > Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> > Reported-by: Zhongkun He <[e-mail]>
> > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> > Cc: [e-mail]      [6.5+]
> > Signed-off-by: Johannes Weiner <[e-mail]>
> > Tested-by: Zhongkun He <[e-mail]>

Acked-by: Barry Song <[e-mail]>

>
> Do we also want to mention somewhere (commit log or comment) that
> keeping the entry in the tree is fine because we are still protected
> from concurrent loads/invalidations/writeback by swapcache_prepare()
> setting SWAP_HAS_CACHE or so?

It seems that Kairui's patch comprehensively addresses the issue at hand.
Johannes's solution, on the other hand, appears to align zswap behavior
more closely with that of a traditional swap device, only releasing an entry
when the corresponding swap slot is freed, particularly in the sync-io case.

Johannes' patch has inspired me to consider whether zRAM could achieve
a comparable outcome by immediately releasing objects in swap cache
scenarios.  When I have the opportunity, I plan to experiment with zRAM.

>
> Anyway, this LGTM.
>
> Acked-by: Yosry Ahmed <[e-mail]>
>
> > ---
> >  mm/zswap.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 535c907345e0..41a1170f7cfe 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,20 @@ bool zswap_load(struct folio *folio)
> >                 spin_unlock(&tree->lock);
> >                 return false;
> >         }
> > -       zswap_rb_erase(&tree->rbroot, entry);
> > +       /*
> > +        * When reading into the swapcache, invalidate our entry. The
> > +        * swapcache can be the authoritative owner of the page and
> > +        * its mappings, and the pressure that results from having two
> > +        * in-memory copies outweighs any benefits of caching the
> > +        * compression work.
> > +        *
> > +        * (Most swapins go through the swapcache. The notable
> > +        * exception is the singleton fault on SWP_SYNCHRONOUS_IO
> > +        * files, which reads into a private page and may free it if
> > +        * the fault fails. We remain the primary owner of the entry.)
> > +        */
> > +       if (swapcache)
> > +               zswap_rb_erase(&tree->rbroot, entry);
> >         spin_unlock(&tree->lock);
> >
> >         if (entry->length)
> > @@ -1649,9 +1663,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;
> >  }
> > --
> > 2.44.0

Thanks
Barry

Yosry Ahmed March 25, 2024, 7:06 a.m. UTC | #5

On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
>
> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
> >
> > On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> > >
> > > Zhongkun He reports data corruption when combining zswap with zram.
> > >
> > > The issue is the exclusive loads we're doing in zswap. They assume
> > > that all reads are going into the swapcache, which can assume
> > > authoritative ownership of the data and so the zswap copy can go.
> > >
> > > However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> > > to bypass the swapcache. This results in an optimistic read of the
> > > swap data into a page that will be dismissed if the fault fails due to
> > > races. In this case, zswap mustn't drop its authoritative copy.
> > >
> > > Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> > > Reported-by: Zhongkun He <[e-mail]>
> > > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> > > Cc: [e-mail]      [6.5+]
> > > Signed-off-by: Johannes Weiner <[e-mail]>
> > > Tested-by: Zhongkun He <[e-mail]>
>
> Acked-by: Barry Song <[e-mail]>
>
> >
> > Do we also want to mention somewhere (commit log or comment) that
> > keeping the entry in the tree is fine because we are still protected
> > from concurrent loads/invalidations/writeback by swapcache_prepare()
> > setting SWAP_HAS_CACHE or so?
>
> It seems that Kairui's patch comprehensively addresses the issue at hand.
> Johannes's solution, on the other hand, appears to align zswap behavior
> more closely with that of a traditional swap device, only releasing an entry
> when the corresponding swap slot is freed, particularly in the sync-io case.

It actually worked out quite well that Kairui's fix landed shortly
before this bug was reported, as this fix wouldn't have been possible
without it as far as I can tell.

>
> Johannes' patch has inspired me to consider whether zRAM could achieve
> a comparable outcome by immediately releasing objects in swap cache
> scenarios.  When I have the opportunity, I plan to experiment with zRAM.

That would be interesting. I am curious if it would be as
straightforward in zram to just mark the folio as dirty in this case
like zswap does, given its implementation as a block device.

Chengming Zhou March 25, 2024, 7:33 a.m. UTC | #6

On 2024/3/25 15:06, Yosry Ahmed wrote:
> On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
>>
>> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
>>>
>>> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
>>>>
>>>> Zhongkun He reports data corruption when combining zswap with zram.
>>>>
>>>> The issue is the exclusive loads we're doing in zswap. They assume
>>>> that all reads are going into the swapcache, which can assume
>>>> authoritative ownership of the data and so the zswap copy can go.
>>>>
>>>> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
>>>> to bypass the swapcache. This results in an optimistic read of the
>>>> swap data into a page that will be dismissed if the fault fails due to
>>>> races. In this case, zswap mustn't drop its authoritative copy.
>>>>
>>>> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
>>>> Reported-by: Zhongkun He <[e-mail]>
>>>> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
>>>> Cc: [e-mail]      [6.5+]
>>>> Signed-off-by: Johannes Weiner <[e-mail]>
>>>> Tested-by: Zhongkun He <[e-mail]>
>>
>> Acked-by: Barry Song <[e-mail]>
>>
>>>
>>> Do we also want to mention somewhere (commit log or comment) that
>>> keeping the entry in the tree is fine because we are still protected
>>> from concurrent loads/invalidations/writeback by swapcache_prepare()
>>> setting SWAP_HAS_CACHE or so?
>>
>> It seems that Kairui's patch comprehensively addresses the issue at hand.
>> Johannes's solution, on the other hand, appears to align zswap behavior
>> more closely with that of a traditional swap device, only releasing an entry
>> when the corresponding swap slot is freed, particularly in the sync-io case.
> 
> It actually worked out quite well that Kairui's fix landed shortly
> before this bug was reported, as this fix wouldn't have been possible
> without it as far as I can tell.
> 
>>
>> Johannes' patch has inspired me to consider whether zRAM could achieve
>> a comparable outcome by immediately releasing objects in swap cache
>> scenarios.  When I have the opportunity, I plan to experiment with zRAM.
> 
> That would be interesting. I am curious if it would be as
> straightforward in zram to just mark the folio as dirty in this case
> like zswap does, given its implementation as a block device.
> 

This makes me wonder who is responsible for marking folio dirty in this swapcache
bypass case? Should we call folio_mark_dirty() after the swap_read_folio()?

Yosry Ahmed March 25, 2024, 8:38 a.m. UTC | #7

On Mon, Mar 25, 2024 at 12:33 AM Chengming Zhou
<[e-mail]> wrote:
>
> On 2024/3/25 15:06, Yosry Ahmed wrote:
> > On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
> >>
> >> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
> >>>
> >>> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> >>>>
> >>>> Zhongkun He reports data corruption when combining zswap with zram.
> >>>>
> >>>> The issue is the exclusive loads we're doing in zswap. They assume
> >>>> that all reads are going into the swapcache, which can assume
> >>>> authoritative ownership of the data and so the zswap copy can go.
> >>>>
> >>>> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> >>>> to bypass the swapcache. This results in an optimistic read of the
> >>>> swap data into a page that will be dismissed if the fault fails due to
> >>>> races. In this case, zswap mustn't drop its authoritative copy.
> >>>>
> >>>> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> >>>> Reported-by: Zhongkun He <[e-mail]>
> >>>> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> >>>> Cc: [e-mail]      [6.5+]
> >>>> Signed-off-by: Johannes Weiner <[e-mail]>
> >>>> Tested-by: Zhongkun He <[e-mail]>
> >>
> >> Acked-by: Barry Song <[e-mail]>
> >>
> >>>
> >>> Do we also want to mention somewhere (commit log or comment) that
> >>> keeping the entry in the tree is fine because we are still protected
> >>> from concurrent loads/invalidations/writeback by swapcache_prepare()
> >>> setting SWAP_HAS_CACHE or so?
> >>
> >> It seems that Kairui's patch comprehensively addresses the issue at hand.
> >> Johannes's solution, on the other hand, appears to align zswap behavior
> >> more closely with that of a traditional swap device, only releasing an entry
> >> when the corresponding swap slot is freed, particularly in the sync-io case.
> >
> > It actually worked out quite well that Kairui's fix landed shortly
> > before this bug was reported, as this fix wouldn't have been possible
> > without it as far as I can tell.
> >
> >>
> >> Johannes' patch has inspired me to consider whether zRAM could achieve
> >> a comparable outcome by immediately releasing objects in swap cache
> >> scenarios.  When I have the opportunity, I plan to experiment with zRAM.
> >
> > That would be interesting. I am curious if it would be as
> > straightforward in zram to just mark the folio as dirty in this case
> > like zswap does, given its implementation as a block device.
> >
>
> This makes me wonder who is responsible for marking folio dirty in this swapcache
> bypass case? Should we call folio_mark_dirty() after the swap_read_folio()?

In shrink_folio_list(), we try to add anonymous folios to the
swapcache if they are not there before checking if they are dirty.
add_to_swap() calls folio_mark_dirty(), so this should take care of
it. There is an interesting comment there though. It says that PTE
should be dirty, so unmapping the folio should have already marked it
as dirty by the time we are adding it to the swapcache, except for the
MADV_FREE case.

However, I think we actually unmap the folio after we add it to the
swapcache in shrink_folio_list(). Also, I don't immediately see why
the PTE would be dirty. In do_swap_page(), making the PTE dirty seems
to be conditional on the fault being a write fault, but I didn't look
thoroughly, maybe I missed it. It is also possible that the comment is
just outdated.

Chengming Zhou March 25, 2024, 9:22 a.m. UTC | #8

On 2024/3/25 16:38, Yosry Ahmed wrote:
> On Mon, Mar 25, 2024 at 12:33 AM Chengming Zhou
> <[e-mail]> wrote:
>>
>> On 2024/3/25 15:06, Yosry Ahmed wrote:
>>> On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
>>>>
>>>> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
>>>>>
>>>>> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
>>>>>>
>>>>>> Zhongkun He reports data corruption when combining zswap with zram.
>>>>>>
>>>>>> The issue is the exclusive loads we're doing in zswap. They assume
>>>>>> that all reads are going into the swapcache, which can assume
>>>>>> authoritative ownership of the data and so the zswap copy can go.
>>>>>>
>>>>>> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
>>>>>> to bypass the swapcache. This results in an optimistic read of the
>>>>>> swap data into a page that will be dismissed if the fault fails due to
>>>>>> races. In this case, zswap mustn't drop its authoritative copy.
>>>>>>
>>>>>> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
>>>>>> Reported-by: Zhongkun He <[e-mail]>
>>>>>> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
>>>>>> Cc: [e-mail]      [6.5+]
>>>>>> Signed-off-by: Johannes Weiner <[e-mail]>
>>>>>> Tested-by: Zhongkun He <[e-mail]>
>>>>
>>>> Acked-by: Barry Song <[e-mail]>
>>>>
>>>>>
>>>>> Do we also want to mention somewhere (commit log or comment) that
>>>>> keeping the entry in the tree is fine because we are still protected
>>>>> from concurrent loads/invalidations/writeback by swapcache_prepare()
>>>>> setting SWAP_HAS_CACHE or so?
>>>>
>>>> It seems that Kairui's patch comprehensively addresses the issue at hand.
>>>> Johannes's solution, on the other hand, appears to align zswap behavior
>>>> more closely with that of a traditional swap device, only releasing an entry
>>>> when the corresponding swap slot is freed, particularly in the sync-io case.
>>>
>>> It actually worked out quite well that Kairui's fix landed shortly
>>> before this bug was reported, as this fix wouldn't have been possible
>>> without it as far as I can tell.
>>>
>>>>
>>>> Johannes' patch has inspired me to consider whether zRAM could achieve
>>>> a comparable outcome by immediately releasing objects in swap cache
>>>> scenarios.  When I have the opportunity, I plan to experiment with zRAM.
>>>
>>> That would be interesting. I am curious if it would be as
>>> straightforward in zram to just mark the folio as dirty in this case
>>> like zswap does, given its implementation as a block device.
>>>
>>
>> This makes me wonder who is responsible for marking folio dirty in this swapcache
>> bypass case? Should we call folio_mark_dirty() after the swap_read_folio()?
> 
> In shrink_folio_list(), we try to add anonymous folios to the
> swapcache if they are not there before checking if they are dirty.
> add_to_swap() calls folio_mark_dirty(), so this should take care of

Right, thanks for your clarification, so should be no problem here.
Although it was a fix just for MADV_FREE case.

> it. There is an interesting comment there though. It says that PTE
> should be dirty, so unmapping the folio should have already marked it
> as dirty by the time we are adding it to the swapcache, except for the
> MADV_FREE case.

It seems to say the folio will be dirtied when unmap later, supposing the
PTE is dirty.

> 
> However, I think we actually unmap the folio after we add it to the
> swapcache in shrink_folio_list(). Also, I don't immediately see why
> the PTE would be dirty. In do_swap_page(), making the PTE dirty seems

If all anon pages on LRU list are faulted by write, it should be true.
We could just use the zero page if faulted by read, right?

> to be conditional on the fault being a write fault, but I didn't look
> thoroughly, maybe I missed it. It is also possible that the comment is
> just outdated.

Yeah, dirty is only marked on write fault.

Thanks.

Yosry Ahmed March 25, 2024, 9:40 a.m. UTC | #9

On Mon, Mar 25, 2024 at 2:22 AM Chengming Zhou <[e-mail]> wrote:
>
> On 2024/3/25 16:38, Yosry Ahmed wrote:
> > On Mon, Mar 25, 2024 at 12:33 AM Chengming Zhou
> > <[e-mail]> wrote:
> >>
> >> On 2024/3/25 15:06, Yosry Ahmed wrote:
> >>> On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
> >>>>
> >>>> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
> >>>>>
> >>>>> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> >>>>>>
> >>>>>> Zhongkun He reports data corruption when combining zswap with zram.
> >>>>>>
> >>>>>> The issue is the exclusive loads we're doing in zswap. They assume
> >>>>>> that all reads are going into the swapcache, which can assume
> >>>>>> authoritative ownership of the data and so the zswap copy can go.
> >>>>>>
> >>>>>> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> >>>>>> to bypass the swapcache. This results in an optimistic read of the
> >>>>>> swap data into a page that will be dismissed if the fault fails due to
> >>>>>> races. In this case, zswap mustn't drop its authoritative copy.
> >>>>>>
> >>>>>> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> >>>>>> Reported-by: Zhongkun He <[e-mail]>
> >>>>>> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> >>>>>> Cc: [e-mail]      [6.5+]
> >>>>>> Signed-off-by: Johannes Weiner <[e-mail]>
> >>>>>> Tested-by: Zhongkun He <[e-mail]>
> >>>>
> >>>> Acked-by: Barry Song <[e-mail]>
> >>>>
> >>>>>
> >>>>> Do we also want to mention somewhere (commit log or comment) that
> >>>>> keeping the entry in the tree is fine because we are still protected
> >>>>> from concurrent loads/invalidations/writeback by swapcache_prepare()
> >>>>> setting SWAP_HAS_CACHE or so?
> >>>>
> >>>> It seems that Kairui's patch comprehensively addresses the issue at hand.
> >>>> Johannes's solution, on the other hand, appears to align zswap behavior
> >>>> more closely with that of a traditional swap device, only releasing an entry
> >>>> when the corresponding swap slot is freed, particularly in the sync-io case.
> >>>
> >>> It actually worked out quite well that Kairui's fix landed shortly
> >>> before this bug was reported, as this fix wouldn't have been possible
> >>> without it as far as I can tell.
> >>>
> >>>>
> >>>> Johannes' patch has inspired me to consider whether zRAM could achieve
> >>>> a comparable outcome by immediately releasing objects in swap cache
> >>>> scenarios.  When I have the opportunity, I plan to experiment with zRAM.
> >>>
> >>> That would be interesting. I am curious if it would be as
> >>> straightforward in zram to just mark the folio as dirty in this case
> >>> like zswap does, given its implementation as a block device.
> >>>
> >>
> >> This makes me wonder who is responsible for marking folio dirty in this swapcache
> >> bypass case? Should we call folio_mark_dirty() after the swap_read_folio()?
> >
> > In shrink_folio_list(), we try to add anonymous folios to the
> > swapcache if they are not there before checking if they are dirty.
> > add_to_swap() calls folio_mark_dirty(), so this should take care of
>
> Right, thanks for your clarification, so should be no problem here.
> Although it was a fix just for MADV_FREE case.
>
> > it. There is an interesting comment there though. It says that PTE
> > should be dirty, so unmapping the folio should have already marked it
> > as dirty by the time we are adding it to the swapcache, except for the
> > MADV_FREE case.
>
> It seems to say the folio will be dirtied when unmap later, supposing the
> PTE is dirty.

Oh yeah it could mean that the folio will be dirted later.

>
> >
> > However, I think we actually unmap the folio after we add it to the
> > swapcache in shrink_folio_list(). Also, I don't immediately see why
> > the PTE would be dirty. In do_swap_page(), making the PTE dirty seems
>
> If all anon pages on LRU list are faulted by write, it should be true.
> We could just use the zero page if faulted by read, right?

This applies for the initial fault that creates the folio, but this is
a swap fault. It could be a read fault and in that case we still need
to make the folio dirty because it's not in the swapcache and we need
to write it out if it's reclaimed, right?

>
> > to be conditional on the fault being a write fault, but I didn't look
> > thoroughly, maybe I missed it. It is also possible that the comment is
> > just outdated.
>
> Yeah, dirty is only marked on write fault.
>
> Thanks.

Chengming Zhou March 25, 2024, 9:46 a.m. UTC | #10

On 2024/3/25 17:40, Yosry Ahmed wrote:
> On Mon, Mar 25, 2024 at 2:22 AM Chengming Zhou <[e-mail]> wrote:
>>
>> On 2024/3/25 16:38, Yosry Ahmed wrote:
>>> On Mon, Mar 25, 2024 at 12:33 AM Chengming Zhou
>>> <[e-mail]> wrote:
>>>>
>>>> On 2024/3/25 15:06, Yosry Ahmed wrote:
>>>>> On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
>>>>>>
>>>>>> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
>>>>>>>
>>>>>>> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> Zhongkun He reports data corruption when combining zswap with zram.
>>>>>>>>
>>>>>>>> The issue is the exclusive loads we're doing in zswap. They assume
>>>>>>>> that all reads are going into the swapcache, which can assume
>>>>>>>> authoritative ownership of the data and so the zswap copy can go.
>>>>>>>>
>>>>>>>> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
>>>>>>>> to bypass the swapcache. This results in an optimistic read of the
>>>>>>>> swap data into a page that will be dismissed if the fault fails due to
>>>>>>>> races. In this case, zswap mustn't drop its authoritative copy.
>>>>>>>>
>>>>>>>> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
>>>>>>>> Reported-by: Zhongkun He <[e-mail]>
>>>>>>>> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
>>>>>>>> Cc: [e-mail]      [6.5+]
>>>>>>>> Signed-off-by: Johannes Weiner <[e-mail]>
>>>>>>>> Tested-by: Zhongkun He <[e-mail]>
>>>>>>
>>>>>> Acked-by: Barry Song <[e-mail]>
>>>>>>
>>>>>>>
>>>>>>> Do we also want to mention somewhere (commit log or comment) that
>>>>>>> keeping the entry in the tree is fine because we are still protected
>>>>>>> from concurrent loads/invalidations/writeback by swapcache_prepare()
>>>>>>> setting SWAP_HAS_CACHE or so?
>>>>>>
>>>>>> It seems that Kairui's patch comprehensively addresses the issue at hand.
>>>>>> Johannes's solution, on the other hand, appears to align zswap behavior
>>>>>> more closely with that of a traditional swap device, only releasing an entry
>>>>>> when the corresponding swap slot is freed, particularly in the sync-io case.
>>>>>
>>>>> It actually worked out quite well that Kairui's fix landed shortly
>>>>> before this bug was reported, as this fix wouldn't have been possible
>>>>> without it as far as I can tell.
>>>>>
>>>>>>
>>>>>> Johannes' patch has inspired me to consider whether zRAM could achieve
>>>>>> a comparable outcome by immediately releasing objects in swap cache
>>>>>> scenarios.  When I have the opportunity, I plan to experiment with zRAM.
>>>>>
>>>>> That would be interesting. I am curious if it would be as
>>>>> straightforward in zram to just mark the folio as dirty in this case
>>>>> like zswap does, given its implementation as a block device.
>>>>>
>>>>
>>>> This makes me wonder who is responsible for marking folio dirty in this swapcache
>>>> bypass case? Should we call folio_mark_dirty() after the swap_read_folio()?
>>>
>>> In shrink_folio_list(), we try to add anonymous folios to the
>>> swapcache if they are not there before checking if they are dirty.
>>> add_to_swap() calls folio_mark_dirty(), so this should take care of
>>
>> Right, thanks for your clarification, so should be no problem here.
>> Although it was a fix just for MADV_FREE case.
>>
>>> it. There is an interesting comment there though. It says that PTE
>>> should be dirty, so unmapping the folio should have already marked it
>>> as dirty by the time we are adding it to the swapcache, except for the
>>> MADV_FREE case.
>>
>> It seems to say the folio will be dirtied when unmap later, supposing the
>> PTE is dirty.
> 
> Oh yeah it could mean that the folio will be dirted later.
> 
>>
>>>
>>> However, I think we actually unmap the folio after we add it to the
>>> swapcache in shrink_folio_list(). Also, I don't immediately see why
>>> the PTE would be dirty. In do_swap_page(), making the PTE dirty seems
>>
>> If all anon pages on LRU list are faulted by write, it should be true.
>> We could just use the zero page if faulted by read, right?
> 
> This applies for the initial fault that creates the folio, but this is
> a swap fault. It could be a read fault and in that case we still need
> to make the folio dirty because it's not in the swapcache and we need
> to write it out if it's reclaimed, right?

Yes, IMHO I think it should be marked as dirty here.

But it should be no problem with that unconditional folio_mark_dirty()
in add_to_swap(). Not sure if there are other issues.

> 
>>
>>> to be conditional on the fault being a write fault, but I didn't look
>>> thoroughly, maybe I missed it. It is also possible that the comment is
>>> just outdated.
>>
>> Yeah, dirty is only marked on write fault.
>>
>> Thanks.

Johannes Weiner March 25, 2024, 4:30 p.m. UTC | #11

On Sun, Mar 24, 2024 at 02:22:46PM -0700, Yosry Ahmed wrote:
> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> >
> > Zhongkun He reports data corruption when combining zswap with zram.
> >
> > The issue is the exclusive loads we're doing in zswap. They assume
> > that all reads are going into the swapcache, which can assume
> > authoritative ownership of the data and so the zswap copy can go.
> >
> > However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> > to bypass the swapcache. This results in an optimistic read of the
> > swap data into a page that will be dismissed if the fault fails due to
> > races. In this case, zswap mustn't drop its authoritative copy.
> >
> > Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> > Reported-by: Zhongkun He <[e-mail]>
> > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> > Cc: [e-mail]      [6.5+]
> > Signed-off-by: Johannes Weiner <[e-mail]>
> > Tested-by: Zhongkun He <[e-mail]>
> 
> Do we also want to mention somewhere (commit log or comment) that
> keeping the entry in the tree is fine because we are still protected
> from concurrent loads/invalidations/writeback by swapcache_prepare()
> setting SWAP_HAS_CACHE or so?

I don't think it's necessary, as zswap isn't doing anything special
here. It's up to the caller to follow the generic swap exclusion
protocol that zswap also adheres to. So IMO the relevant comment
should be, and is, above that swapcache_prepare() in do_swap_page().

> Anyway, this LGTM.
> 
> Acked-by: Yosry Ahmed <[e-mail]>

Thanks!

Nhat Pham March 25, 2024, 5:09 p.m. UTC | #12

On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
>
> Zhongkun He reports data corruption when combining zswap with zram.
>
> The issue is the exclusive loads we're doing in zswap. They assume
> that all reads are going into the swapcache, which can assume
> authoritative ownership of the data and so the zswap copy can go.
>
> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> to bypass the swapcache. This results in an optimistic read of the
> swap data into a page that will be dismissed if the fault fails due to
> races. In this case, zswap mustn't drop its authoritative copy.
>
> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> Reported-by: Zhongkun He <[e-mail]>
> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> Cc: [e-mail]      [6.5+]
> Signed-off-by: Johannes Weiner <[e-mail]>
> Tested-by: Zhongkun He <[e-mail]>
> ---
>  mm/zswap.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..41a1170f7cfe 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,20 @@ bool zswap_load(struct folio *folio)
>                 spin_unlock(&tree->lock);
>                 return false;
>         }
> -       zswap_rb_erase(&tree->rbroot, entry);
> +       /*
> +        * When reading into the swapcache, invalidate our entry. The
> +        * swapcache can be the authoritative owner of the page and
> +        * its mappings, and the pressure that results from having two
> +        * in-memory copies outweighs any benefits of caching the
> +        * compression work.
> +        *
> +        * (Most swapins go through the swapcache. The notable
> +        * exception is the singleton fault on SWP_SYNCHRONOUS_IO
> +        * files, which reads into a private page and may free it if
> +        * the fault fails. We remain the primary owner of the entry.)
> +        */
> +       if (swapcache)
> +               zswap_rb_erase(&tree->rbroot, entry);
>         spin_unlock(&tree->lock);
>
>         if (entry->length)
> @@ -1649,9 +1663,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);
> +       }

This LGTM!

Reviewed-by: Nhat Pham <[e-mail]>

Yosry Ahmed March 25, 2024, 6:35 p.m. UTC | #13

On Mon, Mar 25, 2024 at 2:46 AM Chengming Zhou <[e-mail]> wrote:
>
> On 2024/3/25 17:40, Yosry Ahmed wrote:
> > On Mon, Mar 25, 2024 at 2:22 AM Chengming Zhou <[e-mail]> wrote:
> >>
> >> On 2024/3/25 16:38, Yosry Ahmed wrote:
> >>> On Mon, Mar 25, 2024 at 12:33 AM Chengming Zhou
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On 2024/3/25 15:06, Yosry Ahmed wrote:
> >>>>> On Sun, Mar 24, 2024 at 9:54 PM Barry Song <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Mon, Mar 25, 2024 at 10:23 AM Yosry Ahmed <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> Zhongkun He reports data corruption when combining zswap with zram.
> >>>>>>>>
> >>>>>>>> The issue is the exclusive loads we're doing in zswap. They assume
> >>>>>>>> that all reads are going into the swapcache, which can assume
> >>>>>>>> authoritative ownership of the data and so the zswap copy can go.
> >>>>>>>>
> >>>>>>>> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> >>>>>>>> to bypass the swapcache. This results in an optimistic read of the
> >>>>>>>> swap data into a page that will be dismissed if the fault fails due to
> >>>>>>>> races. In this case, zswap mustn't drop its authoritative copy.
> >>>>>>>>
> >>>>>>>> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> >>>>>>>> Reported-by: Zhongkun He <[e-mail]>
> >>>>>>>> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> >>>>>>>> Cc: [e-mail]      [6.5+]
> >>>>>>>> Signed-off-by: Johannes Weiner <[e-mail]>
> >>>>>>>> Tested-by: Zhongkun He <[e-mail]>
> >>>>>>
> >>>>>> Acked-by: Barry Song <[e-mail]>
> >>>>>>
> >>>>>>>
> >>>>>>> Do we also want to mention somewhere (commit log or comment) that
> >>>>>>> keeping the entry in the tree is fine because we are still protected
> >>>>>>> from concurrent loads/invalidations/writeback by swapcache_prepare()
> >>>>>>> setting SWAP_HAS_CACHE or so?
> >>>>>>
> >>>>>> It seems that Kairui's patch comprehensively addresses the issue at hand.
> >>>>>> Johannes's solution, on the other hand, appears to align zswap behavior
> >>>>>> more closely with that of a traditional swap device, only releasing an entry
> >>>>>> when the corresponding swap slot is freed, particularly in the sync-io case.
> >>>>>
> >>>>> It actually worked out quite well that Kairui's fix landed shortly
> >>>>> before this bug was reported, as this fix wouldn't have been possible
> >>>>> without it as far as I can tell.
> >>>>>
> >>>>>>
> >>>>>> Johannes' patch has inspired me to consider whether zRAM could achieve
> >>>>>> a comparable outcome by immediately releasing objects in swap cache
> >>>>>> scenarios.  When I have the opportunity, I plan to experiment with zRAM.
> >>>>>
> >>>>> That would be interesting. I am curious if it would be as
> >>>>> straightforward in zram to just mark the folio as dirty in this case
> >>>>> like zswap does, given its implementation as a block device.
> >>>>>
> >>>>
> >>>> This makes me wonder who is responsible for marking folio dirty in this swapcache
> >>>> bypass case? Should we call folio_mark_dirty() after the swap_read_folio()?
> >>>
> >>> In shrink_folio_list(), we try to add anonymous folios to the
> >>> swapcache if they are not there before checking if they are dirty.
> >>> add_to_swap() calls folio_mark_dirty(), so this should take care of
> >>
> >> Right, thanks for your clarification, so should be no problem here.
> >> Although it was a fix just for MADV_FREE case.
> >>
> >>> it. There is an interesting comment there though. It says that PTE
> >>> should be dirty, so unmapping the folio should have already marked it
> >>> as dirty by the time we are adding it to the swapcache, except for the
> >>> MADV_FREE case.
> >>
> >> It seems to say the folio will be dirtied when unmap later, supposing the
> >> PTE is dirty.
> >
> > Oh yeah it could mean that the folio will be dirted later.
> >
> >>
> >>>
> >>> However, I think we actually unmap the folio after we add it to the
> >>> swapcache in shrink_folio_list(). Also, I don't immediately see why
> >>> the PTE would be dirty. In do_swap_page(), making the PTE dirty seems
> >>
> >> If all anon pages on LRU list are faulted by write, it should be true.
> >> We could just use the zero page if faulted by read, right?
> >
> > This applies for the initial fault that creates the folio, but this is
> > a swap fault. It could be a read fault and in that case we still need
> > to make the folio dirty because it's not in the swapcache and we need
> > to write it out if it's reclaimed, right?
>
> Yes, IMHO I think it should be marked as dirty here.
>
> But it should be no problem with that unconditional folio_mark_dirty()
> in add_to_swap(). Not sure if there are other issues.

I don't believe there are any issues now. Dirtying the folio in
add_to_swap() was introduced before SWP_SYNCHRONOUS_IO, so I guess
things have always worked.

I think we should update the comment there though to mention that
dirtying the folio is also needed for this case (not just MADV_FREE),
or dirty the PTE during the fault. Otherwise, if someone is making
MADV_FREE changes they could end up breaking SWP_SYNCHRONOUS_IO
faults.

Adding Minchan here in case he can confirm that we in fact rely on
add_to_swap()->folio_mark_dirty() for SWP_SYNCHRONOUS_IO to work as
intended.

Yosry Ahmed March 25, 2024, 6:41 p.m. UTC | #14

On Mon, Mar 25, 2024 at 9:30 AM Johannes Weiner <[e-mail]> wrote:
>
> On Sun, Mar 24, 2024 at 02:22:46PM -0700, Yosry Ahmed wrote:
> > On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
> > >
> > > Zhongkun He reports data corruption when combining zswap with zram.
> > >
> > > The issue is the exclusive loads we're doing in zswap. They assume
> > > that all reads are going into the swapcache, which can assume
> > > authoritative ownership of the data and so the zswap copy can go.
> > >
> > > However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> > > to bypass the swapcache. This results in an optimistic read of the
> > > swap data into a page that will be dismissed if the fault fails due to
> > > races. In this case, zswap mustn't drop its authoritative copy.
> > >
> > > Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> > > Reported-by: Zhongkun He <[e-mail]>
> > > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> > > Cc: [e-mail]      [6.5+]
> > > Signed-off-by: Johannes Weiner <[e-mail]>
> > > Tested-by: Zhongkun He <[e-mail]>
> >
> > Do we also want to mention somewhere (commit log or comment) that
> > keeping the entry in the tree is fine because we are still protected
> > from concurrent loads/invalidations/writeback by swapcache_prepare()
> > setting SWAP_HAS_CACHE or so?
>
> I don't think it's necessary, as zswap isn't doing anything special
> here. It's up to the caller to follow the generic swap exclusion
> protocol that zswap also adheres to. So IMO the relevant comment
> should be, and is, above that swapcache_prepare() in do_swap_page().

From the perspective of someone looking at the zswap code, it isn't
immediately clear what protects the zswap entry in the non-exclusive
load case from being freed from under us. At some point we had a
refcount, then we used to remove it from the tree under lock so others
wouldn't have access to it. Now it's less clear because we rely on
protection outside of zswap code.

We also document other places where we rely on the swapcache for
synchronization, so I think it may be worth briefly mentioning this
here as well, especially that in this code we explicitly check for the
folio not being in the swapcache. That said, I don't feel strongly
about it. Tracking down the SWP_SYNCHRONOUS_IO code should eventually
make it clear. Also, the commit log will end up having a link to this
thread anyway so the details are not completely unfindable :)

Chris Li March 25, 2024, 9:27 p.m. UTC | #15

On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <[e-mail]> wrote:
>
> Zhongkun He reports data corruption when combining zswap with zram.
>
> The issue is the exclusive loads we're doing in zswap. They assume
> that all reads are going into the swapcache, which can assume
> authoritative ownership of the data and so the zswap copy can go.
>
> However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> to bypass the swapcache. This results in an optimistic read of the
> swap data into a page that will be dismissed if the fault fails due to
> races. In this case, zswap mustn't drop its authoritative copy.
>
> Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=[e-mail]/
> Reported-by: Zhongkun He <[e-mail]>
> Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> Cc: [e-mail]      [6.5+]
> Signed-off-by: Johannes Weiner <[e-mail]>
> Tested-by: Zhongkun He <[e-mail]>

The change looks good to me. Thanks for the cleaner solution.
It has conflict with the zswap rb-tree to xarray patch though.
I will resolve the conflict and resubmit the zswap xarray patch.

Acked-by: Chris Li <[e-mail]>

Chris
> ---
>  mm/zswap.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 535c907345e0..41a1170f7cfe 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,20 @@ bool zswap_load(struct folio *folio)
>                 spin_unlock(&tree->lock);
>                 return false;
>         }
> -       zswap_rb_erase(&tree->rbroot, entry);
> +       /*
> +        * When reading into the swapcache, invalidate our entry. The
> +        * swapcache can be the authoritative owner of the page and
> +        * its mappings, and the pressure that results from having two
> +        * in-memory copies outweighs any benefits of caching the
> +        * compression work.
> +        *
> +        * (Most swapins go through the swapcache. The notable
> +        * exception is the singleton fault on SWP_SYNCHRONOUS_IO
> +        * files, which reads into a private page and may free it if
> +        * the fault fails. We remain the primary owner of the entry.)
> +        */
> +       if (swapcache)
> +               zswap_rb_erase(&tree->rbroot, entry);
>         spin_unlock(&tree->lock);
>
>         if (entry->length)
> @@ -1649,9 +1663,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;
>  }
> --
> 2.44.0
>

Patch

diff --git a/mm/zswap.c b/mm/zswap.c
index 535c907345e0..41a1170f7cfe 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,20 @@  bool zswap_load(struct folio *folio)
 		spin_unlock(&tree->lock);
 		return false;
 	}
-	zswap_rb_erase(&tree->rbroot, entry);
+	/*
+	 * When reading into the swapcache, invalidate our entry. The
+	 * swapcache can be the authoritative owner of the page and
+	 * its mappings, and the pressure that results from having two
+	 * in-memory copies outweighs any benefits of caching the
+	 * compression work.
+	 *
+	 * (Most swapins go through the swapcache. The notable
+	 * exception is the singleton fault on SWP_SYNCHRONOUS_IO
+	 * files, which reads into a private page and may free it if
+	 * the fault fails. We remain the primary owner of the entry.)
+	 */
+	if (swapcache)
+		zswap_rb_erase(&tree->rbroot, entry);
 	spin_unlock(&tree->lock);
 
 	if (entry->length)
@@ -1649,9 +1663,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;
 }