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

[Benny Halevy]

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...

Benny

> Dean
>>   
>>> Benny
>>>     
>>