Am 27.05.25 um 17:23 schrieb Kevin Wolf:
> Am 26.05.2025 um 15:21 hat Fiona Ebner geschrieben:
>> @@ -3129,11 +3123,9 @@ 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 mention in the function comment for bdrv_attach_child_common()
> that all block nodes must be drained from before this functoin is called
> until after the transaction is finalized?
>
> A similar note would probably be good for all the other functions you
> mention in the commit message that don't finalize the transaction yet so
> that we convert them in this same patch.
>
>> @@ -4721,6 +4719,8 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs,
>> bool read_only,
>> * Return 0 on success, otherwise return < 0 and set @errp.
>> *
>> * @reopen_state->bs can move to a different AioContext in this function.
>> + *
>> + * The old child bs must be drained.
>> */
>> static int GRAPH_UNLOCKED
>> bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
>
> Only the old child or all nodes?
>
> bdrv_try_change_aio_context_locked() is documented as "Called while all
> bs are drained." and we call it indirectly here (which will be more
> obvious when you add the comments as suggested above).
Yes, it needs to be all nodes. I'll try to document the requirement for
all affected functions in v4 (also what you mentioned in the replies to
the other patches).
Best Regards,
Fiona
