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
