Hi, Peter, On 2023/3/8 δΈε1:04, Peter Xu wrote: > On Tue, Mar 07, 2023 at 09:24:31PM +0800, Chuang Xu wrote: >>> Why do we need address_space_get_flatview_rcu()? I'm not sure whether you >> address_space_cahce_init() uses address_space_get_flatview() to acquire >> a ref-ed flatview. If we want to use address_space_to_flatview_rcu() and >> make the flatview ref-ed, maybe we need to add address_space_get_flatview_rcu()? >> Or if we use address_space_to_flatview_rcu() directly, then we'll get a >> unref-ed flatview, I'm not sure wheather it's safe in other cases.. At least >> I don't want the assertion to be triggered one day. > No we can't touch that, afaict. It was a real fix (447b0d0b9e) to a real > bug. > > What I meant is we make address_space_get_flatview() always use > address_space_to_flatview_rcu().
This time I clearly understand what you mean.π > > I think it's safe, if you see the current callers of it (after my patch 1 > fixed version applied): > > memory_region_sync_dirty_bitmap[2249] view = address_space_get_flatview(as); > memory_region_update_coalesced_range[2457] view = address_space_get_flatview(as); > memory_region_clear_dirty_bitmap[2285] view = address_space_get_flatview(as); > mtree_info_flatview[3418] view = address_space_get_flatview(as); > address_space_cache_init[3350] cache->fv = address_space_get_flatview(as); > > Case 5 is what we're targeting for which I think it's fine. Logically any > commit() hook should just use address_space_get_flatview_raw() to reference > the flatview, but at least address_space_cache_init() is also called in > non-BQL sections, so _rcu is the only thing we can use here, iiuc.. > > Case 1-3 is never called during vm load, so I think it's safe. > > Case 4 can be accessing stalled flatview but I assume fine since it's just > for debugging. E.g. if someone tries "info mtree -f" during vm loading > after your patch applied, then one can see stalled info. I don't think it > can even happen because so far "info mtree" should also take BQL.. so it'll > be blocked until vm load completes. > > The whole thing is still tricky. I didn't yet make my mind through on how IIUC, Do you mean that different ways to get flatview are tricky? > to make it very clean, I think it's slightly beyond what this series does > and I need some more thinkings (or suggestions from others). > As you said, it's slightly beyond what this series does. Maybe it would be better if we discuss it in a new series and keep this series at v6? what's your take? Thanks!
