Am 14/07/2022 um 18:45 schrieb Hanna Reitz:
>> + * First, go recursively through all nodes in the graph, and see if they
>> + * can change their AioContext.
>> + * If so, add for each node a new transaction with a callback to
>> change the
>> + * AioContext with the new one.
>> + * Once recursion is completed, if all nodes are allowed to change their
>> + * AioContext then commit the transaction, running all callbacks in
>> order.
>> + * Otherwise don't do anything.
>
> Nice explanation, but I’d start with something more simple to describe
> the external interface, like “Change bs's and recursively all of its
> parents' and children's AioContext to the given new context, returning
> an error if that isn't possible. If ignore_child is not NULL, that
> child (and its subgraph) will not be touched.”
You want to have your suggestion as a replacement or addition to mine?
>
>> + *
>> + * This function still requires the caller to take the bs current
>> + * AioContext lock, otherwise draining could fail since AIO_WAIT_WHILE
>
> Well, let’s just say “will” instead of “could”. Callers don’t need to
> know they can get lucky when they ignore the rules.
>
>> + * assumes the lock is always held if bs is in another AioContext.
>> + */
>> +int bdrv_child_try_change_aio_context(BlockDriverState *bs,
>> AioContext *ctx,
>> + BdrvChild *ignore_child, Error
>> **errp)
>
> Do the other functions need to be public? Outside of this file, this
> one seems sufficient to me, but I may well be overlooking something.
>
> Also, why is this function called bdrv_child_*, and not just
> bdrv_try_change_aio_context()? (Or maybe have a
> bdrv_try_change_aio_context() variant without the ignore_child
> parameter, just like bdrv_try_set_aio_context()?)
>
> Actually, wouldn’t just bdrv_change_aio_context() be sufficient? I
> mean, it isn’t called bdrv_try_co_pwritev(). Most functions try to do
> something and return errors if they can’t. Not sure if we need to make
> that clear in the name.
>
> (I may be wrong, but I suspect bdrv_try_set_aio_context() is only named
> such that the compiler will check that the bdrv_set_aio_context()
> interface is completely unused;
> 42a65f02f9b380bd8074882d5844d4ea033389cc’s commit message does seem to
> confirm that.)
Ok, I will change the name in bdrv_change_aio_context. And I now
understand your suggestion on the last patch to remove
bdrv_try_set_aio_context.
Emanuele