| Message ID | 20210317143529.615584-19-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: update graph permissions update | expand |
Am 17.03.2021 um 15:35 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 | 189 ++++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 135 insertions(+), 54 deletions(-) > > diff --git a/block.c b/block.c > index 98ff44dbf7..b6bdc534d2 100644 > --- a/block.c > +++ b/block.c > @@ -2921,37 +2921,73 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) > } > } > > -/* > - * This function steals the reference to child_bs from the caller. > - * That reference is later dropped by bdrv_root_unref_child(). > - * > - * On failure NULL is returned, errp is set and the reference to > - * child_bs is also dropped. > - * > - * The caller must hold the AioContext lock @child_bs, but not that of @ctx > - * (unless @child_bs is already in @ctx). > - */ > -BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, > - const char *child_name, > - const BdrvChildClass *child_class, > - BdrvChildRole child_role, > - uint64_t perm, uint64_t shared_perm, > - void *opaque, Error **errp) > +static void bdrv_remove_empty_child(BdrvChild *child) > { > - BdrvChild *child; > - Error *local_err = NULL; > - int ret; > - AioContext *ctx; > + assert(!child->bs); > + QLIST_SAFE_REMOVE(child, next); > + g_free(child->name); > + g_free(child); > +} > > - ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp); > - if (ret < 0) { > - bdrv_abort_perm_update(child_bs); > - bdrv_unref(child_bs); > - return NULL; > +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); > } > > - child = g_new(BdrvChild, 1); > - *child = (BdrvChild) { > + if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { > + GSList *ignore = g_slist_prepend(NULL, child); > + > + child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore, > + &error_abort); > + g_slist_free(ignore); > + ignore = g_slist_prepend(NULL, child); > + child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); > + > + g_slist_free(ignore); > + } > + > + 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, > + Transaction *tran, Error **errp) > +{ > + 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, > @@ -2961,37 +2997,92 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, > .opaque = opaque, > }; > > - ctx = bdrv_child_get_parent_aio_context(child); > - > - /* If the AioContexts don't match, first try to move the subtree of > + /* > + * 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); > + * try moving the parent into the AioContext of child_bs instead. > + */ > + parent_ctx = bdrv_child_get_parent_aio_context(new_child); > + if (child_ctx != parent_ctx) { > + 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) { > - GSList *ignore = g_slist_prepend(NULL, child); > - ctx = bdrv_get_aio_context(child_bs); > - if (child_class->can_set_aio_ctx(child, ctx, &ignore, NULL)) { > + GSList *ignore = g_slist_prepend(NULL, new_child); > + if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore, > + NULL)) > + { > error_free(local_err); > ret = 0; > g_slist_free(ignore); > - ignore = g_slist_prepend(NULL, child); > - child_class->set_aio_ctx(child, ctx, &ignore); > + ignore = g_slist_prepend(NULL, new_child); > + child_class->set_aio_ctx(new_child, child_ctx, &ignore); > } > g_slist_free(ignore); > } > + > if (ret < 0) { > error_propagate(errp, local_err); > - g_free(child); > - bdrv_abort_perm_update(child_bs); > - bdrv_unref(child_bs); > - return NULL; > + bdrv_remove_empty_child(new_child); > + return ret; > } > } > > - /* This performs the matching bdrv_set_perm() for the above check. */ > - bdrv_replace_child(child, child_bs); > + 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_add(tran, &bdrv_attach_child_common_drv, s); Who frees s? Should bdrv_attach_child_common_drv have a .clean? > + > + return 0; > +} Kevin
26.04.2021 19:14, Kevin Wolf wrote: > Am 17.03.2021 um 15:35 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 | 189 ++++++++++++++++++++++++++++++++++++++++---------------- >> 1 file changed, 135 insertions(+), 54 deletions(-) >> >> diff --git a/block.c b/block.c >> index 98ff44dbf7..b6bdc534d2 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2921,37 +2921,73 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) >> } >> } >> >> -/* >> - * This function steals the reference to child_bs from the caller. >> - * That reference is later dropped by bdrv_root_unref_child(). >> - * >> - * On failure NULL is returned, errp is set and the reference to >> - * child_bs is also dropped. >> - * >> - * The caller must hold the AioContext lock @child_bs, but not that of @ctx >> - * (unless @child_bs is already in @ctx). >> - */ >> -BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, >> - const char *child_name, >> - const BdrvChildClass *child_class, >> - BdrvChildRole child_role, >> - uint64_t perm, uint64_t shared_perm, >> - void *opaque, Error **errp) >> +static void bdrv_remove_empty_child(BdrvChild *child) >> { >> - BdrvChild *child; >> - Error *local_err = NULL; >> - int ret; >> - AioContext *ctx; >> + assert(!child->bs); >> + QLIST_SAFE_REMOVE(child, next); >> + g_free(child->name); >> + g_free(child); >> +} >> >> - ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp); >> - if (ret < 0) { >> - bdrv_abort_perm_update(child_bs); >> - bdrv_unref(child_bs); >> - return NULL; >> +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); >> } >> >> - child = g_new(BdrvChild, 1); >> - *child = (BdrvChild) { >> + if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { >> + GSList *ignore = g_slist_prepend(NULL, child); >> + >> + child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore, >> + &error_abort); >> + g_slist_free(ignore); >> + ignore = g_slist_prepend(NULL, child); >> + child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); >> + >> + g_slist_free(ignore); >> + } >> + >> + 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, >> + Transaction *tran, Error **errp) >> +{ >> + 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, >> @@ -2961,37 +2997,92 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, >> .opaque = opaque, >> }; >> >> - ctx = bdrv_child_get_parent_aio_context(child); >> - >> - /* If the AioContexts don't match, first try to move the subtree of >> + /* >> + * 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); >> + * try moving the parent into the AioContext of child_bs instead. >> + */ >> + parent_ctx = bdrv_child_get_parent_aio_context(new_child); >> + if (child_ctx != parent_ctx) { >> + 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) { >> - GSList *ignore = g_slist_prepend(NULL, child); >> - ctx = bdrv_get_aio_context(child_bs); >> - if (child_class->can_set_aio_ctx(child, ctx, &ignore, NULL)) { >> + GSList *ignore = g_slist_prepend(NULL, new_child); >> + if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore, >> + NULL)) >> + { >> error_free(local_err); >> ret = 0; >> g_slist_free(ignore); >> - ignore = g_slist_prepend(NULL, child); >> - child_class->set_aio_ctx(child, ctx, &ignore); >> + ignore = g_slist_prepend(NULL, new_child); >> + child_class->set_aio_ctx(new_child, child_ctx, &ignore); >> } >> g_slist_free(ignore); >> } >> + >> if (ret < 0) { >> error_propagate(errp, local_err); >> - g_free(child); >> - bdrv_abort_perm_update(child_bs); >> - bdrv_unref(child_bs); >> - return NULL; >> + bdrv_remove_empty_child(new_child); >> + return ret; >> } >> } >> >> - /* This performs the matching bdrv_set_perm() for the above check. */ >> - bdrv_replace_child(child, child_bs); >> + 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_add(tran, &bdrv_attach_child_common_drv, s); > > Who frees s? Should bdrv_attach_child_common_drv have a .clean? > Hmm, yes, looks like ".clean = g_free" missed.
diff --git a/block.c b/block.c index 98ff44dbf7..b6bdc534d2 100644 --- a/block.c +++ b/block.c @@ -2921,37 +2921,73 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) } } -/* - * This function steals the reference to child_bs from the caller. - * That reference is later dropped by bdrv_root_unref_child(). - * - * On failure NULL is returned, errp is set and the reference to - * child_bs is also dropped. - * - * The caller must hold the AioContext lock @child_bs, but not that of @ctx - * (unless @child_bs is already in @ctx). - */ -BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, - const char *child_name, - const BdrvChildClass *child_class, - BdrvChildRole child_role, - uint64_t perm, uint64_t shared_perm, - void *opaque, Error **errp) +static void bdrv_remove_empty_child(BdrvChild *child) { - BdrvChild *child; - Error *local_err = NULL; - int ret; - AioContext *ctx; + assert(!child->bs); + QLIST_SAFE_REMOVE(child, next); + g_free(child->name); + g_free(child); +} - ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp); - if (ret < 0) { - bdrv_abort_perm_update(child_bs); - bdrv_unref(child_bs); - return NULL; +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); } - child = g_new(BdrvChild, 1); - *child = (BdrvChild) { + if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { + GSList *ignore = g_slist_prepend(NULL, child); + + child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore, + &error_abort); + g_slist_free(ignore); + ignore = g_slist_prepend(NULL, child); + child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); + + g_slist_free(ignore); + } + + 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, + Transaction *tran, Error **errp) +{ + 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, @@ -2961,37 +2997,92 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, .opaque = opaque, }; - ctx = bdrv_child_get_parent_aio_context(child); - - /* If the AioContexts don't match, first try to move the subtree of + /* + * 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); + * try moving the parent into the AioContext of child_bs instead. + */ + parent_ctx = bdrv_child_get_parent_aio_context(new_child); + if (child_ctx != parent_ctx) { + 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) { - GSList *ignore = g_slist_prepend(NULL, child); - ctx = bdrv_get_aio_context(child_bs); - if (child_class->can_set_aio_ctx(child, ctx, &ignore, NULL)) { + GSList *ignore = g_slist_prepend(NULL, new_child); + if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore, + NULL)) + { error_free(local_err); ret = 0; g_slist_free(ignore); - ignore = g_slist_prepend(NULL, child); - child_class->set_aio_ctx(child, ctx, &ignore); + ignore = g_slist_prepend(NULL, new_child); + child_class->set_aio_ctx(new_child, child_ctx, &ignore); } g_slist_free(ignore); } + if (ret < 0) { error_propagate(errp, local_err); - g_free(child); - bdrv_abort_perm_update(child_bs); - bdrv_unref(child_bs); - return NULL; + bdrv_remove_empty_child(new_child); + return ret; } } - /* This performs the matching bdrv_set_perm() for the above check. */ - bdrv_replace_child(child, child_bs); + 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_add(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); +} + +/* + * This function steals the reference to child_bs from the caller. + * That reference is later dropped by bdrv_root_unref_child(). + * + * On failure NULL is returned, errp is set and the reference to + * child_bs is also dropped. + * + * The caller must hold the AioContext lock @child_bs, but not that of @ctx + * (unless @child_bs is already in @ctx). + */ +BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, + const char *child_name, + const BdrvChildClass *child_class, + BdrvChildRole child_role, + uint64_t perm, uint64_t shared_perm, + void *opaque, Error **errp) +{ + int ret; + BdrvChild *child = NULL; + Transaction *tran = tran_new(); + + ret = bdrv_attach_child_common(child_bs, child_name, child_class, + child_role, perm, shared_perm, opaque, + &child, tran, errp); + if (ret < 0) { + bdrv_unref(child_bs); + return NULL; + } + + ret = bdrv_refresh_perms(child_bs, errp); + tran_finalize(tran, ret); + + bdrv_unref(child_bs); return child; } @@ -3033,16 +3124,6 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, return child; } -static void bdrv_detach_child(BdrvChild *child) -{ - QLIST_SAFE_REMOVE(child, next); - - bdrv_replace_child(child, NULL); - - g_free(child->name); - g_free(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 | 189 ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 135 insertions(+), 54 deletions(-)