On 24/05/22(Tue) 14:16, Martin Pieuchot wrote: > On 19/05/22(Thu) 13:33, Alexander Bluhm wrote: > > On Tue, May 17, 2022 at 05:43:02PM +0200, Martin Pieuchot wrote: > > > Andrew, Alexander, could you test this and report back? > > > > Panic "vref used where vget required" is still there. As usual it > > needs a day to reproduce. This time I was running without the vref > > history diff. > > Thanks for testing. Apparently calling uvm_vnp_terminate() in > getnewvnode() isn't good enough. I suppose that in your case below the > pdaemon is trying to flush the pages before the vnode has been recycled, > so before uvm_vnp_terminate() has been called. > > So either we prevent the vnode from being put on the free list or we get > rid of the persisting mechanism. I don't fully understand what could be > the impact of always flushing the pages and why this hasn't been done in > the first place. It seems the CANPERSIST logic has been"inherited from > 44BSD's original vm. > > On the other hand I feel even less comfortable with preventing vnodes > from being recycled until the pdaemon has freed related pages. > > Diff below has been lightly tested, it get rids of the extra CANPERSIST > referenced. That means pages associated to a vnode will now always be > flushed when uvn_detach() is called. This turns uvm_vnp_uncache() into > a noop and open the possibility of further simplifications. > > I'd be happy to hear if this has any impact of the bug we're chasing. > Please do note that this change can introduce/expose other issues.
Any of you got the chance to try this diff? Could you reproduce the panic with it? > Index: uvm/uvm_vnode.c > =================================================================== > RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v > retrieving revision 1.124 > diff -u -p -r1.124 uvm_vnode.c > --- uvm/uvm_vnode.c 3 May 2022 21:20:35 -0000 1.124 > +++ uvm/uvm_vnode.c 20 May 2022 09:04:08 -0000 > @@ -162,12 +162,9 @@ uvn_attach(struct vnode *vp, vm_prot_t a > * add it to the writeable list, and then return. > */ > if (uvn->u_flags & UVM_VNODE_VALID) { /* already active? */ > + KASSERT(uvn->u_obj.uo_refs > 0); > > rw_enter(uvn->u_obj.vmobjlock, RW_WRITE); > - /* regain vref if we were persisting */ > - if (uvn->u_obj.uo_refs == 0) { > - vref(vp); > - } > uvn->u_obj.uo_refs++; /* bump uvn ref! */ > rw_exit(uvn->u_obj.vmobjlock); > > @@ -234,7 +231,7 @@ uvn_attach(struct vnode *vp, vm_prot_t a > KASSERT(uvn->u_obj.uo_refs == 0); > uvn->u_obj.uo_refs++; > oldflags = uvn->u_flags; > - uvn->u_flags = UVM_VNODE_VALID|UVM_VNODE_CANPERSIST; > + uvn->u_flags = UVM_VNODE_VALID; > uvn->u_nio = 0; > uvn->u_size = used_vnode_size; > > @@ -247,7 +244,7 @@ uvn_attach(struct vnode *vp, vm_prot_t a > /* > * add a reference to the vnode. this reference will stay as long > * as there is a valid mapping of the vnode. dropped when the > - * reference count goes to zero [and we either free or persist]. > + * reference count goes to zero. > */ > vref(vp); > if (oldflags & UVM_VNODE_WANTED) > @@ -320,16 +317,6 @@ uvn_detach(struct uvm_object *uobj) > */ > vp->v_flag &= ~VTEXT; > > - /* > - * we just dropped the last reference to the uvn. see if we can > - * let it "stick around". > - */ > - if (uvn->u_flags & UVM_VNODE_CANPERSIST) { > - /* won't block */ > - uvn_flush(uobj, 0, 0, PGO_DEACTIVATE|PGO_ALLPAGES); > - goto out; > - } > - > /* its a goner! */ > uvn->u_flags |= UVM_VNODE_DYING; > > @@ -379,7 +366,6 @@ uvn_detach(struct uvm_object *uobj) > /* wake up any sleepers */ > if (oldflags & UVM_VNODE_WANTED) > wakeup(uvn); > -out: > rw_exit(uobj->vmobjlock); > > /* drop our reference to the vnode. */ > @@ -495,8 +481,8 @@ uvm_vnp_terminate(struct vnode *vp) > } > > /* > - * done. now we free the uvn if its reference count is zero > - * (true if we are zapping a persisting uvn). however, if we are > + * done. now we free the uvn if its reference count is zero. > + * however, if we are > * terminating a uvn with active mappings we let it live ... future > * calls down to the vnode layer will fail. > */ > @@ -504,14 +490,14 @@ uvm_vnp_terminate(struct vnode *vp) > if (uvn->u_obj.uo_refs) { > /* > * uvn must live on it is dead-vnode state until all references > - * are gone. restore flags. clear CANPERSIST state. > + * are gone. restore flags. > */ > uvn->u_flags &= ~(UVM_VNODE_DYING|UVM_VNODE_VNISLOCKED| > - UVM_VNODE_WANTED|UVM_VNODE_CANPERSIST); > + UVM_VNODE_WANTED); > } else { > /* > * free the uvn now. note that the vref reference is already > - * gone [it is dropped when we enter the persist state]. > + * gone. > */ > if (uvn->u_flags & UVM_VNODE_IOSYNCWANTED) > panic("uvm_vnp_terminate: io sync wanted bit set"); > @@ -1350,46 +1336,14 @@ uvm_vnp_uncache(struct vnode *vp) > } > > /* > - * we have a valid, non-blocked uvn. clear persist flag. > + * we have a valid, non-blocked uvn. > * if uvn is currently active we can return now. > */ > - uvn->u_flags &= ~UVM_VNODE_CANPERSIST; > if (uvn->u_obj.uo_refs) { > rw_exit(uobj->vmobjlock); > return FALSE; > } > > - /* > - * uvn is currently persisting! we have to gain a reference to > - * it so that we can call uvn_detach to kill the uvn. > - */ > - vref(vp); /* seems ok, even with VOP_LOCK */ > - uvn->u_obj.uo_refs++; /* value is now 1 */ > - rw_exit(uobj->vmobjlock); > - > -#ifdef VFSLCKDEBUG > - /* > - * carry over sanity check from old vnode pager: the vnode should > - * be VOP_LOCK'd, and we confirm it here. > - */ > - if ((vp->v_flag & VLOCKSWORK) && !VOP_ISLOCKED(vp)) > - panic("uvm_vnp_uncache: vnode not locked!"); > -#endif > - > - /* > - * now drop our reference to the vnode. if we have the sole > - * reference to the vnode then this will cause it to die [as we > - * just cleared the persist flag]. we have to unlock the vnode > - * while we are doing this as it may trigger I/O. > - * > - * XXX: it might be possible for uvn to get reclaimed while we are > - * unlocked causing us to return TRUE when we should not. we ignore > - * this as a false-positive return value doesn't hurt us. > - */ > - VOP_UNLOCK(vp); > - uvn_detach(&uvn->u_obj); > - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); > - > return TRUE; > } > > @@ -1491,11 +1445,9 @@ uvm_vnp_sync(struct mount *mp) > } > > /* > - * gain reference. watch out for persisting uvns (need to > - * regain vnode REF). > + * gain reference. > */ > - if (uvn->u_obj.uo_refs == 0) > - vref(vp); > + KASSERT(uvn->u_obj.uo_refs > 0); > uvn->u_obj.uo_refs++; > rw_exit(uvn->u_obj.vmobjlock); > >