[RFC,PATCH 2/14] knfsd: delete per transport

[RFC,PATCH 2/14] knfsd: delete per transport

[Greg Banks]

Add sko_detach and sko_free methods to svc_sock_ops and
change svc_delete_socket() and svc_sock_put() to use them.

Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>
Signed-off-by: Peter Leckie <pleckie@melbourne.sgi.com>
---

 include/linux/sunrpc/svcsock.h |   12 ++++++
 net/sunrpc/svcsock.c           |   54 +++++++++++++++++++++++-------
 2 files changed, 55 insertions(+), 11 deletions(-)

Index: linux/net/sunrpc/svcsock.c
===================================================================
--- linux.orig/net/sunrpc/svcsock.c	2007-05-16 23:50:21.721414451 +1000
+++ linux/net/sunrpc/svcsock.c	2007-05-17 00:16:39.911496313 +1000
@@ -83,6 +83,8 @@ static void		svc_udp_data_ready(struct s
 static int		svc_udp_recvfrom(struct svc_rqst *);
 static int		svc_udp_sendto(struct svc_rqst *);
 static void		svc_close_socket(struct svc_sock *svsk);
+static void		svc_tcpip_detach(struct svc_sock *);
+static void		svc_tcpip_free(struct svc_sock *);
 
 static struct svc_deferred_req *svc_deferred_dequeue(struct svc_sock *svsk);
 static int svc_deferred_recv(struct svc_rqst *rqstp);
@@ -377,14 +379,9 @@ svc_sock_put(struct svc_sock *svsk)
 	if (atomic_dec_and_test(&svsk->sk_inuse)) {
 		BUG_ON(! test_bit(SK_DEAD, &svsk->sk_flags));
 
-		dprintk("svc: releasing dead socket\n");
-		if (svsk->sk_sock->file)
-			sockfd_put(svsk->sk_sock);
-		else
-			sock_release(svsk->sk_sock);
 		if (svsk->sk_info_authunix != NULL)
 			svcauth_unix_info_release(svsk->sk_info_authunix);
-		kfree(svsk);
+    	    	svsk->sk_ops->sko_free(svsk);
 	}
 }
 
@@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,
-	.sko_sendto = svc_udp_sendto
+	.sko_sendto = svc_udp_sendto,
+	.sko_detach = svc_tcpip_detach,
+	.sko_free = svc_tcpip_free
 };
 
 static void
@@ -1328,7 +1327,9 @@ svc_tcp_sendto(struct svc_rqst *rqstp)
 static const struct svc_sock_ops svc_tcp_ops = {
 	.sko_name = "tcp",
 	.sko_recvfrom = svc_tcp_recvfrom,
-	.sko_sendto = svc_tcp_sendto
+	.sko_sendto = svc_tcp_sendto,
+	.sko_detach = svc_tcpip_detach,
+	.sko_free = svc_tcpip_free
 };
 
 static void
@@ -1768,6 +1769,38 @@ bummer:
 }
 
 /*
+ * Detach the svc_sock from the socket so that no
+ * more callbacks occur.
+ */
+static void
+svc_tcpip_detach(struct svc_sock *svsk)
+{
+	struct sock *sk = svsk->sk_sk;
+
+	dprintk("svc: svc_sock_detach(%p)\n", svsk);
+
+	/* put back the old socket callbacks */
+	sk->sk_state_change = svsk->sk_ostate;
+	sk->sk_data_ready = svsk->sk_odata;
+	sk->sk_write_space = svsk->sk_owspace;
+}
+
+/*
+ * Free the svc_sock's socket resources and the svc_sock itself.
+ */
+static void
+svc_tcpip_free(struct svc_sock *svsk)
+{
+	dprintk("svc: svc_sock_free(%p)\n", svsk);
+
+	if (svsk->sk_sock->file)
+		sockfd_put(svsk->sk_sock);
+	else
+		sock_release(svsk->sk_sock);
+	kfree(svsk);
+}
+
+/*
  * Remove a dead socket
  */
 static void
@@ -1781,9 +1814,8 @@ svc_delete_socket(struct svc_sock *svsk)
 	serv = svsk->sk_server;
 	sk = svsk->sk_sk;
 
-	sk->sk_state_change = svsk->sk_ostate;
-	sk->sk_data_ready = svsk->sk_odata;
-	sk->sk_write_space = svsk->sk_owspace;
+	if (svsk->sk_ops->sko_detach)
+		svsk->sk_ops->sko_detach(svsk);
 
 	spin_lock_bh(&serv->sv_lock);
 
Index: linux/include/linux/sunrpc/svcsock.h
===================================================================
--- linux.orig/include/linux/sunrpc/svcsock.h	2007-05-16 23:50:21.637425405 +1000
+++ linux/include/linux/sunrpc/svcsock.h	2007-05-17 00:12:50.074342601 +1000
@@ -15,6 +15,18 @@ struct svc_sock_ops {
 	const char		*sko_name;
 	int			(*sko_recvfrom)(struct svc_rqst *rqstp);
 	int			(*sko_sendto)(struct svc_rqst *rqstp);
+	/*
+	 * Detach the svc_sock from it's socket, so that the
+	 * svc_sock will not be enqueued any more.  This is
+	 * the first stage in the destruction of a svc_sock.
+	 */
+	void			(*sko_detach)(struct svc_sock *);
+	/*
+	 * Release all network-level resources held by the svc_sock,
+	 * and the svc_sock itself.  This is the final stage in the
+	 * destruction of a svc_sock.
+	 */
+	void			(*sko_free)(struct svc_sock *);
 };
 
 /*
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH 2/14] knfsd: delete per transport

[J. Bruce Fields]

Trivial comments, since I couldn't find any real problems:

On Thu, May 17, 2007 at 05:19:51AM +1000, Greg Banks wrote:
> @@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
>  static const struct svc_sock_ops svc_udp_ops = {
>  	.sko_name = "udp",
>  	.sko_recvfrom = svc_udp_recvfrom,
> -	.sko_sendto = svc_udp_sendto
> +	.sko_sendto = svc_udp_sendto,

Any objection to always just including a trailing comma and saving a
little noise in each diff?  That seems to be common practice elsewhere.

> +static void
> +svc_tcpip_free(struct svc_sock *svsk)
> +{
> +	dprintk("svc: svc_sock_free(%p)\n", svsk);

We lived without these dprintk's before; do we need them now?

> --- linux.orig/include/linux/sunrpc/svcsock.h	2007-05-16 23:50:21.637425405 +1000
> +++ linux/include/linux/sunrpc/svcsock.h	2007-05-17 00:12:50.074342601 +1000
> @@ -15,6 +15,18 @@ struct svc_sock_ops {
>  	const char		*sko_name;
>  	int			(*sko_recvfrom)(struct svc_rqst *rqstp);
>  	int			(*sko_sendto)(struct svc_rqst *rqstp);
> +	/*
> +	 * Detach the svc_sock from it's socket, so that the

s/it's/its/

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH 2/14] knfsd: delete per transport

[Greg Banks]

On Wed, May 16, 2007 at 04:51:23PM -0400, J. Bruce Fields wrote:
> Trivial comments, since I couldn't find any real problems:

Well, that's hopeful ;-)

> On Thu, May 17, 2007 at 05:19:51AM +1000, Greg Banks wrote:
> > @@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
> >  static const struct svc_sock_ops svc_udp_ops = {
> >  	.sko_name = "udp",
> >  	.sko_recvfrom = svc_udp_recvfrom,
> > -	.sko_sendto = svc_udp_sendto
> > +	.sko_sendto = svc_udp_sendto,
> 
> Any objection to always just including a trailing comma and saving a
> little noise in each diff?  That seems to be common practice elsewhere.

Fixed.

> > +static void
> > +svc_tcpip_free(struct svc_sock *svsk)
> > +{
> > +	dprintk("svc: svc_sock_free(%p)\n", svsk);
> 
> We lived without these dprintk's before; do we need them now?

Not really.  Removed.

> >  	int			(*sko_sendto)(struct svc_rqst *rqstp);
> > +	/*
> > +	 * Detach the svc_sock from it's socket, so that the
> 
> s/it's/its/
> 

Doh!  Fixed.

Thanks Bruce.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH 2/14] knfsd: delete per transport

[Neil Brown]

On Thursday May 17, gnb@sgi.com wrote:
>  
> @@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
>  static const struct svc_sock_ops svc_udp_ops = {
>  	.sko_name = "udp",
>  	.sko_recvfrom = svc_udp_recvfrom,
> -	.sko_sendto = svc_udp_sendto
> +	.sko_sendto = svc_udp_sendto,
> +	.sko_detach = svc_tcpip_detach,
> +	.sko_free = svc_tcpip_free
>  };

Tiny nit:
  Despite widespread usage, I don't see udp as being a part of
  tcp/ip.  So having a function names *tcpip* in the svc_udp_ops looks
  just wrong.
  Maybe svc_ipsock_detach, svc_ipsock_free ??

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH 2/14] knfsd: delete per transport

[Greg Banks]

On Thu, May 17, 2007 at 08:46:20PM +1000, Neil Brown wrote:
> On Thursday May 17, gnb@sgi.com wrote:
> >  
> > @@ -886,7 +883,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
> >  static const struct svc_sock_ops svc_udp_ops = {
> >  	.sko_name = "udp",
> >  	.sko_recvfrom = svc_udp_recvfrom,
> > -	.sko_sendto = svc_udp_sendto
> > +	.sko_sendto = svc_udp_sendto,
> > +	.sko_detach = svc_tcpip_detach,
> > +	.sko_free = svc_tcpip_free
> >  };
> 
> Tiny nit:
>   Despite widespread usage, I don't see udp as being a part of
>   tcp/ip.  So having a function names *tcpip* in the svc_udp_ops looks
>   just wrong.
>   Maybe svc_ipsock_detach, svc_ipsock_free ??

I struggled with this.

The obvious thing would have been to use the word "sock" because what
these functions have in common is that they deal with sockets rather
than TCP/IP or even IP.  None of "tcpip" or "ipsock" accurately
reflect what these functions do.

But unfortunately our abstract structure is called "svc_sock".  So
doing the obvious would result in enormous confusion: is svc_sock_foo()
a generic transport function or a specific sockets-based one?

I considered renaming the existing svc_sock to svc_xprt, which
would be another horribly unpronouncable identifier(*) but would at
least be appropriately abstract and consistent with the client.

Then the naming would all be obvious and correct: svc_xprt_foo()
is a generic transport function, svc_sock_foo() is a more specific
socket-based transport function used as common code between TCP
and UDP transports.  Yay.

But it would be a much larger patch so I shied away.

I'm happy to do the naming any way you want, just let me know.

 * what did the Sun people who originally coined RPC terminology
   have against vowels, I wonder?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH 2/14] knfsd: delete per transport

[Neil Brown]

On Friday May 18, gnb@sgi.com wrote:
> 
> I considered renaming the existing svc_sock to svc_xprt, which
> would be another horribly unpronouncable identifier(*) but would at
> least be appropriately abstract and consistent with the client.
> 
> Then the naming would all be obvious and correct: svc_xprt_foo()
> is a generic transport function, svc_sock_foo() is a more specific
> socket-based transport function used as common code between TCP
> and UDP transports.  Yay.
> 
> But it would be a much larger patch so I shied away.

Sensible.

> 
> I'm happy to do the naming any way you want, just let me know.

I accept that "ipsock" is not accurate (as it is the 'sock' bit that
is significant, the 'ip' is fairly irrelevant) but it is at least
better than tcpip.  Another option is
   svc_detach_sock / svc_free_sock

Which avoid mentioned "svc_sock", but still says what you want.

So go with that, unless you find it objectionable, in which case got
with svc_ipsock_*.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH 2/14] knfsd: delete per transport

[Greg Banks]

On Fri, May 18, 2007 at 04:44:22PM +1000, Neil Brown wrote:
> On Friday May 18, gnb@sgi.com wrote:
> > 
> > I considered renaming the existing svc_sock to svc_xprt, which
> > would be another horribly unpronouncable identifier(*) but would at
> > least be appropriately abstract and consistent with the client.
> > 
> > Then the naming would all be obvious and correct: svc_xprt_foo()
> > is a generic transport function, svc_sock_foo() is a more specific
> > socket-based transport function used as common code between TCP
> > and UDP transports.  Yay.
> > 
> > But it would be a much larger patch so I shied away.
> 
> Sensible.
> 
> > 
> > I'm happy to do the naming any way you want, just let me know.
> 
> I accept that "ipsock" is not accurate (as it is the 'sock' bit that
> is significant, the 'ip' is fairly irrelevant) but it is at least
> better than tcpip.  Another option is
>    svc_detach_sock / svc_free_sock
> 
> Which avoid mentioned "svc_sock", but still says what you want.
> 
> So go with that, unless you find it objectionable, in which case got
> with svc_ipsock_*.

I really like having the module at the start and the verb at the end,
so I'll go for svc_ipsock_foo().

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs