On Thu, May 29, 2014 at 04:44:54PM +0100, Al Viro wrote:
> On Thu, May 29, 2014 at 08:10:57AM -0700, Linus Torvalds wrote:
> > If so, though, that brings up two questions:
> >
> > (a) do we really want to be that aggressive? Can we ever traverse
> > _past_ the point we're actually trying to shrink in
> > shrink_dcache_parent()?
>
> Caller of shrink_dcache_parent() would better hold a reference to the
> argument, or it might get freed right under us ;-) So no, we can't
> go past that point - the subtree root will stay busy.
>
> The reason we want to be aggressive there is to avoid excessive iterations -
> think what happens e.g. if we have a chain of N dentries, with nothing pinning
> them (i.e. the last one has refcount 0, the first - 2, everything else - 1).
> Simply doing dput() would result in O(N^2) vs. O(N)...
>
> > (b) why does the "dput()" (or rather, the dentry_kill()) locking
> > logic have to retain the old trylock case rather than share the parent
> > locking logic?
> >
> > I'm assuming the answer to (b) is that we can't afford to drop the
> > dentry lock in dentry_kill(), but I'd like that answer to the "Why" to
> > be documented somewhere.
>
> We actually might be able to do it that way (rechecking ->d_count after
> lock_parent()), but I would really prefer to leave that until after -final.
> I want to get profiling data from that first - dput() is a much hotter path
> than shrink_dcache_parent() and friends...
FWIW, I've just done more or less edible splitup of stuff past #for-linus -
see #experimental-dentry_kill for that. Again, I really want to get
profiling data to see if that hurts dput() - it takes ->d_lock on parent
before the trylock on ->i_lock and in case of ->d_lock on parent being
held by somebody else it bangs on rename_lock.lock cacheline. I'd expect
that to be non-issue on any loads, but we need something stronger than
my gut feelings...
BTW, lock_parent() might be better off if in contended case it would not
bother with rename_lock and did something like this:
again:
spin_unlock(&dentry->d_lock);
rcu_read_lock();
parent = ACCESS_ONCE(dentry->d_parent);
if (parent != dentry)
spin_lock(&parent->d_lock);
spin_lock(&dentry->d_lock);
if (likely(dentry->d_parent == parent)) {
rcu_read_unlock();
return parent;
}
if (parent)
spin_unlock(&parent->d_lock);
rcu_read_unlock();
goto again;
It's almost certainly not worth bothering with right now, but if dput()
starts using lock_parent(), it might be worth investigating...
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
