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