[pnfs] [PATCH 03/13] pnfsd: Simplify getdeviceinfo export ops.

[Benny Halevy]

Dean Hildebrand wrote:
>
>
> Benny Halevy wrote:
>> Dean, this patch would break compilation of the tree
>> so it makes the patchset non-bisectable.  The main problem
>> with that is if a regression is found we can't use git-bisect
>> to semi-automatically find the offending patch in O(log(#patches)).
>>   
> I agree, but I'm not sure how.  That patch changes a data structure 
> that is used in a lot of different places.  Should I merge all patches 
> together that are affected by the change to this data structure? (I 
> think that is almost all of my patches)
I'm not sure what is the best approach in this case, but in general,
off the top of my head we can use several techniques:

1. make the change atomic, i.e. make the change in data structures and 
in the code using them
in the same patch.

2. If that is too complicated, consider making a serious of patches that 
implements the above
in layers, each atomically changing part of the data structure and its 
usage.

3. If the approaches above still don't work, the new data structures can 
be given new names
and their usage can be implemented gradually, e.g. one function in a patch.
Once everything got converted the old data structures can be deleted, 
and if really wanted
the new structures can then be renamed to the old names (in an atomic 
change).
Each patch in this series should at least compile and ideally run correctly.
If there's a correctness issue while the new API is introduced you can 
introduce the
new functions with new names, without removing the old code yet (and I 
think it makes sense
in this case because the change in functionality is through enough that 
the diff in the patches
that you sent that show the new code with the existing code in a merged 
view are almost useless
as they don't help understanding what you did but rather make it harder...)
and then when the new API is fully implemented, change the layer that 
uses it and remove
the old implementation and interface.

Benny

> Dean
>> Also, minor nit below.
>>
>> Thanks for sending the patches!
>>
>> Benny
>>
>> On Jan. 10, 2008, 23:38 +0200, seattleplus at gmail.com wrote:
>> <snip>
>>
>>  
>>> @@ -35,6 +35,35 @@ enum fid_type {
>>>      FILEID_INO32_GEN_PARENT = 2,
>>>  };
>>>  
>>> +#if defined(CONFIG_PNFS)
>>> +
>>> +/* XDR stream arguments and results.  Exported file system uses this
>>> + * struct to encode information and return how many bytes were 
>>> encoded.
>>> + */
>>> +struct pnfs_xdr_info {
>>> +    u32 *p;            /* in */
>>> +    u32 *end;        /* in */
>>> +    u32 maxcount;        /* in */
>>> +    u32 bytes_written;    /* out */
>>> +};
>>> +
>>> +/* Used by get_device_info to encode a device (da_addr_body in spec)
>>> + * Args:
>>> + * xdr - xdr stream
>>> + * device - pointer to device to be encoded
>>> +*/
>>> +typedef int (*pnfs_encodedev_t)(struct pnfs_xdr_info *xdr, void 
>>> *device);
>>> +
>>> +/* Arguments for get_device_info */
>>> +struct pnfs_devinfo_arg {
>>> +    u32 type;
>>> +    u32 devid;
>>> +    struct pnfs_xdr_info xdr;
>>> +    pnfs_encodedev_t func;
>>> +};
>>> +
>>> +#endif
>>>     
>>
>> please add comment after #endif, like this:
>> #endif /* CONFIG_PNFS */
>>
>>
>>