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

[Emanuele Giuseppe Esposito]

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.

Emanuele
> 
> Stefan
>