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

[Kevin Wolf]

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).

Kevin

>                  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
>