On Thu, 30 Jul 2009, Ian Kent wrote:
> Sage Weil wrote:
> > On Wed, 29 Jul 2009, Ian Kent wrote:
> >>> How is it possible for a revalidate to fail and then to skip over the
> >>> ->lookup() call? That sounds like a problem with the VFS? (Like the one
> >>> I'm trying to fix... you're testing with the real_lookup patch applied, I
> >>> hope?)
> >> It's been hard to work out exactly what's happening but I suspect it is
> >> due to multiple walks occurring while the the dentry hashed state
> >> changes. Like, one process hits revalidate, drops the dentry, another
> >> process comes along and goes straight to lookup and rehashes the dentry,
> >> the original process, that dropped the dentry, then ends up not calling
> >> lookup. The race only happens occasionally, and my test uses 10
> >> processes that tend to hit the same dentrys at the same time at various
> >> points in the mount tree over 150 iterations. Admittedly, it's a fairly
> >> stressful test as autofs goes but it does tend to show up races quite well.
> >
> > If I'm reading namei.c correctly, every ->d_revalidate() that returns NULL
> > will always result in a ->lookup() (unless IS_DEADDIR(inode)), without any
> > regard for the dentry's hashed/unhashed status. The exception appears to
> > be a call in __link_path_walk when FS_REVAL_DOT is set in the superblock
> > (nfs only):
> >
> > if (nd->path.dentry && nd->path.dentry->d_sb &&
> > (nd->path.dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
> > err = -ESTALE;
> > /* Note: we do not d_invalidate() */
> > if (!nd->path.dentry->d_op->d_revalidate(
> > nd->path.dentry, nd))
> > break;
> >
> > in which case userspace would see an ESTALE.
> >
> > It sounds like a few well placed printks that include the pid for unhash,
> > rehash, revalidate, and lookup would narrow down what is going on?
>
> Of course I've been doing that.
>
> I think you need to re-read what I wrote above. Your analysis is fine if
> there is only one process concurrently walking onto the dentry. If a
> second process beats the revalidate process to real_lookup() and
> incorrectly rehashes the dentry the following revalidate process will
> not call ->lookup().
Sorry, you're right. I missed the d_lookup call in real_lookup. So you
mean something like:
do_lookup
__d_lookup -> finds dentry
do_revalidate -> drops dentry, returns null
real_lookup
take i_mutex
d_lookup -> returns dentry rehashed by racing ->lookup()
> My most recent try at this seems to be working.
> I need to clean up the printks and alter one of the comments and re-test
> with a full test (I have been working with only half the test enabled as
> it was most problematic). The testing takes a long time to run, 1 hour
> 10 minutes for each of two configurations and the criteria for success
> in at least six consecutive successful runs, that's about thirteen
> hours. But, hopefully, when that is done I'll have a series to post for
> comment.
Great to hear. I hope my inane blathering hasn't been totally useless! :)
Thanks-
sage
--
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
