Add fi_sx, an sx lock to serialize I/O operations on the socket pair

underlying the POSIX fifo implementation.  In 6.x/7.x, fifo access is
moved from the VFS layer, where it was serialized using the vnode
lock, to the file descriptor layer, where access is protected by a
reference count but not serialized.  This exposed socket buffer
locking to high levels of parallelism in specific fifo workloads, such
as make -j 32, which expose as yet unresolved socket buffer bugs.

fi_sx re-adds serialization about the read and write routines,
although not paths that simply test socket buffer mbuf queue state,
such as the poll and kqueue methods.  This restores the extra locking
cost previously present in some cases, but is an effective workaround
for the instability that has been experienced.  This workaround should
be removed once the bug in socket buffer handling has been fixed.

Reported by:	kris, jhb, Julien Gabel <jpeg at thilelli dot net>,
		Peter Holm <peter at holm dot cc>, others
MFC after:	3 days

1 files changed, 16 insertions, 3 deletions

diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c
index 3c4cdcd..1942802 100644
--- a/sys/fs/fifofs/fifo_vnops.c
+++ b/sys/fs/fifofs/fifo_vnops.c
@@ -74,14 +74,20 @@ struct fileops fifo_ops_f = {
 };
 
 /*
- * This structure is associated with the FIFO vnode and stores
- * the state associated with the FIFO.
+ * This structure is associated with the FIFO vnode and stores the state
+ * associated with the FIFO.
+ *
+ * XXXRW: The presence of an sx lock here is undesirable, and exists to avoid
+ * exposing threading race conditions in the socket code that have not yet
+ * been resolved.  Once those problems are resolved, the sx lock here should
+ * be removed.
  */
 struct fifoinfo {
 	struct socket	*fi_readsock;
 	struct socket	*fi_writesock;
 	long		fi_readers;
 	long		fi_writers;
+	struct sx	fi_sx;
 };
 
 static vop_print_t	fifo_print;
@@ -152,6 +158,7 @@ fifo_cleanup(struct vnode *vp)
 		vp->v_fifoinfo = NULL;
 		(void)soclose(fip->fi_readsock);
 		(void)soclose(fip->fi_writesock);
+		sx_destroy(&fip->fi_sx);
 		FREE(fip, M_VNODE);
 	}
 }
@@ -179,7 +186,8 @@ fifo_open(ap)
 	int error;
 
 	if ((fip = vp->v_fifoinfo) == NULL) {
-		MALLOC(fip, struct fifoinfo *, sizeof(*fip), M_VNODE, M_WAITOK);
+		MALLOC(fip, struct fifoinfo *, sizeof(*fip), M_VNODE,
+		    M_WAITOK | M_ZERO);
 		error = socreate(AF_LOCAL, &rso, SOCK_STREAM, 0, cred, td);
 		if (error)
 			goto fail1;
@@ -198,6 +206,7 @@ fail1:
 			return (error);
 		}
 		fip->fi_readers = fip->fi_writers = 0;
+		sx_init(&fip->fi_sx, "fifo_sx");
 		wso->so_snd.sb_lowat = PIPE_BUF;
 		SOCKBUF_LOCK(&rso->so_rcv);
 		rso->so_rcv.sb_state |= SBS_CANTRCVMORE;
@@ -703,7 +712,9 @@ fifo_read_f(struct file *fp, struct uio *uio, struct ucred *cred, int flags, str
 	if (uio->uio_resid == 0)
 		return (0);
 	sflags = (fp->f_flag & FNONBLOCK) ? MSG_NBIO : 0;
+	sx_xlock(&fip->fi_sx);
 	error = soreceive(fip->fi_readsock, NULL, uio, NULL, NULL, &sflags);
+	sx_xunlock(&fip->fi_sx);
 	return (error);
 }
 
@@ -723,6 +734,8 @@ fifo_write_f(struct file *fp, struct uio *uio, struct ucred *cred, int flags, st
 	fip = fp->f_data;
 	KASSERT(uio->uio_rw == UIO_WRITE,("fifo_write mode"));
 	sflags = (fp->f_flag & FNONBLOCK) ? MSG_NBIO : 0;
+	sx_xlock(&fip->fi_sx);
 	error = sosend(fip->fi_writesock, NULL, uio, 0, NULL, sflags, td);
+	sx_xunlock(&fip->fi_sx);
 	return (error);
 }