Hello Neil,
I realized the first sock_sendmsg in svc_sendto() is meaningless
in case of TCP as the connection is already established.
I fixed it so that sock_sendmsg is called only if it's UDP.
Thank you,
Hirokazu Takahashi.
--- net/sunrpc/svcsock.c.ORG4 Sun Nov 3 08:37:04 2030
+++ net/sunrpc/svcsock.c Sun Nov 3 08:38:34 2030
@@ -351,11 +351,13 @@ svc_sendto(struct svc_rqst *rqstp, struc
down(&svsk->sk_sem);
/* set the destination */
- oldfs = get_fs(); set_fs(KERNEL_DS);
- len = sock_sendmsg(sock, &msg, 0);
- set_fs(oldfs);
- if (len < 0)
- goto out;
+ if (rqstp->rq_prot == IPPROTO_UDP) {
+ oldfs = get_fs(); set_fs(KERNEL_DS);
+ len = sock_sendmsg(sock, &msg, 0);
+ set_fs(oldfs);
+ if (len < 0)
+ goto out;
+ }
/* send head */
if (slen == xdr->head[0].iov_len)
-------------------------------------------------------
This sf.net email is sponsored by: To learn the basics of securing
your web site with SSL, click here to get a FREE TRIAL of a Thawte
Server Certificate: http://www.gothawte.com/rd524.html
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Friday November 15, [email protected] wrote:
> Hello Neil,
>
> I realized the first sock_sendmsg in svc_sendto() is meaningless
> in case of TCP as the connection is already established.
> I fixed it so that sock_sendmsg is called only if it's UDP.
>
Fair enough. I'm not sure that the optimisation is likely to be
noticable, but it is probably worth it.
You can make the whole 'msg' structure local to the "if udp" bit, and
do away with set_fs too as there is no data.
Thanks,
NeilBrown
--------------------------------------------------
Status: compiles
Only set dest addr in NFS/udp reply, not NFS/tcp.
We don't need to send an empty message to
set up remote address when sending tcp reply, so
we don't. Also, as the data is empty, we don't
need to set_fs.
----------- Diffstat output ------------
net/sunrpc/svcsock.c | 30 ++++++++++++++----------------
1 files changed, 14 insertions(+), 16 deletions(-)
diff net/sunrpc/svcsock.c~current~ net/sunrpc/svcsock.c
--- net/sunrpc/svcsock.c~current~ 2002-11-18 10:40:09.000000000 +1100
+++ net/sunrpc/svcsock.c 2002-11-18 10:58:16.000000000 +1100
@@ -324,10 +324,8 @@ svc_wake_up(struct svc_serv *serv)
static int
svc_sendto(struct svc_rqst *rqstp, struct xdr_buf *xdr)
{
- mm_segment_t oldfs;
struct svc_sock *svsk = rqstp->rq_sock;
struct socket *sock = svsk->sk_sock;
- struct msghdr msg;
int slen;
int len = 0;
int result;
@@ -339,23 +337,23 @@ svc_sendto(struct svc_rqst *rqstp, struc
slen = xdr->len;
- msg.msg_name = &rqstp->rq_addr;
- msg.msg_namelen = sizeof(rqstp->rq_addr);
- msg.msg_iov = NULL;
- msg.msg_iovlen = 0;
- msg.msg_control = NULL;
- msg.msg_controllen = 0;
- msg.msg_flags = MSG_MORE;
-
/* Grab svsk->sk_sem to serialize outgoing data. */
down(&svsk->sk_sem);
- /* set the destination */
- oldfs = get_fs(); set_fs(KERNEL_DS);
- len = sock_sendmsg(sock, &msg, 0);
- set_fs(oldfs);
- if (len < 0)
- goto out;
+ if (rqstp->rq_prot == IPPROTO_UDP) {
+ /* set the destination */
+ struct msghdr msg;
+ msg.msg_name = &rqstp->rq_addr;
+ msg.msg_namelen = sizeof(rqstp->rq_addr);
+ msg.msg_iov = NULL;
+ msg.msg_iovlen = 0;
+ msg.msg_control = NULL;
+ msg.msg_controllen = 0;
+ msg.msg_flags = MSG_MORE;
+
+ if (sock_sendmsg(sock, &msg, 0) < 0)
+ goto out;
+ }
/* send head */
if (slen == xdr->head[0].iov_len)
-------------------------------------------------------
This sf.net email is sponsored by: To learn the basics of securing
your web site with SSL, click here to get a FREE TRIAL of a Thawte
Server Certificate: http://www.gothawte.com/rd524.html
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Hello,
> > I realized the first sock_sendmsg in svc_sendto() is meaningless
> > in case of TCP as the connection is already established.
> > I fixed it so that sock_sendmsg is called only if it's UDP.
> >
>
> Fair enough. I'm not sure that the optimisation is likely to be
> noticable, but it is probably worth it.
> You can make the whole 'msg' structure local to the "if udp" bit, and
> do away with set_fs too as there is no data.
Good point!
I didn't realize about set_fs.
Thank you.
-------------------------------------------------------
This sf.net email is sponsored by: To learn the basics of securing
your web site with SSL, click here to get a FREE TRIAL of a Thawte
Server Certificate: http://www.gothawte.com/rd524.html
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Hello Neil,
I found another problem and fixed it.
My box has crashed in heavily loaded condition.
I realized there were races during initializing sunrpc sockets.
We should keep sockets SK_BUSY during its initalization or
the data_ready callbacks may access uninitalized data and
may cause Oops. The callbacks can't handle a packet correctly
just after registering them.
Thank you,
Hirokazu Takahashi.
--- linux/net/sunrpc/svcsock.c.INITRACE Mon Nov 18 18:50:03 2030
+++ linux/net/sunrpc/svcsock.c Mon Nov 18 19:27:43 2030
@@ -765,6 +765,7 @@ svc_tcp_accept(struct svc_sock *svsk)
/* Precharge. Data may have arrived on the socket before we
* installed the data_ready callback.
*/
+ clear_bit(SK_BUSY, &newsvsk->sk_flags);
set_bit(SK_DATA, &newsvsk->sk_flags);
svc_sock_enqueue(newsvsk);
@@ -1238,6 +1239,11 @@ svc_setup_socket(struct svc_serv *serv,
}
memset(svsk, 0, sizeof(*svsk));
+ /* prevent races against the data_ready callbacks
+ * during initialization.
+ */
+ svsk->sk_flags = SK_BUSY;
+
inet = sock->sk;
inet->user_data = svsk;
svsk->sk_sock = sock;
@@ -1325,8 +1331,18 @@ svc_create_socket(struct svc_serv *serv,
goto bummer;
}
- if ((svsk = svc_setup_socket(serv, sock, &error, 1)) != NULL)
+ if ((svsk = svc_setup_socket(serv, sock, &error, 1)) != NULL) {
+ /*
+ * data may have already arrived
+ */
+ clear_bit(SK_BUSY, &svsk->sk_flags);
+ if (protocol == IPPROTO_TCP)
+ set_bit(SK_CONN, &svsk->sk_flags);
+ else
+ set_bit(SK_DATA, &svsk->sk_flags);
+ svc_sock_enqueue(svsk);
return 0;
+ }
bummer:
dprintk("svc: svc_create_socket error = %d\n", -error);
-------------------------------------------------------
This sf.net email is sponsored by: To learn the basics of securing
your web site with SSL, click here to get a FREE TRIAL of a Thawte
Server Certificate: http://www.gothawte.com/rd524.html
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Monday November 18, [email protected] wrote:
> Hello Neil,
>
> I found another problem and fixed it.
>
> My box has crashed in heavily loaded condition.
> I realized there were races during initializing sunrpc sockets.
>
> We should keep sockets SK_BUSY during its initalization or
> the data_ready callbacks may access uninitalized data and
> may cause Oops. The callbacks can't handle a packet correctly
> just after registering them.
>
Well caught...
There is however a bug in your patch :-(
> + svsk->sk_flags = SK_BUSY;
This should be
set_bit(SK_BUSY, &svsk->sk_flags);
I had a look at the code and did some more tidying up and
re-organisation.
This is what I came up with.
NeilBrown
-------------------------------------------
Status: compiles
Reorganise sock init in rpcsvc to avoid races.
If one of the callback (e.g. data ready) is called
before socket setup is complete, oops may occur.
With this patch, socket is kept SK_BUSY until all is
ready to avoid this.
Also some code is moved around to make it cleaner.
----------- Diffstat output ------------
./net/sunrpc/svcsock.c | 55 ++++++++++++++++++++++---------------------------
1 files changed, 25 insertions(+), 30 deletions(-)
diff ./net/sunrpc/svcsock.c~current~ ./net/sunrpc/svcsock.c
--- ./net/sunrpc/svcsock.c~current~ 2002-11-18 17:08:19.000000000 +1100
+++ ./net/sunrpc/svcsock.c 2002-11-19 15:19:23.000000000 +1100
@@ -627,7 +627,7 @@ svc_udp_sendto(struct svc_rqst *rqstp)
return error;
}
-static int
+static void
svc_udp_init(struct svc_sock *svsk)
{
svsk->sk_sk->data_ready = svc_udp_data_ready;
@@ -643,9 +643,8 @@ svc_udp_init(struct svc_sock *svsk)
3 * svsk->sk_server->sv_bufsz,
3 * svsk->sk_server->sv_bufsz);
+ set_bit(SK_DATA, &svsk->sk_flags); /* might have come in before data_ready set up */
set_bit(SK_CHNGBUF, &svsk->sk_flags);
-
- return 0;
}
/*
@@ -773,19 +772,14 @@ svc_tcp_accept(struct svc_sock *svsk)
dprintk("%s: connect from %u.%u.%u.%u:%04x\n", serv->sv_name,
NIPQUAD(sin.sin_addr.s_addr), ntohs(sin.sin_port));
- if (!(newsvsk = svc_setup_socket(serv, newsock, &err, 0)))
- goto failed;
-
/* make sure that a write doesn't block forever when
* low on memory
*/
newsock->sk->sndtimeo = HZ*30;
- /* Precharge. Data may have arrived on the socket before we
- * installed the data_ready callback.
- */
- set_bit(SK_DATA, &newsvsk->sk_flags);
- svc_sock_enqueue(newsvsk);
+ if (!(newsvsk = svc_setup_socket(serv, newsock, &err, 0)))
+ goto failed;
+
/* make sure that we don't have too many active connections.
* If we have, something must be dropped.
@@ -1006,7 +1000,7 @@ svc_tcp_sendto(struct svc_rqst *rqstp)
return sent;
}
-static int
+static void
svc_tcp_init(struct svc_sock *svsk)
{
struct sock *sk = svsk->sk_sk;
@@ -1017,6 +1011,7 @@ svc_tcp_init(struct svc_sock *svsk)
if (sk->state == TCP_LISTEN) {
dprintk("setting up TCP socket for listening\n");
sk->data_ready = svc_tcp_listen_data_ready;
+ set_bit(SK_CONN, &svsk->sk_flags);
} else {
dprintk("setting up TCP socket for reading\n");
sk->state_change = svc_tcp_state_change;
@@ -1035,9 +1030,8 @@ svc_tcp_init(struct svc_sock *svsk)
3 * svsk->sk_server->sv_bufsz);
set_bit(SK_CHNGBUF, &svsk->sk_flags);
+ set_bit(SK_DATA, &svsk->sk_flags);
}
-
- return 0;
}
void
@@ -1258,6 +1252,18 @@ svc_setup_socket(struct svc_serv *serv,
memset(svsk, 0, sizeof(*svsk));
inet = sock->sk;
+
+ /* Register socket with portmapper */
+ if (*errp >= 0 && pmap_register)
+ *errp = svc_register(serv, inet->protocol,
+ ntohs(inet_sk(inet)->sport));
+
+ if (*errp < 0) {
+ kfree(svsk);
+ return NULL;
+ }
+
+ set_bit(SK_BUSY, &svsk->sk_flags);
inet->user_data = svsk;
svsk->sk_sock = sock;
svsk->sk_sk = inet;
@@ -1271,23 +1277,9 @@ svc_setup_socket(struct svc_serv *serv,
/* Initialize the socket */
if (sock->type == SOCK_DGRAM)
- *errp = svc_udp_init(svsk);
+ svc_udp_init(svsk);
else
- *errp = svc_tcp_init(svsk);
-if (svsk->sk_sk == NULL)
- printk(KERN_WARNING "svsk->sk_sk == NULL after svc_prot_init!\n");
-
- /* Register socket with portmapper */
- if (*errp >= 0 && pmap_register)
- *errp = svc_register(serv, inet->protocol,
- ntohs(inet_sk(inet)->sport));
-
- if (*errp < 0) {
- inet->user_data = NULL;
- kfree(svsk);
- return NULL;
- }
-
+ svc_tcp_init(svsk);
spin_lock_bh(&serv->sv_lock);
if (!pmap_register) {
@@ -1302,6 +1294,9 @@ if (svsk->sk_sk == NULL)
dprintk("svc: svc_setup_socket created %p (inet %p)\n",
svsk, svsk->sk_sk);
+
+ clear_bit(SK_BUSY, &svsk->sk_flags);
+ svc_sock_enqueue(svsk);
return svsk;
}
-------------------------------------------------------
This sf.net email is sponsored by: To learn the basics of securing
your web site with SSL, click here to get a FREE TRIAL of a Thawte
Server Certificate: http://www.gothawte.com/rd524.html
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Hello,
> > I found another problem and fixed it.
> >
> > My box has crashed in heavily loaded condition.
> > I realized there were races during initializing sunrpc sockets.
> Well caught...
> There is however a bug in your patch :-(
>
> > + svsk->sk_flags = SK_BUSY;
> This should be
> set_bit(SK_BUSY, &svsk->sk_flags);
Thanks for pointing my failure.
> I had a look at the code and did some more tidying up and
> re-organisation.
> This is what I came up with.
Your code looks much better.
Thank you,
Hirokazu Takahashi.
-------------------------------------------------------
This sf.net email is sponsored by: To learn the basics of securing
your web site with SSL, click here to get a FREE TRIAL of a Thawte
Server Certificate: http://www.gothawte.com/rd524.html
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs