Re: [Qemu-devel] [PATCH 1/2] block: fix assert in qcow2_get_specific_info

["Denis V. Lunev" <den@openvz.org>]

On 01/20/2016 10:12 AM, Denis V. Lunev wrote:
> There is a possibility to hit assert qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
>
> The problem is that in the qcow2_invalidate_cache closes and the image
> and memsets BDRVQcowState in the middle.
>
> The patch moves processing of qcow2_get_specific_info into coroutine
> context and ensures that qcow2_invalidate_cache and qcow2_get_specific_info
> can not run simultaneosly.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/qcow2.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>   block/qcow2.h |  2 ++
>   2 files changed, 61 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1789af4..12eda24 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1740,6 +1740,10 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>       Error *local_err = NULL;
>       int ret;
>   
> +    qemu_co_mutex_lock(&s->lock);
> +    s->in_transient_state = true;
> +    qemu_co_mutex_unlock(&s->lock);
> +
>       /*
>        * Backing files are read-only which makes all of their metadata immutable,
>        * that means we don't have to worry about reopening them here.
> @@ -1753,10 +1757,10 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>       bdrv_invalidate_cache(bs->file->bs, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        return;
> +        goto done;
>       }
>   
> -    memset(s, 0, sizeof(BDRVQcow2State));
> +    memset(s, 0, offsetof(BDRVQcow2State, in_transient_state));
>       options = qdict_clone_shallow(bs->options);
>   
>       ret = qcow2_open(bs, options, flags, &local_err);
> @@ -1765,13 +1769,18 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>           error_setg(errp, "Could not reopen qcow2 layer: %s",
>                      error_get_pretty(local_err));
>           error_free(local_err);
> -        return;
> +        goto done;
>       } else if (ret < 0) {
>           error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
> -        return;
> +        goto done;
>       }
>   
>       s->cipher = cipher;
> +
> +done:
> +    qemu_co_mutex_lock(&s->lock);
> +    s->in_transient_state = false;
> +    qemu_co_mutex_unlock(&s->lock);
>   }
>   
>   static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
> @@ -2778,11 +2787,21 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>       return 0;
>   }
>   
> -static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> +
> +static ImageInfoSpecific *qcow2_co_get_specific_info(BlockDriverState *bs)
>   {
>       BDRVQcow2State *s = bs->opaque;
> +    AioContext *ctx = bdrv_get_aio_context(bs);
> +
>       ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
>   
> +    qemu_co_mutex_lock(&s->lock);
> +    while (s->in_transient_state) {
> +        qemu_co_mutex_unlock(&s->lock);
> +        aio_poll(ctx, true);
> +        qemu_co_mutex_lock(&s->lock);
> +    }
> +
>       *spec_info = (ImageInfoSpecific){
>           .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
>           .u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
> @@ -2808,10 +2827,45 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>            * added without having it covered here */
>           assert(false);
>       }
> +    qemu_co_mutex_unlock(&s->lock);
>   
>       return spec_info;
>   }
>   
> +struct InfoCo {
> +    BlockDriverState *bs;
> +    ImageInfoSpecific *info;
> +};
> +
> +static void qcow2_co_get_specific_info_entry(void *opaque)
> +{
> +    struct InfoCo *ret = opaque;
> +    ret->info = qcow2_co_get_specific_info(ret->bs);
> +}
> +
> +static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> +{
> +    Coroutine *co;
> +    struct InfoCo info_co = {
> +        .bs = bs,
> +    };
> +
> +    if (qemu_in_coroutine()) {
> +        /* Fast-path if already in coroutine context */
> +        qcow2_co_get_specific_info_entry(&info_co);
> +    } else {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        co = qemu_coroutine_create(qcow2_co_get_specific_info_entry);
> +        qemu_coroutine_enter(co, &info_co);
> +        while (info_co.info == NULL) {
> +            aio_poll(aio_context, true);
> +        }
> +    }
> +
> +    return info_co.info;
> +}
> +
>   #if 0
>   static void dump_refcounts(BlockDriverState *bs)
>   {
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a063a3c..1114528 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -293,6 +293,8 @@ typedef struct BDRVQcow2State {
>        * override) */
>       char *image_backing_file;
>       char *image_backing_format;
> +
> +    bool in_transient_state;
>   } BDRVQcow2State;
>   
>   typedef struct Qcow2COWRegion {
ping v2