I came across two somewhat related issues with d_revalidate, and wanted to find out if these were either known issues (possibly even addressed by Al's planned dcache changes?) or just the product of my own confusion.. The d_revalidate() behavior in real_lookup() currently bails out with -ENOENT when it encounters a race with i_mutex that confuses it. Specifically: - in do_lookup(), if no dentry is found (or do_revalidate() fails), we call real_lookup(). - real_lookup() takes the dir i_mutex. - if the dentry now exists (is found by d_lookup()), real_lookup() drops i_mutex, and calls do_revalidate() (again). - if do_revalidate() fails now, we return -ENOENT. This "two chances or -ENOENT" strategy strikes me as fundamentally racy. If, while we wait for i_mutex, something else hashes the dentry, but for one reason or another leaves it in a state in which d_revalidate will fail, we get -ENOENT instead of a fresh call to lookup. For (reasonably long) timeout-based revalidation schemes, that doesn't really happen, but if your timeout is sufficiently short and i_mutex is contended, or you are explicitly invalidating a dentry (e.g. due to a callback from the server), it can happen more frequently. It also looks like we can get into trouble if a ->d_revalidate() doesn't d_drop() when it fails: do_revalidate() will try to d_invalidate(), but that can -EBUSY if the dentry has non-prunable children, and do_revalidate() silently drops that error condition. If I'm reading this correctly, that means if we ever get a hashed dentry with children for which ->d_revalidate() fails (and doesn't d_drop()), we will get -ENOENT forever without ever calling ->lookup(). (I started worrying about d_revalidate()s that don't d_drop() after noticing that at least cifs and ocfs2 don't always do so on failure. It seems reasonable to expect/require d_revalidate() to d_drop() any dentry it fails on, but that doesn't appear to be the case.) Possible solutions? - I looked a bit at whether simply looping in real_lookup() would address the issue (i.e. retake i_mutex, d_lookup(), call ->lookup() as needed). Starvation issues aside, that can loop forever if it gets a bad-but-hashed dentry (as described above). - Instead of returning -ENOENT (which seems kind of unlikely to ever be the "right" answer), real_lookup() could call ->lookup(), even though the dentry is hashed. Assuming that's even allowed (?), that might lead to more ->lookup() calls than we need... - A new ->d_revalidate_locked() method that can be called with the dir i_mutex held would allow real_lookup() to decide whether to call ->lookup() while avoiding any additional race/starvaction issues. None of these possibilities seem especially attractive. :( Are these known issues, or am I just misreading the code here? 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
