[Pvfs2-developers] kernel module copy attrs race

[Sam Lang]

On Mar 25, 2008, at 2:40 PM, Pete Wyckoff wrote:
> slang at mcs.anl.gov 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
>> [..]
>> +        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
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2417 bytes
Desc: not available
Url : http://www.beowulf-underground.org/pipermail/pvfs2-developers/attachments/20080325/281c450a/smime.bin