Re: ->d_lock FUBAR (was Re: Linux 3.0-rc6)

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Jul 12, 2011 at 07:40:01PM -0700, Linus Torvalds wrote:

> It's dentry_lock_for_move() that makes me really nervous. Not only
> does it lock up to four dentries, but it mixes the whole parenthood vs
> pointer ordering.  Or course, it does have those BUG_ON() checks, so
> it should never cause any circular dependencies, but still..

Me too, obviously.

> The actual main protection to get lookups correct in the presence of
> concurrent moves largely depends on the sequence numbers (ie
> d_lookup() retrying if it hits a rename), which is why I also find it
> unlikely that we really should need to hold all those d_lock cases all
> at the same time.
> 
> So does d_move() really need to get all the locks at the same time and
> then do all the operations inside that "super-locked" region? Or could
> we get the locks in sequence and do individual parts of the rename
> operations under individual locks?
> 
> Are there any other d_lock cases that depend on the pointer ordering?
> Most everything else seems to be about direct parenthood, no?

It's not that easy.  We want ->d_lock on parents - not only because
there's code iterating through the list of children, but because
ordering on direct parenthood bloody depends on children not moving
out while we hold ->d_lock on their parent.  Otherwise we are looking
for nightmares in shrink_dcache_parent() et.al.

I'm not sure how much do we care about stability of x->d_parent when
x->d_lock is held.  ->d_compare() is the most obvious potential area
of trouble in that respect, but there might be more.

I'm still not finished reviewing ->d_lock uses; about a couple of hundreds
is left to wade through.  I would really, *REALLY* appreciate explicitly
defined locking rules from Nick (it's his changes, mostly).  As in "this,
this and that field is protected by ->d_lock on..."

Note that ->d_parent is stable when i_mutex is held on parent, which
makes most of the users of ->d_parent safe and fine (->lookup(), etc.
are all called with directory locked).  I've not finished reviewing
->d_parent users either, but IMO ->d_lock review is more important, so
it got bumped in front of queue...

Back to GrepTFS and stripping the paint off the walls...