> Date: Wed, 23 Nov 2022 17:33:26 +0100
> From: Martin Pieuchot <m...@openbsd.org>
>
> On 23/11/22(Wed) 16:34, Mark Kettenis wrote:
> > > Date: Wed, 23 Nov 2022 10:52:32 +0100
> > > From: Martin Pieuchot <m...@openbsd.org>
> > >
> > > On 22/11/22(Tue) 23:40, Mark Kettenis wrote:
> > > > > Date: Tue, 22 Nov 2022 17:47:44 +0000
> > > > > From: Miod Vallat <m...@online.fr>
> > > > >
> > > > > > Here is a diff. Maybe bluhm@ can try this on the macppc machine
> > > > > > that
> > > > > > triggered the original "vref used where vget required" problem?
> > > > >
> > > > > On a similar machine it panics after a few hours with:
> > > > >
> > > > > panic: uvn_flush: PGO_SYNCIO return 'try again' error (impossible)
> > > > >
> > > > > The trace (transcribed by hand) is
> > > > > uvn_flush+0x820
> > > > > uvm_vnp_terminate+0x79
> > > > > vclean+0xdc
> > > > > vgonel+0x70
> > > > > getnewvnode+0x240
> > > > > ffs_vget+0xcc
> > > > > ffs_inode_alloc+0x13c
> > > > > ufs_makeinode+0x94
> > > > > ufs_create+0x58
> > > > > VOP_CREATE+0x48
> > > > > vn_open+0x188
> > > > > doopenat+0x1b4
> > > >
> > > > Ah right, there is another path where we end up with a refcount of
> > > > zero. Should be fixable, but I need to think about this for a bit.
> > >
> > > Not sure to understand what you mean with refcount of 0. Could you
> > > elaborate?
> >
> > Sorry, I was thinking ahead a bit. I'm pretty much convinced that the
> > issue we're dealing with is a race between a vnode being
> > recycled/cleaned and the pagedaemon paging out pages associated with
> > that same vnode.
> >
> > The crashes we've seen before were all in the pagedaemon path where we
> > end up calling into the VFS layer with a vnode that has v_usecount ==
> > 0. My "fix" avoids that, but hits the issue that when we are in the
> > codepath that is recycling/cleaning the vnode, we can't use vget() to
> > get a reference to the vnode since it checks that the vnode isn't in
> > the process of being cleaned.
> >
> > But if we avoid that issue (by for example) skipping the vget() call
> > if the UVM_VNODE_DYING flag is set, we run into the same scenario
> > where we call into the VFS layer with v_usecount == 0. Now that may
> > not actually be a problem, but I need to investigate this a bit more.
>
> When the UVM_VNODE_DYING flag is set the caller always own a valid
> reference to the vnode. Either because it is in the process of cleaning
> it via uvm_vnp_terminate() or because it uvn_detach() has been called
> which means the reference to the vnode hasn't been dropped yet. So I
> believe `v_usecount' for such vnode is positive.
I don't think so. The vnode that can be recycled is sitting on the
freelist with v_usecount == 0. When getnewvnode() decides to recycle
a vnode it takes it off the freelist and calls vgonel(), which i turn
calls vclean(), which only increases v_usecount if it is non-zero. So
at the point where uvn_vnp_terminate() gets called v_usecount
definitely is 0.
That said, the vnode is no longer on the freelist at that point and
since UVM_VNODE_DYING is set, uvn_vnp_uncache() will return
immediately without calling vref() to get another reference. So that
is fine.
> > Or maybe calling into the VFS layer with a vnode that has v_usecount
> > == 0 is perfectly fine and we should do the vget() dance I propose in
> > uvm_vnp_unache() instead of in uvn_put().
>
> I'm not following. uvm_vnp_uncache() is always called with a valid
> vnode, no?
Not sure what you mean with a "valid vnode"; uvm_vnp_uncache() checks
the UVM_VNODE_VALID flag at the start, which suggests that it can be
called in cases where that flag is not set. But it will unlock and
return immediately in that case, so it should be safe.
Anyway, I think I have convinced myself that in the case where the
pagedaemon ends up calling uvn_put() for a persisting vnode vm object,
we do need to get a refence before calling uvn_io(). Otherwise we run
the risk of the vnode being recycled under our feet while we're doing
I/O.
So here is an updated diff that checks the UVM_VNODE_DYING flag and
skips the refcount manipulation if it is set.
What do you think?
Index: uvm/uvm_vnode.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
retrieving revision 1.130
diff -u -p -r1.130 uvm_vnode.c
--- uvm/uvm_vnode.c 20 Oct 2022 13:31:52 -0000 1.130
+++ uvm/uvm_vnode.c 28 Nov 2022 14:01:20 -0000
@@ -899,11 +899,21 @@ uvn_cluster(struct uvm_object *uobj, vof
int
uvn_put(struct uvm_object *uobj, struct vm_page **pps, int npages, int flags)
{
- int retval;
+ struct uvm_vnode *uvn = (struct uvm_vnode *)uobj;
+ int dying, retval;
KASSERT(rw_write_held(uobj->vmobjlock));
+ dying = (uvn->u_flags & UVM_VNODE_DYING);
+ if (!dying) {
+ if (vget(uvn->u_vnode, LK_NOWAIT))
+ return VM_PAGER_AGAIN;
+ }
+
retval = uvn_io((struct uvm_vnode*)uobj, pps, npages, flags, UIO_WRITE);
+
+ if (!dying)
+ vrele(uvn->u_vnode);
return retval;
}
