Re: [PATCH 2/2] rpc: Add -EPERM processing for xs_udp_send_request()

[Jason Baron <jbaron@akamai.com>]

On 09/18/2014 05:20 PM, Trond Myklebust wrote:
> On Thu, Sep 18, 2014 at 5:02 PM, Jason Baron <jbaron@akamai.com> wrote:
>> On 09/18/2014 04:51 PM, Trond Myklebust wrote:
>>> On Thu, Sep 18, 2014 at 3:51 PM, Jason Baron <jbaron@akamai.com> wrote:
>>>> If an iptables drop rule is added for an nfs server, the client can end up in
>>>> a softlockup. Because of the way that xs_sendpages() is structured, the -EPERM
>>>> is ignored since the prior bits of the packet may have been successfully queued
>>>> and thus xs_sendpages() returns a non-zero value. Then, xs_udp_send_request()
>>>> thinks that because some bits were queued it should return -EAGAIN. We then try
>>>> the request and again and a softlockup occurs. The test sequence is simply:
>>>>
>>>> 1) open a file on the nfs server '/nfs/foo' (mounted using udp)
>>>> 2) iptables -A OUTPUT -d <nfs server ip> -j DROP
>>>> 3) write to /nfs/foo
>>>> 4) close /nfs/foo
>>>> 5) iptables -D OUTPUT -d <nfs server ip> -j DROP
>>>>
>>>> The softlockup occurs in step 4 above.
>>> For UDP, the expected and documented behaviour in the case above is as follows:
>>> - if the mount is soft, then return EIO on the first major timeout.
>> yeah - so this case is a softlockup in my testing :(
>>
>>> - if the mount is hard, then retry indefinitely on timeout.
>>>
>>> Won't these 2 patches end up propagating an EPERM to the application?
>>> That would be a definite violation of both hard and soft semantics.
>> ok, yeah it does propogate the -EPERM up - I wasn't aware of the correct
>> semantics - thanks.
>>
>> I can rework the patches such that they return -EIO instead for a soft mount,
>> and verify that we keep retrying for a hard one.
>>
> Doesn't the soft timeout currently trigger after the major timeout? If
> not, do we understand why it isn't doing so?

No, the soft timeout does not currently trigger after the major timeout. Instead,
the kernel spins indefinitely, and triggers a softlockup.

The reason is that xs_sendpages() returns a positive value in this case
and xs_udp_send_request() turns it in an -EAGAIN for the write operation.
Subsequently, we call call_transmit_status() and then call_status() which
sees the EAGAIN, which just starts all over again with a 'call_transmit()'.
So we are stuck spinning indefinitely in kernel space.

Simply moving the -EPERM up in this patch, results in the behavior you
described above - EIO after a major timeout on a soft mount, and indefinte
retries on a hard mount - but without the cpu consumption. IE applying
this on top of this patch:

--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2019,6 +2019,7 @@ call_status(struct rpc_task *task)
        case -EHOSTDOWN:
        case -EHOSTUNREACH:
        case -ENETUNREACH:
+       case -EPERM:
                if (RPC_IS_SOFTCONN(task)) {
                        rpc_exit(task, status);
                        break;
@@ -2048,7 +2049,6 @@ call_status(struct rpc_task *task)
        case -EAGAIN:
                task->tk_action = call_transmit;
                break;
-       case -EPERM:
        case -EIO:
                /* shutdown or soft timeout */
                rpc_exit(task, status);

We could also 'translate' the -EPERM into an '-ENETUNREACH' or such,
in the return from xs_udp_send_request(), if you think that would make
more sense?

Hopefully, I've explained things better.

Thanks,

-Jason