Replace all direct usage of ->can_set_aio_ctx and ->set_aio_ctx,
and call bdrv_child_try_change_aio_context() in
bdrv_try_set_aio_context(), the main function called through
the whole block layer.
From this point onwards, ->can_set_aio_ctx and ->set_aio_ctx
won't be used anymore.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block.c | 30 ++++++++++++++++--------------
block/block-backend.c | 8 ++++++--
2 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/block.c b/block.c
index a7ba590dfa..101188a2d4 100644
--- a/block.c
+++ b/block.c
@@ -2966,17 +2966,18 @@ static void bdrv_attach_child_common_abort(void *opaque)
}
if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
+ Transaction *tran;
GSList *ignore;
+ bool ret;
- /* No need to ignore `child`, because it has been detached already */
- ignore = NULL;
- child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
- &error_abort);
- g_slist_free(ignore);
+ tran = tran_new();
+ /* No need to ignore `child`, because it has been detached already */
ignore = NULL;
- child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
+ ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore,
+ tran, &error_abort);
g_slist_free(ignore);
+ tran_finalize(tran, ret ? ret : -1);
}
bdrv_unref(bs);
@@ -3037,17 +3038,18 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
Error *local_err = NULL;
int ret = bdrv_try_set_aio_context(child_bs, parent_ctx, &local_err);
- if (ret < 0 && child_class->can_set_aio_ctx) {
+ if (ret < 0 && child_class->change_aio_ctx) {
+ Transaction *tran = tran_new();
GSList *ignore = g_slist_prepend(NULL, new_child);
- if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore,
- NULL))
- {
+ bool ret_child;
+
+ ret_child = child_class->change_aio_ctx(new_child, child_ctx,
+ &ignore, tran, NULL);
+ if (ret_child) {
error_free(local_err);
ret = 0;
- g_slist_free(ignore);
- ignore = g_slist_prepend(NULL, new_child);
- child_class->set_aio_ctx(new_child, child_ctx, &ignore);
}
+ tran_finalize(tran, ret_child ? ret_child : -1);
g_slist_free(ignore);
}
@@ -7708,7 +7710,7 @@ int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
Error **errp)
{
GLOBAL_STATE_CODE();
- return bdrv_child_try_set_aio_context(bs, ctx, NULL, errp);
+ return bdrv_child_try_change_aio_context(bs, ctx, NULL, errp);
}
void bdrv_add_aio_context_notifier(BlockDriverState *bs,
diff --git a/block/block-backend.c b/block/block-backend.c
index 674eaaa2bf..6e90ac3a6a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
bdrv_ref(bs);
if (update_root_node) {
- ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
- errp);
+ /*
+ * update_root_node MUST be false for blk_root_set_aio_ctx_commit(),
+ * as we are already in the commit function of a transaction.
+ */
+ ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
+ errp);
if (ret < 0) {
bdrv_unref(bs);
return ret;
--
2.31.1On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote: > + /* No need to ignore `child`, because it has been detached already */ > ignore = NULL; > - child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); > + ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore, > + tran, &error_abort); > g_slist_free(ignore); > + tran_finalize(tran, ret ? ret : -1); Should this instead assert that ret is true, and call tran_commit() directly? Paolo
On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 674eaaa2bf..6e90ac3a6a 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
> bdrv_ref(bs);
>
> if (update_root_node) {
> - ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
> - errp);
> + /*
> + * update_root_node MUST be false for blk_root_set_aio_ctx_commit(),
> + * as we are already in the commit function of a transaction.
> + */
> + ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
> + errp);
> if (ret < 0) {
> bdrv_unref(bs);
> return ret;
Looking further at blk_do_set_aio_context:
if (tgm->throttle_state) {
bdrv_drained_begin(bs);
throttle_group_detach_aio_context(tgm);
throttle_group_attach_aio_context(tgm, new_context);
bdrv_drained_end(bs);
}
Perhaps the drained_begin/drained_end pair can be moved to
blk_set_aio_context? It shouldn't be needed from the change_aio_ctx
callback, because bs is already drained. If so, blk_do_set_aio_context
would become just:
if (tgm->throttle_state) {
throttle_group_detach_aio_context(tgm);
throttle_group_attach_aio_context(tgm, new_context);
}
blk->ctx = new_context;
and blk_set_aio_context would be something like:
if (bs) {
bdrv_ref(bs);
ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
errp);
if (ret < 0) {
goto out_no_drain;
}
bdrv_drained_begin(bs);
}
ret = blk_do_set_aio_context(blk, new_context, errp);
if (bs) {
bdrv_drained_end(bs);
out_no_drain;
bdrv_unref(bs);
}
return ret;
Paolo
Am 18/07/2022 um 18:39 schrieb Paolo Bonzini:
> On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 674eaaa2bf..6e90ac3a6a 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend
>> *blk, AioContext *new_context,
>> bdrv_ref(bs);
>> if (update_root_node) {
>> - ret = bdrv_child_try_set_aio_context(bs, new_context,
>> blk->root,
>> - errp);
>> + /*
>> + * update_root_node MUST be false for
>> blk_root_set_aio_ctx_commit(),
>> + * as we are already in the commit function of a
>> transaction.
>> + */
>> + ret = bdrv_child_try_change_aio_context(bs, new_context,
>> blk->root,
>> + errp);
>> if (ret < 0) {
>> bdrv_unref(bs);
>> return ret;
>
>
> Looking further at blk_do_set_aio_context:
>
> if (tgm->throttle_state) {
> bdrv_drained_begin(bs);
> throttle_group_detach_aio_context(tgm);
> throttle_group_attach_aio_context(tgm, new_context);
> bdrv_drained_end(bs);
> }
>
> Perhaps the drained_begin/drained_end pair can be moved to
> blk_set_aio_context? It shouldn't be needed from the change_aio_ctx
> callback, because bs is already drained. If so, blk_do_set_aio_context
> would become just:
>
> if (tgm->throttle_state) {
> throttle_group_detach_aio_context(tgm);
> throttle_group_attach_aio_context(tgm, new_context);
> }
> blk->ctx = new_context;
>
> and blk_set_aio_context would be something like:
>
> if (bs) {
> bdrv_ref(bs);
> ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
> errp);
> if (ret < 0) {
> goto out_no_drain;
> }
> bdrv_drained_begin(bs);
> }
> ret = blk_do_set_aio_context(blk, new_context, errp);
> if (bs) {
> bdrv_drained_end(bs);
> out_no_drain;
> bdrv_unref(bs);
> }
> return ret;
>
> Paolo
>
This is another example on how things here do not take into account many
other use cases outside the common ones: if I use the above suggestion,
test-block-iothread fails.
The reason why it fails is simple: now we drain regardless of
tgm->throttle_state being true or false. And this requires yet another
aiocontext lock.
Which means that if we are calling blk_set_aio_context where the bs
exists and tgm->throttle_state is true, we have a bug.
Test is test_attach_blockjob and function is line 435
blk_set_aio_context(blk, ctx, &error_abort);
The reason is that bs is first switched to the new aiocontext and then
we try to drain it without holding the lock.
Wrapping the new drains in aio_context_acquire/release(new_context) is
not so much helpful either, since apparently the following
blk_set_aio_context makes aio_poll() hang.
I am not sure why, any ideas?
Code:
static int blk_do_set_aio_context(BlockBackend *blk, AioContext
*new_context,
Error **errp)
{
BlockDriverState *bs = blk_bs(blk);
ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
if (bs) {
bdrv_ref(bs);
if (tgm->throttle_state) {
throttle_group_detach_aio_context(tgm);
throttle_group_attach_aio_context(tgm, new_context);
}
bdrv_unref(bs);
}
blk->ctx = new_context;
return 0;
}
int blk_set_aio_context(BlockBackend *blk, AioContext *new_context,
Error **errp)
{
BlockDriverState *bs = blk_bs(blk);
int ret;
GLOBAL_STATE_CODE();
if (bs) {
bdrv_ref(bs);
ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
errp);
if (ret < 0) {
goto out_no_drain;
}
if (new_context != qemu_get_aio_context()) {
aio_context_acquire(new_context);
}
bdrv_drained_begin(bs); // <-------------------- hangs here!
}
ret = blk_do_set_aio_context(blk, new_context, errp);
if (bs) {
bdrv_drained_end(bs);
if (new_context != qemu_get_aio_context()) {
aio_context_release(new_context);
}
out_no_drain:
bdrv_unref(bs);
}
return ret;
}
Emanuele
On 7/19/22 11:57, Emanuele Giuseppe Esposito wrote: > > Wrapping the new drains in aio_context_acquire/release(new_context) is > not so much helpful either, since apparently the following > blk_set_aio_context makes aio_poll() hang. > I am not sure why, any ideas? I'll take a look, thanks. In any case this doesn't block this series, it was just a suggestion and blk_do_set_aio_context can be improved on top. Paolo
Am 19/07/2022 um 20:07 schrieb Paolo Bonzini: > On 7/19/22 11:57, Emanuele Giuseppe Esposito wrote: >> >> Wrapping the new drains in aio_context_acquire/release(new_context) is >> not so much helpful either, since apparently the following >> blk_set_aio_context makes aio_poll() hang. >> I am not sure why, any ideas? > > I'll take a look, thanks. In any case this doesn't block this series, > it was just a suggestion and blk_do_set_aio_context can be improved on top. > Neither is the "using drain_begin/end_unlocked" part. That won't be straightforward either. That will go in a future serie. Emanuele
On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> Replace all direct usage of ->can_set_aio_ctx and ->set_aio_ctx,
> and call bdrv_child_try_change_aio_context() in
> bdrv_try_set_aio_context(), the main function called through
> the whole block layer.
>
> From this point onwards, ->can_set_aio_ctx and ->set_aio_ctx
> won't be used anymore.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> block.c | 30 ++++++++++++++++--------------
> block/block-backend.c | 8 ++++++--
> 2 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index a7ba590dfa..101188a2d4 100644
> --- a/block.c
> +++ b/block.c
> @@ -2966,17 +2966,18 @@ static void bdrv_attach_child_common_abort(void *opaque)
> }
>
> if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
> + Transaction *tran;
> GSList *ignore;
> + bool ret;
>
> - /* No need to ignore `child`, because it has been detached already */
> - ignore = NULL;
> - child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
> - &error_abort);
> - g_slist_free(ignore);
> + tran = tran_new();
>
> + /* No need to ignore `child`, because it has been detached already */
> ignore = NULL;
> - child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
> + ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore,
> + tran, &error_abort);
> g_slist_free(ignore);
> + tran_finalize(tran, ret ? ret : -1);
As far as I understand, the transaction is supposed to always succeed;
that’s why we pass `&error_abort`, I thought.
If so, `ret` should always be true. More importantly, though, I think
the `ret ? ret : -1` is wrong because it’ll always evaluate to either 1
or -1, but never to 0, which would indicate success. I think it should
be `ret == true ? 0 : -1`, or even better `assert(ret == true);
tran_finalize(tran, 0);`.
> }
>
> bdrv_unref(bs);
> @@ -3037,17 +3038,18 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
> Error *local_err = NULL;
> int ret = bdrv_try_set_aio_context(child_bs, parent_ctx, &local_err);
>
> - if (ret < 0 && child_class->can_set_aio_ctx) {
> + if (ret < 0 && child_class->change_aio_ctx) {
> + Transaction *tran = tran_new();
> GSList *ignore = g_slist_prepend(NULL, new_child);
> - if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore,
> - NULL))
> - {
> + bool ret_child;
> +
> + ret_child = child_class->change_aio_ctx(new_child, child_ctx,
> + &ignore, tran, NULL);
> + if (ret_child) {
To be honest, due to the mix of return value styles we have, perhaps a
`ret_child == true` would help to signal that this is a success path.
> error_free(local_err);
> ret = 0;
> - g_slist_free(ignore);
> - ignore = g_slist_prepend(NULL, new_child);
> - child_class->set_aio_ctx(new_child, child_ctx, &ignore);
> }
> + tran_finalize(tran, ret_child ? ret_child : -1);
This too should probably be `ret_child == true ? 0 : -1`.
> g_slist_free(ignore);
> }
>
> @@ -7708,7 +7710,7 @@ int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
> Error **errp)
> {
> GLOBAL_STATE_CODE();
> - return bdrv_child_try_set_aio_context(bs, ctx, NULL, errp);
> + return bdrv_child_try_change_aio_context(bs, ctx, NULL, errp);
Why not remove this function and adjust all callers?
Hanna
> }
>
> void bdrv_add_aio_context_notifier(BlockDriverState *bs,
© 2016 - 2026 Red Hat, Inc.