From 7028887eacc57bcb19aba085f6276ec97896e5c5 Mon Sep 17 00:00:00 2001 From: Robert Watson Date: Thu, 22 Sep 2005 10:51:12 +0000 Subject: [PATCH] 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 , Peter Holm , others MFC after: 3 days --- sys/fs/fifofs/fifo_vnops.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c index 3c4cdcdd182..1942802f2dc 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); }