On  9 May, Robert Watson wrote:
> 
> On Sun, 9 May 2004, Don Lewis wrote:
> 
>> >> So the upshot is that we really can't acquire any other mutexes while
>> >> holding the interlock.  I'm not clear on what precisely the vnode
>> >> interlock protects against here -- I assume against simultenous opens on
>> >> the vnode, but is there another strategy we could use to protect against
>> >> this?  Could we just use a single global sleep mutex to serialize all fifo
>> >> opens to avoid introducing this lock order problem?
>> > 
>> > The interlock is being used to protect against simultaneous opens and
>> > closes, the manipulation of # of readers and writers, and to prevent
>> > wakeups from being lost.  It may be possible to move the socket
>> > manipulation out from under the interlock and just rely on the vnode
>> > lock.
>> > 
>> > I'll think about this some more after my caffeine level has been brought
>> > up to snuff.
>> 
>> I went the wrong way on caffeine but here are my thoughts anyway ... 
>> 
>> I used the vnode interlock just because it was handy.  I think the path
>> of least resistance is to add a mutex to struct fifoinfo.  The code
>> bails out if things aren't as expected after msleep() returns, so I
>> think using a global mutex would cause spurious failures. 
>> 
>> The vnode lock has to be dropped for the msleep() calls, so we have to
>> rely on a mutex. The interlock gets dropped wherever LK_INTERLOCK is
>> specified. 
> 
> Is there any chance I could convince you to hack up a prototype of what
> you've described, test it a little, and drop me a copy?  (Or just merge
> it, so I can't pick it up from CVS) :-).  Right now I'm chasing down races
> in the socket listen/accept/close code, and my comfort level with the fifo
> code is a little low right now.

After a mere three panics I arrived at the patch below.  It seems to be
passing my fifo tests so far.  Bonus points if we can now drop the
unlock and lock calls around so?wakeup() calls.


Index: sys/fs/fifofs/fifo_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/fifofs/fifo_vnops.c,v
retrieving revision 1.93
diff -u -r1.93 fifo_vnops.c
--- sys/fs/fifofs/fifo_vnops.c	7 Apr 2004 20:45:59 -0000	1.93
+++ sys/fs/fifofs/fifo_vnops.c	9 May 2004 21:08:39 -0000
@@ -60,6 +60,7 @@
 	struct socket	*fi_writesock;
 	long		fi_readers;
 	long		fi_writers;
+	struct mtx	fi_mtx;
 };
 
 static int	fifo_print(struct vop_print_args *);
@@ -162,6 +163,7 @@
 		vp->v_fifoinfo = NULL;
 		(void)soclose(fip->fi_readsock);
 		(void)soclose(fip->fi_writesock);
+		mtx_destroy(&fip->fi_mtx);
 		FREE(fip, M_VNODE);
 	}
 }
@@ -188,7 +190,8 @@
 	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_ZERO | M_WAITOK);
 		error = socreate(AF_LOCAL, &rso, SOCK_STREAM, 0, cred, td);
 		if (error)
 			goto fail1;
@@ -207,6 +210,7 @@
 			return (error);
 		}
 		fip->fi_readers = fip->fi_writers = 0;
+		mtx_init(&fip->fi_mtx, "fifo mutex", NULL, MTX_DEF);
 		wso->so_snd.sb_lowat = PIPE_BUF;
 		rso->so_state |= SS_CANTRCVMORE;
 		vp->v_fifoinfo = fip;
@@ -217,28 +221,27 @@
 	 * the vnode lock.
 	 *
 	 * Protect the increment of fi_readers and fi_writers and the
-	 * associated calls to wakeup() with the vnode interlock in
-	 * addition to the vnode lock.  This allows the vnode lock to
-	 * be dropped for the msleep() calls below, and using the vnode
-	 * interlock as the msleep() mutex prevents the wakeup from
-	 * being missed.
+	 * associated calls to wakeup() with the fifo mutex in addition
+	 * to the vnode lock.  This allows the vnode lock to be dropped
+	 * for the msleep() calls below, and using the fifo mutex with
+	 * msleep() prevents the wakeup from being missed.
 	 */
-	VI_LOCK(vp);
+	mtx_lock(&fip->fi_mtx);
 	if (ap->a_mode & FREAD) {
 		fip->fi_readers++;
 		if (fip->fi_readers == 1) {
 			fip->fi_writesock->so_state &= ~SS_CANTSENDMORE;
 			if (fip->fi_writers > 0) {
 				wakeup(&fip->fi_writers);
-				VI_UNLOCK(vp);
+				mtx_unlock(&fip->fi_mtx);
 				sowwakeup(fip->fi_writesock);
-				VI_LOCK(vp);
+				mtx_lock(&fip->fi_mtx);
 			}
 		}
 	}
 	if (ap->a_mode & FWRITE) {
 		if ((ap->a_mode & O_NONBLOCK) && fip->fi_readers == 0) {
-			VI_UNLOCK(vp);
+			mtx_unlock(&fip->fi_mtx);
 			return (ENXIO);
 		}
 		fip->fi_writers++;
@@ -246,18 +249,18 @@
 			fip->fi_readsock->so_state &= ~SS_CANTRCVMORE;
 			if (fip->fi_readers > 0) {
 				wakeup(&fip->fi_readers);
-				VI_UNLOCK(vp);
+				mtx_unlock(&fip->fi_mtx);
 				sorwakeup(fip->fi_writesock);
-				VI_LOCK(vp);
+				mtx_lock(&fip->fi_mtx);
 			}
 		}
 	}
 	if ((ap->a_mode & O_NONBLOCK) == 0) {
 		if ((ap->a_mode & FREAD) && fip->fi_writers == 0) {
 			VOP_UNLOCK(vp, 0, td);
-			error = msleep(&fip->fi_readers, VI_MTX(vp),
-			    PCATCH | PSOCK, "fifoor", 0);
-			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY | LK_INTERLOCK, td);
+			error = msleep(&fip->fi_readers, &fip->fi_mtx,
+			    PDROP | PCATCH | PSOCK, "fifoor", 0);
+			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
 			if (error) {
 				fip->fi_readers--;
 				if (fip->fi_readers == 0)
@@ -265,7 +268,7 @@
 				fifo_cleanup(vp);
 				return (error);
 			}
-			VI_LOCK(vp);
+			mtx_lock(&fip->fi_mtx);
 			/*
 			 * We must have got woken up because we had a writer.
 			 * That (and not still having one) is the condition
@@ -274,10 +277,9 @@
 		}
 		if ((ap->a_mode & FWRITE) && fip->fi_readers == 0) {
 			VOP_UNLOCK(vp, 0, td);
-			error = msleep(&fip->fi_writers, VI_MTX(vp),
-			    PCATCH | PSOCK, "fifoow", 0);
-			vn_lock(vp,
-			    LK_EXCLUSIVE | LK_RETRY | LK_INTERLOCK, td);
+			error = msleep(&fip->fi_writers, &fip->fi_mtx,
+			    PDROP | PCATCH | PSOCK, "fifoow", 0);
+			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
 			if (error) {
 				fip->fi_writers--;
 				if (fip->fi_writers == 0)
@@ -293,7 +295,7 @@
 			return (0);
 		}
 	}
-	VI_UNLOCK(vp);
+	mtx_unlock(&fip->fi_mtx);
 	return (0);
 }