2008-04-22 14:21:00

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH 07/33] NFS: Ensure that rpc_run_task() errors are propagated back to the caller

On Apr 21, 2008, at 8:17 PM, Trond Myklebust wrote:
> On Mon, 2008-04-21 at 15:55 -0400, Chuck Lever wrote:
>> Hi Trond-
>>
>> On Apr 19, 2008, at 4:40 PM, Trond Myklebust wrote:
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> ---
>>>
>>> fs/nfs/direct.c | 10 ++++++----
>>> fs/nfs/read.c | 23 +++++++++++++++--------
>>> fs/nfs/write.c | 33 +++++++++++++++++++--------------
>>> 3 files changed, 40 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
>>> index abf8e02..4757a2b 100644
>>> --- a/fs/nfs/direct.c
>>> +++ b/fs/nfs/direct.c
>>> @@ -347,8 +347,9 @@ static ssize_t
>>> nfs_direct_read_schedule_segment(struct nfs_direct_req *dreq,
>>> NFS_PROTO(inode)->read_setup(data, &msg);
>>>
>>> task = rpc_run_task(&task_setup_data);
>>> - if (!IS_ERR(task))
>>> - rpc_put_task(task);
>>> + if (IS_ERR(task))
>>> + break;
>>> + rpc_put_task(task);
>>>
>>> dprintk("NFS: %5u initiated direct read call "
>>> "(req %s/%Ld, %zu bytes @ offset %Lu)\n",
>>
>> My reading of this logic suggests that if the very first
>> rpc_run_task() call in the loop fails, we'll return -EFAULT instead
>> of
>> something sensible, like -ENOMEM.
>
> I'm confused. How could ENOMEM be a sensible substitute for EFAULT?


My point is that EFAULT isn't an appropriate return code if
rpc_run_task() fails. EFAULT is appropriate only when the I/O engine
can't access the user's memory.

If no bytes were read, an error code that reflects the problem should
be returned.

task = rpc_run_task(&task_setup_data);
if (IS_ERR(task) {
result = PTR_ERR(task);
break;
}
rpc_put_task(task);

The logic at the end of nfs_direct_read_schedule_segment() will take
care to return the number of bytes read, or an error code if no bytes
were read.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com


2008-04-22 15:11:29

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 07/33] NFS: Ensure that rpc_run_task() errors are propagated back to the caller


On Tue, 2008-04-22 at 10:19 -0400, Chuck Lever wrote:

>
> My point is that EFAULT isn't an appropriate return code if
> rpc_run_task() fails. EFAULT is appropriate only when the I/O engine
> can't access the user's memory.
>
> If no bytes were read, an error code that reflects the problem should
> be returned.
>
> task = rpc_run_task(&task_setup_data);
> if (IS_ERR(task) {
> result = PTR_ERR(task);
> break;
> }
> rpc_put_task(task);
>
> The logic at the end of nfs_direct_read_schedule_segment() will take
> care to return the number of bytes read, or an error code if no bytes
> were read.

Then a fix should be applied to auth_gss.c: the EFAULT error message is
coming from the downcall code. If it is not appropriate to pass that on
to the RPC caller, then we should substitute an EACCES.

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com