xfs
[Top] [All Lists]

Re: [PATCH] mm: fix direct reclaim writeback regression

To: Hugh Dickins <hughd@xxxxxxxxxx>
Subject: Re: [PATCH] mm: fix direct reclaim writeback regression
From: Johannes Weiner <hannes@xxxxxxxxxxx>
Date: Mon, 28 Jul 2014 10:23:13 -0400
Cc: Vlastimil Babka <vbabka@xxxxxxx>, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>, David Rientjes <rientjes@xxxxxxxxxx>, Rik van Riel <riel@xxxxxxxxxx>, Dave Jones <davej@xxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=cmpxchg.org; s=zene; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=91u7purxXsRVFY4SZeYFwJghsFsGiRSycfaBR4mwFeU=; b=obzfNQqSjrsYLZnAxtKTm2a1cdaHxA3IPJ0o7wOGtVxEbff2HI9M7SYZqxleUD163XGYfNPy5nBOlnBFlUiP8WrDEjbKyCCxZON2ZASUkR2y/cv/JVQf8D58EGuLtqW1auiil1jsGuIzweOxmi8kTS0sXmcoiF0bV0zeW2BvfiI=;
In-reply-to: <alpine.LSU.2.11.1407261600570.14577@xxxxxxxxxxxx>
References: <alpine.LSU.2.11.1407261248140.13796@xxxxxxxxxxxx> <53D42F80.7000000@xxxxxxx> <alpine.LSU.2.11.1407261600570.14577@xxxxxxxxxxxx>
On Sat, Jul 26, 2014 at 04:15:25PM -0700, Hugh Dickins wrote:
> On Sun, 27 Jul 2014, Vlastimil Babka wrote:
> > On 07/26/2014 09:58 PM, Hugh Dickins wrote:
> > > Yes, 3.16-rc1's 68711a746345 ("mm, migration: add destination page
> > > freeing callback") has provided such a way to compaction: if migrating
> > > a SwapBacked page fails, its newpage may be put back on the list for
> > > later use with PageSwapBacked still set, and nothing will clear it.
> > 
> > Ugh good catch. So is this the only flag that can become "stray" like
> > this? It seems so from quick check...
> 
> Yes, it seemed so to me too; but I would prefer a regime in which
> we only mess with newpage once it's sure to be successful.
> 
> > > --- 3.16-rc6/mm/migrate.c 2014-06-29 15:22:10.584003935 -0700
> > > +++ linux/mm/migrate.c    2014-07-26 11:28:34.488126591 -0700
> > > @@ -988,9 +988,10 @@ out:
> > >    * it.  Otherwise, putback_lru_page() will drop the reference grabbed
> > >    * during isolation.
> > >    */
> > > - if (rc != MIGRATEPAGE_SUCCESS && put_new_page)
> > > + if (rc != MIGRATEPAGE_SUCCESS && put_new_page) {
> > > +         ClearPageSwapBacked(newpage);
> > >           put_new_page(newpage, private);
> > > - else
> > > + } else
> > >           putback_lru_page(newpage);
> > >  
> > >   if (result) {
> > 
> > What about unmap_and_move_huge_page()? Seems to me it can also get the
> > same stray flag. Although compaction, who is the only user so far of
> > custom put_new_page, wouldn't of course migrate huge pages. But might
> > bite us in the future, if a new user appears before a cleanup...
> 
> I think you're right, thanks for pointing it out.  We don't have an
> actual bug there at present, so no need to rush back and fix up the
> patch now in Linus's tree; but unmap_and_move_huge_page() gives
> another reason why my choice was "probably the worst place to fix it".
> 
> More reason for a cleanup, but not while the memcg interface is in flux.
> In mmotm I'm a little anxious about the PageAnon case when newpage's
> mapping is left set, I wonder if that might also be problematic: I
> mailed Hannes privately to think about that - perhaps that will give
> more impulse for a cleanup, though I've not noticed any bug from it.

I made that change for oldpage because uncharge in the final put_page
relies on PageAnon() to work for statistics.

The newpage case could have been left alone, but it looked like an
anomaly to me - anonymous mappings are usually sticky and only cleared
by the page allocator - so I was eager to make the cases symmetrical.
I don't see a bug there because if the page is reused its mapping will
be overwritten right away, and if freed the allocator will reset it.

mem_cgroup_migrate() has since changed to fully uncharge the old page
and not leave this task to the final put_page, so ->mapping does not
need to be maintained past that point.  I'll send a revert of these
conditional ->mapping resets to Andrew.

<Prev in Thread] Current Thread [Next in Thread>