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
> * 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
> * 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.
> */
---snip 8<---
>> @@ -7720,7 +7717,11 @@ int bdrv_try_change_aio_context(BlockDriverState *bs,
>> AioContext *ctx,
>> if (ignore_child) {
>> g_hash_table_add(visited, ignore_child);
>> }
>> + bdrv_drain_all_begin();
>> + bdrv_graph_rdlock_main_loop();
>> ret = bdrv_change_aio_context(bs, ctx, visited, tran, errp);
>> + bdrv_graph_rdunlock_main_loop();
>> + bdrv_drain_all_end();
>> g_hash_table_destroy(visited);
>
> I think you're ending the drained section too early here. Previously,
> the nodes were kept drained until after tran_abort/commit(), and I think
> that's important (tran_commit() is the thing that actually switches the
> AioContext).
Right, I'll change this in v3.
Best Regards,
Fiona
