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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]

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?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]