| Message ID | 3b38569c5caeb92ae2fd6b3c6c7272bcce2dcf85.1459776815.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04.04.2016 15:43, Alberto Garcia wrote: > This makes sure that the image we are steaming into is open in > read-write mode during the operation. > > The block job is created on the destination image, but operation > blockers are also set on the active layer. We do this in order to > prevent other block jobs from running in parallel in the same chain. > See here for details on why that is currently not supported: > > [Qemu-block] RFC: Status of the intermediate block streaming work > https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html > > Finally, this also unblocks the stream operation in backing files. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 4 +++- > block/stream.c | 39 ++++++++++++++++++++++++++++++++++++++- > blockdev.c | 2 +- > include/block/block_int.h | 5 ++++- > 4 files changed, 46 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 48638c9..f7698a1 100644 > --- a/block.c > +++ b/block.c > @@ -1252,9 +1252,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > backing_hd->drv ? backing_hd->drv->format_name : ""); > > bdrv_op_block_all(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(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET, > bs->backing_blocker); > + bdrv_op_unblock(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 332b9a1..ac02ddf 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -35,8 +35,10 @@ typedef struct StreamBlockJob { > BlockJob common; > RateLimit limit; > BlockDriverState *base; > + BlockDriverState *active; > BlockdevOnError on_error; > char *backing_file_str; > + int bs_flags; > } StreamBlockJob; > > static int coroutine_fn stream_populate(BlockDriverState *bs, > @@ -66,6 +68,11 @@ static void stream_complete(BlockJob *job, void *opaque) > StreamCompleteData *data = opaque; > BlockDriverState *base = s->base; > > + if (s->active) { > + bdrv_op_unblock_all(s->active, s->common.blocker); > + bdrv_unref(s->active); > + } > + > if (!block_job_is_cancelled(&s->common) && data->reached_end && > data->ret == 0) { > const char *base_id = NULL, *base_fmt = NULL; > @@ -79,6 +86,11 @@ static void stream_complete(BlockJob *job, void *opaque) > bdrv_set_backing_hd(job->bs, base); > } > > + /* 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); > @@ -216,13 +228,15 @@ static const BlockJobDriver stream_job_driver = { > .set_speed = stream_set_speed, > }; > > -void stream_start(BlockDriverState *bs, BlockDriverState *base, > +void stream_start(BlockDriverState *bs, BlockDriverState *active, > + BlockDriverState *base, > const char *backing_file_str, int64_t speed, > BlockdevOnError on_error, > BlockCompletionFunc *cb, > void *opaque, Error **errp) > { > StreamBlockJob *s; > + int orig_bs_flags; > > if ((on_error == BLOCKDEV_ON_ERROR_STOP || > on_error == BLOCKDEV_ON_ERROR_ENOSPC) && > @@ -231,13 +245,36 @@ 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) { > + return; > + } > + } > + > s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp); > if (!s) { > return; > } > > + /* If we are streaming to an intermediate image, we need to block > + * the active layer. Due to a race condition, having several block > + * jobs running in the same chain is broken and we currently don't > + * support it. See here for details: > + * https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html > + */ > + if (active) { > + bdrv_op_block_all(active, s->common.blocker); block_job_create() unblocks BLOCK_OP_TYPE_DATAPLANE. Maybe this should do the same? No other objections from me. Max > + /* Hold a reference to the active layer, otherwise > + * bdrv_close_all() will destroy it if we shut down the VM */ > + bdrv_ref(active); > + } > + > s->base = base; > + s->active = active; > 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); > diff --git a/blockdev.c b/blockdev.c > index d1f5dfb..2e7712e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3038,7 +3038,7 @@ void qmp_block_stream(const char *device, > /* backing_file string overrides base bs filename */ > base_name = has_backing_file ? backing_file : base_name; > > - stream_start(bs, base_bs, base_name, has_speed ? speed : 0, > + stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0, > on_error, block_job_cb, bs, &local_err); > if (local_err) { > error_propagate(errp, local_err); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 10d8759..0a53d5f 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -597,6 +597,8 @@ int is_windows_drive(const char *filename); > /** > * stream_start: > * @bs: Block device to operate on. > + * @active: The active layer of the chain where @bs belongs, or %NULL > + * if @bs is already the topmost device. > * @base: Block device that will become the new base, or %NULL to > * flatten the whole backing file chain onto @bs. > * @base_id: The file name that will be written to @bs as the new > @@ -613,7 +615,8 @@ int is_windows_drive(const char *filename); > * streaming job, the backing file of @bs will be changed to > * @base_id in the written image and to @base in the live BlockDriverState. > */ > -void stream_start(BlockDriverState *bs, BlockDriverState *base, > +void stream_start(BlockDriverState *bs, BlockDriverState *active, > + BlockDriverState *base, > const char *base_id, int64_t speed, BlockdevOnError on_error, > BlockCompletionFunc *cb, > void *opaque, Error **errp); >
On Wed 27 Apr 2016 03:04:57 PM CEST, Max Reitz <mreitz@redhat.com> wrote: >> + /* If we are streaming to an intermediate image, we need to block >> + * the active layer. Due to a race condition, having several block >> + * jobs running in the same chain is broken and we currently don't >> + * support it. See here for details: >> + * https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html >> + */ >> + if (active) { >> + bdrv_op_block_all(active, s->common.blocker); > > block_job_create() unblocks BLOCK_OP_TYPE_DATAPLANE. Maybe this should > do the same? I don't know the internals of how dataplane works, but I don't see that it tries to call bdrv_reopen() so it should be safe. I'll unblock it. Berto
Am 04.04.2016 um 15:43 hat Alberto Garcia geschrieben: > This makes sure that the image we are steaming into is open in s/steaming/streaming/ > read-write mode during the operation. > > The block job is created on the destination image, but operation > blockers are also set on the active layer. We do this in order to > prevent other block jobs from running in parallel in the same chain. > See here for details on why that is currently not supported: > > [Qemu-block] RFC: Status of the intermediate block streaming work > https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html > > Finally, this also unblocks the stream operation in backing files. > > Signed-off-by: Alberto Garcia <berto@igalia.com> Would be great to have the new op blockers already, because having to know the active layer before streaming can be started is certainly not nice. Kevin
diff --git a/block.c b/block.c index 48638c9..f7698a1 100644 --- a/block.c +++ b/block.c @@ -1252,9 +1252,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) backing_hd->drv ? backing_hd->drv->format_name : ""); bdrv_op_block_all(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(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET, bs->backing_blocker); + bdrv_op_unblock(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 332b9a1..ac02ddf 100644 --- a/block/stream.c +++ b/block/stream.c @@ -35,8 +35,10 @@ typedef struct StreamBlockJob { BlockJob common; RateLimit limit; BlockDriverState *base; + BlockDriverState *active; BlockdevOnError on_error; char *backing_file_str; + int bs_flags; } StreamBlockJob; static int coroutine_fn stream_populate(BlockDriverState *bs, @@ -66,6 +68,11 @@ static void stream_complete(BlockJob *job, void *opaque) StreamCompleteData *data = opaque; BlockDriverState *base = s->base; + if (s->active) { + bdrv_op_unblock_all(s->active, s->common.blocker); + bdrv_unref(s->active); + } + if (!block_job_is_cancelled(&s->common) && data->reached_end && data->ret == 0) { const char *base_id = NULL, *base_fmt = NULL; @@ -79,6 +86,11 @@ static void stream_complete(BlockJob *job, void *opaque) bdrv_set_backing_hd(job->bs, base); } + /* 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); @@ -216,13 +228,15 @@ static const BlockJobDriver stream_job_driver = { .set_speed = stream_set_speed, }; -void stream_start(BlockDriverState *bs, BlockDriverState *base, +void stream_start(BlockDriverState *bs, BlockDriverState *active, + BlockDriverState *base, const char *backing_file_str, int64_t speed, BlockdevOnError on_error, BlockCompletionFunc *cb, void *opaque, Error **errp) { StreamBlockJob *s; + int orig_bs_flags; if ((on_error == BLOCKDEV_ON_ERROR_STOP || on_error == BLOCKDEV_ON_ERROR_ENOSPC) && @@ -231,13 +245,36 @@ 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) { + return; + } + } + s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp); if (!s) { return; } + /* If we are streaming to an intermediate image, we need to block + * the active layer. Due to a race condition, having several block + * jobs running in the same chain is broken and we currently don't + * support it. See here for details: + * https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html + */ + if (active) { + bdrv_op_block_all(active, s->common.blocker); + /* Hold a reference to the active layer, otherwise + * bdrv_close_all() will destroy it if we shut down the VM */ + bdrv_ref(active); + } + s->base = base; + s->active = active; 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); diff --git a/blockdev.c b/blockdev.c index d1f5dfb..2e7712e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3038,7 +3038,7 @@ void qmp_block_stream(const char *device, /* backing_file string overrides base bs filename */ base_name = has_backing_file ? backing_file : base_name; - stream_start(bs, base_bs, base_name, has_speed ? speed : 0, + stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0, on_error, block_job_cb, bs, &local_err); if (local_err) { error_propagate(errp, local_err); diff --git a/include/block/block_int.h b/include/block/block_int.h index 10d8759..0a53d5f 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -597,6 +597,8 @@ int is_windows_drive(const char *filename); /** * stream_start: * @bs: Block device to operate on. + * @active: The active layer of the chain where @bs belongs, or %NULL + * if @bs is already the topmost device. * @base: Block device that will become the new base, or %NULL to * flatten the whole backing file chain onto @bs. * @base_id: The file name that will be written to @bs as the new @@ -613,7 +615,8 @@ int is_windows_drive(const char *filename); * streaming job, the backing file of @bs will be changed to * @base_id in the written image and to @base in the live BlockDriverState. */ -void stream_start(BlockDriverState *bs, BlockDriverState *base, +void stream_start(BlockDriverState *bs, BlockDriverState *active, + BlockDriverState *base, const char *base_id, int64_t speed, BlockdevOnError on_error, BlockCompletionFunc *cb, void *opaque, Error **errp);
This makes sure that the image we are steaming into is open in read-write mode during the operation. The block job is created on the destination image, but operation blockers are also set on the active layer. We do this in order to prevent other block jobs from running in parallel in the same chain. See here for details on why that is currently not supported: [Qemu-block] RFC: Status of the intermediate block streaming work https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html Finally, this also unblocks the stream operation in backing files. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 4 +++- block/stream.c | 39 ++++++++++++++++++++++++++++++++++++++- blockdev.c | 2 +- include/block/block_int.h | 5 ++++- 4 files changed, 46 insertions(+), 4 deletions(-)