On Tue, 2008-06-03 at 09:35 -0700, Linus Torvalds wrote: > > On Wed, 4 Jun 2008, Ian Kent wrote: > > > > The code we're talking about deals with a race between expiring and > > mounting an autofs mount point at the same time. > > > > I'll have a closer look and see if I can make it work without turning > > the dentry negative. > > Hmm. > > Can you walk me through this? > > If the dentry is unhashed, it means that it _either_ > > - has already been deleted (rmdir'ed) or d_invalidate()'d. Right? In the current code the only way a dentry gets onto this list is by one of the two operations ->unlink() or ->rmdir(), it is d_drop'ed and left positive by both of these operations (a carry over from 2.4 when d_lookup() returned unhashed dentrys, I missed that detail for quite a while). > > I don't see why you should ever return the dentry in this case.. It's been a while now but the original patch description should help. "What happens is that during an expire the situation can arise that a directory is removed and another lookup is done before the expire issues a completion status to the kernel module. In this case, since the the lookup gets a new dentry, it doesn't know that there is an expire in progress and when it posts its mount request, matches the existing expire request and waits for its completion. ENOENT is then returned to user space from lookup (as the dentry passed in is now unhashed) without having performed the mount request. The solution used here is to keep track of dentrys in this unhashed state and reuse them, if possible, in order to preserve the flags. Additionally, this infrastructure will provide the framework for the reintroduction of caching of mount fails removed earlier in development." I wasn't able to do an acceptable re-implementation of the negative caching we had in 2.4 with this framework, so just ignore the last sentence in the above description. > > - or it has not yet been hashed at all It has been previously hashed, yes. > > But then d_inode should be NULL too, no? Unfortunately no, but I thought that once the dentry became unhashed (aka ->rmdir() or ->unlink()) it was invisible to the dcache. But, of course there may be descriptors open on the dentry, which I think is the problem that's being pointed out. > > Anyway, as far as I can tell, you should handle the race between expiring > and re-mounting not by unhashing at expire time (which causes these kinds > of problems), but by setting a bit in the dentry and using the dentry > "revalidate()" callback to wait for the revalidate. Yes, that would be ideal but the reason we arrived here is that, because we must release the directory mutex before calling back to the daemon (the heart of the problem, actually having to drop the mutex) to perform the mount, we can get a deadlock. The cause of the problem was that for "create" like operations the mutex is held for ->lookup() and ->revalidate() but for a "path walks" the mutex is only held for ->lookup(), so if the mutex is held when we're in ->revalidate(), we could never be sure that we where the code path that acquired it. Sorry, this last bit is unclear. I'll need to work a bit harder on the explanation if you're interested in checking further. Anyway, I'm sure I made the dentry negative upon getting a rehash hit for a reason so I'll need to revisit it. Perhaps I was misguided in the first place or perhaps just plain lazy. Ian -- 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
