On Fri, Sep 09, 2022 at 02:41:30PM +0200, Martin Pieuchot wrote:
> On 09/09/22(Fri) 12:25, Theo Buehler wrote:
> > > Yesterday gnezdo@ fixed a race in uvn_attach() that lead to the same
> > > assert. Here's an rebased diff for the bug discussed in this thread,
> > > could you try again and let us know? Thanks!
> >
> > This seems to be stable now. It's been running for nearly 5 days.
> > Without gnezdo's fix it would blow up within at most 2 days.
>
> Thanks! I'm looking for oks then.
ok semarie@
just one question: UVM_VNODE_CANPERSIST is still defined in sys/uvm/uvm_vnode.h
should it be removed ? or at least defined to 0 ? or maybe it is too early for
now.
> Index: uvm/uvm_vnode.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
> retrieving revision 1.127
> diff -u -p -r1.127 uvm_vnode.c
> --- uvm/uvm_vnode.c 31 Aug 2022 09:07:35 -0000 1.127
> +++ uvm/uvm_vnode.c 1 Sep 2022 12:54:27 -0000
> @@ -163,11 +163,8 @@ uvn_attach(struct vnode *vp, vm_prot_t a
> */
> rw_enter(uvn->u_obj.vmobjlock, RW_WRITE);
> if (uvn->u_flags & UVM_VNODE_VALID) { /* already active? */
> + KASSERT(uvn->u_obj.uo_refs > 0);
>
> - /* 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);
>
> @@ -235,7 +232,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;
>
> @@ -248,7 +245,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)
> @@ -321,16 +318,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;
>
> @@ -380,7 +367,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. */
> @@ -496,8 +482,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.
> */
> @@ -505,14 +491,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");
> @@ -1349,46 +1335,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;
> }
>
> @@ -1483,11 +1437,9 @@ uvm_vnp_sync(struct mount *mp)
> continue;
>
> /*
> - * 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++;
>
> SIMPLEQ_INSERT_HEAD(&uvn_sync_q, uvn, u_syncq);
--
Sebastien Marie
