Re: hunting an IO hang

[Minchan Kim <minchan.kim@gmail.com>]

Hi Andrea,

On Mon, Jan 17, 2011 at 06:11:35AM +0100, Andrea Arcangeli wrote:
> On Sun, Jan 16, 2011 at 09:41:41PM -0500, Chris Mason wrote:
> > Excerpts from Andrew Morton's message of 2011-01-16 21:30:00 -0500:
> > > (lots of cc's added)
> > > 
> > > On Sun, 16 Jan 2011 21:07:40 -0500 Chris Mason <chris.mason@oracle.com> wrote:
> > > 
> > > > Excerpts from Linus Torvalds's message of 2011-01-16 20:53:04 -0500:
> > > > > .. except I actually didn't add Andrew to the cc after all.
> > > > > 
> > > > > NOW I did.
> > > > > 
> > > > > Oh, and if you can repeat this and bisect it, it would obviously be
> > > > > great. But that sounds rather painful.
> > > > 
> > > > Ok, so I've got 3 different problems in 3 totally different areas.
> > > > I'm running w/kvm, but this VM is very stable with 2.6.37.  Running
> > > > Linus' current git it goes boom in exotic ways, this time it was only on
> > > > ext3, btrfs code never loaded.
> > > > 
> > > > Linus, if you're planning on rc1 tonight I'll send my pull request out
> > > > the door.  Otherwise I'd prefer to fix this and send my pull after
> > > > actually getting a long btrfs run on the current code.
> > > > 
> > > > Next up, CONFIG_DEBUG*, always an adventure on rc1 kernels ;)
> > > > 
> > > > WARNING: at lib/list_debug.c:57 list_del+0xc0/0xed()
> > > > Hardware name: Bochs
> > > > list_del corruption. next->prev should be ffffea000010cde0, but was ffff88007cff6bc8
> > > > Modules linked in:
> > > > Pid: 524, comm: kswapd0 Not tainted 2.6.37-josef+ #180
> > > > Call Trace:
> > > >  [<ffffffff8106ec94>] ? warn_slowpath_common+0x85/0x9d
> > > >  [<ffffffff8106ed4f>] ? warn_slowpath_fmt+0x46/0x48
> > > >  [<ffffffff81263d6c>] ? list_del+0xc0/0xed
> > > >  [<ffffffff81106d9d>] ? migrate_pages+0x26f/0x357
> > > >  [<ffffffff81100e18>] ? compaction_alloc+0x0/0x2dc
> > > >  [<ffffffff8110150d>] ? compact_zone+0x391/0x5c4
> > > >  [<ffffffff81101905>] ? compact_zone_order+0xc2/0xd1
> > > >  [<ffffffff815c321e>] ? _raw_spin_unlock+0xe/0x10
> > > >  [<ffffffff810dc446>] ? kswapd+0x5c8/0x88f
> > > >  [<ffffffff810dbe7e>] ? kswapd+0x0/0x88f
> > > >  [<ffffffff81089ce8>] ? kthread+0x82/0x8a
> > > >  [<ffffffff810347d4>] ? kernel_thread_helper+0x4/0x10
> > > >  [<ffffffff81089c66>] ? kthread+0x0/0x8a
> > > >  [<ffffffff810347d0>] ? kernel_thread_helper+0x0/0x10
> > > > ---[ end trace 5c6b7933d16b301f ]---
> > > 
> > > uh-oh.  Does disabling CONFIG_COMPACTION make this go away (requires
> > > disabling CONFIG_TRANSPARENT_HUGEPAGE first).
> > 
> > We'll see.  I gave THP this same run of tests back in November, it
> > passed without any problems (after fixing the related btrfs migration
> > bug).  All of the crashes I've seen this weekend had this in the
> > .config:
> > 
> > # CONFIG_TRANSPARENT_HUGEPAGE is not set
> > CONFIG_COMPACTION=y
> > CONFIG_MIGRATION=y
> 
> I think it's unrelated but reading commit
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab if page_count(page) == 1 we
> leave the page in the lru but we return 0 (so the caller of
> migrate_pages won't call putback_lru_pages to actually free the page,

Good catch. Totally It's my fault. I made linux memory hogger. :(
Sorry for that.

> however compaction would free it because it checks if the list is
> empty and it ignores the migrate_pages retval). And in

I want to change it with checking retval for consistency.

> mm/memory-failure.c:1419, nobody is calling putback_lru_pages (it
> seems a missing bit from that older patch). They seem just two memleak
> unrelated to the above though.

Nice point. Thanks fot notice me.
It's strange in memory-failure.c:1419. I modified it at that time
and didn't modified migrate_huge_pages(At that time, I didn't noticed hugepage
migration so I missed it but I want to change it like migrate_pages)
You can see my final patch in https://lkml.org/lkml/2010/8/24/248. It was changed. 
Hmm. The putback_lru_pages in soft_offline_huge_page makes critical BUG(early free) 
so we should fix it, too.

We need 3 patch.

1. fix migrate_huge_pages(take out of put_page in migrate_huge_pages) for page corruption.
2. Your patch for memory leak
3. compaction retval check for putback lru pages for consistency

Hmm. Could I make a patch for any kernel tree?
Unfortunately I can make it out of office and we may spend 1 day at least.
If it is urgent, could you make the patch for me?

Thanks, Andrea.
> 
> NOTE: with the last changes compaction is used for all order > 0 and
> even from kswapd, so you will now be able to trigger bugs in
> compaction or migration even with THP off. However I'm surprised that
> you have issues with compaction...
> 
> I'm posting this for Minchan to review (not meant for merging, untested).
> 
> ======
> Subject: when migrate_pages returns 0, all pages must have been released
> 
> From: Andrea Arcangeli <aarcange@redhat.com>
> 
> In some cases migrate_pages could return zero while still leaving a
> few pages in the pagelist (and some caller wouldn't notice it has to
> call putback_lru_pages).
> 
> Add one missing putback_lru_pages not added by commit
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 548fbd7..75398b0 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1419,6 +1419,7 @@ int soft_offline_page(struct page *page, int flags)
>  		ret = migrate_pages(&pagelist, new_page, MPOL_MF_MOVE_ALL,
>  								0, true);
>  		if (ret) {
> +			putback_lru_pages(&pagelist);
>  			pr_info("soft offline: %#lx: migration failed %d, type %lx\n",
>  				pfn, ret, page->flags);
>  			if (ret > 0)
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 46fe8cc..bea2a34 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -611,6 +611,14 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>  	return rc;
>  }
>  
> +static void unmap_and_move_release_page(struct page *page)
> +{
> +	list_del(&page->lru);
> +	dec_zone_page_state(page, NR_ISOLATED_ANON +
> +			    page_is_file_cache(page));
> +	putback_lru_page(page);
> +}
> +
>  /*
>   * Obtain the lock on page, remove all ptes and migrate the page
>   * to the newly allocated page in newpage.
> @@ -631,11 +639,14 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
>  
>  	if (page_count(page) == 1) {
>  		/* page was freed from under us. So we are done. */
> +		unmap_and_move_release_page(page);
>  		goto move_newpage;
>  	}
>  	if (unlikely(PageTransHuge(page)))
> -		if (unlikely(split_huge_page(page)))
> +		if (unlikely(split_huge_page(page))) {
> +			unmap_and_move_release_page(page);
>  			goto move_newpage;
> +		}
>  
>  	/* prepare cgroup just returns 0 or -ENOMEM */
>  	rc = -EAGAIN;
> @@ -779,10 +790,7 @@ unlock:
>   		 * migrated will have kepts its references and be
>   		 * restored.
>   		 */
> - 		list_del(&page->lru);
> -		dec_zone_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> -		putback_lru_page(page);
> +		unmap_and_move_release_page(page);
>  	}
>  
>  move_newpage:

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>