On 11/08/17(Fri) 20:31, Alexander Bluhm wrote:
> On Fri, Aug 11, 2017 at 01:18:48PM -0400, Martin Pieuchot wrote:
> > Diff below merge all solock()/sounlock() dances inside nfs_connect().
>
> Now we sleep for memory while holding the lock. Is this a good
> idea?
As long as a subsystem outside the NET_LOCK() can release memory,
sleeping with the lock held is fine.
> What is the advantage of changing it?
Not introducing multiple sleeping points in a function manipulating a
socket.
> The label names "bad" and "out" are confusing. Both are bad, but
> bad also does an unlock.
I kept 'bad' which now does the unlock and get rid of the 'out' label,
ok?
Index: nfs/nfs_socket.c
===================================================================
RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.122
diff -u -p -r1.122 nfs_socket.c
--- nfs/nfs_socket.c 10 Aug 2017 19:20:43 -0000 1.122
+++ nfs/nfs_socket.c 11 Aug 2017 19:31:41 -0000
@@ -244,9 +244,13 @@ nfs_connect(struct nfsmount *nmp, struct
saddr = mtod(nmp->nm_nam, struct sockaddr *);
error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
nmp->nm_soproto);
- if (error)
- goto bad;
+ if (error) {
+ nfs_disconnect(nmp);
+ return (error);
+ }
+
so = nmp->nm_so;
+ s = solock(so);
nmp->nm_soflags = so->so_proto->pr_flags;
/*
@@ -262,9 +266,7 @@ nfs_connect(struct nfsmount *nmp, struct
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_LOW;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
if (error)
goto bad;
@@ -275,9 +277,7 @@ nfs_connect(struct nfsmount *nmp, struct
sin->sin_family = AF_INET;
sin->sin_addr.s_addr = INADDR_ANY;
sin->sin_port = htons(0);
- s = solock(so);
error = sobind(so, m, &proc0);
- sounlock(s);
m_freem(m);
if (error)
goto bad;
@@ -286,9 +286,7 @@ nfs_connect(struct nfsmount *nmp, struct
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_DEFAULT;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
if (error)
goto bad;
}
@@ -303,12 +301,9 @@ nfs_connect(struct nfsmount *nmp, struct
goto bad;
}
} else {
- s = solock(so);
error = soconnect(so, nmp->nm_nam);
- if (error) {
- sounlock(s);
+ if (error)
goto bad;
- }
/*
* Wait for the connection to complete. Cribbed from the
@@ -321,23 +316,19 @@ nfs_connect(struct nfsmount *nmp, struct
so->so_error == 0 && rep &&
(error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
so->so_state &= ~SS_ISCONNECTING;
- sounlock(s);
goto bad;
}
}
if (so->so_error) {
error = so->so_error;
so->so_error = 0;
- sounlock(s);
goto bad;
}
- sounlock(s);
}
/*
* Always set receive timeout to detect server crash and reconnect.
* Otherwise, we can get stuck in soreceive forever.
*/
- s = solock(so);
so->so_rcv.sb_timeo = (5 * hz);
if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
so->so_snd.sb_timeo = (5 * hz);
@@ -372,11 +363,11 @@ nfs_connect(struct nfsmount *nmp, struct
sizeof (u_int32_t)) * 2;
}
error = soreserve(so, sndreserve, rcvreserve);
- sounlock(s);
if (error)
goto bad;
so->so_rcv.sb_flags |= SB_NOINTR;
so->so_snd.sb_flags |= SB_NOINTR;
+ sounlock(s);
/* Initialize other non-zero congestion variables */
nfs_init_rtt(nmp);
@@ -386,6 +377,7 @@ nfs_connect(struct nfsmount *nmp, struct
return (0);
bad:
+ sounlock(s);
nfs_disconnect(nmp);
return (error);
}
