Re: MADV_FREE is broken

From: Artem Savkov
Date: Thu Sep 21 2017 - 05:41:44 EST


On Wed, Sep 20, 2017 at 03:37:33PM -0700, Shaohua Li wrote:
> On Wed, Sep 20, 2017 at 11:01:47AM +0200, Artem Savkov wrote:
> > Hi All,
> >
> > We recently started noticing madvise09[1] test from ltp failing strangely. The
> > test does the following: maps 32 pages, sets MADV_FREE for the range it got,
> > dirties 2 of the pages, creates memory pressure and check that nondirty pages
> > are free. The test hanged while accessing the last 4 pages(29-32) of madvised
> > range at line 121 [2]. Any other process (gdb/cat) accessing those pages
> > would also hang as would rebooting the machine. It doesn't trigger any debug
> > warnings or kasan.
> >
> > The issue bisected to "802a3a92ad7a mm: reclaim MADV_FREE pages" (so 4.12 and
> > up are affected).
> >
> > I did some poking around and found out that the "bad" pages had SwapBacked flag
> > set in shrink_page_list() which confused it a lot. It looks like
> > mark_page_lazyfree() only calls lru_lazyfree_fn() when the pagevec is full
> > (that is in batches of 14) and never drains the rest (so last four in madvise09
> > case).
> >
> > The patch below greatly reduces the failure rate, but doesn't fix it
> > completely, it still shows up with the same symptoms (hanging trying to access
> > last 4 pages) after a bunch of retries.
> >
> > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/madvise/madvise09.c
> > [2] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/madvise/madvise09.c#L121
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 21261ff0466f..a0b868e8b7d2 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -453,6 +453,7 @@ static void madvise_free_page_range(struct mmu_gather *tlb,
> >
> > tlb_start_vma(tlb, vma);
> > walk_page_range(addr, end, &free_walk);
> > + lru_add_drain();
> > tlb_end_vma(tlb, vma);
> > }
>
> Looks there is a race between clear pte dirty bit and clear SwapBacked bit.
> draining the vect helps a little, but not sufficient. If SwapBacked is set, we
> could add the page to swapcache, but since we the page isn't dirty, we don't
> write the page out. This could cause data corruption. There is another place we
> wrongly clear SwapBacked bit. Below is a test patch which seems to fix the
> issue, please give a try.

Ran it for quite some time and it does seem to fix the issue.

> diff --git a/mm/swap.c b/mm/swap.c
> index 62d96b8..5c58257 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -575,7 +575,7 @@ static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
> void *arg)
> {
> if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
> - !PageUnevictable(page)) {
> + !PageSwapCache(page) && !PageUnevictable(page)) {
> bool active = PageActive(page);
>
> del_page_from_lru_list(page, lruvec,

Shouldn't the same check be added to mark_page_lazyfree()?

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 13d711d..be1c98e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -980,6 +980,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> int may_enter_fs;
> enum page_references references = PAGEREF_RECLAIM_CLEAN;
> bool dirty, writeback;
> + bool new_added_swapcache = false;
>
> cond_resched();
>
> @@ -1165,6 +1166,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>
> /* Adding to swap updated mapping */
> mapping = page_mapping(page);
> + new_added_swapcache = true;
> }
> } else if (unlikely(PageTransHuge(page))) {
> /* Split file THP */
> @@ -1185,6 +1187,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> nr_unmap_fail++;
> goto activate_locked;
> }
> + /* race with MADV_FREE */
> + if (PageAnon(page) && !PageDirty(page) &&
> + PageSwapBacked(page) && new_added_swapcache)
> + set_page_dirty(page);
> }
>
> if (PageDirty(page)) {

--
Regards,
Artem