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

On 3/5/2014 1:50 PM, Konstantin Belousov wrote:
> On Wed, Mar 05, 2014 at 02:21:04PM -0500, John Baldwin wrote:
>> 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.
> 
> The object->un_pager.swp.swp_tmpfs is cleared under the object lock
> before the vnode is reclaimed, i.e. long before the vnode can be freed.
> swp_tmpfs should be kept in sync with the OBJ_TMPFS flag, so the
> vhold() is safe while flag is set and object is locked.
> 

This has been stable. Not sure if it fixes the original problem, but I
have not seen any panics since using it and doing many poudriere builds.

-- 
Regards,
Bryan Drewery