On Mon, Jul 04, 2022 at 12:49:24PM +0200, Martin Pieuchot wrote:
> >
> > a possible construct could be:
> >
> > for (;;) {
> > LIST_FOREACH(uvn, &uvn_wlist, u_wlist) {
> > vp = uvn->u_vnode;
> > if (mp && vp->v_mount != mp)
> > continue;
> >
> > if (rw_enter(uvn->u_obj.vmobjlock, RW_WRITE|RW_NOSLEEP)
> > != 0)
> > /* ignore locked uvn for now */
> > continue;
> >
> > /* ... */
> > }
> >
> > /* no more uvn in uvn_wlist */
> > if (LIST_EMPTY(&uvn_wlist))
> > break;
> >
> > /* there are still uvn, let's others make progress */
> > sched_yield();
> > }
> >
> >
> > the loop will process all unlocked uvn, sleep only at end of the loop, and
> > restart the uvn_wlist processing.
>
> This might have drawbacks. We could endup synching vnodes forever.
if new vnodes are added in continious way in the list, yes.
alternatively we could just walk the list once, ignoring locked vnodes. but I
am
unsure if we are still respecting the sync(2) semantic as we are returning
while
still having dirty buffers.
other way is to have a counter to ensure to walk the list at most N times.
> I'd like to start reverting the chunk that causes a deadlock then see how we
> can fix it properly. Do you agree?
reverting is a good first step, even if we are going back in the previous
situation where uo_refs could be modified without the lock. but it is what we
were doing before.
ok semarie@
> 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 4 Jul 2022 10:47:27 -0000
> @@ -1472,11 +1472,6 @@ uvm_vnp_sync(struct mount *mp)
> if (mp && vp->v_mount != mp)
> continue;
>
> - /* Spin to ensure `uvn_wlist' isn't modified concurrently. */
> - while (rw_enter(uvn->u_obj.vmobjlock, RW_WRITE|RW_NOSLEEP)) {
> - CPU_BUSY_CYCLE();
> - }
> -
> /*
> * If the vnode is "blocked" it means it must be dying, which
> * in turn means its in the process of being flushed out so
> @@ -1485,10 +1480,8 @@ uvm_vnp_sync(struct mount *mp)
> * note that uvn must already be valid because we found it on
> * the wlist (this also means it can't be ALOCK'd).
> */
> - if ((uvn->u_flags & UVM_VNODE_BLOCKED) != 0) {
> - rw_exit(uvn->u_obj.vmobjlock);
> + if ((uvn->u_flags & UVM_VNODE_BLOCKED) != 0)
> continue;
> - }
>
> /*
> * gain reference. watch out for persisting uvns (need to
> @@ -1497,7 +1490,6 @@ uvm_vnp_sync(struct mount *mp)
> if (uvn->u_obj.uo_refs == 0)
> vref(vp);
> uvn->u_obj.uo_refs++;
> - rw_exit(uvn->u_obj.vmobjlock);
>
> SIMPLEQ_INSERT_HEAD(&uvn_sync_q, uvn, u_syncq);
> }
--
Sebastien Marie
