| Message ID | 20201127144522.29991-21-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: update graph permissions update | expand |
Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > Split out no-perm part of bdrv_root_attach_child() into separate > transaction action. bdrv_root_attach_child() now moves to new > permission update paradigm: first update graph relations then update > permissions. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block.c | 162 ++++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 117 insertions(+), 45 deletions(-) > > diff --git a/block.c b/block.c > index f0fcd75555..a7ccbb4fb1 100644 > --- a/block.c > +++ b/block.c > @@ -86,6 +86,13 @@ static void bdrv_parent_set_aio_context_ignore(BdrvChild *c, AioContext *ctx, > GSList **ignore); > static void bdrv_replace_child_noperm(BdrvChild *child, > BlockDriverState *new_bs); > +static int bdrv_attach_child_common(BlockDriverState *child_bs, > + const char *child_name, > + const BdrvChildClass *child_class, > + BdrvChildRole child_role, > + uint64_t perm, uint64_t shared_perm, > + void *opaque, BdrvChild **child, > + GSList **tran, Error **errp); If you added the new code above bdrv_root_attach_child(), we wouldn't need the forward declaration and the patch would probably be simpler to read (because it's the first part of bdrv_root_attach_child() that is factored out). > static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue > *queue, Error **errp); > @@ -2898,55 +2905,22 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, > uint64_t perm, uint64_t shared_perm, > void *opaque, Error **errp) > { > - BdrvChild *child; > - Error *local_err = NULL; > int ret; > - AioContext *ctx; > + BdrvChild *child = NULL; > + GSList *tran = NULL; > > - ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp); > + ret = bdrv_attach_child_common(child_bs, child_name, child_class, > + child_role, perm, shared_perm, opaque, > + &child, &tran, errp); > if (ret < 0) { > - bdrv_abort_perm_update(child_bs); > bdrv_unref(child_bs); > return NULL; > } > > - child = g_new(BdrvChild, 1); > - *child = (BdrvChild) { > - .bs = NULL, > - .name = g_strdup(child_name), > - .klass = child_class, > - .role = child_role, > - .perm = perm, > - .shared_perm = shared_perm, > - .opaque = opaque, > - }; > - > - ctx = bdrv_child_get_parent_aio_context(child); > - > - /* If the AioContexts don't match, first try to move the subtree of > - * child_bs into the AioContext of the new parent. If this doesn't work, > - * try moving the parent into the AioContext of child_bs instead. */ > - if (bdrv_get_aio_context(child_bs) != ctx) { > - ret = bdrv_try_set_aio_context(child_bs, ctx, &local_err); > - if (ret < 0) { > - if (bdrv_parent_try_set_aio_context(child, ctx, NULL) == 0) { > - ret = 0; > - error_free(local_err); > - local_err = NULL; > - } > - } > - if (ret < 0) { > - error_propagate(errp, local_err); > - g_free(child); > - bdrv_abort_perm_update(child_bs); > - bdrv_unref(child_bs); > - return NULL; > - } > - } > - > - /* This performs the matching bdrv_set_perm() for the above check. */ > - bdrv_replace_child(child, child_bs); > + ret = bdrv_refresh_perms(child_bs, errp); > + tran_finalize(tran, ret); > > + bdrv_unref(child_bs); > return child; > } > > @@ -2988,16 +2962,114 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > return child; > } > > -static void bdrv_detach_child(BdrvChild *child) > +static void bdrv_remove_empty_child(BdrvChild *child) > { > + assert(!child->bs); > QLIST_SAFE_REMOVE(child, next); > - > - bdrv_replace_child(child, NULL); > - > g_free(child->name); > g_free(child); > } > > +typedef struct BdrvAttachChildCommonState { > + BdrvChild **child; > + AioContext *old_parent_ctx; > + AioContext *old_child_ctx; > +} BdrvAttachChildCommonState; > + > +static void bdrv_attach_child_common_abort(void *opaque) > +{ > + BdrvAttachChildCommonState *s = opaque; > + BdrvChild *child = *s->child; > + BlockDriverState *bs = child->bs; > + > + bdrv_replace_child_noperm(child, NULL); > + > + if (bdrv_get_aio_context(bs) != s->old_child_ctx) { > + bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); Would failure actually be fatal? I think we can ignore it, the node is in an AioContext that works for it. > + } > + > + if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { > + bdrv_parent_try_set_aio_context(child, s->old_parent_ctx, > + &error_abort); And the same here. > + } > + > + bdrv_unref(bs); > + bdrv_remove_empty_child(child); > + *s->child = NULL; > +} > + > +static TransactionActionDrv bdrv_attach_child_common_drv = { > + .abort = bdrv_attach_child_common_abort, > +}; > + > +/* > + * Common part of attoching bdrv child to bs or to blk or to job > + */ > +static int bdrv_attach_child_common(BlockDriverState *child_bs, > + const char *child_name, > + const BdrvChildClass *child_class, > + BdrvChildRole child_role, > + uint64_t perm, uint64_t shared_perm, > + void *opaque, BdrvChild **child, > + GSList **tran, Error **errp) > +{ > + int ret; > + BdrvChild *new_child; > + AioContext *parent_ctx; > + AioContext *child_ctx = bdrv_get_aio_context(child_bs); > + > + assert(child); > + assert(*child == NULL); > + > + new_child = g_new(BdrvChild, 1); > + *new_child = (BdrvChild) { > + .bs = NULL, > + .name = g_strdup(child_name), > + .klass = child_class, > + .role = child_role, > + .perm = perm, > + .shared_perm = shared_perm, > + .opaque = opaque, > + }; > + > + parent_ctx = bdrv_child_get_parent_aio_context(new_child); > + if (child_ctx != parent_ctx) { > + ret = bdrv_try_set_aio_context(child_bs, parent_ctx, NULL); > + if (ret < 0) { > + /* > + * bdrv_try_set_aio_context_tran don't need rollback after failure, > + * so we don't care. > + */ > + ret = bdrv_parent_try_set_aio_context(new_child, child_ctx, errp); > + } > + if (ret < 0) { > + bdrv_remove_empty_child(new_child); > + return ret; > + } > + } Not sure why you decided to rewrite this block while moving it from bdrv_root_attach_child(). We're losing the comment above it, and a possible error message is now related to changing the context of the parent node instead of the newly added node, which I imagine is less obvious in the general case. > + bdrv_ref(child_bs); > + bdrv_replace_child_noperm(new_child, child_bs); > + > + *child = new_child; > + > + BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1); > + *s = (BdrvAttachChildCommonState) { > + .child = child, > + .old_parent_ctx = parent_ctx, > + .old_child_ctx = child_ctx, > + }; > + tran_prepend(tran, &bdrv_attach_child_common_drv, s); > + > + return 0; > +} Kevin
04.02.2021 00:01, Kevin Wolf wrote: > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Split out no-perm part of bdrv_root_attach_child() into separate >> transaction action. bdrv_root_attach_child() now moves to new >> permission update paradigm: first update graph relations then update >> permissions. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block.c | 162 ++++++++++++++++++++++++++++++++++++++++---------------- >> 1 file changed, 117 insertions(+), 45 deletions(-) >> >> diff --git a/block.c b/block.c >> index f0fcd75555..a7ccbb4fb1 100644 >> --- a/block.c >> +++ b/block.c >> @@ -86,6 +86,13 @@ static void bdrv_parent_set_aio_context_ignore(BdrvChild *c, AioContext *ctx, >> GSList **ignore); >> static void bdrv_replace_child_noperm(BdrvChild *child, >> BlockDriverState *new_bs); >> +static int bdrv_attach_child_common(BlockDriverState *child_bs, >> + const char *child_name, >> + const BdrvChildClass *child_class, >> + BdrvChildRole child_role, >> + uint64_t perm, uint64_t shared_perm, >> + void *opaque, BdrvChild **child, >> + GSList **tran, Error **errp); > > If you added the new code above bdrv_root_attach_child(), we wouldn't > need the forward declaration and the patch would probably be simpler to > read (because it's the first part of bdrv_root_attach_child() that is > factored out). > >> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue >> *queue, Error **errp); >> @@ -2898,55 +2905,22 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, >> uint64_t perm, uint64_t shared_perm, >> void *opaque, Error **errp) >> { >> - BdrvChild *child; >> - Error *local_err = NULL; >> int ret; >> - AioContext *ctx; >> + BdrvChild *child = NULL; >> + GSList *tran = NULL; >> >> - ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp); >> + ret = bdrv_attach_child_common(child_bs, child_name, child_class, >> + child_role, perm, shared_perm, opaque, >> + &child, &tran, errp); >> if (ret < 0) { >> - bdrv_abort_perm_update(child_bs); >> bdrv_unref(child_bs); >> return NULL; >> } >> >> - child = g_new(BdrvChild, 1); >> - *child = (BdrvChild) { >> - .bs = NULL, >> - .name = g_strdup(child_name), >> - .klass = child_class, >> - .role = child_role, >> - .perm = perm, >> - .shared_perm = shared_perm, >> - .opaque = opaque, >> - }; >> - >> - ctx = bdrv_child_get_parent_aio_context(child); >> - >> - /* If the AioContexts don't match, first try to move the subtree of >> - * child_bs into the AioContext of the new parent. If this doesn't work, >> - * try moving the parent into the AioContext of child_bs instead. */ >> - if (bdrv_get_aio_context(child_bs) != ctx) { >> - ret = bdrv_try_set_aio_context(child_bs, ctx, &local_err); >> - if (ret < 0) { >> - if (bdrv_parent_try_set_aio_context(child, ctx, NULL) == 0) { >> - ret = 0; >> - error_free(local_err); >> - local_err = NULL; >> - } >> - } >> - if (ret < 0) { >> - error_propagate(errp, local_err); >> - g_free(child); >> - bdrv_abort_perm_update(child_bs); >> - bdrv_unref(child_bs); >> - return NULL; >> - } >> - } >> - >> - /* This performs the matching bdrv_set_perm() for the above check. */ >> - bdrv_replace_child(child, child_bs); >> + ret = bdrv_refresh_perms(child_bs, errp); >> + tran_finalize(tran, ret); >> >> + bdrv_unref(child_bs); >> return child; >> } >> >> @@ -2988,16 +2962,114 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, >> return child; >> } >> >> -static void bdrv_detach_child(BdrvChild *child) >> +static void bdrv_remove_empty_child(BdrvChild *child) >> { >> + assert(!child->bs); >> QLIST_SAFE_REMOVE(child, next); >> - >> - bdrv_replace_child(child, NULL); >> - >> g_free(child->name); >> g_free(child); >> } >> >> +typedef struct BdrvAttachChildCommonState { >> + BdrvChild **child; >> + AioContext *old_parent_ctx; >> + AioContext *old_child_ctx; >> +} BdrvAttachChildCommonState; >> + >> +static void bdrv_attach_child_common_abort(void *opaque) >> +{ >> + BdrvAttachChildCommonState *s = opaque; >> + BdrvChild *child = *s->child; >> + BlockDriverState *bs = child->bs; >> + >> + bdrv_replace_child_noperm(child, NULL); >> + >> + if (bdrv_get_aio_context(bs) != s->old_child_ctx) { >> + bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); > > Would failure actually be fatal? I think we can ignore it, the node is > in an AioContext that works for it. As far as I explored the code, check-aio-context is transparent enough, nothing rely on IO, etc, and if we succeeded to change it we must success in revert. And as I understand it is critical: if we failed to rollback aio-context change somewhere (but succeeded in reverting graph relation change), it means that we end up with different aio contexts inside one block subtree.. > >> + } >> + >> + if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { >> + bdrv_parent_try_set_aio_context(child, s->old_parent_ctx, >> + &error_abort); > > And the same here. > >> + } >> + >> + bdrv_unref(bs); >> + bdrv_remove_empty_child(child); >> + *s->child = NULL; >> +} >> + >> +static TransactionActionDrv bdrv_attach_child_common_drv = { >> + .abort = bdrv_attach_child_common_abort, >> +}; >> + >> +/* >> + * Common part of attoching bdrv child to bs or to blk or to job >> + */ >> +static int bdrv_attach_child_common(BlockDriverState *child_bs, >> + const char *child_name, >> + const BdrvChildClass *child_class, >> + BdrvChildRole child_role, >> + uint64_t perm, uint64_t shared_perm, >> + void *opaque, BdrvChild **child, >> + GSList **tran, Error **errp) >> +{ >> + int ret; >> + BdrvChild *new_child; >> + AioContext *parent_ctx; >> + AioContext *child_ctx = bdrv_get_aio_context(child_bs); >> + >> + assert(child); >> + assert(*child == NULL); >> + >> + new_child = g_new(BdrvChild, 1); >> + *new_child = (BdrvChild) { >> + .bs = NULL, >> + .name = g_strdup(child_name), >> + .klass = child_class, >> + .role = child_role, >> + .perm = perm, >> + .shared_perm = shared_perm, >> + .opaque = opaque, >> + }; >> + >> + parent_ctx = bdrv_child_get_parent_aio_context(new_child); >> + if (child_ctx != parent_ctx) { >> + ret = bdrv_try_set_aio_context(child_bs, parent_ctx, NULL); >> + if (ret < 0) { >> + /* >> + * bdrv_try_set_aio_context_tran don't need rollback after failure, >> + * so we don't care. >> + */ >> + ret = bdrv_parent_try_set_aio_context(new_child, child_ctx, errp); >> + } >> + if (ret < 0) { >> + bdrv_remove_empty_child(new_child); >> + return ret; >> + } >> + } > > Not sure why you decided to rewrite this block while moving it from > bdrv_root_attach_child(). > > We're losing the comment above it, and a possible error message is now > related to changing the context of the parent node instead of the newly > added node, which I imagine is less obvious in the general case. Don't remember:( Will try to revert, and if find that it's really needed, will leave some good comment on it. > >> + bdrv_ref(child_bs); >> + bdrv_replace_child_noperm(new_child, child_bs); >> + >> + *child = new_child; >> + >> + BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1); >> + *s = (BdrvAttachChildCommonState) { >> + .child = child, >> + .old_parent_ctx = parent_ctx, >> + .old_child_ctx = child_ctx, >> + }; >> + tran_prepend(tran, &bdrv_attach_child_common_drv, s); >> + >> + return 0; >> +} > > Kevin >
Am 04.02.2021 um 08:34 hat Vladimir Sementsov-Ogievskiy geschrieben: > 04.02.2021 00:01, Kevin Wolf wrote: > > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Split out no-perm part of bdrv_root_attach_child() into separate > > > transaction action. bdrv_root_attach_child() now moves to new > > > permission update paradigm: first update graph relations then update > > > permissions. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > +static void bdrv_attach_child_common_abort(void *opaque) > > > +{ > > > + BdrvAttachChildCommonState *s = opaque; > > > + BdrvChild *child = *s->child; > > > + BlockDriverState *bs = child->bs; > > > + > > > + bdrv_replace_child_noperm(child, NULL); > > > + > > > + if (bdrv_get_aio_context(bs) != s->old_child_ctx) { > > > + bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); > > > > Would failure actually be fatal? I think we can ignore it, the node is > > in an AioContext that works for it. > > As far as I explored the code, check-aio-context is transparent > enough, nothing rely on IO, etc, and if we succeeded to change it we > must success in revert. > > And as I understand it is critical: if we failed to rollback > aio-context change somewhere (but succeeded in reverting graph > relation change), it means that we end up with different aio contexts > inside one block subtree.. Ah, right, we're going to change the graph once again, so what is working now doesn't have to be working for the changed graph. Ok, let's leave this as &error_abort. Kevin
diff --git a/block.c b/block.c index f0fcd75555..a7ccbb4fb1 100644 --- a/block.c +++ b/block.c @@ -86,6 +86,13 @@ static void bdrv_parent_set_aio_context_ignore(BdrvChild *c, AioContext *ctx, GSList **ignore); static void bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs); +static int bdrv_attach_child_common(BlockDriverState *child_bs, + const char *child_name, + const BdrvChildClass *child_class, + BdrvChildRole child_role, + uint64_t perm, uint64_t shared_perm, + void *opaque, BdrvChild **child, + GSList **tran, Error **errp); static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp); @@ -2898,55 +2905,22 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, uint64_t perm, uint64_t shared_perm, void *opaque, Error **errp) { - BdrvChild *child; - Error *local_err = NULL; int ret; - AioContext *ctx; + BdrvChild *child = NULL; + GSList *tran = NULL; - ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp); + ret = bdrv_attach_child_common(child_bs, child_name, child_class, + child_role, perm, shared_perm, opaque, + &child, &tran, errp); if (ret < 0) { - bdrv_abort_perm_update(child_bs); bdrv_unref(child_bs); return NULL; } - child = g_new(BdrvChild, 1); - *child = (BdrvChild) { - .bs = NULL, - .name = g_strdup(child_name), - .klass = child_class, - .role = child_role, - .perm = perm, - .shared_perm = shared_perm, - .opaque = opaque, - }; - - ctx = bdrv_child_get_parent_aio_context(child); - - /* If the AioContexts don't match, first try to move the subtree of - * child_bs into the AioContext of the new parent. If this doesn't work, - * try moving the parent into the AioContext of child_bs instead. */ - if (bdrv_get_aio_context(child_bs) != ctx) { - ret = bdrv_try_set_aio_context(child_bs, ctx, &local_err); - if (ret < 0) { - if (bdrv_parent_try_set_aio_context(child, ctx, NULL) == 0) { - ret = 0; - error_free(local_err); - local_err = NULL; - } - } - if (ret < 0) { - error_propagate(errp, local_err); - g_free(child); - bdrv_abort_perm_update(child_bs); - bdrv_unref(child_bs); - return NULL; - } - } - - /* This performs the matching bdrv_set_perm() for the above check. */ - bdrv_replace_child(child, child_bs); + ret = bdrv_refresh_perms(child_bs, errp); + tran_finalize(tran, ret); + bdrv_unref(child_bs); return child; } @@ -2988,16 +2962,114 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, return child; } -static void bdrv_detach_child(BdrvChild *child) +static void bdrv_remove_empty_child(BdrvChild *child) { + assert(!child->bs); QLIST_SAFE_REMOVE(child, next); - - bdrv_replace_child(child, NULL); - g_free(child->name); g_free(child); } +typedef struct BdrvAttachChildCommonState { + BdrvChild **child; + AioContext *old_parent_ctx; + AioContext *old_child_ctx; +} BdrvAttachChildCommonState; + +static void bdrv_attach_child_common_abort(void *opaque) +{ + BdrvAttachChildCommonState *s = opaque; + BdrvChild *child = *s->child; + BlockDriverState *bs = child->bs; + + bdrv_replace_child_noperm(child, NULL); + + if (bdrv_get_aio_context(bs) != s->old_child_ctx) { + bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); + } + + if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { + bdrv_parent_try_set_aio_context(child, s->old_parent_ctx, + &error_abort); + } + + bdrv_unref(bs); + bdrv_remove_empty_child(child); + *s->child = NULL; +} + +static TransactionActionDrv bdrv_attach_child_common_drv = { + .abort = bdrv_attach_child_common_abort, +}; + +/* + * Common part of attoching bdrv child to bs or to blk or to job + */ +static int bdrv_attach_child_common(BlockDriverState *child_bs, + const char *child_name, + const BdrvChildClass *child_class, + BdrvChildRole child_role, + uint64_t perm, uint64_t shared_perm, + void *opaque, BdrvChild **child, + GSList **tran, Error **errp) +{ + int ret; + BdrvChild *new_child; + AioContext *parent_ctx; + AioContext *child_ctx = bdrv_get_aio_context(child_bs); + + assert(child); + assert(*child == NULL); + + new_child = g_new(BdrvChild, 1); + *new_child = (BdrvChild) { + .bs = NULL, + .name = g_strdup(child_name), + .klass = child_class, + .role = child_role, + .perm = perm, + .shared_perm = shared_perm, + .opaque = opaque, + }; + + parent_ctx = bdrv_child_get_parent_aio_context(new_child); + if (child_ctx != parent_ctx) { + ret = bdrv_try_set_aio_context(child_bs, parent_ctx, NULL); + if (ret < 0) { + /* + * bdrv_try_set_aio_context_tran don't need rollback after failure, + * so we don't care. + */ + ret = bdrv_parent_try_set_aio_context(new_child, child_ctx, errp); + } + if (ret < 0) { + bdrv_remove_empty_child(new_child); + return ret; + } + } + + bdrv_ref(child_bs); + bdrv_replace_child_noperm(new_child, child_bs); + + *child = new_child; + + BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1); + *s = (BdrvAttachChildCommonState) { + .child = child, + .old_parent_ctx = parent_ctx, + .old_child_ctx = child_ctx, + }; + tran_prepend(tran, &bdrv_attach_child_common_drv, s); + + return 0; +} + +static void bdrv_detach_child(BdrvChild *child) +{ + bdrv_replace_child(child, NULL); + bdrv_remove_empty_child(child); +} + /* Callers must ensure that child->frozen is false. */ void bdrv_root_unref_child(BdrvChild *child) {
Split out no-perm part of bdrv_root_attach_child() into separate transaction action. bdrv_root_attach_child() now moves to new permission update paradigm: first update graph relations then update permissions. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block.c | 162 ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 117 insertions(+), 45 deletions(-)