Subject: Re: CVS commit: src/sys
To: Chuck Silvers <firstname.lastname@example.org>
From: Bill Studenmund <email@example.com>
Date: 12/27/2005 17:20:26
Content-Type: text/plain; charset=us-ascii
On Tue, Dec 27, 2005 at 03:54:15PM -0800, Chuck Silvers wrote:
> 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:
> > > 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 w=
> > > 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 n=
> > 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 sp=
> 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().=20
It took me a while to figure that out, but the only times one of these=20
vrele()s would call VOP_INACTIVE() is: 1) if the vnode were revoked, in=20
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=20
case, we _should_ call VOP_INACTIVE(), as the new user has let the vnode=20
go back onto the free list. In the former case, I think we will do the=20
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.
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=20
the vget() code not held a reference.
> > > also, do we really another flag that means the same thing as VXLOCK?
> > > why not just use VXLOCK? this would mean rearranging vgonel(), vclea=
> > > 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 th=
> > FS-specific code.=20
> 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 ide=
> 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=20
lot of code. Mainly as I personally like the recursion detection we have=20
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=20
Yamamoto-san do all that work. ;-)
> > The only thing I find missing is that there isn't any code which USES t=
> > new flag. Did a checkin get missed?
> the use is in vget().
Yes, I missed it. I got wrapped up in what the other change was doing. :-)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (NetBSD)
-----END PGP SIGNATURE-----