[Pvfs2-developers] Trove patches

[Sam Lang]

On Jul 20, 2006, at 3:17 PM, Julian Martin Kunkel wrote:

> Hi,
>> Maybe if we could differentiate between user errors (ENOENT) and
>> unexpected system errors (ENOMEM, etc.), we could do that.
>> Unfortunately, right now we don't differentiate, so some fellow might
>> come along and do an ls on a directory or file that doesn't exist,
>> get an error,  causing a flush of the dbs, and stalling the other
>> file creates that another user or processing is trying to do.  I
>> would argue for not flushing for now.
> ok. I agree. I thought about the case that a operation does multiple
> modifications to the db but fails at the end and reverts the  
> changes. I
> haven't found such a op in pvfs2, yet. But in case there are some  
> the db has
> to be flushed to guarantee consistency. Are there such ops ?

I don't think so, no.

>
>> Sorry if I missed previous conversations about this, but doesn't
>> unlink just free the inode?  Wouldn't rename take just as long if not
>> longer..
> I don't think so, in think in case the file is still open it is not  
> deleted,
> but in our case it is. Mh.. maybe we could fake the same behavior  
> by closing
> the file later ?
> I think phil talked about the problem during our meeting ?
> Anyway that changes are only a couple of LOC...

Yes I forgot about Phil's comments during the meeting.  It looks like  
others have implemented changes in ext3 to fix this:

http://ext2.sourceforge.net/2005-ols/paper-html/node32.html

Those changes haven't made it into the everyday code though.

>> AFAICT, the only case where id_gen_safe_register _should_ be used, is
>> in returning an id back through the sysint calls.  This is especially
>> needed for pvfs2-client-core, where the id is then passed to the
>> kernel module, so it must be an opaque 64 bit value.  In all the
>> other cases (pvfs internal use), we are really just passing around
>> pointers.  If the pointers somehow become invalid, using gen_safe
>> instead of gen_fast isn't going to help us.  I would argue that the
>> other internal uses (it looks like there are a couple) of gen_safe
>> should be changed to gen_fast.
> I think gen_safe register and lookup deal with the fact that a  
> object is
> freed. Isn't the pointer still valid in case an object is  
> unregistered ? Thus
> a free operation from test or testcontext can not interfere with  
> the other
> test* ops, same with the cancel op.
> The modification would make the interface
> more stable and understandable and in case we later wan't to add  
> some test
> functions functionable. I know currently these functions are called  
> in a way
> that these cases can not happen. But, you never know what changes  
> happen to
> the upper layers.

gen_safe just stores the pointer in a hashtable and returns a hash  
key in replace of it.  There's no reference counting done on the  
pointer, so its not a way of implementing smart pointers.
There are already mutex locks around the dspace_test* calls, so they  
can't be called simultaneously anyway.

In essence what you have is code that fails cleanly if someone tries  
to call dspace_testcontext with an id, and then later call  
dspace_test with the _same_ id, after dspace_testcontext had already  
removed the id from the hashtable (because the operation had  
completed).  So its an awefully expensive way to show the developer  
that he has bugs in his code.

What I would be in favor of is a modification to the id_gen_fast  
calls that checks if debugging is enabled, and calls id_gen_safe if  
so.  That will help solve the safety concerns you have and still  
allow the non-debug compiled code to use the faster version.

I would also change the checks in the dspace_test* functions to  
asserts, since a null value from an id lookup is clearly a  
programming error.

-sam

>
>> Overall this does cleanup the dbpf code nicely, so good work Julian.
>> Hopefully we can get some really good performance numbers out of all
>> this.  There's still some style quirks in there in different places,
>> but those will go away over time, right? ;-)
> Thanks, I still try to eliminate a bug in the code. Style quirks =>  
> *hehehe*
> I try to adapt to your styles :)
>
> See you tommorow,
> Julian
> _______________________________________________
> Pvfs2-developers mailing list
> Pvfs2-developers at beowulf-underground.org
> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
>