Am 05/08/2022 um 15:22 schrieb Emanuele Giuseppe Esposito:
Am 27/07/2022 um 18:13 schrieb 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>
So, I read your email before going on PTO and at that point I got what
your concerns were, but now after re-reading it I don't understand
anymore what you mean :)
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.
This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
transaction parameter" supports taking transaction as a parameter.
bdrv_attach_child_common could simply call
bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
it could work).
No actually I don't get how it can work in bdrv_attach_child_common.
We have the following:
parent_ctx = bdrv_child_get_parent_aio_context(new_child);
if (child_ctx != parent_ctx) {
int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
&local_err);
if (ret < 0 && child_class->change_aio_ctx) {
ret_child = child_class->change_aio_ctx(new_child, child_ctx,
visited, tran, NULL);
tran_finalize(tran, ret_child == true ? 0 : -1);
}
if (ret < 0) {
return ret;
}
}
bdrv_replace_child_noperm(&new_child, child_bs, true);
So bdrv_try_change_aio_context would be changed in
bdrv_try_change_aio_context_tran, but then how can we call
bdrv_replace_child_noperm if no aiocontext has been changed at all?
I don't think we can mix transactional operations with non-transactional.