Re: [PATCH v2 8/8] block: do not take AioContext around reopen

[Paolo Bonzini]

On 19/04/21 10:55, Emanuele Giuseppe Esposito wrote:
> Reopen needs to handle AioContext carefully due to calling
> bdrv_drain_all_begin/end.  By not taking AioContext around calls to
> bdrv_reopen_multiple, we can drop the function's release/acquire
> pair and the AioContext argument too.

So... I wrote this commit message and I cannot parse it anymore---much 
less relate it to the code in the patch.  This is a problem, but it 
doesn't mean that the patch is wrong.

bdrv_reopen_multiple does not have the AioContext argument anymore. 
It's not doing release/acquire either.  The relevant commit is commit 
1a63a90750 ("block: Keep nodes drained between reopen_queue/multiple", 
2017-12-22).  You're basically cleaning up after that code in the same 
way as patch 7: reopen functions take care of keeping the BDS quiescent, 
so there's nothing to synchronize on.

For the future, the important step you missed was to check your diff 
against the one that you cherry-picked from.  Then you would have 
noticed that 1) it's much smaller 2) one thing that is mentioned in the 
commit message ("drop the function's release/acquire pair and argument") 
is not needed anymore.

Paolo

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-backend.c |  4 ----
>   block/mirror.c        |  9 ---------
>   blockdev.c            | 19 ++++++-------------
>   3 files changed, 6 insertions(+), 26 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 413af51f3b..6fdc698e9e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2291,20 +2291,16 @@ int blk_commit_all(void)
>       BlockBackend *blk = NULL;
>   
>       while ((blk = blk_all_next(blk)) != NULL) {
> -        AioContext *aio_context = blk_get_aio_context(blk);
>           BlockDriverState *unfiltered_bs = bdrv_skip_filters(blk_bs(blk));
>   
> -        aio_context_acquire(aio_context);
>           if (blk_is_inserted(blk) && bdrv_cow_child(unfiltered_bs)) {
>               int ret;
>   
>               ret = bdrv_commit(unfiltered_bs);
>               if (ret < 0) {
> -                aio_context_release(aio_context);
>                   return ret;
>               }
>           }
> -        aio_context_release(aio_context);
>       }
>       return 0;
>   }
> diff --git a/block/mirror.c b/block/mirror.c
> index 5a71bd8bbc..43174bbc6b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -631,7 +631,6 @@ static int mirror_exit_common(Job *job)
>       MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>       BlockJob *bjob = &s->common;
>       MirrorBDSOpaque *bs_opaque;
> -    AioContext *replace_aio_context = NULL;
>       BlockDriverState *src;
>       BlockDriverState *target_bs;
>       BlockDriverState *mirror_top_bs;
> @@ -699,11 +698,6 @@ static int mirror_exit_common(Job *job)
>           }
>       }
>   
> -    if (s->to_replace) {
> -        replace_aio_context = bdrv_get_aio_context(s->to_replace);
> -        aio_context_acquire(replace_aio_context);
> -    }
> -
>       if (s->should_complete && !abort) {
>           BlockDriverState *to_replace = s->to_replace ?: src;
>           bool ro = bdrv_is_read_only(to_replace);
> @@ -740,9 +734,6 @@ static int mirror_exit_common(Job *job)
>           error_free(s->replace_blocker);
>           bdrv_unref(s->to_replace);
>       }
> -    if (replace_aio_context) {
> -        aio_context_release(replace_aio_context);
> -    }
>       g_free(s->replaces);
>       bdrv_unref(target_bs);
>   
> diff --git a/blockdev.c b/blockdev.c
> index e901107344..1672ef756e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3469,7 +3469,6 @@ void qmp_change_backing_file(const char *device,
>                                Error **errp)
>   {
>       BlockDriverState *bs = NULL;
> -    AioContext *aio_context;
>       BlockDriverState *image_bs = NULL;
>       Error *local_err = NULL;
>       bool ro;
> @@ -3480,37 +3479,34 @@ void qmp_change_backing_file(const char *device,
>           return;
>       }
>   
> -    aio_context = bdrv_get_aio_context(bs);
> -    aio_context_acquire(aio_context);
> -
>       image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        goto out;
> +        return;
>       }
>   
>       if (!image_bs) {
>           error_setg(errp, "image file not found");
> -        goto out;
> +        return;
>       }
>   
>       if (bdrv_find_base(image_bs) == image_bs) {
>           error_setg(errp, "not allowing backing file change on an image "
>                            "without a backing file");
> -        goto out;
> +        return;
>       }
>   
>       /* even though we are not necessarily operating on bs, we need it to
>        * determine if block ops are currently prohibited on the chain */
>       if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
> -        goto out;
> +        return;
>       }
>   
>       /* final sanity check */
>       if (!bdrv_chain_contains(bs, image_bs)) {
>           error_setg(errp, "'%s' and image file are not in the same chain",
>                      device);
> -        goto out;
> +        return;
>       }
>   
>       /* if not r/w, reopen to make r/w */
> @@ -3518,7 +3514,7 @@ void qmp_change_backing_file(const char *device,
>   
>       if (ro) {
>           if (bdrv_reopen_set_read_only(image_bs, false, errp) != 0) {
> -            goto out;
> +            return;
>           }
>       }
>   
> @@ -3536,9 +3532,6 @@ void qmp_change_backing_file(const char *device,
>       if (ro) {
>           bdrv_reopen_set_read_only(image_bs, true, errp);
>       }
> -
> -out:
> -    aio_context_release(aio_context);
>   }
>   
>   void qmp_blockdev_add(BlockdevOptions *options, Error **errp)