Coming back to this now, just trying to review the filehandle-lookup/dcache interactions: On Fri, Mar 11, 2011 at 03:07:49PM +1100, NeilBrown wrote: > 1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence > was only a hint, its absence was a strong statement. > If the flag is set, the dentry might not be linked to the root. > If it is clear, it definitely is link through to the root. > However I think it was used with stronger intent than that. > > Now it seems to mean a little bit more: If it is set and the dentry > is hashed, then it must be on the sb->s_anon list. The code that makes that assumption is __d_shrink (which does the work of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to lock. I can't find any basis for that assumption. The only code that clears DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as hashing. Am I missing something? > This is a significant > which I never noticed (I haven't been watching). Originally a > disconnected dentry would be attached (and hashed) to its parent. Then > that parent would get its own parent and so on until it was attached all > the way to the root. Only then would be start clearing > DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if > that is correct. It looks wrong to me: If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle lookup thinking the dentry is OK to use. That could mean for example trying to rename across directories that don't have any ancestor relationship to each other in the dcache yet. So we need to wait to clear DCACHE_DISCONNECTED until we *know* the dentry's parents go all the way back to the root. As you say, that's what the current code does. But that means DCACHE_DISCONNECTED dentries can be hashed to their parents, and __d_shrink can be handed such dentries and then get the locking wrong. It looks like this bug might originate with Nick Piggin's ceb5bdc2d246 "fs: dcache per-bucket dcache hash locking"? There's no discussion in the changelog, so probably it was just based on an unexamined assumption about DCACHE_DISCONNECTED. I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test in __d_shrink(), or if we need another flag, or ? --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
