Re: [PATCH v2 20/36] block: add bdrv_attach_child_common() transaction action

[Vladimir Sementsov-Ogievskiy]

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


--
Best regards,
Vladimir