Re: [Pvfs2-developers] kernel module copy attrs race

[Sam Lang]

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 also

includes 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
[..]

+        return 0; /* not valid */

+ gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: parent not found. \n", __func__);

I see that 0 is bad a few places here.

+    gossip_debug(GOSSIP_DCACHE_DEBUG,

+                 __func__,
+                 (ret == 0 ? "succeeded" : "failed"),
+                 ret,
+                 (ret == 0 ? "valid" : "INVALID"));
+
+    return ((ret == 0) ? 1 : 0);

+ "%s: getattr %s (ret = %d), returning %s for dentry\n",

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.  :)

-sam

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.

                -- 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