On Fri, Feb 03, 2023 at 11:02:46PM +0800, Huang, Ying wrote:
> Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> writes:
>
> > On Fri, Feb 03, 2023 at 07:17:14AM +0800, Huang, Ying wrote:
> >> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
> >>
> >> > Hi, Hyeonggon,
> >> >
> >> > Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> writes:
> >> >
> >> >> On Wed, Feb 01, 2023 at 01:09:10AM +0900, Hyeonggon Yoo wrote:
> >> >>> I've observed random list_del corruption on mm-unstable,
> >> >>> where HEAD is commit d69862e693c069f4
> >> >>> ("mm/migrate: convert putback_movable_pages() to use folios").
> >> >>>
> >> >>> The issue can be easily reproduced by stressing MM multiple times:
> >> >>> stress-ng --bigheap 0 --timeout 300
> >> >>>
> >> >>> The compiler is gcc 12.2.1 and config, dmesg are included as attachment.
> >> >>> I will try to bisect but can't promise quick resolution :)
> >> >>
> >> >>
> >> >> The first bad commits appears to be:
> >> >> c203c6d5b3f0597 ("migrate_pages: batch _unmap and _move")
> >> >>
> >> >> the first bad commit _probably_ be earlier, but this is quite
> >> >> easy to reproduce so at this point I think above is the real bad commit.
> >> >
> >> > Thank you very much for reporting the bug. I'm in travel now but I will
> >> > try to find some time to reproduce and debug it.
> >>
> >> Still haven't reproduced the issue. But after reviewing the code, I
> >> found a bug in the code, which may cause list corruption. Can you try
> >> the debug patch below?
> >
> > Unfortunately my home server seems to be broken again :(
> > That means I only have access to VMs and not a real machine now.
> >
> > FYI it was not reproduced on KVM but reproduced on real machine.
> > Could you try checking on your machine with the config I attached? [1]
>
> Thank you very much for testing!
>
> > Sorry to bother your travel!
>
> Never mind. Your report helps me very much!
>
> > [1] https://marc.info/?l=linux-mm&m=167518135116956
>
> I have reproduced the bug successfully! And I can reproduce the bug
> with the previous debug patch too, although the reproduction rate isn't
> high.
>
> And in my test, the following patch can fix the bug.
>
> It appears that zswap code will touch dst->lru during moving page.
After setting swap space I was able to reproduce even on VM.
> -------------------------8<----------------------------------
> From b2e3f4aab16d8af0033286fde669b46e7467c7ec Mon Sep 17 00:00:00 2001
> From: Huang Ying <ying.huang@xxxxxxxxx>
> Date: Fri, 3 Feb 2023 22:03:24 +0800
> Subject: [PATCH] dbg,migrate_pages: restore destination folio state before
> move
>
> ---
> mm/migrate.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
This fixes the bug on my test:
Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
Thanks for such a quick fix!
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 143d96775b4d..fa7212330cb6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1225,13 +1225,19 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
> int page_was_mapped = 0;
> struct anon_vma *anon_vma = NULL;
> bool is_lru = !__PageMovable(&src->page);
> + struct list_head *prev;
>
> __migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
> + prev = dst->lru.prev;
> + list_del(&dst->lru);
BTW may be silly questions,
- How can zswap touch dst->lru during moving page, is there no lock
that prevents this to happen?
- Does this race (?) happen only during moving page?
(I mean, why is it safe to perform list_del()/list_add() before and
after moving page?)
>
> rc = move_to_new_folio(dst, src, mode);
>
> - if (rc != -EAGAIN)
> - list_del(&dst->lru);
> + if (rc == -EAGAIN) {
> + list_add(&dst->lru, prev);
> + __migrate_folio_record(dst, page_was_mapped, anon_vma);
> + return rc;
> + }
>
>
> if (unlikely(!is_lru))
> goto out_unlock_both;
> @@ -1251,11 +1257,6 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst,
> lru_add_drain();
> }
>
> - if (rc == -EAGAIN) {
> - __migrate_folio_record(dst, page_was_mapped, anon_vma);
> - return rc;
> - }
> -
> if (page_was_mapped)
> remove_migration_ptes(src,
> rc == MIGRATEPAGE_SUCCESS ? dst : src, false);
> --
> 2.35.1
