| Message ID | 20220712211911.1302836-7-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: > blk_root_change_aio_ctx() is very similar to blk_root_can_set_aio_ctx(), > but implements a new transaction so that if all check pass, the new > transaction's .commit will take care of changing the BlockBackend > AioContext. blk_root_set_aio_ctx_commit() is the same as > blk_root_set_aio_ctx(). > > Note: bdrv_child_try_change_aio_context() is not called by > anyone at this point. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/block-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/block/block-backend.c b/block/block-backend.c > index f425b00793..674eaaa2bf 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c [...] > @@ -2208,6 +2212,56 @@ int blk_set_aio_context(BlockBackend *blk, AioContext *new_context, [...] > +static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx, > + GSList **visited, Transaction *tran, > + Error **errp) > +{ > + BlockBackend *blk = child->opaque; > + BdrvStateBlkRootContext *s; > + > + if (blk->allow_aio_context_change) { > + goto finish; > + } > + > + /* > + * Only manually created BlockBackends that are not attached to anything > + * can change their AioContext without updating their user. > + */ > + if (!blk->name || blk->dev) { > + /* TODO Add BB name/QOM path */ > + error_setg(errp, "Cannot change iothread of active block backend"); > + return false; > + } Is the goto really necessary? Or, rather, do you prefer this to something like if (!blk->allow_aio_context_change) { /* * Manually created BlockBackends (those with a name) that are not * attached to anything can change their AioContext without updating * their user; return an error for others. */ if (!blk->name || blk->dev) { ... } } If you prefer the goto, I’d at least rename the label to “change_context” or “allowed” or something. Hanna > + > +finish: > + s = g_new(BdrvStateBlkRootContext, 1); > + *s = (BdrvStateBlkRootContext) { > + .new_ctx = ctx, > + .blk = blk, > + }; > + > + tran_add_tail(tran, &set_blk_root_context, s); (Again, not a huge fan of this.) > + return true; > +} > + > static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx, > GSList **ignore, Error **errp) > {
diff --git a/block/block-backend.c b/block/block-backend.c index f425b00793..674eaaa2bf 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -138,6 +138,9 @@ static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx, GSList **ignore, Error **errp); static void blk_root_set_aio_ctx(BdrvChild *child, AioContext *ctx, GSList **ignore); +static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx, + GSList **visited, Transaction *tran, + Error **errp); static char *blk_root_get_parent_desc(BdrvChild *child) { @@ -336,6 +339,7 @@ static const BdrvChildClass child_root = { .can_set_aio_ctx = blk_root_can_set_aio_ctx, .set_aio_ctx = blk_root_set_aio_ctx, + .change_aio_ctx = blk_root_change_aio_ctx, .get_parent_aio_context = blk_root_get_parent_aio_context, }; @@ -2208,6 +2212,56 @@ int blk_set_aio_context(BlockBackend *blk, AioContext *new_context, return blk_do_set_aio_context(blk, new_context, true, errp); } +typedef struct BdrvStateBlkRootContext { + AioContext *new_ctx; + BlockBackend *blk; +} BdrvStateBlkRootContext; + +static void blk_root_set_aio_ctx_commit(void *opaque) +{ + BdrvStateBlkRootContext *s = opaque; + BlockBackend *blk = s->blk; + + blk_do_set_aio_context(blk, s->new_ctx, false, &error_abort); +} + +static TransactionActionDrv set_blk_root_context = { + .commit = blk_root_set_aio_ctx_commit, + .clean = g_free, +}; + +static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx, + GSList **visited, Transaction *tran, + Error **errp) +{ + BlockBackend *blk = child->opaque; + BdrvStateBlkRootContext *s; + + if (blk->allow_aio_context_change) { + goto finish; + } + + /* + * Only manually created BlockBackends that are not attached to anything + * can change their AioContext without updating their user. + */ + if (!blk->name || blk->dev) { + /* TODO Add BB name/QOM path */ + error_setg(errp, "Cannot change iothread of active block backend"); + return false; + } + +finish: + s = g_new(BdrvStateBlkRootContext, 1); + *s = (BdrvStateBlkRootContext) { + .new_ctx = ctx, + .blk = blk, + }; + + tran_add_tail(tran, &set_blk_root_context, s); + return true; +} + static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx, GSList **ignore, Error **errp) {
blk_root_change_aio_ctx() is very similar to blk_root_can_set_aio_ctx(), but implements a new transaction so that if all check pass, the new transaction's .commit will take care of changing the BlockBackend AioContext. blk_root_set_aio_ctx_commit() is the same as blk_root_set_aio_ctx(). Note: bdrv_child_try_change_aio_context() is not called by anyone at this point. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/block-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)