Re: Linux 3.0-rc6 ..

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

On Mon, Jul 04, 2011 at 04:09:29PM -0700, Linus Torvalds wrote:

> Other than the isci driver, the rest really is just lots of random
> small stuff. It's getting to the point where I'm thinking I should
> just release 3.0, because it's been pretty quiet, and the fixes
> haven't been earth-shakingly exciting. Some drm (radeon and intel)
> fixes might be noticeable to more people, the rest would tend to be
> pretty esoteric.

Sigh...  Looks like we have serious problems around ->d_parent handling.
First of all, __d_unalias() is fscked - calling d_ancestor() is not
going to do us any good before we made sure that tree topology won't
change right under us.  Used to be protected by dcache_lock, but not
anymore.  Moreover, there's a similar problem with __d_materialise_dentry()
side of things; there we don't check for loop creation at all and with
NFS we just might try to attach a root of disconnected subtree *inside*
that subtree.  No check and no locking either...

Another piece of PITA - cifs_get_root() will cheerfully call
d_materialise_unique() on dentry it got from d_lookup() if it happens to
be negative to start with *and* directory had been created on server in
the meanwhile.  BUG_ON() triggered in d_materialise_unique()... The lack
of i_mutex on parent also doesn't help.  cifs_get_root() mess is from
this cycle, BTW.

btrfs get_default_root() doesn't grab i_mutex either.  It should, since it
calls d_splice_alias().

I'm really not fond of the code in dentry_lock_for_move(); we _might_ manage
to avoid deadlocks since i_mutex serialization might prevent contention on
d_lock, but I'm still not convinced, especially due to missing i_mutex
in several callers...  I mean, this

 * If there is an ancestor relationship:
 * dentry->d_parent->...->d_parent->d_lock
 *   ...
 *     dentry->d_parent->d_lock
 *       dentry->d_lock
 *
 * If no ancestor relationship:
 * if (dentry1 < dentry2)
 *   dentry1->d_lock
 *     dentry2->d_lock

is no good: suppose A is ancestor of B and C is unrelated to either.
With B sitting at lower address than C and A at higher one.  We have
A before B, since it's an ancestor; C before A since they are unrelated
and addresses compare that way; B before C (ditto).  Loops in lock
ordering are generally bad; we _might_ get away with that in this case
since we serialize d_move() callers to hell and back, but...

Al, going through ->d_parent code review and not happy about the state of
that animal...