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

[Dean Hildebrand]

Hi Benny,

I resent my device export changes as 5 patches (instead of the original 
11).  The first 3 are "setup" type patches and the 4th and 5th handle 
the device changes.

Thanks for the feedback.  Please let me know if you have any questions. 
Dean

Benny Halevy wrote:
> On Jan. 13, 2008, 0:01 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
>   
>> Benny Halevy wrote:
>>     
>>> 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.
>>>       
>> ok, I guess these days bisection is more important than small patches, 
>> so I'll go with option 1.
>> Dean
>>     
>
> More than a matter of temporal taste or mere preference, I try to follow
> Documentation/SubmittingPatches:
>
> 	3) Separate your changes.
>
> 	Separate _logical changes_ into a single patch file.
>
> 	For example, if your changes include both bug fixes and performance
> 	enhancements for a single driver, separate those changes into two
> 	or more patches.  If your changes include an API update, and a new
> 	driver which uses that new API, separate those into two patches.
>
> 	On the other hand, if you make a single change to numerous files,
> 	group those changes into a single patch.  Thus a single logical change
> 	is contained within a single patch.
>
> Breaking one logical change into smaller pieces just because it appears
> too big is bad.  However, if that can be done by distilling it into several
> smaller, logical, _and self-sufficient_ patches then go ahead with it.
> This is certainly the way to go.
>
> Benny
>
>   
>>> 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 */
>>>>>
>>>>>
>>>>>   
>>>>>           
>> _______________________________________________
>> pNFS mailing list
>> pNFS at linux-nfs.org
>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>>
>>     
>
>