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);
>
>
