[pnfs] [PATCH 12/17] Reroute a callback to be processed by thecallback service.

[Benny Halevy]

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.