Re: [PATCH v2 20/36] block: add bdrv_attach_child_common() transaction action
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
> > > +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, _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 _abort.
Kevin
Re: [PATCH v2 20/36] block: add bdrv_attach_child_common() transaction action
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
---
block.c | 162
1 file changed, 117 insertions(+), 45 deletions(-)
diff --git a/block.c b/block.c
index f0fcd7..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,
+ , , 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, _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, _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,
Re: [PATCH v2 20/36] block: add bdrv_attach_child_common() transaction action
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
> ---
> block.c | 162
> 1 file changed, 117 insertions(+), 45 deletions(-)
>
> diff --git a/block.c b/block.c
> index f0fcd7..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,
> + , , 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, _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,
[PATCH v2 20/36] block: add bdrv_attach_child_common() transaction action
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
---
block.c | 162
1 file changed, 117 insertions(+), 45 deletions(-)
diff --git a/block.c b/block.c
index f0fcd7..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,
+ , , 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, _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, _abort);
+}
+
+if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
+bdrv_parent_try_set_aio_context(child, s->old_parent_ctx,
+_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
