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

[Emanuele Giuseppe Esposito]

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?

Emanuele