[pnfs] [PATCH 14/37] pnfs: initialize pnfs_layout_type generic fields in pnfs layer

[Dean Hildebrand]

Benny Halevy wrote:
> On Jan. 03, 2008, 20:07 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
>   
>> Benny Halevy wrote:
>>     
>>> On Jan. 03, 2008, 10:54 +0200, Benny Halevy <bhalevy at panasas.com> wrote:
>>>   
>>>       
>>>> On Jan. 03, 2008, 0:20 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
>>>>     
>>>>         
>>>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>>>>>         
>>>>>>             
>>>>>>> index 9abd263..fdb3d40 100644
>>>>>>> --- a/fs/nfs/pnfs.c
>>>>>>> +++ b/fs/nfs/pnfs.c
>>>>>>> @@ -430,6 +430,8 @@ pnfs_inject_layout(struct nfs_inode *nfsi,
>>>>>>>  	if (nfsi->current_layout == NULL) {
>>>>>>>  		dprintk("%s Alloc'ing layout\n", __FUNCTION__);
>>>>>>>  		layid = io_ops->alloc_layout(server->pnfs_mountid, inode);
>>>>>>> +		if (layid)
>>>>>>> +			layid->inode = inode;
>>>>>>>  	} else {
>>>>>>>  		dprintk("%s Adding to current layout\n", __FUNCTION__);
>>>>>>>  		layid = nfsi->current_layout;
>>>>>>>   
>>>>>>>           
>>>>>>>               
>>>>> What happens if layid is NULL?  Isn't that an error? 
>>>>> Dean
>>>>>
>>>>>       
>>>>>           
>>>> Right, it is.
>>>>
>>>> In a later patch this code was moved to alloc_init_layout()
>>>> which, when returning NULL, triggers return of -ENOMEM from
>>>> get_alloc_layout() that's propagated up in pnfs_update_layout().
>>>>     
>>>>         
>>> One more thing, even in this version of pnfs_inject_layout as of this patch
>>> if layid==NULL we return NULL on line 445 resulting in -EIO from
>>> pnfs_update_layout().
>>>   
>>>       
>> Ok, but I thought the agreement was to not send out patches where a 
>> later patch changes/fixes things from an earlier patch.  There is no 
>> point in me reviewing this patch if the function is totally rewritten in 
>> a later patch.
>>     
>
> I think there's a tension here between the need to make the patchset
> easy enough to understand and review by making it "layered" in a sense
> that the changes are applied in a graduate manner vs. not changing
> things again in a later patch.  Since I wanted to keep the patchset
> also bisectable each patch needs to compile and run in a valid way
> so regressions can be pinpointed more easily and this sometimes
> leads to intermediate solutions such as this one that change
> later on when the code keeps mutating.
>
> Anyhow, I should've probably mentioned that in the patch's
> description...
>   
Well, I don't mind gradual changes, but adding code in one patch that 
will be removed in a later patch is definitely wrong.  I don't think the 
patchset should show a history of all the different ways to implement a 
feature. 

Thanks for all your explanations,
Dean
> Benny
>
>   
>> Dean
>>     
>>>   
>>>       
>>>> Benny
>>>>     
>>>>         
>>>   
>>>       
>
>