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. 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 - 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. Thanks. -- Sebastien Marie
