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

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.