[pnfs] [PATCH 12/17] Reroute a callback to be processed by thecallback service.
Benny Halevy
bhalevy at panasas.com
Tue May 22 05:34:20 EDT 2007
Iyer, Rahul wrote:
>
>
>> -----Original Message-----
>> From: Benny Halevy [mailto:bhalevy at panasas.com]
>> Sent: Monday, May 21, 2007 12:03 PM
>> To: Iyer, Rahul
>> Cc: pnfs at linux-nfs.org
>> Subject: Re: [pnfs] [PATCH 12/17] Reroute a callback to be
>> processed by thecallback service.
>>
>> Iyer, Rahul wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Iyer, Rahul
>>>> Sent: Monday, May 21, 2007 10:55 AM
>>>> To: Benny Halevy
>>>> Cc: pnfs at linux-nfs.org
>>>> Subject: Re: [pnfs] [PATCH 12/17] Reroute a callback to be
>> processed
>>>> by thecallback service.
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Benny Halevy [mailto:bhalevy at panasas.com]
>>>>> Sent: Monday, May 21, 2007 6:27 AM
>>>>> To: Iyer, Rahul
>>>>> Cc: pnfs at linux-nfs.org
>>>>> Subject: Re: [PATCH 12/17] Reroute a callback to be
>>>> processed by the
>>>>> callback service.
>>>>>
>>>>> Iyer, Rahul wrote:
>>>>>
>>>>>> From 205cbe5b3b6371126973ba1a682b0b922c1d9195 Mon Sep 17
>>>>> 00:00:00 2001
>>>>>> Message-Id:
>>>>> <205cbe5b3b6371126973ba1a682b0b922c1d9195.1179276403.git.iyer@
>>>>> netapp.com>
>>>>>> In-Reply-To:
>>>>> <19b0fc3bc27cc1b1527dd685f71b79cd249a3779.1179276402.git.iyer@
>>>>> netapp.com>
>>>>>> References:
>>>>> <19b0fc3bc27cc1b1527dd685f71b79cd249a3779.1179276402.git.iyer@
>>>>> netapp.com>
>>>>>> From: Rahul Iyer <iyer at netapp.com>
>>>>>> Date: Tue, 15 May 2007 16:57:28 -0700
>>>>>> Subject: [PATCH 12/17] Reroute a callback to be processed
>>>>> by the callback service.
>>>>>> This routine is the heart of the mux/demux of the
>>>>> callbacks. When a RPC request
>>>>>> arrives on the channel and is determined to be a callback,
>>>>> the following thinfs
>>>>>> happen:
>>>>>>
>>>>>> 1. A request (rpc_rqst struct) is allocated for the rpc
>>>>> request and the request
>>>>>> is read in.
>>>>>>
>>>>>> 2. The request is queued onto the callback request list of
>>>>> the corresponding
>>>>>> svc_serv for processing by the callback service
>>>>>>
>>>>>> Signed-off-by: Rahul Iyer <iyer at netapp.com>
>>>>>> ---
>>>>>> net/sunrpc/xprtsock.c | 30 ++++++++++++++++++++++++------
>>>>>> 1 files changed, 24 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index
>>>>>> 50e000e..0c0d7e0 100644
>>>>>> --- a/net/sunrpc/xprtsock.c
>>>>>> +++ b/net/sunrpc/xprtsock.c
>>>>>> @@ -29,6 +29,7 @@
>>>>>> #include <linux/tcp.h>
>>>>>> #include <linux/sunrpc/clnt.h>
>>>>>> #include <linux/sunrpc/sched.h>
>>>>>> +#include <linux/sunrpc/bc_xprt.h>
>>>>>> #include <linux/file.h>
>>>>>>
>>>>>> #include <net/sock.h>
>>>>>> @@ -689,11 +690,20 @@ static inline void
>>>>> xs_tcp_read_request(struct rpc_xprt *xprt, skb_reader_t *desc
>>>>>> spin_lock(&xprt->transport_lock);
>>>>>> calldir = ntohl(xprt->tcp_calldir);
>>>>>> if (calldir == RPC_CALL) {
>>>>>> - dprintk("Callback received on backchannel!\n");
>>>>>> - spin_unlock(&xprt->transport_lock);
>>>>>>
>>>>>> - /* Ignore callbacks for now */
>>>>>> - return;
>>>>>> + req = xprt_alloc_bc_request(xprt);
>>>>>> +
>>>>>> + if (!req) {
>>>>>> + printk(KERN_NOTICE "Couldn't get
>>>>> rpc_rqst for the allback! Dropping callback...\n");
>>>>>> + xprt->tcp_flags &= ~XPRT_COPY_DATA;
>>>>>> + spin_unlock(&xprt->transport_lock);
>>>>>> +
>>>>>> + /*
>>>>>> + * Ignore the callback because we
>>>>> couldn't allocate a
>>>>>> + * request
>>>>>> + */
>>>>>> + return;
>>>>>> + }
>>>>> The spec says you should close the connection in this
>> case assuming
>>>>> returning an appropriate error here is too complicated for now.
>>>>> In any case the client MUST NOT silently drop a request
>>>> (and a printk
>>>>> counts as silence from the server POV ;-)
>>>>>
>>>>> Benny
>>>>>
>>>> Ok. A possible solution is to set the size of the mempool to
>>>> ca_maxrequests. Any allocation that fails means that the
>>>> ca_maxrequests has been exceeded (though exceeding the
>> limit doesn't
>>>> mean allocation failure, because it is a mempool). In this
>> scenario,
>>>> we can alert the xprt that this happened and return an
>> error. I can
>>>> look at this.
>>> Actually, scratch that. Since this is in softirq context,
>> there is no
>>> guarantee any allocation will succeed. If the size of the
>> mempool is
>>> at least 2 greater than the ca_maxrequests, then the
>> CB_SEQUENCE logic
>>> will notice that the slotid has exceeded the max_slots (or
>> there is a
>>> duplicate slotid). Thus it will return an error. In such a
>> case, the
>>> server should realize that there is a problem with its
>> sequence logic
>>> and probably close the connection. Despite this, if it keeps on
>>> sending CB_COMPOUND requests, this is a genuine case of resource
>>> exhaustion and I believe it should be ok to start dropping
>> requests.
>>> The server has overwhelmed the client's resources and
>> there's not much we can do.
>>> Does this make sense?
>> I don't think so. The idea behind sessions is that they
>> provide EOS and the receiver must not drop requests.
>>
>> See secion 2.9.2 in draft-10:
>>
>> In order to reduce congestion, if a connection-oriented
>> transport is
>> used, and the request is not the NULL procedure,
>>
>> o A requester MUST NOT retry a request unless the connection the
>> request was issued over was disconnected before the reply was
>> received.
>>
>> o A replier MUST NOT silently drop a request, even if the
>> request is
>> a retry. (The silent drop behavior of RPCSEC_GSS [5] does not
>> apply because this behavior happens at the RPCSEC_GSS layer, a
>> lower layer in the request processing). Instead, the replier
>> SHOULD return an appropriate error (see Section 2.10.4.1) or it
>> MAY disconnect the connection.
>
> I understand and agree with this. But we're at the point where the
> sender isn't following session rules anyways, and has been sent at least
> one error telling it that the slot management is messed up. Despite
> this, the sender is still sending requests. We're dealing with an
> illegal client here.
> Regars
> Rahul
>
Like the spec says. Keep returning errors or disconnect but you MUST NOT
silently drop requests. Error handling should just escalate. First return an
error, then, if that doesn't help, disconnect the connection and re-establish
the connection and back channel, then disconnect all connections and reestablish
the session. If that doesn't help, call 911 :) If it makes it simpler we
can just jump to the last resource and reestablish the session if the sender
is insane.
>>>> Ok. A possible solution is to set the size of the mempool to
>>>> ca_maxrequests.
Preallocating csa_back_chan_attrs.ca_maxrequests buffers is the right thing to do.
>>>> Any allocation that fails means that the
>>>> ca_maxrequests has been exceeded (though exceeding the
>> limit doesn't
>>>> mean allocation failure, because it is a mempool). In this
>> scenario,
>>>> we can alert the xprt that this happened and return an
>> error. I can
>>>> look at this.
>>> Actually, scratch that. Since this is in softirq context,
>> there is no
>>> guarantee any allocation will succeed. If the size of the
>> mempool is
>>> at least 2 greater than the ca_maxrequests, then the
>> CB_SEQUENCE logic
>>> will notice that the slotid has exceeded the max_slots (or
>> there is a
>>> duplicate slotid). Thus it will return an error. In such a
>> case, the
>>> server should realize that there is a problem with its
>> sequence logic
>>> and probably close the connection. Despite this, if it keeps on
>>> sending CB_COMPOUND requests, this is a genuine case of resource
>>> exhaustion and I believe it should be ok to start dropping
>> requests.
>>> The server has overwhelmed the client's resources and
>> there's not much we can do.
Hmm, we don't shrink the slot table yet so unless there's a leak in the
receiver's code we should never run out of resources.
I think we must examine csa_slotid and csa_sequenceid before attempting
xprt_alloc_bc_request().
If the slotid is greater than ca_maxrequests we should return NFS4ERR_BADSLOT.
If it's a slot reuse then depending on the sequenceid we should return either
NFS4ERR_SEQ_MISORDERED or the yet undefined error if the slot is reused while
the callback is in progress or tear down the connection/session.
Failing to allocate means there's a bug in our code and the best thing
we can probably do is print an error message to the log and reestablish the
session after pre-allocating (again) csa_back_chan_attrs.ca_maxrequests
requests. This means we identified a memory leak, but we can resume
operation.
More information about the pNFS
mailing list