Date: Sat, 17 May 2003 00:40:02 -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: <200305170740.h4H7e2M7059876@gw.catspoiler.org> In-Reply-To: <20030517160738.U15076@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 17 May, Bruce Evans wrote: > On Fri, 16 May 2003, Don Lewis wrote: > >> On 16 May, Bruce Evans wrote: >> > Why not just lock the vnode in fifo_close()? RELENG[2-4] seems to have >> > the same bug. I cannot be fixed there using the vnode interlock. >> >> That is probably the proper fix for RELENG4 where finer-grained locking >> isn't needed because of Giant. I'd still probably move the resource >> deallocation to fifo_inactive(). > > RELENG_4 actually can probably be fixed using the vnode interlock. The > interlock exists there. It just may require more care because it is a > spinlock. > > 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(). > Hmm, I see that at least ufs_close() likes to use only the vnode interlock, > but this seems to be wrong. From ufs_vnops.c:vop_close(): > > % VI_LOCK(vp); > % if (vp->v_usecount > 1) > % ufs_itimes(vp); > % VI_UNLOCK(vp); > % return (VOCALL(fifo_vnodeop_p, VOFFSET(vop_close), ap)); > > The interlock protects the v_usecount check here, but AFAIK it doesn't > protect ufs_itimes(). ufs_itimes() hacks on the inode's flags and > timestamps without acquiring any other locks. I believe it expects to > be locked by the vnode lock. This behaviour goes back to Lite1 or before > (not much except the names have changed since rev.1.1). I also stumbled across some other suspicious-looking timestamp manipulation code the other day. The comments in <sys/vnode.h> indicate that v_usecount should be protected by the interlock, and that seems to be fairly consistently done throughout the tree.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200305170740.h4H7e2M7059876>