| Message ID | 20220712211911.1302836-8-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor bdrv_try_set_aio_context using transactions | expand |
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,
On 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
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;
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(-)