[PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jan Kara]

Under heavy memory and filesystem load, users observe the assertion
mapping->nrpages == 0 in end_writeback() trigger. This can be caused
by page reclaim reclaiming the last page from a mapping in the following
race:
	CPU0				CPU1
  ...
  shrink_page_list()
    __remove_mapping()
      __delete_from_page_cache()
        radix_tree_delete()
					evict_inode()
					  truncate_inode_pages()
					    truncate_inode_pages_range()
					      pagevec_lookup() - finds nothing
					  end_writeback()
					    mapping->nrpages != 0 -> BUG
        page->mapping = NULL
        mapping->nrpages--

Fix the problem by cycling the mapping->tree_lock at the end of
truncate_inode_pages_range() to synchronize with page reclaim.

Analyzed by Jay <jinshan.xiong@whamcloud.com>, lost in LKML, and dug
out by Miklos Szeredi <mszeredi@suse.de>.

CC: Jay <jinshan.xiong@whamcloud.com>
CC: stable@kernel.org
Acked-by: Miklos Szeredi <mszeredi@suse.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/truncate.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

 Andrew, would you merge this patch please? Thanks.

diff --git a/mm/truncate.c b/mm/truncate.c
index a956675..ec3d292 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -291,6 +291,13 @@ void truncate_inode_pages_range(struct address_space *mapping,
 		pagevec_release(&pvec);
 		mem_cgroup_uncharge_end();
 	}
+	/*
+	 * Cycle the tree_lock to make sure all __delete_from_page_cache()
+	 * calls run from page reclaim have finished as well (this handles the
+	 * case when page reclaim took the last page from our range).
+	 */
+	spin_lock_irq(&mapping->tree_lock);
+	spin_unlock_irq(&mapping->tree_lock);
 }
 EXPORT_SYMBOL(truncate_inode_pages_range);
 
-- 
1.7.1

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Andrew Morton]

On Mon, 30 May 2011 11:37:38 +0200
Jan Kara <jack@suse.cz> wrote:

> Under heavy memory and filesystem load, users observe the assertion
> mapping->nrpages == 0 in end_writeback() trigger. This can be caused
> by page reclaim reclaiming the last page from a mapping in the following
> race:
> 	CPU0				CPU1
>   ...
>   shrink_page_list()
>     __remove_mapping()
>       __delete_from_page_cache()
>         radix_tree_delete()
> 					evict_inode()
> 					  truncate_inode_pages()
> 					    truncate_inode_pages_range()
> 					      pagevec_lookup() - finds nothing
> 					  end_writeback()
> 					    mapping->nrpages != 0 -> BUG
>         page->mapping = NULL
>         mapping->nrpages--
> 
> Fix the problem by cycling the mapping->tree_lock at the end of
> truncate_inode_pages_range() to synchronize with page reclaim.
> 
> Analyzed by Jay <jinshan.xiong@whamcloud.com>, lost in LKML, and dug
> out by Miklos Szeredi <mszeredi@suse.de>.
> 
> CC: Jay <jinshan.xiong@whamcloud.com>
> CC: stable@kernel.org
> Acked-by: Miklos Szeredi <mszeredi@suse.de>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  mm/truncate.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
>  Andrew, would you merge this patch please? Thanks.
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index a956675..ec3d292 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -291,6 +291,13 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  		pagevec_release(&pvec);
>  		mem_cgroup_uncharge_end();
>  	}
> +	/*
> +	 * Cycle the tree_lock to make sure all __delete_from_page_cache()
> +	 * calls run from page reclaim have finished as well (this handles the
> +	 * case when page reclaim took the last page from our range).
> +	 */
> +	spin_lock_irq(&mapping->tree_lock);
> +	spin_unlock_irq(&mapping->tree_lock);
>  }
>  EXPORT_SYMBOL(truncate_inode_pages_range);

That's one ugly patch.


Perhaps this regression was added by Nick's RCUification of pagecache. 

Before that patch, mapping->nrpages and the radix-tree state were
coherent for holders of tree_lock.  So pagevec_lookup() would never
return "no pages" while ->nrpages is non-zero.

After that patch, find_get_pages() uses RCU to protect the radix-tree
but I don't think it correctly protects the aggregate (radix-tree +
nrpages).


If it's not that then I see another possibility. 
truncate_inode_pages_range() does

        if (mapping->nrpages == 0)
                return;

Is there anything to prevent a page getting added to the inode _after_
this test?  i_mutex?  If not, that would trigger the BUG.


Either way, I don't think that the uglypatch expresses a full
understanding of te bug ;)

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Miklos Szeredi]

On Mon, 2011-06-06 at 15:16 -0700, Andrew Morton wrote:
> On Mon, 30 May 2011 11:37:38 +0200
> Jan Kara <jack@suse.cz> wrote:
> 
> > Under heavy memory and filesystem load, users observe the assertion
> > mapping->nrpages == 0 in end_writeback() trigger. This can be caused
> > by page reclaim reclaiming the last page from a mapping in the following
> > race:
> > 	CPU0				CPU1
> >   ...
> >   shrink_page_list()
> >     __remove_mapping()
> >       __delete_from_page_cache()
> >         radix_tree_delete()
> > 					evict_inode()
> > 					  truncate_inode_pages()
> > 					    truncate_inode_pages_range()
> > 					      pagevec_lookup() - finds nothing
> > 					  end_writeback()
> > 					    mapping->nrpages != 0 -> BUG
> >         page->mapping = NULL
> >         mapping->nrpages--
> > 
> > Fix the problem by cycling the mapping->tree_lock at the end of
> > truncate_inode_pages_range() to synchronize with page reclaim.
> > 
> > Analyzed by Jay <jinshan.xiong@whamcloud.com>, lost in LKML, and dug
> > out by Miklos Szeredi <mszeredi@suse.de>.
> > 
> > CC: Jay <jinshan.xiong@whamcloud.com>
> > CC: stable@kernel.org
> > Acked-by: Miklos Szeredi <mszeredi@suse.de>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  mm/truncate.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> >  Andrew, would you merge this patch please? Thanks.
> > 
> > diff --git a/mm/truncate.c b/mm/truncate.c
> > index a956675..ec3d292 100644
> > --- a/mm/truncate.c
> > +++ b/mm/truncate.c
> > @@ -291,6 +291,13 @@ void truncate_inode_pages_range(struct address_space *mapping,
> >  		pagevec_release(&pvec);
> >  		mem_cgroup_uncharge_end();
> >  	}
> > +	/*
> > +	 * Cycle the tree_lock to make sure all __delete_from_page_cache()
> > +	 * calls run from page reclaim have finished as well (this handles the
> > +	 * case when page reclaim took the last page from our range).
> > +	 */
> > +	spin_lock_irq(&mapping->tree_lock);
> > +	spin_unlock_irq(&mapping->tree_lock);
> >  }
> >  EXPORT_SYMBOL(truncate_inode_pages_range);
> 
> That's one ugly patch.
> 
> 
> Perhaps this regression was added by Nick's RCUification of pagecache. 
> 
> Before that patch, mapping->nrpages and the radix-tree state were
> coherent for holders of tree_lock.  So pagevec_lookup() would never
> return "no pages" while ->nrpages is non-zero.
> 
> After that patch, find_get_pages() uses RCU to protect the radix-tree
> but I don't think it correctly protects the aggregate (radix-tree +
> nrpages).

Yes, that's the case.

> 
> 
> If it's not that then I see another possibility. 
> truncate_inode_pages_range() does
> 
>         if (mapping->nrpages == 0)
>                 return;
> 
> Is there anything to prevent a page getting added to the inode _after_
> this test?  i_mutex?  If not, that would trigger the BUG.

That BUG is in the inode eviction phase, so there's nothing that could
be adding a page.

And the only thing that could be removing one is page reclaim.

> Either way, I don't think that the uglypatch expresses a full
> understanding of te bug ;)

I don't see a better way, how would we make nrpages update atomically
wrt the radix-tree while using only RCU?

The question is, does it matter that those two can get temporarily out
of sync?

In case of inode eviction it does, not only because of that BUG_ON, but
because page reclaim must be somehow synchronised with eviction.
Otherwise it may access tree_lock on the mapping of an already freed
inode.

In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
weren't under tree_lock before Nick's RCUification, so their use were
just optimization.   

Thanks,
Miklos


--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jinshan Xiong]

[-- Attachment #1: Type: text/plain, Size: 4502 bytes --]


On Jun 6, 2011, at 10:46 PM, Miklos Szeredi wrote:

> On Mon, 2011-06-06 at 15:16 -0700, Andrew Morton wrote:
>> On Mon, 30 May 2011 11:37:38 +0200
>> Jan Kara <jack@suse.cz> wrote:
>> 
>>> Under heavy memory and filesystem load, users observe the assertion
>>> mapping->nrpages == 0 in end_writeback() trigger. This can be caused
>>> by page reclaim reclaiming the last page from a mapping in the following
>>> race:
>>> 	CPU0				CPU1
>>>  ...
>>>  shrink_page_list()
>>>    __remove_mapping()
>>>      __delete_from_page_cache()
>>>        radix_tree_delete()
>>> 					evict_inode()
>>> 					  truncate_inode_pages()
>>> 					    truncate_inode_pages_range()
>>> 					      pagevec_lookup() - finds nothing
>>> 					  end_writeback()
>>> 					    mapping->nrpages != 0 -> BUG
>>>        page->mapping = NULL
>>>        mapping->nrpages--
>>> 
>>> Fix the problem by cycling the mapping->tree_lock at the end of
>>> truncate_inode_pages_range() to synchronize with page reclaim.
>>> 
>>> Analyzed by Jay <jinshan.xiong@whamcloud.com>, lost in LKML, and dug
>>> out by Miklos Szeredi <mszeredi@suse.de>.
>>> 
>>> CC: Jay <jinshan.xiong@whamcloud.com>
>>> CC: stable@kernel.org
>>> Acked-by: Miklos Szeredi <mszeredi@suse.de>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>> mm/truncate.c |    7 +++++++
>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>> 
>>> Andrew, would you merge this patch please? Thanks.
>>> 
>>> diff --git a/mm/truncate.c b/mm/truncate.c
>>> index a956675..ec3d292 100644
>>> --- a/mm/truncate.c
>>> +++ b/mm/truncate.c
>>> @@ -291,6 +291,13 @@ void truncate_inode_pages_range(struct address_space *mapping,
>>> 		pagevec_release(&pvec);
>>> 		mem_cgroup_uncharge_end();
>>> 	}
>>> +	/*
>>> +	 * Cycle the tree_lock to make sure all __delete_from_page_cache()
>>> +	 * calls run from page reclaim have finished as well (this handles the
>>> +	 * case when page reclaim took the last page from our range).
>>> +	 */
>>> +	spin_lock_irq(&mapping->tree_lock);
>>> +	spin_unlock_irq(&mapping->tree_lock);
>>> }
>>> EXPORT_SYMBOL(truncate_inode_pages_range);
>> 
>> That's one ugly patch.
>> 
>> 
>> Perhaps this regression was added by Nick's RCUification of pagecache. 
>> 
>> Before that patch, mapping->nrpages and the radix-tree state were
>> coherent for holders of tree_lock.  So pagevec_lookup() would never
>> return "no pages" while ->nrpages is non-zero.
>> 
>> After that patch, find_get_pages() uses RCU to protect the radix-tree
>> but I don't think it correctly protects the aggregate (radix-tree +
>> nrpages).
> 
> Yes, that's the case.
> 
>> 
>> 
>> If it's not that then I see another possibility. 
>> truncate_inode_pages_range() does
>> 
>>        if (mapping->nrpages == 0)
>>                return;
>> 
>> Is there anything to prevent a page getting added to the inode _after_
>> this test?  i_mutex?  If not, that would trigger the BUG.
> 
> That BUG is in the inode eviction phase, so there's nothing that could
> be adding a page.
> 
> And the only thing that could be removing one is page reclaim.
> 
>> Either way, I don't think that the uglypatch expresses a full
>> understanding of te bug ;)
> 
> I don't see a better way, how would we make nrpages update atomically
> wrt the radix-tree while using only RCU?
> 
> The question is, does it matter that those two can get temporarily out
> of sync?
> 
> In case of inode eviction it does, not only because of that BUG_ON, but
> because page reclaim must be somehow synchronised with eviction.
> Otherwise it may access tree_lock on the mapping of an already freed
> inode.

I tend to think your patch is absolutely ok to fix this problem. However, I think it would be better to move:

spin_lock(&mapping->tree_lock);
spin_unlock(&mapping->tree_lock);

into end_writeback(). This is because truncate_inode_pages_range() is a generic function and it will be called somewhere else, maybe unnecessarily to do this extra thing.

Actually, I'd like to hold an inode refcount in page stealing process. The reason is obvious: it makes no sense to steal pages from a to-be-freed inode. However, the problem is the overhead to grab an inode is damned heavy.

Thanks,
Jinshan

> 
> In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
> weren't under tree_lock before Nick's RCUification, so their use were
> just optimization.   
> 
> Thanks,
> Miklos
> 
> 


[-- Attachment #2: Type: text/html, Size: 13277 bytes --]

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Andrew Morton]

On Tue, 07 Jun 2011 07:46:37 +0200
Miklos Szeredi <mszeredi@suse.cz> wrote:

> > Either way, I don't think that the uglypatch expresses a full
> > understanding of te bug ;)
> 
> I don't see a better way, how would we make nrpages update atomically
> wrt the radix-tree while using only RCU?
> 
> The question is, does it matter that those two can get temporarily out
> of sync?
> 
> In case of inode eviction it does, not only because of that BUG_ON, but
> because page reclaim must be somehow synchronised with eviction.
> Otherwise it may access tree_lock on the mapping of an already freed
> inode.
> 
> In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
> weren't under tree_lock before Nick's RCUification, so their use were
> just optimization.   

Gee, we've made a bit of a mess here.

Rather than bodging around particualr codesites where that mess exposes
itself, how about we step back and work out what our design is here,
then implement it and check that all sites comply with it?

What is the relationship between the radix-tree and nrpages?  What are
the locking rules?  Can anyone come up with a one-sentence proposal?

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jan Kara]

On Tue 07-06-11 14:33:01, Andrew Morton wrote:
> On Tue, 07 Jun 2011 07:46:37 +0200
> Miklos Szeredi <mszeredi@suse.cz> wrote:
> 
> > > Either way, I don't think that the uglypatch expresses a full
> > > understanding of te bug ;)
> > 
> > I don't see a better way, how would we make nrpages update atomically
> > wrt the radix-tree while using only RCU?
> > 
> > The question is, does it matter that those two can get temporarily out
> > of sync?
> > 
> > In case of inode eviction it does, not only because of that BUG_ON, but
> > because page reclaim must be somehow synchronised with eviction.
> > Otherwise it may access tree_lock on the mapping of an already freed
> > inode.
> > 
> > In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
> > weren't under tree_lock before Nick's RCUification, so their use were
> > just optimization.   
> 
> Gee, we've made a bit of a mess here.
> 
> Rather than bodging around particualr codesites where that mess exposes
> itself, how about we step back and work out what our design is here,
> then implement it and check that all sites comply with it?
> 
> What is the relationship between the radix-tree and nrpages?  What are
> the locking rules?  Can anyone come up with a one-sentence proposal?
AFAIU, nrpages and radix-tree are consistent under tree_lock.

nrpages is only used (well, apart from shmfs and other filesystems which
use the value as a guess how much should they expect to write or similar
heuristics) to test mapping->nrpages == 0 and the test is performed without
any synchronization which looks natural because we later do only
rcu-protected lookups anyway. So it seems it's expected the test is
unreliable and we just use it to make things faster. The same race as with
nrpages test can happen during the radix tree lookup anyway...

I went through the tests and the only place which seems to really care
about the races with __add_to_page_cache() or __delete_from_page_cache()
is when the inode should be removed from memory. There we have to be
careful. Races with __add_to_page_cache() cannot happen because there is
noone who could trigger addition of new page to the inode being evicted.
Races with __delete_from_page_cache() are possible though...

But I don't feel to be expert in this code so maybe I missed something.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jan Kara]

On Tue 07-06-11 11:22:48, Jinshan Xiong wrote:
> 
> On Jun 6, 2011, at 10:46 PM, Miklos Szeredi wrote:
> 
> > On Mon, 2011-06-06 at 15:16 -0700, Andrew Morton wrote:
> >> On Mon, 30 May 2011 11:37:38 +0200
> >> Jan Kara <jack@suse.cz> wrote:
> >> 
> >>> Under heavy memory and filesystem load, users observe the assertion
> >>> mapping->nrpages == 0 in end_writeback() trigger. This can be caused
> >>> by page reclaim reclaiming the last page from a mapping in the following
> >>> race:
> >>> 	CPU0				CPU1
> >>>  ...
> >>>  shrink_page_list()
> >>>    __remove_mapping()
> >>>      __delete_from_page_cache()
> >>>        radix_tree_delete()
> >>> 					evict_inode()
> >>> 					  truncate_inode_pages()
> >>> 					    truncate_inode_pages_range()
> >>> 					      pagevec_lookup() - finds nothing
> >>> 					  end_writeback()
> >>> 					    mapping->nrpages != 0 -> BUG
> >>>        page->mapping = NULL
> >>>        mapping->nrpages--
> >>> 
> >>> Fix the problem by cycling the mapping->tree_lock at the end of
> >>> truncate_inode_pages_range() to synchronize with page reclaim.
> >>> 
> >>> Analyzed by Jay <jinshan.xiong@whamcloud.com>, lost in LKML, and dug
> >>> out by Miklos Szeredi <mszeredi@suse.de>.
> >>> 
> >>> CC: Jay <jinshan.xiong@whamcloud.com>
> >>> CC: stable@kernel.org
> >>> Acked-by: Miklos Szeredi <mszeredi@suse.de>
> >>> Signed-off-by: Jan Kara <jack@suse.cz>
> >>> ---
> >>> mm/truncate.c |    7 +++++++
> >>> 1 files changed, 7 insertions(+), 0 deletions(-)
> >>> 
> >>> Andrew, would you merge this patch please? Thanks.
> >>> 
> >>> diff --git a/mm/truncate.c b/mm/truncate.c
> >>> index a956675..ec3d292 100644
> >>> --- a/mm/truncate.c
> >>> +++ b/mm/truncate.c
> >>> @@ -291,6 +291,13 @@ void truncate_inode_pages_range(struct address_space *mapping,
> >>> 		pagevec_release(&pvec);
> >>> 		mem_cgroup_uncharge_end();
> >>> 	}
> >>> +	/*
> >>> +	 * Cycle the tree_lock to make sure all __delete_from_page_cache()
> >>> +	 * calls run from page reclaim have finished as well (this handles the
> >>> +	 * case when page reclaim took the last page from our range).
> >>> +	 */
> >>> +	spin_lock_irq(&mapping->tree_lock);
> >>> +	spin_unlock_irq(&mapping->tree_lock);
> >>> }
> >>> EXPORT_SYMBOL(truncate_inode_pages_range);
> >> 
> >> That's one ugly patch.
> >> 
> >> 
> >> Perhaps this regression was added by Nick's RCUification of pagecache. 
> >> 
> >> Before that patch, mapping->nrpages and the radix-tree state were
> >> coherent for holders of tree_lock.  So pagevec_lookup() would never
> >> return "no pages" while ->nrpages is non-zero.
> >> 
> >> After that patch, find_get_pages() uses RCU to protect the radix-tree
> >> but I don't think it correctly protects the aggregate (radix-tree +
> >> nrpages).
> > 
> > Yes, that's the case.
> > 
> >> 
> >> 
> >> If it's not that then I see another possibility. 
> >> truncate_inode_pages_range() does
> >> 
> >>        if (mapping->nrpages == 0)
> >>                return;
> >> 
> >> Is there anything to prevent a page getting added to the inode _after_
> >> this test?  i_mutex?  If not, that would trigger the BUG.
> > 
> > That BUG is in the inode eviction phase, so there's nothing that could
> > be adding a page.
> > 
> > And the only thing that could be removing one is page reclaim.
> > 
> >> Either way, I don't think that the uglypatch expresses a full
> >> understanding of te bug ;)
> > 
> > I don't see a better way, how would we make nrpages update atomically
> > wrt the radix-tree while using only RCU?
> > 
> > The question is, does it matter that those two can get temporarily out
> > of sync?
> > 
> > In case of inode eviction it does, not only because of that BUG_ON, but
> > because page reclaim must be somehow synchronised with eviction.
> > Otherwise it may access tree_lock on the mapping of an already freed
> > inode.
> 
> I tend to think your patch is absolutely ok to fix this problem. However, I think it would be better to move:
> 
> spin_lock(&mapping->tree_lock);
> spin_unlock(&mapping->tree_lock);
> 
> into end_writeback(). This is because truncate_inode_pages_range() is a
> generic function and it will be called somewhere else, maybe
> unnecessarily to do this extra thing.
  Possible. I just thought it would be nice from
truncate_inode_pages_range() to return only after we are really sure there
are no outstanding pages in the requested range...

> Actually, I'd like to hold an inode refcount in page stealing process.
> The reason is obvious: it makes no sense to steal pages from a
> to-be-freed inode. However, the problem is the overhead to grab an inode
> is damned heavy.
  No a good idea I think. If you happen to be the last one to drop inode
reference, you have to handle inode deletion and you really want to limit
places from where that can happen because that needs all sorts of
filesystem locks etc.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jinshan Xiong]

[-- Attachment #1: Type: text/plain, Size: 5163 bytes --]


On Jun 8, 2011, at 9:40 AM, Jan Kara wrote:

> On Tue 07-06-11 11:22:48, Jinshan Xiong wrote:
>> 
>> On Jun 6, 2011, at 10:46 PM, Miklos Szeredi wrote:
>> 
>>> On Mon, 2011-06-06 at 15:16 -0700, Andrew Morton wrote:
>>>> On Mon, 30 May 2011 11:37:38 +0200
>>>> Jan Kara <jack@suse.cz> wrote:
>>>> 
>>>>> Under heavy memory and filesystem load, users observe the assertion
>>>>> mapping->nrpages == 0 in end_writeback() trigger. This can be caused
>>>>> by page reclaim reclaiming the last page from a mapping in the following
>>>>> race:
>>>>> 	CPU0				CPU1
>>>>> ...
>>>>> shrink_page_list()
>>>>>   __remove_mapping()
>>>>>     __delete_from_page_cache()
>>>>>       radix_tree_delete()
>>>>> 					evict_inode()
>>>>> 					  truncate_inode_pages()
>>>>> 					    truncate_inode_pages_range()
>>>>> 					      pagevec_lookup() - finds nothing
>>>>> 					  end_writeback()
>>>>> 					    mapping->nrpages != 0 -> BUG
>>>>>       page->mapping = NULL
>>>>>       mapping->nrpages--
>>>>> 
>>>>> Fix the problem by cycling the mapping->tree_lock at the end of
>>>>> truncate_inode_pages_range() to synchronize with page reclaim.
>>>>> 
>>>>> Analyzed by Jay <jinshan.xiong@whamcloud.com>, lost in LKML, and dug
>>>>> out by Miklos Szeredi <mszeredi@suse.de>.
>>>>> 
>>>>> CC: Jay <jinshan.xiong@whamcloud.com>
>>>>> CC: stable@kernel.org
>>>>> Acked-by: Miklos Szeredi <mszeredi@suse.de>
>>>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>>>> ---
>>>>> mm/truncate.c |    7 +++++++
>>>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>>> 
>>>>> Andrew, would you merge this patch please? Thanks.
>>>>> 
>>>>> diff --git a/mm/truncate.c b/mm/truncate.c
>>>>> index a956675..ec3d292 100644
>>>>> --- a/mm/truncate.c
>>>>> +++ b/mm/truncate.c
>>>>> @@ -291,6 +291,13 @@ void truncate_inode_pages_range(struct address_space *mapping,
>>>>> 		pagevec_release(&pvec);
>>>>> 		mem_cgroup_uncharge_end();
>>>>> 	}
>>>>> +	/*
>>>>> +	 * Cycle the tree_lock to make sure all __delete_from_page_cache()
>>>>> +	 * calls run from page reclaim have finished as well (this handles the
>>>>> +	 * case when page reclaim took the last page from our range).
>>>>> +	 */
>>>>> +	spin_lock_irq(&mapping->tree_lock);
>>>>> +	spin_unlock_irq(&mapping->tree_lock);
>>>>> }
>>>>> EXPORT_SYMBOL(truncate_inode_pages_range);
>>>> 
>>>> That's one ugly patch.
>>>> 
>>>> 
>>>> Perhaps this regression was added by Nick's RCUification of pagecache. 
>>>> 
>>>> Before that patch, mapping->nrpages and the radix-tree state were
>>>> coherent for holders of tree_lock.  So pagevec_lookup() would never
>>>> return "no pages" while ->nrpages is non-zero.
>>>> 
>>>> After that patch, find_get_pages() uses RCU to protect the radix-tree
>>>> but I don't think it correctly protects the aggregate (radix-tree +
>>>> nrpages).
>>> 
>>> Yes, that's the case.
>>> 
>>>> 
>>>> 
>>>> If it's not that then I see another possibility. 
>>>> truncate_inode_pages_range() does
>>>> 
>>>>       if (mapping->nrpages == 0)
>>>>               return;
>>>> 
>>>> Is there anything to prevent a page getting added to the inode _after_
>>>> this test?  i_mutex?  If not, that would trigger the BUG.
>>> 
>>> That BUG is in the inode eviction phase, so there's nothing that could
>>> be adding a page.
>>> 
>>> And the only thing that could be removing one is page reclaim.
>>> 
>>>> Either way, I don't think that the uglypatch expresses a full
>>>> understanding of te bug ;)
>>> 
>>> I don't see a better way, how would we make nrpages update atomically
>>> wrt the radix-tree while using only RCU?
>>> 
>>> The question is, does it matter that those two can get temporarily out
>>> of sync?
>>> 
>>> In case of inode eviction it does, not only because of that BUG_ON, but
>>> because page reclaim must be somehow synchronised with eviction.
>>> Otherwise it may access tree_lock on the mapping of an already freed
>>> inode.
>> 
>> I tend to think your patch is absolutely ok to fix this problem. However, I think it would be better to move:
>> 
>> spin_lock(&mapping->tree_lock);
>> spin_unlock(&mapping->tree_lock);
>> 
>> into end_writeback(). This is because truncate_inode_pages_range() is a
>> generic function and it will be called somewhere else, maybe
>> unnecessarily to do this extra thing.
>  Possible. I just thought it would be nice from
> truncate_inode_pages_range() to return only after we are really sure there
> are no outstanding pages in the requested range...
> 
>> Actually, I'd like to hold an inode refcount in page stealing process.
>> The reason is obvious: it makes no sense to steal pages from a
>> to-be-freed inode. However, the problem is the overhead to grab an inode
>> is damned heavy.
>  No a good idea I think. If you happen to be the last one to drop inode
> reference, you have to handle inode deletion and you really want to limit
> places from where that can happen because that needs all sorts of
> filesystem locks etc.

Indeed. Thanks for pointing it out.

> 
> 								Honza
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR


[-- Attachment #2: Type: text/html, Size: 22131 bytes --]

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jan Kara]

On Wed 08-06-11 18:36:43, Jan Kara wrote:
> On Tue 07-06-11 14:33:01, Andrew Morton wrote:
> > On Tue, 07 Jun 2011 07:46:37 +0200
> > Miklos Szeredi <mszeredi@suse.cz> wrote:
> > 
> > > > Either way, I don't think that the uglypatch expresses a full
> > > > understanding of te bug ;)
> > > 
> > > I don't see a better way, how would we make nrpages update atomically
> > > wrt the radix-tree while using only RCU?
> > > 
> > > The question is, does it matter that those two can get temporarily out
> > > of sync?
> > > 
> > > In case of inode eviction it does, not only because of that BUG_ON, but
> > > because page reclaim must be somehow synchronised with eviction.
> > > Otherwise it may access tree_lock on the mapping of an already freed
> > > inode.
> > > 
> > > In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
> > > weren't under tree_lock before Nick's RCUification, so their use were
> > > just optimization.   
> > 
> > Gee, we've made a bit of a mess here.
> > 
> > Rather than bodging around particualr codesites where that mess exposes
> > itself, how about we step back and work out what our design is here,
> > then implement it and check that all sites comply with it?
> > 
> > What is the relationship between the radix-tree and nrpages?  What are
> > the locking rules?  Can anyone come up with a one-sentence proposal?
> AFAIU, nrpages and radix-tree are consistent under tree_lock.
> 
> nrpages is only used (well, apart from shmfs and other filesystems which
> use the value as a guess how much should they expect to write or similar
> heuristics) to test mapping->nrpages == 0 and the test is performed without
> any synchronization which looks natural because we later do only
> rcu-protected lookups anyway. So it seems it's expected the test is
> unreliable and we just use it to make things faster. The same race as with
> nrpages test can happen during the radix tree lookup anyway...
> 
> I went through the tests and the only place which seems to really care
> about the races with __add_to_page_cache() or __delete_from_page_cache()
> is when the inode should be removed from memory. There we have to be
> careful. Races with __add_to_page_cache() cannot happen because there is
> noone who could trigger addition of new page to the inode being evicted.
> Races with __delete_from_page_cache() are possible though...
  Andrew, any opinion on this? I'd like to get the bug fixed... I'll
happily move the nrpages check in end_writeback() under the spinlock if
people find that nicer. That place really looks like the only one which
depends on nrpages being consistent and uptodate.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Andrew Morton]

On Tue, 14 Jun 2011 00:01:44 +0200
Jan Kara <jack@suse.cz> wrote:

> On Wed 08-06-11 18:36:43, Jan Kara wrote:
> > On Tue 07-06-11 14:33:01, Andrew Morton wrote:
> > > On Tue, 07 Jun 2011 07:46:37 +0200
> > > Miklos Szeredi <mszeredi@suse.cz> wrote:
> > > 
> > > > > Either way, I don't think that the uglypatch expresses a full
> > > > > understanding of te bug ;)
> > > > 
> > > > I don't see a better way, how would we make nrpages update atomically
> > > > wrt the radix-tree while using only RCU?
> > > > 
> > > > The question is, does it matter that those two can get temporarily out
> > > > of sync?
> > > > 
> > > > In case of inode eviction it does, not only because of that BUG_ON, but
> > > > because page reclaim must be somehow synchronised with eviction.
> > > > Otherwise it may access tree_lock on the mapping of an already freed
> > > > inode.
> > > > 
> > > > In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
> > > > weren't under tree_lock before Nick's RCUification, so their use were
> > > > just optimization.   
> > > 
> > > Gee, we've made a bit of a mess here.
> > > 
> > > Rather than bodging around particualr codesites where that mess exposes
> > > itself, how about we step back and work out what our design is here,
> > > then implement it and check that all sites comply with it?
> > > 
> > > What is the relationship between the radix-tree and nrpages?  What are
> > > the locking rules?  Can anyone come up with a one-sentence proposal?
> > AFAIU, nrpages and radix-tree are consistent under tree_lock.
> > 
> > nrpages is only used (well, apart from shmfs and other filesystems which
> > use the value as a guess how much should they expect to write or similar
> > heuristics) to test mapping->nrpages == 0 and the test is performed without
> > any synchronization which looks natural because we later do only
> > rcu-protected lookups anyway. So it seems it's expected the test is
> > unreliable and we just use it to make things faster. The same race as with
> > nrpages test can happen during the radix tree lookup anyway...
> > 
> > I went through the tests and the only place which seems to really care
> > about the races with __add_to_page_cache() or __delete_from_page_cache()
> > is when the inode should be removed from memory. There we have to be
> > careful. Races with __add_to_page_cache() cannot happen because there is
> > noone who could trigger addition of new page to the inode being evicted.
> > Races with __delete_from_page_cache() are possible though...
>   Andrew, any opinion on this? I'd like to get the bug fixed... I'll
> happily move the nrpages check in end_writeback() under the spinlock if
> people find that nicer. That place really looks like the only one which
> depends on nrpages being consistent and uptodate.

That seems a cleaner way of avoiding one manifestation of the bug.

But what *is* the bug?  That we've made nrpages incoherent with the
state of the tree?  Or is it simply that the rule has always been "you
must hold tree_lock to access nrpages", and the rcuification exposed
that?

I want to actually fix this stuff up and get a good clear design which
we can describe and understand.  No band-aids, please.  Not in here.

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Jan Kara]

On Mon 13-06-11 15:14:01, Andrew Morton wrote:
> On Tue, 14 Jun 2011 00:01:44 +0200
> Jan Kara <jack@suse.cz> wrote:
> > On Wed 08-06-11 18:36:43, Jan Kara wrote:
> > > On Tue 07-06-11 14:33:01, Andrew Morton wrote:
> > > > On Tue, 07 Jun 2011 07:46:37 +0200
> > > > Miklos Szeredi <mszeredi@suse.cz> wrote:
> > > > 
> > > > > > Either way, I don't think that the uglypatch expresses a full
> > > > > > understanding of te bug ;)
> > > > > 
> > > > > I don't see a better way, how would we make nrpages update atomically
> > > > > wrt the radix-tree while using only RCU?
> > > > > 
> > > > > The question is, does it matter that those two can get temporarily out
> > > > > of sync?
> > > > > 
> > > > > In case of inode eviction it does, not only because of that BUG_ON, but
> > > > > because page reclaim must be somehow synchronised with eviction.
> > > > > Otherwise it may access tree_lock on the mapping of an already freed
> > > > > inode.
> > > > > 
> > > > > In other cases?  AFAICS it doesn't matter.  Most ->nrpages accesses
> > > > > weren't under tree_lock before Nick's RCUification, so their use were
> > > > > just optimization.   
> > > > 
> > > > Gee, we've made a bit of a mess here.
> > > > 
> > > > Rather than bodging around particualr codesites where that mess exposes
> > > > itself, how about we step back and work out what our design is here,
> > > > then implement it and check that all sites comply with it?
> > > > 
> > > > What is the relationship between the radix-tree and nrpages?  What are
> > > > the locking rules?  Can anyone come up with a one-sentence proposal?
> > > AFAIU, nrpages and radix-tree are consistent under tree_lock.
> > > 
> > > nrpages is only used (well, apart from shmfs and other filesystems which
> > > use the value as a guess how much should they expect to write or similar
> > > heuristics) to test mapping->nrpages == 0 and the test is performed without
> > > any synchronization which looks natural because we later do only
> > > rcu-protected lookups anyway. So it seems it's expected the test is
> > > unreliable and we just use it to make things faster. The same race as with
> > > nrpages test can happen during the radix tree lookup anyway...
> > > 
> > > I went through the tests and the only place which seems to really care
> > > about the races with __add_to_page_cache() or __delete_from_page_cache()
> > > is when the inode should be removed from memory. There we have to be
> > > careful. Races with __add_to_page_cache() cannot happen because there is
> > > noone who could trigger addition of new page to the inode being evicted.
> > > Races with __delete_from_page_cache() are possible though...
> >   Andrew, any opinion on this? I'd like to get the bug fixed... I'll
> > happily move the nrpages check in end_writeback() under the spinlock if
> > people find that nicer. That place really looks like the only one which
> > depends on nrpages being consistent and uptodate.
> 
> That seems a cleaner way of avoiding one manifestation of the bug.
  OK.

> But what *is* the bug?  That we've made nrpages incoherent with the
> state of the tree?  Or is it simply that the rule has always been "you
> must hold tree_lock to access nrpages", and the rcuification exposed
> that?
> 
> I want to actually fix this stuff up and get a good clear design which
> we can describe and understand.  No band-aids, please.  Not in here.
  OK, I belive the rule is "you must hold tree_lock to access nrpages" but
there are plenty of places which don't hold tree_lock and still peek at
nrpages to see if they have anything to do (and they were there even before
radix tree was rcuified). These are inherently racy and usually they don't
care - but possibly each such place should carry a comment explaining why
this racy check does not matter...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix assertion mapping->nrpages == 0 in end_writeback()

[Andrew Morton]

On Tue, 14 Jun 2011 00:49:24 +0200
Jan Kara <jack@suse.cz> wrote:

> > > people find that nicer. That place really looks like the only one which
> > > depends on nrpages being consistent and uptodate.
> > 
> > That seems a cleaner way of avoiding one manifestation of the bug.
>   OK.
> 
> > But what *is* the bug?  That we've made nrpages incoherent with the
> > state of the tree?  Or is it simply that the rule has always been "you
> > must hold tree_lock to access nrpages", and the rcuification exposed
> > that?
> > 
> > I want to actually fix this stuff up and get a good clear design which
> > we can describe and understand.  No band-aids, please.  Not in here.
>   OK, I belive the rule is "you must hold tree_lock to access nrpages" but
> there are plenty of places which don't hold tree_lock and still peek at
> nrpages to see if they have anything to do (and they were there even before
> radix tree was rcuified). These are inherently racy and usually they don't
> care - but possibly each such place should carry a comment explaining why
> this racy check does not matter...

OK, but it's weird and unexpected that a call to
truncate_inode_pages(everything) can return with nrpages non-zero. 
Worth documenting this somewhere?  And mention the
behaviour/requirement at the nrpages definition site?


--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>