| Message ID | 7e86a00cdda718d14f583e3228ecb1524396b21b.1475757437.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben: > This makes sure that the image we are streaming 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 <berto@igalia.com> > --- > block.c | 4 +++- > block/stream.c | 24 ++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index c80b528..8255d75 100644 > --- a/block.c > +++ b/block.c > @@ -1428,9 +1428,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); > /* > * We do backup in 3 ways: > * 1. drive backup > diff --git a/block/stream.c b/block/stream.c > index 3187481..b8ab89a 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -37,6 +37,7 @@ typedef struct StreamBlockJob { > BlockDriverState *base; > BlockdevOnError on_error; > char *backing_file_str; > + int bs_flags; > } StreamBlockJob; > > static int coroutine_fn stream_populate(BlockBackend *blk, > @@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque) > bdrv_set_backing_hd(bs, base); > } > > + /* Reopen the image back in read-only mode if necessary */ > + if (s->bs_flags != bdrv_get_flags(bs)) { > + bdrv_reopen(bs, s->bs_flags, NULL); > + } > + > g_free(s->backing_file_str); > block_job_completed(&s->common, data->ret); > g_free(data); > @@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState *bs, > BlockCompletionFunc *cb, void *opaque, Error **errp) > { > StreamBlockJob *s; > + BlockDriverState *iter; > + int orig_bs_flags; > > s = block_job_create(job_id, &stream_job_driver, bs, speed, > cb, opaque, errp); > @@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState *bs, > 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) { > + block_job_unref(&s->common); > + return; > + } > + } > + > + /* Block all intermediate nodes between bs and base, because they > + * will disappear from the chain after this operation */ > + for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) { > + block_job_add_bdrv(&s->common, iter); > + } In patch 6, you had a comment that the top node must be blocked as well because its backing file string will be updated. Isn't it the same for streaming? > 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, s); Kevin
On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote: >> + /* Block all intermediate nodes between bs and base, because they >> + * will disappear from the chain after this operation */ >> + for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) { >> + block_job_add_bdrv(&s->common, iter); >> + } > > In patch 6, you had a comment that the top node must be blocked as > well because its backing file string will be updated. Isn't it the > same for streaming? In the block-stream case, 'device' is always the top node, and creating the block job there already blocks all operations in it. In the block-commit case, 'device' and 'top' are different parameters, that's why 'top' must be explicitly blocked. I actually don't think that we need to block 'device' at all, and we could even make the parameter optional. That would be for a future patch, though. Also, the backing file string is not updated in 'top', but in its overlay (unlike in block-stream, 'top' disappears after a block-commit job). Berto
Am 12.10.2016 um 16:33 hat Alberto Garcia geschrieben: > On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote: > >> + /* Block all intermediate nodes between bs and base, because they > >> + * will disappear from the chain after this operation */ > >> + for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) { > >> + block_job_add_bdrv(&s->common, iter); > >> + } > > > > In patch 6, you had a comment that the top node must be blocked as > > well because its backing file string will be updated. Isn't it the > > same for streaming? > > In the block-stream case, 'device' is always the top node, and creating > the block job there already blocks all operations in it. > > In the block-commit case, 'device' and 'top' are different parameters, > that's why 'top' must be explicitly blocked. I actually don't think that > we need to block 'device' at all, and we could even make the parameter > optional. That would be for a future patch, though. Also, the backing > file string is not updated in 'top', but in its overlay (unlike in > block-stream, 'top' disappears after a block-commit job). I see. So the block job for commit is owned by a node that is potentially completely unrelated to the operation except that it holds op blockers and because we use it to finde the overlay to change the backing file pointers. This is _so_ broken. :-) With your series (for the op blockers) and BdrvChild (for the operations on the overlay) we should actually be much closer to finally remove this. Good news. Kevin
diff --git a/block.c b/block.c index c80b528..8255d75 100644 --- a/block.c +++ b/block.c @@ -1428,9 +1428,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); /* * We do backup in 3 ways: * 1. drive backup diff --git a/block/stream.c b/block/stream.c index 3187481..b8ab89a 100644 --- a/block/stream.c +++ b/block/stream.c @@ -37,6 +37,7 @@ typedef struct StreamBlockJob { BlockDriverState *base; BlockdevOnError on_error; char *backing_file_str; + int bs_flags; } StreamBlockJob; static int coroutine_fn stream_populate(BlockBackend *blk, @@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque) bdrv_set_backing_hd(bs, base); } + /* Reopen the image back in read-only mode if necessary */ + if (s->bs_flags != bdrv_get_flags(bs)) { + bdrv_reopen(bs, s->bs_flags, NULL); + } + g_free(s->backing_file_str); block_job_completed(&s->common, data->ret); g_free(data); @@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque, Error **errp) { StreamBlockJob *s; + BlockDriverState *iter; + int orig_bs_flags; s = block_job_create(job_id, &stream_job_driver, bs, speed, cb, opaque, errp); @@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState *bs, 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) { + block_job_unref(&s->common); + return; + } + } + + /* Block all intermediate nodes between bs and base, because they + * will disappear from the chain after this operation */ + for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) { + block_job_add_bdrv(&s->common, iter); + } + 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, s);
This makes sure that the image we are streaming 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 <berto@igalia.com> --- block.c | 4 +++- block/stream.c | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-)