[pnfs] [PATCH 1/1] pnfsfile: Remove duplicated code in filelayout_read_pagelist

[Benny Halevy]

Fredric Isaman wrote:
> On Thu, 13 Mar 2008, Dean Hildebrand wrote:
>
>   
>> Fred Isaman wrote:
>>     
>>> Some offset setting code was duplicated via copy and paste, and then
>>> unecessarily called twice in a row in certain cases.
>>>
>>> Signed-off-by: Fred Isaman <iisaman at citi.umich.edu>
>>> ---
>>>  fs/nfs/nfs4filelayout.c |    7 -------
>>>  1 files changed, 0 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>> index 18fad9f..c5c102e 100644
>>> --- a/fs/nfs/nfs4filelayout.c
>>> +++ b/fs/nfs/nfs4filelayout.c
>>> @@ -288,13 +288,6 @@ ssize_t filelayout_read_pagelist(
>>>  			data->pnfs_client = ds->ds_clp->cl_rpcclient;
>>>  			data->ds_nfs_client = ds->ds_clp;
>>>  			data->args.fh = dserver.fh;
>>> -
>>> -			/* Now get the file offset on the dserver
>>> -			 * Set the read offset to this offset, and
>>> -			 * save the original offset in orig_offset
>>> -			 */
>>> -			data->args.offset = 
>>> filelayout_get_dserver_offset(offset, flseg);
>>> -			data->orig_offset = offset;
>>>
>>>       
>> Hmmmm, I think we want to delete the duplicated code after the if statement 
>> non?  We only need to save the offset for the pNFS case, which is the code 
>> you have deleted above.  If we are using the MDS, then we don't need to do 
>> this.
>>     
>>>  		}
>>>  	} else { /* If no layout use MDS */
>>>  		dprintk("%s: no layout, use MDS\n", __FUNCTION__);
>>>
>>>       
>
>
> OK.  My patch just removed the duplicate code in a fashion that did 
> not change behavior.  I can resubmit with your suggestion that changes 
> behavior, but someone who knows the file layout code better than I should 
> verify that the change is correct.
>
>  	Fred
>   
Dean is 100% right.
The current code works since flseg is set to NULL by default and
filelayout_get_dserver_offset returns the offset arg if the layout 
segment is NULL.
I'd rather leave the call only if the layoutid != NULL case and remove the
initialization to NULL so the compiler will issue a warning if flseg is used
outside the appropriate scope.


That said, I don't think the LD I/O functions are/will ever be called with
pnfs_layout_type == NULL :)
So actually, if this is true and there's no non-pnfs path that's
calling them, the right thing to do is to remove this whole useless code 
path.

Benny