From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yt5Ru-00034w-9X for qemu-devel@nongnu.org; Thu, 14 May 2015 22:34:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yt5Rq-0002H1-RF for qemu-devel@nongnu.org; Thu, 14 May 2015 22:34:06 -0400 Date: Fri, 15 May 2015 10:33:19 +0800 From: Fam Zheng Message-ID: <20150515023319.GF28144@ad.nay.redhat.com> References: <6957969aef43d7f379807c7646c7aeae3ce1d208.1431523498.git.berto@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6957969aef43d7f379807c7646c7aeae3ce1d208.1431523498.git.berto@igalia.com> Subject: Re: [Qemu-devel] [PATCH 04/11] block: Support streaming to an intermediate layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org On Wed, 05/13 16:27, Alberto Garcia wrote: > This makes sure that the image we are steaming into is open in > read-write mode during the operation. > > Operation blockers are also set in all intermediate nodes, since they > will be removed from the chain afterwards. > > Finally, this also unblocks the stream operation in backing files. > > Signed-off-by: Alberto Garcia > Reviewed-by: Max Reitz > --- > block.c | 4 +++- > block/stream.c | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 7904098..8fab7e4 100644 > --- a/block.c > +++ b/block.c > @@ -1044,9 +1044,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > backing_hd->drv ? backing_hd->drv->format_name : ""); > > bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); > - /* Otherwise we won't be able to commit due to check in bdrv_commit */ > + /* Otherwise we won't be able to commit or stream */ > bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET, > bs->backing_blocker); > + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_STREAM, > + bs->backing_blocker); > out: > bdrv_refresh_limits(bs, NULL); > } > diff --git a/block/stream.c b/block/stream.c > index 37bfd8b..8ba5a24 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -33,6 +33,8 @@ typedef struct StreamBlockJob { > BlockDriverState *base; > BlockdevOnError on_error; > char *backing_file_str; > + int bs_flags; > + Error *blocker; > } StreamBlockJob; > > static int coroutine_fn stream_populate(BlockDriverState *bs, > @@ -89,6 +91,13 @@ static void stream_complete(BlockJob *job, void *opaque) > StreamBlockJob *s = container_of(job, StreamBlockJob, common); > StreamCompleteData *data = opaque; > BlockDriverState *base = s->base; > + BlockDriverState *bs; > + > + /* Remove all blockers set in stream_start() */ > + for (bs = job->bs->backing_hd; bs && bs != s->base; bs = bs->backing_hd) { > + bdrv_op_unblock_all(bs, s->blocker); > + } > + error_free(s->blocker); > > if (!block_job_is_cancelled(&s->common) && data->reached_end && > data->ret == 0) { > @@ -103,6 +112,11 @@ static void stream_complete(BlockJob *job, void *opaque) > close_unused_images(job->bs, base, base_id); > } > > + /* Reopen the image back in read-only mode if necessary */ > + if (s->bs_flags != bdrv_get_flags(job->bs)) { > + bdrv_reopen(job->bs, s->bs_flags, NULL); > + } > + > g_free(s->backing_file_str); > block_job_completed(&s->common, data->ret); > g_free(data); > @@ -246,7 +260,9 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, > BlockCompletionFunc *cb, > void *opaque, Error **errp) > { > + BlockDriverState *iter; > StreamBlockJob *s; > + int orig_bs_flags; > > if ((on_error == BLOCKDEV_ON_ERROR_STOP || > on_error == BLOCKDEV_ON_ERROR_ENOSPC) && > @@ -255,13 +271,30 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, > return; > } > > + /* Make sure that the image is opened in read-write mode */ > + orig_bs_flags = bdrv_get_flags(bs); > + if (!(orig_bs_flags & BDRV_O_RDWR)) { > + if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) { I don't think all bdrv_reopen implementation will guarantee to set errp on error :(. For example: static int qcow2_reopen_prepare(BDRVReopenState *state, BlockReopenQueue *queue, Error **errp) { int ret; if ((state->flags & BDRV_O_RDWR) == 0) { ret = bdrv_flush(state->bs); if (ret < 0) { return ret; } ret = qcow2_mark_clean(state->bs); if (ret < 0) { return ret; } } return 0; } Maybe we should set one here so the user sees the failure? Fam > + return; > + } > + } > + > s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp); > if (!s) { > return; > } > > + /* Block all intermediate nodes between bs and base, because they > + * will disappear from the chain after this operation */ > + error_setg(&s->blocker, "blocked by the block-stream operation in '%s'", > + bdrv_get_device_or_node_name(bs)); > + for (iter = bs->backing_hd; iter && iter != base; iter = iter->backing_hd) { > + bdrv_op_block_all(iter, s->blocker); > + } > + > s->base = base; > s->backing_file_str = g_strdup(backing_file_str); > + s->bs_flags = orig_bs_flags; > > s->on_error = on_error; > s->common.co = qemu_coroutine_create(stream_run); > -- > 2.1.4 > >