Re: [RFC,PATCH 07/20] svc: centralise close handling

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 3296 bytes --]

Tom Tucker wrote:
> Centralise the handling of the SK_CLOSE bit so that future
> server transport implementations will be easier to
> write correctly. The xpt_recvfrom method does not 
> need to check for SK_CLOSE anymore, that's handled in 
> core code.

You are removing the test_bit(SK_CLOSE) from the transport specific 
recvfrom functions into the main svc_recv() entry point.  This moves the 
test before this code:

         if ((rqstp->rq_deferred = svc_deferred_dequeue(svsk))) {
                 svc_sock_received(svsk);
                 return svc_deferred_recv(rqstp);
         }

Why not move this test too (it's the same in both the tcp and udp 
recvfrom routines)?

Is it always safe to check if the socket is closed before checking the 
deferred queue?  Does that mean that now deferred requests are 
automatically thrown away if SK_CLOSE is set?

> Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>
> Signed-off-by: Peter Leckie <pleckie@melbourne.sgi.com>
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> ---
> 
>  net/sunrpc/svcsock.c |   28 +++++++++++++---------------
>  1 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 8fad53d..5c3a794 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -756,11 +756,6 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
>  		return svc_deferred_recv(rqstp);
>  	}
>  
> -	if (test_bit(SK_CLOSE, &svsk->sk_flags)) {
> -		svc_delete_socket(svsk);
> -		return 0;
> -	}
> -
>  	clear_bit(SK_DATA, &svsk->sk_flags);
>  	skb = NULL;
>  	err = kernel_recvmsg(svsk->sk_sock, &msg, NULL,
> @@ -1158,11 +1153,6 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
>  		return svc_deferred_recv(rqstp);
>  	}
>  
> -	if (test_bit(SK_CLOSE, &svsk->sk_flags)) {
> -		svc_delete_socket(svsk);
> -		return 0;
> -	}
> -

Should you also remove the "test_bit(SK_CLOSE)" from the dprintk in this 
function?

>  	if (svsk->sk_sk->sk_state == TCP_LISTEN) {
>  		svc_tcp_accept(svsk);
>  		svc_sock_received(svsk);
> @@ -1406,8 +1396,10 @@ svc_tcp_init(struct svc_sock *svsk)
>  
>  		set_bit(SK_CHNGBUF, &svsk->sk_flags);
>  		set_bit(SK_DATA, &svsk->sk_flags);
> -		if (sk->sk_state != TCP_ESTABLISHED)
> +		if (sk->sk_state != TCP_ESTABLISHED) {
> +			/* note: caller calls svc_sock_enqueue() */
>  			set_bit(SK_CLOSE, &svsk->sk_flags);
> +		}
>  	}
>  }

The comment is unclear.  Maybe a longer block comment is needed before 
the if statement.  Do you really need the brackets here?

> @@ -1525,10 +1517,16 @@ svc_recv(struct svc_rqst *rqstp, long ti
>  	}
>  	spin_unlock_bh(&pool->sp_lock);
>  
> -	dprintk("svc: server %p, pool %u, socket %p, inuse=%d\n",
> -		 rqstp, pool->sp_id, svsk, atomic_read(&svsk->sk_inuse));
> -	len = svsk->sk_xprt->xpt_recvfrom(rqstp);
> -	dprintk("svc: got len=%d\n", len);
> +	len = 0;
> +	if (test_bit(SK_CLOSE, &svsk->sk_flags)) {
> +		dprintk("svc_recv: found SK_CLOSE\n");
> +		svc_delete_socket(svsk);
> +	} else {
> +		dprintk("svc: server %p, pool %u, socket %p, inuse=%d\n",
> +			 rqstp, pool->sp_id, svsk, atomic_read(&svsk->sk_inuse));
> +		len = svsk->sk_xprt->xpt_recvfrom(rqstp);
> +		dprintk("svc: got len=%d\n", len);
> +	}
>  
>  	/* No data, incomplete (TCP) read, or accept() */
>  	if (len == 0 || len == -EAGAIN) {


[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 315 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
email;internet:chuck dot lever at nospam oracle dot com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs