--0rSojgWGcpz+ezC3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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=
ant
> > > to reduce the amount of duplicate code, why not just split it into
> > > multiple functions?
> >=20
> > A separate checkin might have been good. However at this point, a "cvs=
=20
> > admin -m" will probably be enough to fix things up. I agree we should n=
ote=20
> > this change, but I don't think it's so bad to include it.
>=20
> if the purpose is to reduce the amount of duplicate code, why not just sp=
lit
> 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=
=20
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=
=20
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=
=20
the vget() call happens after the new user has already vrele()'d the node,=
=20
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=
n(),
> > > etc, a bit, but the result would be much nicer.
> >=20
> > The difference I see is who gets to set it. VXLOCK is set by vfs_subr.c=
;=20
> > it is owned by the FS-independent code. VFREEING, however, is set by th=
e=20
> > FS-specific code.=20
>=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=
ntity.
> 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=
=20
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=
he=20
> > new flag. Did a checkin get missed?
>=20
> the use is in vget().

Yes, I missed it. I got wrapped up in what the other change was doing. :-)

Take care,

Bill

--0rSojgWGcpz+ezC3
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (NetBSD)

iD8DBQFDsehZWz+3JHUci9cRAiwxAKCXW1aeO2/qbkei7CZ7e0+ZsAL3CwCdE+dC
ayocHdTBRFRBdHu3+e5mMJw=
=Y0Va
-----END PGP SIGNATURE-----

--0rSojgWGcpz+ezC3--