Re: CFR: fifo_open()/fifo_close() patch

From: Don Lewis <truckman_at_FreeBSD.org>
Date: Sat, 17 May 2003 14:15:10 -0700 (PDT)
On 17 May, Bruce Evans wrote:
> On Sat, 17 May 2003, Don Lewis wrote:
> 
>> On 17 May, Bruce Evans wrote:
>> > My question is mainly: why do you want or need the extra complexity for
>> > using the vnode interlock here?
>>
>> I want to use the vnode interlock so that I can msleep() on it to avoid
>> a race condition.  If I rely on the vnode lock to protect fi_readers and
>> fi_writers, another thread could sneak in, update them, and call
>> wakeup() between the VOP_UNLOCK() call and the tsleep() call, causing
>> the thread calling tsleep() to miss the wakeup().
> 
> I see.
> 
> I now think fifo_close() needs both the vnode lock and the interlock.
> Its socantrcvmore() calls should be atomic with decrementing the
> reader/writer counts to 0.  I think locking them with the interlock
> would work, but this depends too much on their internals (not sleeping).
> Sorry, I deleted your original patch and don't remember exactly what it
> does here.

Now this is why I wanted my patch reviewed ...

I think you are correct.  The patch below protects fi_readers and
fi_writers with the vnode lock in all cases.  It also uses the interlock
to protect incrementing them and doing the wakeup(), along with calling
msleep() if they are initially zero.

The socant*more() and so*wakeup() calls appear to generally be safe in
terms of holding the interlock.  The exception is if the SS_ASYNC flag
is set on the socket and pgsigio() needs to be called, which needs to
grab a bunch of other locks.  I've restructured this patch to unlock the
interlock when making the above mentioned calls.

> NetBSD changed VOP_CLOSE() to "L L L" 4+ years ago.

By my count, that makes four of us who have reached that conclusion in
this thread.  Unfortunately, this is probably not the time to make an
API change.


Index: sys/fs/fifofs/fifo_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/fifofs/fifo_vnops.c,v
retrieving revision 1.85
diff -u -r1.85 fifo_vnops.c
--- sys/fs/fifofs/fifo_vnops.c	24 Mar 2003 11:03:42 -0000	1.85
+++ sys/fs/fifofs/fifo_vnops.c	17 May 2003 20:31:21 -0000
_at__at_ -70,6 +70,7 _at__at_
 static int	fifo_lookup(struct vop_lookup_args *);
 static int	fifo_open(struct vop_open_args *);
 static int	fifo_close(struct vop_close_args *);
+static int	fifo_inactive(struct vop_inactive_args *);
 static int	fifo_read(struct vop_read_args *);
 static int	fifo_write(struct vop_write_args *);
 static int	fifo_ioctl(struct vop_ioctl_args *);
_at__at_ -97,6 +98,7 _at__at_
 	{ &vop_create_desc,		(vop_t *) vop_panic },
 	{ &vop_getattr_desc,		(vop_t *) vop_ebadf },
 	{ &vop_getwritemount_desc, 	(vop_t *) vop_stdgetwritemount },
+	{ &vop_inactive_desc,		(vop_t *) fifo_inactive },
 	{ &vop_ioctl_desc,		(vop_t *) fifo_ioctl },
 	{ &vop_kqfilter_desc,		(vop_t *) fifo_kqfilter },
 	{ &vop_lease_desc,		(vop_t *) vop_null },
_at__at_ -171,7 +173,7 _at__at_
 	struct fifoinfo *fip;
 	struct thread *td = ap->a_td;
 	struct socket *rso, *wso;
-	int error;
+	int error = 0;
 
 	if ((fip = vp->v_fifoinfo) == NULL) {
 		MALLOC(fip, struct fifoinfo *, sizeof(*fip), M_VNODE, M_WAITOK);
_at__at_ -205,13 +207,21 _at__at_
 		wso->so_snd.sb_lowat = PIPE_BUF;
 		rso->so_state |= SS_CANTRCVMORE;
 	}
+
+	/*
+	 * Protect increment of fi_readers and fi_writers and associated
+	 * msleep()/wakeup() with vnode interlock.
+	 */
+	VI_LOCK(vp);
 	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);
 				sowwakeup(fip->fi_writesock);
+				VI_LOCK(vp);
 			}
 		}
 	}
_at__at_ -221,18 +231,25 _at__at_
 			fip->fi_readsock->so_state &= ~SS_CANTRCVMORE;
 			if (fip->fi_readers > 0) {
 				wakeup(&fip->fi_readers);
+				VI_UNLOCK(vp);
 				sorwakeup(fip->fi_writesock);
+				VI_LOCK(vp);
 			}
 		}
 	}
 	if ((ap->a_mode & FREAD) && (ap->a_mode & O_NONBLOCK) == 0) {
 		if (fip->fi_writers == 0) {
 			VOP_UNLOCK(vp, 0, td);
-			error = tsleep(&fip->fi_readers,
+			error = msleep(&fip->fi_readers, VI_MTX(vp),
 			    PCATCH | PSOCK, "fifoor", 0);
-			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
-			if (error)
-				goto bad;
+			vn_lock(vp, LK_EXCLUSIVE | LK_RETRY | LK_INTERLOCK, td);
+			if (error) {
+				fip->fi_readers--;
+				if (fip->fi_readers == 0)
+					socantsendmore(fip->fi_writesock);
+				goto unlocked;
+			}
+			VI_LOCK(vp);
 			/*
 			 * We must have got woken up because we had a writer.
 			 * That (and not still having one) is the condition
_at__at_ -243,28 +260,37 _at__at_
 	if (ap->a_mode & FWRITE) {
 		if (ap->a_mode & O_NONBLOCK) {
 			if (fip->fi_readers == 0) {
-				error = ENXIO;
-				goto bad;
+				VI_UNLOCK(vp);
+				fip->fi_writers--;
+				if (fip->fi_writers == 0) {
+					socantrcvmore(fip->fi_readsock);
+				}
+				goto unlocked;
 			}
 		} else {
 			if (fip->fi_readers == 0) {
 				VOP_UNLOCK(vp, 0, td);
-				error = tsleep(&fip->fi_writers,
+				error = msleep(&fip->fi_writers, VI_MTX(vp),
 				    PCATCH | PSOCK, "fifoow", 0);
-				vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
-				if (error)
-					goto bad;
+				vn_lock(vp,
+				    LK_EXCLUSIVE | LK_RETRY | LK_INTERLOCK, td);
+				if (error) {
+					fip->fi_writers--;
+					if (fip->fi_writers == 0)
+						socantrcvmore(fip->fi_readsock);
+					goto unlocked;
+				}
 				/*
 				 * We must have got woken up because we had
 				 * a reader.  That (and not still having one)
 				 * is the condition that we must wait for.
 				 */
+				goto unlocked;
 			}
 		}
 	}
-	return (0);
-bad:
-	VOP_CLOSE(vp, ap->a_mode, ap->a_cred, td);
+	VI_UNLOCK(vp);
+unlocked:
 	return (error);
 }
 
_at__at_ -525,9 +551,12 _at__at_
 		struct thread *a_td;
 	} */ *ap;
 {
-	register struct vnode *vp = ap->a_vp;
-	register struct fifoinfo *fip = vp->v_fifoinfo;
-	int error1, error2;
+	struct vnode *vp = ap->a_vp;
+	struct thread *td = ap->a_td;
+	struct fifoinfo *fip = vp->v_fifoinfo;
+
+	/* Protect fi_readers and fi_writers with vnode lock */
+	vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
 
 	if (ap->a_fflag & FREAD) {
 		fip->fi_readers--;
_at__at_ -539,15 +568,34 _at__at_
 		if (fip->fi_writers == 0)
 			socantrcvmore(fip->fi_readsock);
 	}
-	if (vrefcnt(vp) > 1)
-		return (0);
-	error1 = soclose(fip->fi_readsock);
-	error2 = soclose(fip->fi_writesock);
-	FREE(fip, M_VNODE);
-	vp->v_fifoinfo = NULL;
-	if (error1)
-		return (error1);
-	return (error2);
+	VOP_UNLOCK(vp, 0, td);
+	return (0);
+}
+
+/*
+ * Device inactive routine
+ */
+/* ARGSUSED */
+static int
+fifo_inactive(ap)
+	struct vop_inactive_args /* {
+		struct vnode *a_vp;
+		struct thread *a_td;
+	} */ *ap;
+{
+	struct vnode *vp = ap->a_vp;
+	struct fifoinfo *fip = vp->v_fifoinfo;
+
+	VI_LOCK(vp);
+	if (fip != NULL && vp->v_usecount == 0) {
+		vp->v_fifoinfo = NULL;
+		VI_UNLOCK(vp);
+		(void) soclose(fip->fi_readsock);
+		(void) soclose(fip->fi_writesock);
+		FREE(fip, M_VNODE);
+	}
+	VOP_UNLOCK(vp, 0, ap->a_td);
+	return (0);
 }
 
 
Received on Sat May 17 2003 - 12:15:19 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:08 UTC