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? What is the advantage of changing it?
The label names "bad" and "out" are confusing. Both are bad, but
bad also does an unlock.
bluhm
> 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 17:14:57 -0000
> @@ -245,8 +245,9 @@ nfs_connect(struct nfsmount *nmp, struct
> error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
> nmp->nm_soproto);
> if (error)
> - goto bad;
> + goto out;
> so = nmp->nm_so;
> + s = solock(so);
> nmp->nm_soflags = so->so_proto->pr_flags;
>
> /*
> @@ -262,9 +263,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 +274,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 +283,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 +298,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 +313,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 +360,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 +374,8 @@ nfs_connect(struct nfsmount *nmp, struct
> return (0);
>
> bad:
> + sounlock(s);
> +out:
> nfs_disconnect(nmp);
> return (error);
> }
