Re: [RFC PATCH 6/8] block-backend: implement .change_aio_ctx in child_root

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> blk_root_change_aio_ctx() is very similar to blk_root_can_set_aio_ctx(),
> but implements a new transaction so that if all check pass, the new
> transaction's .commit will take care of changing the BlockBackend
> AioContext. blk_root_set_aio_ctx_commit() is the same as
> blk_root_set_aio_ctx().
>
> Note: bdrv_child_try_change_aio_context() is not called by
> anyone at this point.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index f425b00793..674eaaa2bf 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -2208,6 +2212,56 @@ int blk_set_aio_context(BlockBackend *blk, AioContext 
> *new_context,

[...]

> +static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
> +                                    GSList **visited, Transaction *tran,
> +                                    Error **errp)
> +{
> +    BlockBackend *blk = child->opaque;
> +    BdrvStateBlkRootContext *s;
> +
> +    if (blk->allow_aio_context_change) {
> +        goto finish;
> +    }
> +
> +    /*
> +     * Only manually created BlockBackends that are not attached to anything
> +     * can change their AioContext without updating their user.
> +     */
> +    if (!blk->name || blk->dev) {
> +        /* TODO Add BB name/QOM path */
> +        error_setg(errp, "Cannot change iothread of active block backend");
> +        return false;
> +    }

Is the goto really necessary?  Or, rather, do you prefer this to 
something like

if (!blk->allow_aio_context_change) {
    /*
     * Manually created BlockBackends (those with a name) that are not
     * attached to anything can change their AioContext without updating
     * their user; return an error for others.
     */
    if (!blk->name || blk->dev) {
        ...
    }
}

If you prefer the goto, I’d at least rename the label to 
“change_context” or “allowed” or something.

Hanna

> +
> +finish:
> +    s = g_new(BdrvStateBlkRootContext, 1);
> +    *s = (BdrvStateBlkRootContext) {
> +        .new_ctx = ctx,
> +        .blk = blk,
> +    };
> +
> +    tran_add_tail(tran, &set_blk_root_context, s);

(Again, not a huge fan of this.)

> +    return true;
> +}
> +
>   static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
>                                        GSList **ignore, Error **errp)
>   {