From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: nfs shared socket lock
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Sun, 16 Feb 2025 13:11:12 +0300

> On 16 Feb 2025, at 01:10, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> 
> 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.
> 

ok mvs

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