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 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? 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. 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. > > > 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. ;-) > > 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(). Yes, I missed it. I got wrapped up in what the other change was doing. :-) Take care, Bill
Attachment:
pgp49JnUDimlk.pgp
Description: PGP signature