knfsd: centralise SK_CLOSE handling

RFC PATCH In review Greg Banks 2 participants 4 messages May 16, 2007 – May 20, 2007

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Greg Banks

author

2007-05-16 19:25 UTC #0 lore raw

RFC PATCH 8/14 knfsd: centralise SK_CLOSE handling

Centralise the handling of the SK_CLOSE bit to that future sunrpc server transport implementations will be easier to write correctly. The bit should now not be manipulated directly, inline exist to wrap that. Also, the sko_recvfrom method does not need to check for SK_CLOSE anymore, that's handled in core code.

Signed-off-by Greg Banks <[email protected]>

Signed-off-by Peter Leckie <[email protected]>

---

 include/linux/sunrpc/svcsock.h |   23 ++++++++++++++++
 net/sunrpc/svcsock.c           |   42 +++++++++++++++---------------
 2 files changed, 44 insertions(+), 21 deletions(-)

include/linux/sunrpc/svcsock.h +23 -0

Index: linux/include/linux/sunrpc/svcsock.h
===================================================================
@@ -124,4 +124,27 @@ void		svc_sock_received(struct svc_sock 
 #define SVC_SOCK_ANONYMOUS	(1U << 0)	/* don't register with pmap */
 #define SVC_SOCK_TEMPORARY	(1U << 1)	/* flag socket as temporary */
 
+/*
+ * To delete a svc_sock, you must do one of two things.  In process
+ * (e.g. nfsd) context, you can call svc_delete_socket().  In other
+ * contexts (softirq, timers etc) call svc_sock_set_close() and
+ * then svc_sock_enqueue().  This enqueues the svc_sock so that an nfsd
+ * will eventually come along and call svc_delete_socket() for you.
+ */
+static inline void svc_sock_set_close(struct svc_sock *svsk)
+{
+	set_bit(SK_CLOSE, &svsk->sk_flags);
+}
+
+/*
+ * Convenience function for the common idiom of enqueuing
+ * a svc_sock for later deletion by an nfsd.  Useful for
+ * error handling in non-process context.
+ */
+static inline void svc_sock_delete_bh(struct svc_sock *svsk)
+{
+	svc_sock_set_close(svsk);
+	svc_sock_enqueue(svsk);
+}
+
 #endif /* SUNRPC_SVCSOCK_H */

net/sunrpc/svcsock.c +21 ~~-21~~

89 lines not on this page · raw message

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

Neil Brown

2007-05-17 10:59 UTC #1 lore raw

Quoted 7 lines · On Thursday May 17, [email protected] wrote:

Centralise the handling of the SK_CLOSE bit to that future sunrpc server transport implementations will be easier to write correctly. The bit should now not be manipulated directly, inline exist to wrap that. Also, the sko_recvfrom method does not need to check for SK_CLOSE anymore, that's handled in core code.

...

net/sunrpc/svcsock.c svc_tcp_accept(struct svc_sock *svsk) · line 1147

Quoted 10 lines

net/sunrpc/svcsock.c +5 -1

 		if (svsk) {
-			svc_sock_enqueue(svsk);
+			/*
+			 * We're always called in nfsd context so we
+			 * don't have to muck around with SK_CLOSE.
+			 */
+			svc_delete_socket(svsk);
 			svc_sock_put(svsk);
 		}

I'm not convinced that this is right. svc_delete_socket has a comment:
/*
* We used to delete the svc_sock from whichever list
* it's sk_ready node was on, but we don't actually
* need to. This is because the only time we're called
* while still attached to a queue, the queue itself
* is about to be destroyed (in svc_destroy). */
and I think this change invalidate the premise of that comment. I would much rather this was svc_sock_delete_bh.

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

Greg Banks

author

2007-05-18 06:36 UTC #2 lore raw

Quoted 10 lines · Neil Brown

Quoted 7 lines · On Thursday May 17, [email protected] wrote:
Centralise the handling of the SK_CLOSE bit to that future sunrpc server transport implementations will be easier to write correctly. The bit should now not be manipulated directly, inline exist to wrap that. Also, the sko_recvfrom method does not need to check for SK_CLOSE anymore, that's handled in core code.
...
Quoted 1 line

Quoted 11 lines

net/sunrpc/svcsock.c svc_tcp_accept(struct svc_sock *svsk) · line 1147
Quoted 9 lines
net/sunrpc/svcsock.c +5 -1
 		if (svsk) {
-			svc_sock_enqueue(svsk);
+			/*
+			 * We're always called in nfsd context so we
+			 * don't have to muck around with SK_CLOSE.
+			 */
+			svc_delete_socket(svsk);
 			svc_sock_put(svsk);
 		}
I'm not convinced that this is right.

I believe it is right, but I'll admit it's not obvious enough ;-)

Quoted 9 lines

svc_delete_socket has a comment:
/*
* We used to delete the svc_sock from whichever list
* it's sk_ready node was on, but we don't actually
* need to. This is because the only time we're called
* while still attached to a queue, the queue itself
* is about to be destroyed (in svc_destroy). */
and I think this change invalidate the premise of that comment.

The hunk above, which now calls svc_delete_socket() directly, is in svc_tcp_accept(), which is only ever called from svc_recv() *after* the svc_sock has been dequeued from the pool. At this point the svc_sock is not attached to a queue by sk_ready. So the premise of the comment is still valid.

I'll improve the comment to read:

/*
* We're always called in nfsd context when not
* attached to a queue by sk_ready, so can call
* svc_delete_socket() directly and not have to
* muck around with SK_CLOSE.
*/

BTW, I have a patch that moves that code out of svc_tcp_accept() into more generic code, so that RDMA connection transports are also subject to a connection limit. Coming soon.

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

Greg Banks

author

2007-05-20 23:57 UTC #3 lore raw

In reply to Neil Brown

Quoted 14 lines · Neil Brown

net/sunrpc/svcsock.c svc_tcp_accept(struct svc_sock *svsk) · line 1147
Quoted 11 lines · On Thursday May 17, [email protected] wrote:
net/sunrpc/svcsock.c +5 -1
 		if (svsk) {
-			svc_sock_enqueue(svsk);
+			/*
+			 * We're always called in nfsd context so we
+			 * don't have to muck around with SK_CLOSE.
+			 */
+			svc_delete_socket(svsk);
 			svc_sock_put(svsk);
 		}
I'm not convinced that this is right.

On further thought, you were entirely right and that part of my patch was bogus. At that point in the code, my comments about it being safe to call svc_delete_socket() are true for the svsk' that is passed in from the caller. However the svsk' I was passing to svc_delete_socket() is *another* struct svc_sock which was just chosen as a sacrificial victim, and it is *not* safe to call svc_delete_socket() directly on that. Woops.

The code now reads:

                if (svsk) {
                        /*
                         * Although we're always called in nfsd context,
                         * there's no guarantee that the svsk we've just
                         * chosen is not attached to a queue by sk_ready,
                         * so we cannot just svc_delete_socket() directly.
                         */
                        svc_sock_delete_bh(svsk);
                        svc_sock_put(svsk);
                }

I'm glad you caught that one.

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