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

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?

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).

Bruce