[pnfs] [PATCH 12/17] [SQUASHME] nfs41: return 0from nfs4_read_done

[Benny Halevy]

Rahul, it's git://linux-nfs.org/~bhalevy/linux-pnfs.git

Thanks!

Benny

On Jun. 05, 2008, 2:07 +0300, "Iyer, Rahul" <Rahul.Iyer at netapp.com> wrote:
> I'll take a look. Can you send me a pointer to the git tree? I'll clone
> it this time and try to keep up :)
> -Rahul
>  
> 
> -----Original Message-----
> From: Labiaga, Ricardo 
> Sent: Wednesday, June 04, 2008 2:08 PM
> To: Benny Halevy; Iyer, Rahul
> Cc: pnfs at linux-nfs.org
> Subject: Re: [pnfs] [PATCH 12/17] [SQUASHME] nfs41: return 0from
> nfs4_read_done
> 
> Hi Rahul,
> 
> Could you take a minute to see if you recall why nfs4_read_done was
> changed to return status instead of 0 (as it is in upstream)?
> 
> Thanks!
> 
> - ricardo
> 
> 
> On Wed, 2008-06-04 at 11:11 +0300, Benny Halevy wrote:
>> On Jun. 04, 2008, 2:40 +0300, Ricardo Labiaga
> <ricardo.labiaga at netapp.com> wrote:
>>> On Tue, 2008-06-03 at 20:40 +0300, Benny Halevy wrote:
>>>> nfs4 code always returned 0 from nfs4_read_done there is no reason 
>>>> for nfs41 to change that afaict.
>>>>
>>>> Signed-off-by: Benny Halevy <bhalevy at panasas.com>
>>>> ---
>>>>  fs/nfs/nfs4proc.c |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 
>>>> 04cba79..09b3410 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -2936,8 +2936,8 @@ static int nfs4_read_done(struct rpc_task
> *task, struct nfs_read_data *data)
>>>>  	nfs_invalidate_atime(data->inode);
>>>>  	if (task->tk_status > 0)
>>>>  		renew_lease(server, data->timestamp);
>>>> -	dprintk("<-- %s status= 0\n", __func__);
>>>> -	return status;
>>>> +	dprintk("<-- %s status= %d. returning 0\n", __func__, status);
>>>> +	return 0;
>>> We used to return status (which could have been 0 or
> task->tk_status).
>>> This now changes to always return 0.  Are you sure this is what we
> want?
>> This is how the original code looks:
>>
>> git show master:fs/nfs/nfs4proc.c:
>> ...
>>         nfs_invalidate_atime(data->inode);
>>         if (task->tk_status > 0)
>>                 renew_lease(server, data->timestamp);
>>         return 0;
>>
>> The question we need to ask is why we need to change that.
>> If there's a good reason to return status rather than 0 we need to 
>> spell it out and if it's not nfs4.1 specific we need to send a 
>> respective patch upstream.
>>
>>> - ricardo
>>>
>>>>  }
>>>>  
>>>>  static void nfs4_proc_read_setup(struct nfs_read_data *data, 
>>>> struct rpc_message *msg)