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 <[email protected]> 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 <[email protected]>
SUSE Labs, CR
_______________________________________________
stable mailing list
[email protected]
http://linux.kernel.org/mailman/listinfo/stable
