Re: [PATCH 02/11] block: move drain outside of read-locked bdrv_reopen_queue_child()

[Kevin Wolf <kwolf@redhat.com>]

Am 08.05.2025 um 16:09 hat Fiona Ebner geschrieben:
> This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
> 
> More granular draining is not trivially possible, because
> bdrv_reopen_queue_child() can recursively call itself.

Yeah, we would have to collect the affected nodes first and only then do
whatever the draining is needed for. And consider that draining could
invalidate the list of nodes so that we would have to start over...

Draining only selectively was probably premature optimisation anyway.

> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  block.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 33f78dbe1c..0085dbfa74 100644
> --- a/block.c
> +++ b/block.c
> @@ -4358,7 +4358,7 @@ bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child)
>   * returns a pointer to bs_queue, which is either the newly allocated
>   * bs_queue, or the existing bs_queue being used.
>   *
> - * bs is drained here and undrained by bdrv_reopen_queue_free().
> + * bs needs to be drained
>   */
>  static BlockReopenQueue * GRAPH_RDLOCK
>  bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
> @@ -4368,6 +4368,7 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
>                          bool keep_old_opts)
>  {
>      assert(bs != NULL);
> +    assert(qatomic_read(&bs->quiesce_counter) > 0);

BlockDriverState.quiesce_counter isn't accessed with atomics elsewhere.
Did you confuse it with BlockBackend.quiesce_counter?

>  
>      BlockReopenQueueEntry *bs_entry;
>      BdrvChild *child;
> @@ -4377,13 +4378,6 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
>  
>      GLOBAL_STATE_CODE();
>  
> -    /*
> -     * Strictly speaking, draining is illegal under GRAPH_RDLOCK. We know that
> -     * we've been called with bdrv_graph_rdlock_main_loop(), though, so it's ok
> -     * in practice.
> -     */
> -    bdrv_drained_begin(bs);
> -
>      if (bs_queue == NULL) {
>          bs_queue = g_new0(BlockReopenQueue, 1);
>          QTAILQ_INIT(bs_queue);
> @@ -4522,6 +4516,14 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                      QDict *options, bool keep_old_opts)
>  {
>      GLOBAL_STATE_CODE();
> +
> +    if (bs_queue == NULL) {
> +        /*
> +         * paired with bdrv_drain_all_end() in bdrv_reopen_queue_free().
> +         */
> +        bdrv_drain_all_begin();

Having this comment is a good idea. It fits on a single line, though.

> +    }
> +
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
>      return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, 0, false,
> @@ -4534,12 +4536,16 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
>      if (bs_queue) {
>          BlockReopenQueueEntry *bs_entry, *next;
>          QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> -            bdrv_drained_end(bs_entry->state.bs);
>              qobject_unref(bs_entry->state.explicit_options);
>              qobject_unref(bs_entry->state.options);
>              g_free(bs_entry);
>          }
>          g_free(bs_queue);
> +
> +        /*
> +         * paired with bdrv_drain_all_begin() in bdrv_reopen_queue().
> +         */
> +        bdrv_drain_all_end();

This could be a single line comment, too.

Kevin