knfsd: max_payload per transport

RFC PATCH In review Greg Banks 3 participants 5 messages May 16, 2007 – May 22, 2007

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Greg Banks

author

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

RFC PATCH 5/14 knfsd: max_payload per transport

Make svc_max_payload() delegate to a new method in svc_sock_ops instead of reaching into the socket (beause later the NFS/RDMA transport will not even have a socket).

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

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

---

 include/linux/sunrpc/svcsock.h |    6 ++++++
 net/sunrpc/svc.c               |    5 ++---
 net/sunrpc/svcsock.c           |   16 ++++++++++++++--
 3 files changed, 22 insertions(+), 5 deletions(-)

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

Index: linux/include/linux/sunrpc/svcsock.h
===================================================================
@@ -37,6 +37,12 @@ struct svc_sock_ops {
 	 * Return 1 if sufficient space to write reply to network.
 	 */
 	int			(*sko_has_wspace)(struct svc_sock *);
+	/*
+	 * Return the largest payload (i.e. READ, WRITE or READDIR
+	 * data length not including NFS headers) supported by the
+	 * svc_sock.
+	 */
+	u32			(*sko_max_payload)(struct svc_sock *);
 };
 
 /*

net/sunrpc/svc.c +2 -3

Index: linux/net/sunrpc/svc.c
===================================================================
@@ -1020,10 +1020,9 @@ err_bad:
  */
 u32 svc_max_payload(const struct svc_rqst *rqstp)
 {
-	int max = RPCSVC_MAXPAYLOAD_TCP;
+	struct svc_sock *svsk = rqstp->rq_sock;
+	int max = svsk->sk_ops->sko_max_payload(svsk);
 
-	if (rqstp->rq_sock->sk_sock->type == SOCK_DGRAM)
-		max = RPCSVC_MAXPAYLOAD_UDP;
 	if (rqstp->rq_server->sv_max_payload < max)
 		max = rqstp->rq_server->sv_max_payload;
 	return max;

net/sunrpc/svcsock.c +14 -2

Index: linux/net/sunrpc/svcsock.c
===================================================================
@@ -910,6 +910,11 @@ svc_udp_has_wspace(struct svc_sock *svsk
 	return svc_sock_has_write_space(svsk, sock_wspace(svsk->sk_sk));
 }
 
+static u32 svc_udp_max_payload(struct svc_sock *svsk)
+{
+	return RPCSVC_MAXPAYLOAD_UDP;
+}
+
 static const struct svc_sock_ops svc_udp_ops = {
 	.sko_name = "udp",
 	.sko_recvfrom = svc_udp_recvfrom,

@@ -917,7 +922,8 @@ static const struct svc_sock_ops svc_udp
 	.sko_detach = svc_tcpip_detach,
 	.sko_free = svc_tcpip_free,
 	.sko_prepare_reply = svc_tcpip_prepare_reply,
-	.sko_has_wspace = svc_udp_has_wspace
+	.sko_has_wspace = svc_udp_has_wspace,
+	.sko_max_payload = svc_udp_max_payload
 };
 
 static void

@@ -1378,6 +1384,11 @@ svc_tcp_has_wspace(struct svc_sock *svsk
 	return svc_sock_has_write_space(svsk, sk_stream_wspace(svsk->sk_sk));
 }
 
+static u32 svc_tcp_max_payload(struct svc_sock *svsk)
+{
+	return RPCSVC_MAXPAYLOAD_TCP;
+}
+
 static const struct svc_sock_ops svc_tcp_ops = {
 	.sko_name = "tcp",
 	.sko_recvfrom = svc_tcp_recvfrom,

@@ -1385,7 +1396,8 @@ static const struct svc_sock_ops svc_tcp
 	.sko_detach = svc_tcpip_detach,
 	.sko_free = svc_tcpip_free,
 	.sko_prepare_reply = svc_tcp_prepare_reply,
-	.sko_has_wspace = svc_tcp_has_wspace
+	.sko_has_wspace = svc_tcp_has_wspace,
+	.sko_max_payload = svc_tcp_max_payload
 };
 
 static void

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:34 UTC #1 lore raw

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

Make svc_max_payload() delegate to a new method in svc_sock_ops instead of reaching into the socket (beause later the NFS/RDMA transport will not even have a socket).
Signed-off-by Greg Banks <[email protected]>
Signed-off-by Peter Leckie <[email protected]>

net/sunrpc/svcsock.c svc_tcp_has_wspace(struct svc_sock *svsk · line 1391

Quoted 6 lines

net/sunrpc/svcsock.c +5 -0

+static u32 svc_tcp_max_payload(struct svc_sock *svsk)
+{
+	return RPCSVC_MAXPAYLOAD_TCP;
+}
+

Seeing these are implementation (or protocol) defined constants, do we really need a function call? How about a

	int max_payload;

in struct svc_sock_ops?? Or is it tacky to put an integer in a *_ops structure?

-------------------------------------------------------------------------
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

Tom Tucker

2007-05-17 15:23 UTC #2 lore raw

Quoted 18 lines · Neil Brown

Quoted 7 lines · On Thursday May 17, [email protected] wrote:
Make svc_max_payload() delegate to a new method in svc_sock_ops instead of reaching into the socket (beause later the NFS/RDMA transport will not even have a socket).
Signed-off-by Greg Banks <[email protected]>
Signed-off-by Peter Leckie <[email protected]>
..
net/sunrpc/svcsock.c svc_tcp_has_wspace(struct svc_sock *svsk · line 1391
Quoted 6 lines
net/sunrpc/svcsock.c +5 -0
+static u32 svc_tcp_max_payload(struct svc_sock *svsk)
+{
+	return RPCSVC_MAXPAYLOAD_TCP;
+}
+
Seeing these are implementation (or protocol) defined constants, do we really need a function call? How about a

Quoted 3 lines

	int max_payload;
in struct svc_sock_ops?? Or is it tacky to put an integer in a *_ops structure?

I think if we called it a "svc_transport" structure instead of a "svc_sock_ops" structure, it removes the tackiness. It's also more accurate since "svc_sock_ops" implies these are socket ops which they are not.

Greg Banks

author

2007-05-22 07:16 UTC #3 lore raw

Quoted 23 lines · Tom Tucker

Quoted 18 lines · Neil Brown
Quoted 7 lines · On Thursday May 17, [email protected] wrote:
Make svc_max_payload() delegate to a new method in svc_sock_ops instead of reaching into the socket (beause later the NFS/RDMA transport will not even have a socket).
Signed-off-by Greg Banks <[email protected]>
Signed-off-by Peter Leckie <[email protected]>
..
net/sunrpc/svcsock.c svc_tcp_has_wspace(struct svc_sock *svsk · line 1391
Quoted 6 lines
net/sunrpc/svcsock.c +5 -0
+static u32 svc_tcp_max_payload(struct svc_sock *svsk)
+{
+	return RPCSVC_MAXPAYLOAD_TCP;
+}
+
Seeing these are implementation (or protocol) defined constants, do we really need a function call? How about a
Quoted 3 lines
	int max_payload;
in struct svc_sock_ops?? Or is it tacky to put an integer in a *_ops structure?

Quoted 4 lines

I think if we called it a "svc_transport" structure instead of a "svc_sock_ops" structure, it removes the tackiness. It's also more accurate since "svc_sock_ops" implies these are socket ops which they are not.

I like this idea, so I renamed

svc_sock_ops -> svc_transport_type
sko_foo -> st_foo
sk_ops -> sk_type

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-18 04:56 UTC #4 lore raw

In reply to Neil Brown

Quoted 18 lines · Neil Brown

Quoted 7 lines · On Thursday May 17, [email protected] wrote:
Make svc_max_payload() delegate to a new method in svc_sock_ops instead of reaching into the socket (beause later the NFS/RDMA transport will not even have a socket).
Signed-off-by Greg Banks <[email protected]>
Signed-off-by Peter Leckie <[email protected]>
..
net/sunrpc/svcsock.c svc_tcp_has_wspace(struct svc_sock *svsk · line 1391
Quoted 6 lines
net/sunrpc/svcsock.c +5 -0
+static u32 svc_tcp_max_payload(struct svc_sock *svsk)
+{
+	return RPCSVC_MAXPAYLOAD_TCP;
+}
+
Seeing these are implementation (or protocol) defined constants, do we really need a function call? How about a

Quoted 2 lines

	int max_payload;
in struct svc_sock_ops??

These ones are indeed constants, as is RDMA's.

Quoted 2 lines

Or is it tacky to put an integer in a *_ops
structure?

No tackier than putting a string name like I did, or the things done in tcp_congestion_ops or sock_request_ops. It comes down to whether you want to allow future protocols to have variable limits, and right now that seems unnecesssary.

I'll change the field to an int.

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