On Mar 25, 2008, at 2:40 PM, Pete Wyckoff wrote:
[EMAIL PROTECTED] wrote on Tue, 25 Mar 2008 14:01 -0500:Attached patch adds the lock around the getattr for revalidate. It alsoincludes some cleanup to the d_revalidate function for my sanity.Looks good to me. I didn't study all the whitespace changes in detail.Index: src/kernel/linux-2.6/dcache.c [..]+ gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: parent not found. \n", __func__);+ return 0; /* not valid */I see that 0 is bad a few places here.+ gossip_debug(GOSSIP_DCACHE_DEBUG,+ "%s: getattr %s (ret = %d), returning %s for dentry\n",+ __func__, + (ret == 0 ? "succeeded" : "failed"), + ret, + (ret == 0 ? "valid" : "INVALID")); + + return ((ret == 0) ? 1 : 0);But now 0 is good and 1 is bad. As long as you have it all straight in your head. Maybe some "goto out" would help to clean all this up, or not.
Yeah getattr returns 0 on success. If getattr succeeds, we want to say that the inode was revalidated (return 1). I agree its a bit confusing, but I think its correct. I can add gotos.
I certainly enjoy that it will tell you "failed" and "INVALID" in the same error message. Way to reinforce those negatives. :)
In fact our error handling here looks to be a little wrong. When we return 0 from d_revalidate, the kernel assumes the file has been removed and populates the dcache with a negative dentry (then tries to create the file in the open case). But if getattr fails after lookup succeeds, we may have more than an invalid dentry.
In most cases where getattr returns an error we probably want to push that error all the way back up the stack, instead of just populating the dcache with a negative dentry. We could check for ENOENT from getattr and return 0, otherwise return the errno? Do the same for lookup failure?
Also, if the kernel gives us a null dentry, do we really want to return invalid dentry, or EINVAL with a big hairy gossip_err message to the log? At least in the current kernel, d_revalidate is never called with a null dentry.
-sam
-- Pete
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Pvfs2-developers mailing list Pvfs2-developers@beowulf-underground.org http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers