[Pvfs2-developers] request unify and cleanup

[Sam Lang]

Hi Pete,

Thanks for the comments.

On Feb 9, 2008, at 10:46 AM, Pete Wyckoff wrote:

> slang at mcs.anl.gov wrote on Fri, 08 Feb 2008 18:27 -0600:
>> The attached patch tries to cleanup some of our server request  
>> code, so
>> that adding a new server request+state machine doesn't mean  
>> searching for
>> all the places where we perform a switch/case on the server op  
>> enum.  It
>> was motivated by the need for a more generic prelude state machine  
>> and
>> request scheduler, but its been something that I've been itching to  
>> knock
>> out for a while now.  I think there are still improvements that can  
>> be made
>> here, especially to the encoding/decoding and request scheduler,  
>> but this
>> gives me the functionality that I needed, so I decided its a good  
>> place to
>> stop.
>
> This looks like a very worthwhile cleanup.  The patch includes maybe
> 3 or 4 independent changes, but they are all tied to this one theme.
> They are:  mode change, ref count moving, rename parent_handle some
> places, server req params.  Even though I find it easier to digest
> small bits, it's all quite nice.
>
> Some minor comments.
>
>> +PINT_GET_OBJECT_REF_DEFINE(flush);
>
> Instead of defining a static inline function for each one of these
> (which won't be inlined of course), can you just add a single
> generic PINT_get_object_ref_generic() that they all can use?  Then
> we only see special functions for the oddball cases like mkdir, and
> have less code in the binary.  Maybe the same idea applies for
> PINT_server_req_access_io and _small_io; could be a generic function
> somewhere.
>

I would do that if I could think of a way.  Those functions have to  
access the request specific field in the request structure. delete for  
example needs to get at:

req->u.delete.handle

I couldn't come up with a generic function for doing that.  I was  
tempted to move the fsid+handle to the common parts of the request, so  
that every request had those fields (for some they would just be  
NULL), but that would have been a much larger change, and a protocol  
change to boot.

If I were able to assume that the fsid and handle were always the  
first two fields in the request (for the requests that operated on a  
handle), I could just operate on the req->u union (a poor man's  
polymorphism), but I don't know if that's an assumption/requirement we  
want to make (and would be another protocol change for the requests  
that don't have those as the first two).

If we wanted to go down this path further, I would probably want to  
work up some sort of bindings generator that would do a lot of the  
manual labor for us.

>> +    SERVERBINOBJS := $(patsubst %.c,%-server.o, $(filter %.c,$ 
>> (SERVERBINSRC)))
>> +    SERVERBINDEPENDS := $(patsubst %.c,%.d, $(filter %.c,$ 
>> (SERVERBINSRC)))
>
> Probably want to add these to the clean and deps generation lists
> too, respectively.  It is a bit clunky how we do this.  A nice
> makefile cleanup for somebody in the future perhaps.

Agreed.

>
>
> I bet that the new file pvfs2-server-req.c is as brilliant as the
> rest of this patch, wherever it may be.

Oops.  See new attached version.
-sam

-------------- next part --------------
A non-text attachment was scrubbed...
Name: request-unify.patch
Type: application/octet-stream
Size: 105328 bytes
Desc: not available
Url : http://www.beowulf-underground.org/pipermail/pvfs2-developers/attachments/20080209/93adc3d1/request-unify-0001.obj
-------------- next part --------------



>
>
> 		-- Pete
>