Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Paolo Bonzini]

On 8/6/22 17:32, Vladimir Sementsov-Ogievskiy wrote:
> > if I understand correctly you suggest:
> >
> > .prepare = check and then change aiocontext
> > .abort = revert aiocontext change
> > .commit = nothing
>
> Yes. And that's is how it used actually now in transactions, for example:
>    bdrv_attach_child_common (which is .prepare) calls 
> bdrv_try_set_aio_context() (which is check and then change)
>    bdrv_attach_child_common_abort (which is .abort) calls 
> bdrv_try_set_aio_context() to revert .prepare

I see your point, but I think this can be solved just with a doc comment 
explaining why the transaction is "local".  This is already clear in 
bdrv_child_try_change_aio_context (which doesn't take a Transaction*), 
it can be documented in bdrv_child_change_aio_context as well.

After all, the point of this series is just to avoid code duplication in 
the two visits of the graph, and secondarily to unify the .can_set and 
.set callbacks into one.  We could do it just as well without 
transaction with just a GList of BdrvChild* (keeping the callbcks 
separate); what the Transaction API gives is a little more abstraction, 
by not having to do linked list manipulation by hand.

To be honest I also don't like very much the placement of the 
drained_begin/drained_end in bdrv_change_aio_context.  But if the needs 
arises to call bdrv_change_aio_context within another transaction, I 
think they can be pulled relatively easily (as a subtree drain) in 
bdrv_child_try_change_aio_context or in its callers.  For now, this 
series is already an improvement on several counts.

Paolo