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

[Vladimir Sementsov-Ogievskiy]

On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
> The aim of this series is to reorganize bdrv_try_set_aio_context
> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in
> favour of a new one, ->change_aio_ctx.
>
> More informations in patch 3 (which is also RFC, due to the doubts
> I have with AioContext locks).
>
> Patch 1 just add assertions in the code, 2 extends the transactions API to be 
> able to add also transactions in the tail
> of the list.
> Patch 3 is the core of this series, and introduces the new callback.
> It is marked as RFC and the reason is explained in the commit message.
> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob
> and block-backend BDSes.
> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and
> patch 8 takes care of deleting the old callbacks and unused code.
>
> This series is based on "job: replace AioContext lock with job_mutex",
> but just because it uses job_set_aio_context() introduced there.
>
> Suggested-by: Paolo Bonzini<pbonzini@redhat.com>
> Based-on:<20220706201533.289775-1-eesposit@redhat.com>


What I dislike here is that you refactor aio-context-change to use transactions, but you 
use it "internally" for aio-context-change. aio-context-change doesn't become a 
native part of block-graph modifiction transaction system after the series.

For example, bdrv_attach_child_common(..., tran) still calls 
bdrv_try_change_aio_context() function which doesn't take @tran argument. And 
we have to call bdrv_try_change_aio_context() again in 
bdrv_attach_child_common_abort() with opposite arguments by hand. We create in 
_try_ separate Transaction object which is unrelated to the original 
block-graph-change transaction.

With good refactoring we should get rid of these _try_ functions, and have just 
bdrv_change_aio_context(..., tran) that can be natively used with external tran 
object, where other block-graph change actions participate. This way we'll not 
have to call reverse aio_context_change() in .abort, it will be done 
automatically.

Moreover, your  *aio_context_change* functions that do have tran parameter 
cannot be simply used in the block-graph change transaction, as you don't 
follow the common paradigm, that in .prepare we do all visible changes. That's 
why you have to still use _try_*() version that creates seaparate transaction 
object and completes it: after that the action is actually done and other 
graph-modifying actions can be done on top.

So, IMHO, we shouldn't go this way, as that adds transaction actions that 
actually can't be used in common block-graph-modifying transaction but only 
context of bdrv_try_change_aio_context() internal transaction.

I agree that aio-context change should finally be rewritten to take a native 
place in block-graph transactions, but that should be a complete solution, 
introducing native bdrv_change_aio_context(..., tran) transaction action that 
is directly used in the block-graph transaction, do visible effect in .prepare 
and don't create extra Transaction objects.

--
Best regards,
Vladimir