Re: panic: lockmgr still held [tmpfs] [vm_map_remove()->vdropl()] (r262186: Thu Feb 20)

On Wednesday, March 05, 2014 6:07:23 am Konstantin Belousov wrote:
> On Wed, Mar 05, 2014 at 11:41:24AM +0200, Andriy Gapon wrote:
> > on 04/03/2014 18:45 John Baldwin said the following:
> > > So I'm not sure how to fix this.  The crash is in this code in 
> > > vm_object_deallocate():
> > > 
> > > 			if (object->type == OBJT_SWAP &&
> > > 			    (object->flags & OBJ_TMPFS) != 0) {
> > > 				vp = object->un_pager.swp.swp_tmpfs;
> > > 				vhold(vp);
> > > 				VM_OBJECT_WUNLOCK(object);
> > > 				vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> > > 				vdrop(vp);
> > > 				VM_OBJECT_WLOCK(object);
> > > 				if (object->type == OBJT_DEAD ||
> > > 				    object->ref_count != 1) {
> > > 					VM_OBJECT_WUNLOCK(object);
> > > 					VOP_UNLOCK(vp, 0);
> > > 					return;
> > > 				}
> > > 				if ((object->flags & OBJ_TMPFS) != 0)
> > > 					VOP_UNSET_TEXT(vp);
> > > 				VOP_UNLOCK(vp, 0);
> > > 			}
> > > 
> > > The vdrop() is dropping the count to zero and trying to free the vnode.  The 
> > > real problem I think is that swp_tmpfs doesn't have an implicit vhold() on the 
> > > vnode, so in this case, the code is doing a vhold/vn_lock/vdrop of an already-
> > > free vnode.  For OBJT_VNODE objects, the reference from the object back to the 
> > > vnode holds a vref() that gets released by a vput() in 
> > > vm_object_vndeallocate().
> > > 
> > > One fix might be to chagne smp_tmpfs to hold a vhold reference.  This is 
> > > untested but might work (but I'm also not sure that this is the right thing in 
> > > that I don't know what other effects it might have).
> > 
> > I agree with your analysis, but I don't think that a filesystem holding its own
> > vnode is a good idea.  If I am not mistaken, that would prevent tmpfs vnodes
> > from going to free list.
> > I'd rather try to modify vm_object_deallocate() code.  E.g. vdrop() could be
> > called after VOP_UNLOCK().  Alternatively, the code could handle a doomed vnode
> > in a different way.
> 
> I agree with Andrey, it is just a bug to vdrop() before unlock.
> Please try this.

Ok, my only worry is in the case of Bryan's panic, the hold count on the vnode
was already zero before vhold() was called, so is it possible that it is a stale
pointer or is there some other implicit reference that prevents that?  If it can't
be stale, I think deferring the vdrop() is fine.

-- 
John Baldwin