Date: Sat, 17 May 2003 14:15:10 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: bde@zeta.org.au Cc: current@FreeBSD.org Subject: Re: CFR: fifo_open()/fifo_close() patch Message-ID: <200305172115.h4HLFAM7063786@gw.catspoiler.org> In-Reply-To: <20030517190409.Q15481@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
@@ -70,6 +70,7 @@
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 *);
@@ -97,6 +98,7 @@
{ &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 },
@@ -171,7 +173,7 @@
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);
@@ -205,13 +207,21 @@
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);
}
}
}
@@ -221,18 +231,25 @@
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
@@ -243,28 +260,37 @@
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);
}
@@ -525,9 +551,12 @@
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--;
@@ -539,15 +568,34 @@
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200305172115.h4HLFAM7063786>