On Tue, Dec 27, 2005 at 03:37:58PM -0800, Bill Studenmund wrote:
> On Tue, Dec 27, 2005 at 02:49:24PM -0800, Chuck Silvers wrote:
> > On Sat, Dec 24, 2005 at 03:28:06PM +0900, YAMAMOTO Takashi wrote:
> > > > One thing I noted in this change is that vget() will now call 
> > > > VOP_INACTIVE() on vnodes that were never activated. Will this cause issues 
> > > > with other file systems?
> > > 
> > > what kind of issues?
> > > afaik, vn_lock fails only when the vnode is being reclaimed.
> > > in that case, calling vrele doesn't hurt.
> > 
> > I'm also suspicious of this change.  this change doesn't seem related
> > to the purpose given in the check-in comment, is it?  if you really want
> > to reduce the amount of duplicate code, why not just split it into
> > multiple functions?
> 
> A separate checkin might have been good. However at this point, a "cvs 
> admin -m" will probably be enough to fix things up. I agree we should note 
> this change, but I don't think it's so bad to include it.

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?


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


> The only thing I find missing is that there isn't any code which USES the 
> new flag. Did a checkin get missed?

the use is in vget().

-Chuck