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()?

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.

Stefan