On 02/06/22(Thu) 13:54, Sebastien Marie wrote: > On Tue, May 24, 2022 at 02:16:44PM +0200, 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. > > I don't know if you are looking for ok or not regarding this diff.
I'd appreciate if it could be stress tested before that. > For me, it makes sense: it simplifies uvm_vnode code, and seems to fix the > vref > problem. > > Just some notes in case you want to commit it: > > - UVM_VNODE_CANPERSIST define should be removed from uvm/uvm_vnode.h too Indeed. > - uvm_vnp_uncache(9) man page (src/share/man/man9/uvn_attach.9) should be > amended: it mentions "uvm_vnp_uncache() function disables vnode vp from > persisting" > > - I wonder if UVM_VNODE_VALID could be removed too (it could be separated > commit): once UVM_VNODE_CANPERSIST disappear, UVM_VNODE_VALID flag should > be > equivalent to have uo_refs>0 if I properly understood the code > > - more simplification might be possible inside uvm_vnp_terminate() or > uvm_vnp_uncache(): they have both code path for uo_refs ==0 and >0 (and > without UVM_VNODE_CANPERSIST, a uvn should always have refs or be "free"). > uvm_vnp_uncache() might be deletable. I agree we should then simplify/cleanup all this logic. I'd like first to be sure there is no regression.
