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<pbonz...@redhat.com>
Based-on:<20220706201533.289775-1-eespo...@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
