On Fri Jul 06 2007 at 13:53:41 -0700, Bill Stouder-Studenmund wrote:
> vclean() locks the vnode with LK_DRAIN, then either unlocks the node 
> directly or lets VOP_INACTIVE() do it. After that point, calling the lock 
> manager on the lock triggers the panic we're seeing.
> 
> The difficulty is that upper layers don't know when the lower layer's been 
> vclean()ed, so they keep calling the lock manager directly. Boom!

Yes.

> So there are a number of things we can do.
> 
> 1) Stop exporting the lock, and force recursion across the board. This 
> will have performance impacts, and botch the interlock.
> 
> 2) Stop exporting the lock, force recursion, but recurse the interlock. 
> Lock the lowervp interlock then release the upper one, then hand the lock 
> call off.

These also work and solve the file system problem within the file
system code.  Although I'd argue that recursing the interlock is *wrong*
since you release the original interlock before you lock the object.
So I'd say keep the interlock, recurse, and lock.  The problem, thought,
then is that locking methods assume vp->v_interlock is the interlock.

> 3) Add LK_REENABLE to the unlock of the lock in vclean(). I like this idea 
> the best, as it means the least change; a change pulled into netbsd-4 and 
> netbsd-3 will leave the kernel working mostly as-is. It is a feature of 
> the lock manager that lets an LK_DRAINed lock come back to life.
>
> The issue here is that LK_REENABLE only works when it is passed during the 
> unlock that releases the LK_DRAIN. If the vnode is in use, the very case 
> we care about, this unlock will happen in the call to VOP_INACTIVE(). 
> So...
>
> 3a) We change all the VOP_INACTIVE routines to add LK_REENABLE to their 
> unlock calls. This change will have no impact for anything other than 
> LK_DRAIN, as it's the only thing that looks at this flag. It however means 
> changing EVERY inactive routine to handle something that happens rarely.
>
> 3b) Call lockmgr on the lock (or more accurately VOP_LOCK w/ flags) to do 
> the same thing as LK_REENABLE. Just passing it in would be fine, the 
> problem is we don't have a "do nothing" lock operation. So we'd also need 
> to add one.
> 
> 3c) Have vclean() grab a recursive vnode lock, then LK_RELEASE |
> LK_REENABLE it. The problem is that anything other than LK_RELEASE as the
> lock operation will trigger a panic.
> 
> So what do folks think of 3b?

With what you're suggesting, sounds easier that we simply don't drain the
lock at all.  Reference counting of vnodes, VXLOCK, taking an exclusive
lock in vclean() etc. should take care of the effects of LK_DRAIN?

I still don't like it, though, but I think it's the best of the bad
options we have.

-- 
Antti Kantee <pooka@iki.fi>                     Of course he runs NetBSD
http://www.iki.fi/pooka/                          http://www.NetBSD.org/
    "la qualité la plus indispensable du cuisinier est l'exactitude"