From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: nfs shared socket lock
To: Vitaliy Makkoveev <otto@bsdbox.dev>
Cc: tech@openbsd.org
Date: Sat, 15 Feb 2025 23:10:38 +0100

On Sat, Feb 15, 2025 at 01:39:23AM +0300, Vitaliy Makkoveev wrote:
> > On 14 Feb 2025, at 22:02, Alexander Bluhm <bluhm@openbsd.org> wrote:
> > 
> > Hi,
> > 
> > The socket functions that are called by NFS code are MP safe.
> > Shared net lock together with socket lock is sufficient.
> > 
> > ok?
> > 
> 
> I pointed two hunks inline, where socket lock could be omitted.

I also saw this and think solock can go away for sb_flags.  But I
want to move in small steps, especially with NFS.  And shared net
lock is a different story than socket buffer mutex.

ok to commit this tested diff now?  Then you can remove needless
solock around sb mutex.

bluhm

> > Index: nfs/krpc_subr.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/krpc_subr.c,v
> > diff -u -p -r1.39 krpc_subr.c
> > --- nfs/krpc_subr.c	1 May 2024 13:15:59 -0000	1.39
> > +++ nfs/krpc_subr.c	13 Feb 2025 12:54:08 -0000
> > @@ -276,9 +276,9 @@ krpc_call(struct sockaddr_in *sa, u_int 
> > 	sin->sin_family = AF_INET;
> > 	sin->sin_addr.s_addr = INADDR_ANY;
> > 	sin->sin_port = htons(0);
> > -	solock(so);
> > +	solock_shared(so);
> > 	error = sobind(so, m, &proc0);
> > -	sounlock(so);
> > +	sounlock_shared(so);
> > 	m_freem(m);
> > 	if (error) {
> > 		printf("bind failed\n");
> > Index: nfs/nfs_socket.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_socket.c,v
> > diff -u -p -r1.155 nfs_socket.c
> > --- nfs/nfs_socket.c	30 Jan 2025 14:40:50 -0000	1.155
> > +++ nfs/nfs_socket.c	13 Feb 2025 12:48:34 -0000
> > @@ -281,9 +281,9 @@ nfs_connect(struct nfsmount *nmp, struct
> > 		sin->sin_family = AF_INET;
> > 		sin->sin_addr.s_addr = INADDR_ANY;
> > 		sin->sin_port = htons(0);
> > -		solock(so);
> > +		solock_shared(so);
> > 		error = sobind(so, nam, &proc0);
> > -		sounlock(so);
> > +		sounlock_shared(so);
> > 		if (error)
> > 			goto bad;
> > 
> > @@ -305,7 +305,7 @@ nfs_connect(struct nfsmount *nmp, struct
> > 			goto bad;
> > 		}
> > 	} else {
> > -		solock(so);
> > +		solock_shared(so);
> > 		error = soconnect(so, nmp->nm_nam);
> > 		if (error)
> > 			goto bad_locked;
> > @@ -330,7 +330,7 @@ nfs_connect(struct nfsmount *nmp, struct
> > 			so->so_error = 0;
> > 			goto bad_locked;
> > 		}
> > -		sounlock(so);
> > +		sounlock_shared(so);
> > 	}
> > 	/*
> > 	 * Always set receive timeout to detect server crash and reconnect.
> > @@ -367,7 +367,7 @@ nfs_connect(struct nfsmount *nmp, struct
> > 	} else {
> > 		panic("%s: nm_sotype %d", __func__, nmp->nm_sotype);
> > 	}
> > -	solock(so);
> > +	solock_shared(so);
> > 	error = soreserve(so, sndreserve, rcvreserve);
> > 	if (error)
> > 		goto bad_locked;
> > @@ -377,7 +377,7 @@ nfs_connect(struct nfsmount *nmp, struct
> > 	mtx_enter(&so->so_snd.sb_mtx);
> > 	so->so_snd.sb_flags |= SB_NOINTR;
> > 	mtx_leave(&so->so_snd.sb_mtx);
> > -	sounlock(so);
> > +	sounlock_shared(so);
> > 
> 
> The socket lock is not required for sb_flags modification. I left
> it to avoid SB_MTXLOCK dances, but now it could be completely
> removed.
> 
> You could follow the FIOASYNC case of soo_ioctl() and take both
> mutexes together if you want atomicy.
> 
> > 	m_freem(mopt);
> > 	m_freem(nam);
> > @@ -390,7 +390,7 @@ nfs_connect(struct nfsmount *nmp, struct
> > 	return (0);
> > 
> > bad_locked:
> > -	sounlock(so);
> > +	sounlock_shared(so);
> > bad:
> > 
> > 	m_freem(mopt);
> > Index: nfs/nfs_syscalls.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_syscalls.c,v
> > diff -u -p -r1.128 nfs_syscalls.c
> > --- nfs/nfs_syscalls.c	18 Sep 2024 05:21:19 -0000	1.128
> > +++ nfs/nfs_syscalls.c	13 Feb 2025 12:55:27 -0000
> > @@ -247,9 +247,9 @@ nfssvc_addsock(struct file *fp, struct m
> > 		siz = NFS_MAXPACKET + sizeof (u_long);
> > 	else
> > 		siz = NFS_MAXPACKET;
> > -	solock(so);
> > +	solock_shared(so);
> > 	error = soreserve(so, siz, siz); 
> > -	sounlock(so);
> > +	sounlock_shared(so);
> > 	if (error) {
> > 		m_freem(mynam);
> > 		return (error);
> > @@ -275,7 +275,7 @@ nfssvc_addsock(struct file *fp, struct m
> > 		sosetopt(so, IPPROTO_TCP, TCP_NODELAY, m);
> > 		m_freem(m);
> > 	}
> > -	solock(so);
> > +	solock_shared(so);
> > 	mtx_enter(&so->so_rcv.sb_mtx);
> > 	so->so_rcv.sb_flags &= ~SB_NOINTR;
> > 	so->so_rcv.sb_timeo_nsecs = INFSLP;
> > @@ -284,7 +284,7 @@ nfssvc_addsock(struct file *fp, struct m
> > 	so->so_snd.sb_flags &= ~SB_NOINTR;
> > 	so->so_snd.sb_timeo_nsecs = INFSLP;
> > 	mtx_leave(&so->so_snd.sb_mtx);
> > -	sounlock(so);
> > +	sounlock_shared(so);
> > 	if (tslp)
> > 		slp = tslp;
> > 	else {
> > 
> 
> The same as above, socket lock is not required here.