Message-ID: <20100207163444.GA7434@linux.vnet.ibm.com>
Date:	Sun, 7 Feb 2010 08:34:44 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH][RFC] %pd - for printing dentry name

On Thu, Feb 04, 2010 at 05:36:09PM +0000, Al Viro wrote:
> On Thu, Feb 04, 2010 at 09:13:18AM -0800, Linus Torvalds wrote:
> > 
> > 
> > On Thu, 4 Feb 2010, Paul E. McKenney wrote:
> > > 
> > > Ah, good point on the hash size.  And given that DNAME_INLINE_LEN_MIN
> > > is 40 characters on 32-bit systems and 32 characters on 64-bit systems,
> > > I agree that while a four-character increase might be nice, it cannot be
> > > said to be an emergency.
> > 
> > Well, what we _could_ do is to make the 'length' field be part of the name 
> > itself, and just keep the hash separate. The hash (and parenthood) is what 
> > we check most in the hot inner loop, and don't want to have any extra 
> > indirection (or cache misses) for. The name length we check only later, 
> > after we've done all other checks (and after we've gotten the spinlock, 
> > which is the big thing).
> > 
> > So qstr->len is _not_ performance critical in the same way that qstr->hash 
> > is.
> 
> We could also try to put the hash chain in that sucker, copy d_parent in
> there *and* put a pointer back to struct dentry in it.  Then the walk
> itself would go through those and we'd actually looked at the dentry
> only once - in the end of it.  Normally that thing would be just embedded
> into dentry, with ability to allocate separately.

Good point!!!

But wouldn't this also require that the permission bits
be in qstr as well, along with a flag indicating ACLs?

> That might deal with lockless lookups if we did it right, but delayed
> copying back into dentry and freeing of out-of-line copy (after d_move())
> would still cause all sorts of fun.
> 
> The thing is, we have places where ->d_name.name uses rely on "I hold
> i_mutex on parent, so this thing won't change or go away under me" and
> that's actually the majority of code using ->d_name.  All directory
> operations.
> 
> How about doing that delayed work just before dropping i_mutex on parent?
> There we definitely can sleep, etc., so if we have d_move mark dentry as
> "got out-of-line hash chain+name+hash+len+d_parent_copy, want to collapse
> it back into dentry" and do d_collapse_that_stuff(dentry) before the
> matching drop of i_mutex...

This sounds like a good way to solve the problem of successive renames
of the same file -- the second rename would be unable to acquire i_mutex
until after the d_collapse_that_stuff() completed, right?

> It would be one hell of a patch size, probably, but it seems that the rest
> of problems wouldn't be there...  All such out-of-line structs would be
> freed via RCU and never modified.  And inline ones would be modified only
> when
> 	a) everyone who looks at hash chains already sees out-of-line one
> 	b) i_mutex on parent is still held
> They'd get out-of-line one copied into them, replace it in hash chains
> and schedule freeing of out-of-line sucker.

And during the time that the dentry is switching from out-of-line to
inline, it can safely be referenced by both, so no need for fancy
hash-chain traversal tactics.

> The reason why I'm talking about copy of d_parent and not just taking the
> field over there: we avoid messing with dentry refcounting, etc. that way,
> assuming that this copy is never dereferenced (used only for comparisons
> during dcache lookups) and doesn't contribute to d_count.  Freeing dentries
> themselves would be also RCU-delayed, of course.
> 
> Comments?

Looks pretty good at first glance!

							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists