Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

[Kevin Wolf]

Am 02.05.2022 um 10:02 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> Am 30/04/2022 um 07:17 schrieb Stefan Hajnoczi:
> > On Thu, Apr 28, 2022 at 11:56:09PM +0200, Emanuele Giuseppe Esposito wrote:
> >>
> >>
> >> Am 28/04/2022 um 12:45 schrieb Stefan Hajnoczi:
> >>> On Wed, Apr 27, 2022 at 08:55:35AM +0200, Emanuele Giuseppe Esposito 
> >>> wrote:
> >>>>
> >>>>
> >>>> Am 26/04/2022 um 10:51 schrieb Emanuele Giuseppe Esposito:
> >>>>> Luckly, most of the cases where we recursively go through a graph are
> >>>>> the BlockDriverState callback functions in block_int-common.h
> >>>>> In order to understand what to protect, I categorized the callbacks in
> >>>>> block_int-common.h depending on the type of function that calls them:
> >>>>>
> >>>>> 1) If the caller is a generated_co_wrapper, this function must be
> >>>>>    protected by rdlock. The reason is that generated_co_wrapper create
> >>>>>    coroutines that run in the given bs AioContext, so it doesn't matter
> >>>>>    if we are running in the main loop or not, the coroutine might run
> >>>>>    in an iothread.
> >>>>> 2) If the caller calls it directly, and has the GLOBAL_STATE_CODE() 
> >>>>> macro,
> >>>>>    then the function is safe. The main loop is the writer and thus won't
> >>>>>    read and write at the same time.
> >>>>> 3) If the caller calls it directly, but has not the GLOBAL_STATE_CODE()
> >>>>>    macro, then we need to check the callers and see case-by-case if the
> >>>>>    caller is in the main loop, if it needs to take the lock, or delegate
> >>>>>    this duty to its caller (to reduce the places where to take it).
> >>>>>
> >>>>> I used the vrc script (https://github.com/bonzini/vrc) to get help 
> >>>>> finding
> >>>>> all the callers of a callback. Using its filter function, I can
> >>>>> omit all functions protected by the added lock to avoid having 
> >>>>> duplicates
> >>>>> when querying for new callbacks.
> >>>>
> >>>> I was wondering, if a function is in category (3) and runs in an
> >>>> Iothread but the function itself is not (currently) recursive, meaning
> >>>> it doesn't really traverse the graph or calls someone that traverses it,
> >>>> should I add the rdlock anyways or not?
> >>>>
> >>>> Example: bdrv_co_drain_end
> >>>>
> >>>> Pros:
> >>>>    + Covers if in future a new recursive callback for a new/existing
> >>>>      BlockDriver is implemented.
> >>>>    + Covers also the case where I or someone missed the recursive part.
> >>>>
> >>>> Cons:
> >>>>    - Potentially introducing an unnecessary critical section.
> >>>>
> >>>> What do you think?
> >>>
> >>> ->bdrv_co_drain_end() is a callback function. Do you mean whether its
> >>> caller, bdrv_drain_invoke_entry(), should take the rdlock around
> >>> ->bdrv_co_drain_end()?
> >>
> >> Yes. The problem is that the coroutine is created in bs AioContext, so
> >> it might be in an iothread.
> >>
> >>>
> >>> Going up further in the call chain (and maybe switching threads),
> >>> bdrv_do_drained_end() has QLIST_FOREACH(child, &bs->children, next) so
> >>> it needs protection. If the caller of bdrv_do_drained_end() holds then
> >>> rdlock then I think none of the child functions (including
> >>> ->bdrv_co_drain_end()) need to take it explicitly.
> >>
> >> Regarding bdrv_do_drained_end and similar, they are either running in
> >> the main loop (or they will be, if coming from a coroutine) or in the
> >> iothread running the AioContext of the bs involved.
> >>
> >> I think that most of the drains except for mirror.c are coming from main
> >> loop. I protected mirror.c in patch 8, even though right now I am not
> >> really sure that what I did is necessary, since the bh will be scheduled
> >> in the main loop.
> >>
> >> Therefore we don't really need locks around drains.
> > 
> > Are you saying rdlock isn't necessary in the main loop because nothing
> > can take the wrlock while our code is executing in the main loop?
> 
> Yes, that's the idea.
> If I am not mistaken (and I hope I am not), only the main loop currently
> modifies/is allowed to modify the graph.

Aren't you completely ignoring coroutines in this? What would a
coroutine do that requires the graph not to change across a yield?

(It's not easily possible to protect this today, and I think this was
the source of some bugs in the past. But if we introduce some proper
locking, I would expect it to solve this.)

Kevin