Re: [PATCH v2 08/24] block: move drain outside of bdrv_change_aio_context() and mark GRAPH_RDLOCK

[Kevin Wolf <kwolf@redhat.com>]

Am 22.05.2025 um 11:09 hat Fiona Ebner geschrieben:
> Am 21.05.25 um 18:05 schrieb Kevin Wolf:
> > Am 20.05.2025 um 12:29 hat Fiona Ebner geschrieben:
> >> This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
> >>
> >> Note that even if bdrv_drained_begin() would already be marked as
> > 
> > "if ... were already marked"
> 
> Ack.
> 
> ---snip 8<---
> 
> >> diff --git a/block.c b/block.c
> >> index 01144c895e..7148618504 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -106,9 +106,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
> >>  
> >>  static bool bdrv_backing_overridden(BlockDriverState *bs);
> >>  
> >> -static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> >> -                                    GHashTable *visited, Transaction *tran,
> >> -                                    Error **errp);
> >> +static bool GRAPH_RDLOCK
> >> +bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> >> +                        GHashTable *visited, Transaction *tran, Error **errp);
> > 
> > For static functions, we should have rhe GRAPH_RDLOCK annotation both
> > here and in the actual definition, too.
> 
> Will fix in v3!
> 
> >>  /* If non-zero, use only whitelisted block drivers */
> >>  static int use_bdrv_whitelist;
> >> @@ -3040,8 +3040,10 @@ static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque)
> >>  
> >>          /* No need to visit `child`, because it has been detached already */
> >>          visited = g_hash_table_new(NULL, NULL);
> >> +        bdrv_drain_all_begin();
> >>          ret = s->child->klass->change_aio_ctx(s->child, s->old_parent_ctx,
> >>                                                visited, tran, &error_abort);
> >> +        bdrv_drain_all_end();
> >>          g_hash_table_destroy(visited);
> >>  
> >>          /* transaction is supposed to always succeed */
> >> @@ -3122,9 +3124,11 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
> >>              bool ret_child;
> >>  
> >>              g_hash_table_add(visited, new_child);
> >> +            bdrv_drain_all_begin();
> >>              ret_child = child_class->change_aio_ctx(new_child, child_ctx,
> >>                                                      visited, aio_ctx_tran,
> >>                                                      NULL);
> >> +            bdrv_drain_all_end();
> >>              if (ret_child == true) {
> >>                  error_free(local_err);
> >>                  ret = 0;
> > 
> > Should we document in the header file that BdrvChildClass.change_aio_ctx
> > is called with the node drained?
> > 
> > We could add assertions to bdrv_child/parent_change_aio_context or at
> > least comments to this effect. (Assertions might be over the top because
> > it's easy to verify that both are only called from
> > bdrv_change_aio_context().)
> 
> Sounds good. Would the following be okay?
> 
> For bdrv_parent_change_aio_context() and for change_aio_ctx() the same
> (except the name of @child is @c for the former):
> 
> > /*
> >  * Changes the AioContext of for @child to @ctx and recursively for the
> >  * associated block nodes and all their children and parents. Returns true if

"of for"?

This might be a bit too specific for child_of_bds, while the function is
also implemented for BlockBackends and block jobs. Keeping the
description more in line with other BdrvChildClass functions, maybe
something generic like:

    Notifies the parent that the child is trying to change its
    AioContext. The parent may in turn change the AioContext of other
    nodes in the same transaction.

> >  * the change is possible and the transaction @tran can be continued. Returns
> >  * false and sets @errp if not and the transaction must be aborted.
> >  *
> >  * @visited will accumulate all visited BdrvChild objects. The caller is
> >  * responsible for freeing the list afterwards.
> >  *
> >  * Must be called with the affected block nodes drained.
> >  */
> 
> and for bdrv_child_change_aio_context() slightly different:
> 
> > /*
> >  * Changes the AioContext of for @c->bs to @ctx and recursively for all its

Again "of for".

> >  * children and parents. Returns true if the change is possible and the
> >  * transaction @tran can be continued. Returns false and sets @errp if not and
> >  * the transaction must be aborted.
> >  *
> >  * @visited will accumulate all visited BdrvChild objects. The caller is
> >  * responsible for freeing the list afterwards.
> >  *
> >  * Must be called with the affected block nodes drained.
> >  */

The rest looks good. (Much better than what I would have done, I was
only thinking of the last line without a proper documentation of the
whole function.)

Kevin