Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Vladimir Sementsov-Ogievskiy]

On 7/13/22 00:19, Emanuele Giuseppe Esposito wrote:
> +/*
> + * @visited will accumulate all visited BdrvChild object. The caller is
> + * responsible for freeing the list afterwards.
> + */
> +bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                             GSList **visited, Transaction *tran,
> +                             Error **errp)
> +{
> +    BdrvChild *c;
> +    BdrvStateSetAioContext *state;
> +
> +    GLOBAL_STATE_CODE();
> +
> +    if (bdrv_get_aio_context(bs) == ctx) {
> +        return true;
> +    }
> +
> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
> +        if (!bdrv_parent_change_aio_context(c, ctx, visited, tran, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    QLIST_FOREACH(c, &bs->children, next) {
> +        if (!bdrv_child_change_aio_context(c, ctx, visited, tran, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    state = g_new(BdrvStateSetAioContext, 1);
> +    *state = (BdrvStateSetAioContext) {
> +        .new_ctx = ctx,
> +        .bs = bs,
> +    };
> +
> +    /* First half of transactions are drains */
> +    tran_add(tran, &drained_begin_end, state);
> +    /*
> +     * Second half of transactions takes care of setting the
> +     * AioContext and free the state
> +     */
> +    tran_add_tail(tran, &set_aio_context, state);
> +
> +    return true;
> +}


First, it looks like you want to use .commit() as .prepare(), .clean() as 
commit(), and .prepare() as something that just schedule other actions.

In block transaction any effect that is visible to other transaction actions 
should be done in .prepare(). (and .prepare is the main function of the action 
with *tran parameter, i.e. bdrv_change_aio_context() function is actually 
.prepare() phase).

So, ideally, the action:

 - does the complete thing in .prepare (i.e. in the main function of the action)
 - rollback it in .abort
 - nothing in .commit

Usually we still need a .commit phase for irreversible part of the action (like 
calling free() on the object). But that should be transparent for other actions.

So, generally we do:

tran = tran_new()

action_a(..., tran);
action_b(..., tran);
action_c(..., tran);


And we expect, that action_b() operates on top of action_a() already done. And 
action_c() operate on top of action_b() done. So we cannot postpone visible (to 
other actions) effect of the action to .commit phase.

So, for example, if you want a kind of smart drained section in transaction, 
you may add simple

bdrv_drain_tran(..., tran);

that just call drained_begin(), and have only .clean() handler that call 
drained_end(). This way you may do

bdrv_drain_tran(...., tran);
action_a(..., tran);
action_b(..., tran);

And be sure that both actions and all their .abort/.commit/.clean handlers are 
called inside drained seaction. Isn't it what you need?


Second, this all becomes extremely difficult when we mix transactions and 
recursion. When I moved permission system to transactions, I've introduced 
topological dfs search to just get a list of nodes to handle. I think, if we 
want to rework aio context change, we should first get rid of recursion.

--
Best regards,
Vladimir