From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP4bS-0005H8-Bm for qemu-devel@nongnu.org; Fri, 29 Jan 2016 03:40:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aP4bN-0004T0-6G for qemu-devel@nongnu.org; Fri, 29 Jan 2016 03:40:26 -0500 Received: from mx2.parallels.com ([199.115.105.18]:33585) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP4bM-0004RM-TV for qemu-devel@nongnu.org; Fri, 29 Jan 2016 03:40:21 -0500 References: <1453273940-15382-1-git-send-email-den@openvz.org> <1453273940-15382-2-git-send-email-den@openvz.org> From: "Denis V. Lunev" Message-ID: <56AB255E.5080200@openvz.org> Date: Fri, 29 Jan 2016 11:39:58 +0300 MIME-Version: 1.0 In-Reply-To: <1453273940-15382-2-git-send-email-den@openvz.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] block: fix assert in qcow2_get_specific_info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.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 > CC: Kevin Wolf > CC: Paolo Bonzini > --- > 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