knfsd: delete per transport
Subsystems (3)
RFC PATCH 2/14 knfsd: delete per transport
Add sko_detach and sko_free methods to svc_sock_ops and change svc_delete_socket() and svc_sock_put() to use them.
---
include/linux/sunrpc/svcsock.h | 12 ++++++ net/sunrpc/svcsock.c | 54 +++++++++++++++++++++++------- 2 files changed, 55 insertions(+), 11 deletions(-)
net/sunrpc/svcsock.c +43 -11
@@ -83,6 +83,8 @@ static void svc_udp_data_ready(struct sstatic 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);
include/linux/sunrpc/svcsock.h +12 -0
@@ -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 *); };/*
Signature · 12 lines
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 - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
Trivial comments, since I couldn't find any real problems:
net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 886
Quoted 6 lines · Greg Banks
net/sunrpc/svcsock.c +1
-1@@ -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.
net/sunrpc/svcsock.c bummer: · line 1794
Quoted 4 lines
net/sunrpc/svcsock.c +4
-0+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?
include/linux/sunrpc/svcsock.h struct svc_sock_ops { · line 19
Quoted 8 lines
include/linux/sunrpc/svcsock.h +2
-0@@ -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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 1 line · J. Bruce Fields
Trivial comments, since I couldn't find any real problems:
Well, that's hopeful ;-)
Quoted 10 lines · Greg Banks
net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 886
Quoted 6 lines
net/sunrpc/svcsock.c +1
-1@@ -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.
Quoted 6 lines
net/sunrpc/svcsock.c bummer: · line 1794
Quoted 4 lines
net/sunrpc/svcsock.c +4
-0+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.
Quoted 6 lines
include/linux/sunrpc/svcsock.h struct svc_sock_ops { · line 19
Quoted 3 lines
include/linux/sunrpc/svcsock.h +2
-0int (*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.
Signature · 12 lines
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 - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 889
Quoted 10 lines · On Thursday May 17, [email protected] wrote:
net/sunrpc/svcsock.c +3
-1@@ -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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 17 lines · Neil Brown
net/sunrpc/svcsock.c svc_udp_sendto(struct svc_rqst *rqstp) · line 889
Quoted 10 lines · On Thursday May 17, [email protected] wrote:
net/sunrpc/svcsock.c +3
-1@@ -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 thesvc_udp_opslooks just wrong.
Maybesvc_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.
Signature · 12 lines
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 - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 11 lines · On Friday May 18, [email protected] wrote:
I considered renaming the existing
svc_socktosvc_xprt, which would be another horribly unpronouncableidentifier(*)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.
Quoted 2 lines
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 23 lines · Neil Brown
Quoted 11 lines · On Friday May 18, [email protected] wrote:
I considered renaming the existing
svc_socktosvc_xprt, which would be another horribly unpronouncableidentifier(*)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.
Quoted 2 lines
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
Quoted 4 lines
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.
Signature · 12 lines
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 - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs