Re: [PATCH v2 05/36] block: add bdrv_parent_try_set_aio_context

[Vladimir Sementsov-Ogievskiy]

18.01.2021 18:08, Kevin Wolf wrote:
> Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > We already have bdrv_parent_can_set_aio_context(). Add corresponding
> > bdrv_parent_set_aio_context_ignore() and
> > bdrv_parent_try_set_aio_context() and use them instead of open-coding.
> >
> > Make bdrv_parent_try_set_aio_context() public, as it will be used in
> > further commit.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   include/block/block.h |  2 ++
> >   block.c               | 51 +++++++++++++++++++++++++++++++++----------
> >   2 files changed, 41 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/block/block.h b/include/block/block.h
> > index ee3f5a6cca..550c5a7513 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -686,6 +686,8 @@ bool bdrv_child_can_set_aio_context(BdrvChild *c, 
> > AioContext *ctx,
> >                                       GSList **ignore, Error **errp);
> >   bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
> >                                 GSList **ignore, Error **errp);
> > +int bdrv_parent_try_set_aio_context(BdrvChild *c, AioContext *ctx,
> > +                                    Error **errp);
> >   int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
> >   int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
> >   
> > diff --git a/block.c b/block.c
> > index 916087ee1a..5d925c208d 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -81,6 +81,9 @@ static BlockDriverState *bdrv_open_inherit(const char 
> > *filename,
> >                                              BdrvChildRole child_role,
> >                                              Error **errp);
> >   
> > +static void bdrv_parent_set_aio_context_ignore(BdrvChild *c, AioContext *ctx,
> > +                                               GSList **ignore);
> > +
> >   /* If non-zero, use only whitelisted block drivers */
> >   static int use_bdrv_whitelist;
> >   
> > @@ -2655,17 +2658,12 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
> >        * 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 && child_class->can_set_aio_ctx) {
> > -            GSList *ignore = g_slist_prepend(NULL, child);
> > -            ctx = bdrv_get_aio_context(child_bs);
>
> You are losing this line...
>
> > -            if (child_class->can_set_aio_ctx(child, ctx, &ignore, NULL)) {
> > -                error_free(local_err);
> > +        if (ret < 0) {
> > +            if (bdrv_parent_try_set_aio_context(child, ctx, NULL) == 0) {
>
> ...before this one, so I think the wrong context is passed now. Instead
> of trying to move the parent to the AioContext of the child, we'll try
> to move it to the AioContext in which it already is (and which doesn't
> match the AioContext of the child).

Oops, right, will fix

> >                   ret = 0;
> > -                g_slist_free(ignore);
> > -                ignore = g_slist_prepend(NULL, child);
> > -                child_class->set_aio_ctx(child, ctx, &ignore);
> > +                error_free(local_err);
> > +                local_err = NULL;
> >               }
> > -            g_slist_free(ignore);
> >           }
> >           if (ret < 0) {
> >               error_propagate(errp, local_err);
> > @@ -6452,9 +6450,7 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
> >           if (g_slist_find(*ignore, child)) {
> >               continue;
> >           }
> > -        assert(child->klass->set_aio_ctx);
> > -        *ignore = g_slist_prepend(*ignore, child);
> > -        child->klass->set_aio_ctx(child, new_context, ignore);
> > +        bdrv_parent_set_aio_context_ignore(child, new_context, ignore);
> >       }
> >   
> >       bdrv_detach_aio_context(bs);
> > @@ -6511,6 +6507,37 @@ static bool bdrv_parent_can_set_aio_context(BdrvChild 
> > *c, AioContext *ctx,
> >       return true;
> >   }
> >   
> > +static void bdrv_parent_set_aio_context_ignore(BdrvChild *c, AioContext *ctx,
> > +                                               GSList **ignore)
> > +{
> > +    if (g_slist_find(*ignore, c)) {
> > +        return;
> > +    }
> > +    *ignore = g_slist_prepend(*ignore, c);
> > +
> > +    assert(c->klass->set_aio_ctx);
> > +    c->klass->set_aio_ctx(c, ctx, ignore);
> > +}
> > +
> > +int bdrv_parent_try_set_aio_context(BdrvChild *c, AioContext *ctx,
> > +                                    Error **errp)
> > +{
> > +    GSList *ignore = NULL;
> > +
> > +    if (!bdrv_parent_can_set_aio_context(c, ctx, &ignore, errp)) {
> > +        g_slist_free(ignore);
> > +        return -EPERM;
> > +    }
> > +
> > +    g_slist_free(ignore);
> > +    ignore = NULL;
> > +
> > +    bdrv_parent_set_aio_context_ignore(c, ctx, &ignore);
> > +    g_slist_free(ignore);
> > +
> > +    return 0;
> > +}
> > +
> >   bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
> >                                       GSList **ignore, Error **errp)
> >   {
> > --
> > 2.21.3


--
Best regards,
Vladimir