Re: CVS commit: src/sys

[Chuck Silvers <chuq%chuq.com@localhost>]

On Tue, Dec 27, 2005 at 05:20:26PM -0800, Bill Studenmund wrote:
> > if the purpose is to reduce the amount of duplicate code, why not just split
> > the function into multiple functions rather than changing the requirements
> > of VOP_INACTIVE() in subtle ways?
> 
> I'm sorry, I don't think it is changing the semantics of VOP_INACTIVE(). 
> It took me a while to figure that out, but the only times one of these 
> vrele()s would call VOP_INACTIVE() is: 1) if the vnode were revoked, in 
> which case we send the call is handed by deadfs. 2) if the vnode has been 
> reused and the reusing thread has already called vrele(). In the latter 
> case, we _should_ call VOP_INACTIVE(), as the new user has let the vnode 
> go back onto the free list. In the former case, I think we will do the 
> same call to deadfs we would do otherwise; right now if you VOP_REVOKE() a 
> vnode, there will be a vrele() call that will VOP_INACTIVE() on deadfs.

I don't think that case 2 can happen.  the vnode reuse is serialized with
vn_lock() by VXLOCK and v_interlock and v_usecount, so the vnode's identity
is stable while vn_lock() is sleeping for the lock (except for revoke /
forced-unmount, but that's case 1).


> So I think the semantics are the same.
> 
> Actually, the change closes a small window of error. In the reuse case, if 
> the vget() call happens after the new user has already vrele()'d the node, 
> the old code would skip a VOP_INACTIVE() call that would of happened had 
> the vget() code not held a reference.

since yamt said he's going to be rewriting all this soon anyway,
I'm not going to argue anymore.


> > > > also, do we really another flag that means the same thing as VXLOCK?
> > > > why not just use VXLOCK?  this would mean rearranging vgonel(), 
> > > > vclean(),
> > > > etc, a bit, but the result would be much nicer.
> > > 
> > > The difference I see is who gets to set it. VXLOCK is set by vfs_subr.c; 
> > > it is owned by the FS-independent code. VFREEING, however, is set by the 
> > > FS-specific code. 
> > 
> > my point is that the two flags have the same purpose: to prevent other
> > threads from gaining a reference to the vnode while it's changing its 
> > identity.
> > I see no need to have two flags for one purpose, there's no reason it
> > couldn't be set by both vfs_subr.c and the file system code.
> > vnode identity changes can be triggered from both contexts.
> 
> It could be done. However it'd be a lot of work, as we'd have to audit a 
> lot of code. Mainly as I personally like the recursion detection we have 
> in vclean() with VXLOCK().
> 
> We also then have to have all file systems _do_ the fs-specific stuff, so 
> the change gets very large. To be honest, I don't feel like making 
> Yamamoto-san do all that work. ;-)

if all file systems would have to use VXLOCK for freeing files,
why don't they all have to this other VFREEING flag?
they don't, it's optional.

anyway, this will apparently all be moot shortly.

-Chuck