Re: [PATCH 3/3] block: Start/end drain on correct AioContext

[Kevin Wolf <kwolf@redhat.com>]

Am 23.09.2022 um 14:52 hat Hanna Reitz geschrieben:
> bdrv_parent_drained_{begin,end}_single() are supposed to operate on the
> parent, not on the child, so they should not attempt to get the context
> to poll from the child but the parent instead.  BDRV_POLL_WHILE(c->bs)
> does get the context from the child, so we should replace it with
> AIO_WAIT_WHILE() on the parent's context instead.
> 
> This problem becomes apparent when bdrv_replace_child_noperm() invokes
> bdrv_parent_drained_end_single() after removing a child from a subgraph
> that is in an I/O thread.  By the time bdrv_parent_drained_end_single()
> is called, child->bs is NULL, and so BDRV_POLL_WHILE(c->bs, ...) will
> poll the main loop instead of the I/O thread; but anything that
> bdrv_parent_drained_end_single_no_poll() may have scheduled is going to
> want to run in the I/O thread, but because we poll the main loop, the
> I/O thread is never unpaused, and nothing is run, resulting in a
> deadlock.
> 
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1215
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Hmm... Seeing a BdrvChild with child->bs == NULL outside of functions
directly manipulating it is surprising. I think we need to document that
at least for bdrv_parent_drained_begin/end_single(), that they can get
such BdrvChild objects passed.

Even then, polling with an incomplete BdrvChild in the graph doesn't
sound like the best idea either, but not sure how to avoid that best.
Maybe we would need a different order in bdrv_replace_child_noperm() if
either old_bs or new_bs is NULL.

Anyway, the code change itself looks reasonable:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>