| Message ID | 1553866154-257311-4-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/stream: get rid of the base | expand |
On Fri 29 Mar 2019 02:29:14 PM CET, Andrey Shinkevich wrote: > @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, > job_flags |= JOB_MANUAL_DISMISS; > } > > - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, > + /* Find the bottom node that has the base as its backing image */ > + bottom_node = bs; > + while ((iter = backing_bs(bottom_node)) != base_bs) { > + bottom_node = iter; > + } > + assert(bottom_node); > + > + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, > job_flags, has_speed ? speed : 0, on_error, &local_err); Isn't it simpler to pass 'base' to stream_start() and find the bottom node there? (with bdrv_find_overlay()). I think bottom should be an internal implementation detail of the block-stream driver, callers don't need to know about it, or do they? Berto
29.03.2019 19:07, Alberto Garcia wrote: > On Fri 29 Mar 2019 02:29:14 PM CET, Andrey Shinkevich wrote: >> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >> job_flags |= JOB_MANUAL_DISMISS; >> } >> >> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >> + /* Find the bottom node that has the base as its backing image */ >> + bottom_node = bs; >> + while ((iter = backing_bs(bottom_node)) != base_bs) { >> + bottom_node = iter; >> + } >> + assert(bottom_node); >> + >> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >> job_flags, has_speed ? speed : 0, on_error, &local_err); > > Isn't it simpler to pass 'base' to stream_start() and find the bottom > node there? (with bdrv_find_overlay()). > > I think bottom should be an internal implementation detail of the > block-stream driver, callers don't need to know about it, or do they? > My idea is that we should get rid of base before any yield, and better do it as soon as possible. Better to get rid of it in qapi interface, but we can't do it due to backward compatibility. So, I'd prefer to not deal with base in block/stream.c code at all, and only qmp_block_stream is responsible to recalculate it's parameters to something more meaningful before any context switch.
On 29/03/2019 19:07, Alberto Garcia wrote: > On Fri 29 Mar 2019 02:29:14 PM CET, Andrey Shinkevich wrote: >> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >> job_flags |= JOB_MANUAL_DISMISS; >> } >> >> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >> + /* Find the bottom node that has the base as its backing image */ >> + bottom_node = bs; >> + while ((iter = backing_bs(bottom_node)) != base_bs) { >> + bottom_node = iter; >> + } >> + assert(bottom_node); >> + >> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >> job_flags, has_speed ? speed : 0, on_error, &local_err); > > Isn't it simpler to pass 'base' to stream_start() and find the bottom > node there? (with bdrv_find_overlay()). > > I think bottom should be an internal implementation detail of the > block-stream driver, callers don't need to know about it, or do they? > > Berto > Thank you for the idea to use the bdrv_find_overlay() with the similar functionality.
29.03.2019 16:29, Andrey Shinkevich wrote: > The bottom node is the intermediate block device that has the base as its > backing image. It is used instead of the base node while a block stream > job is running to avoid dependency on the base that may change due to the > parallel jobs. The change may take place due to a filter node as well that > is inserted between the base and the intermediate bottom node. It occurs > when the base node is the top one for another commit or stream job. Good to mention here that don't freeze backing child of bottom from this patch (and iotest change shows it) > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/stream.c | 56 +++++++++++++++++++++++++---------------------- > block/trace-events | 2 +- > blockdev.c | 10 ++++++++- > include/block/block_int.h | 6 ++--- > tests/qemu-iotests/245 | 4 ++-- > 5 files changed, 45 insertions(+), 33 deletions(-) > > diff --git a/block/stream.c b/block/stream.c > index c065e99..d4d0737 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -31,7 +31,7 @@ enum { > > typedef struct StreamBlockJob { > BlockJob common; > - BlockDriverState *base; > + BlockDriverState *bottom; > BlockdevOnError on_error; > char *backing_file_str; > bool bs_read_only; > @@ -56,7 +56,7 @@ static void stream_abort(Job *job) > > if (s->chain_frozen) { > BlockJob *bjob = &s->common; > - bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base); > + bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom); > } > } > > @@ -65,11 +65,11 @@ static int stream_prepare(Job *job) > StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); > BlockJob *bjob = &s->common; > BlockDriverState *bs = blk_bs(bjob->blk); > - BlockDriverState *base = s->base; > + BlockDriverState *base = backing_bs(s->bottom); > Error *local_err = NULL; > int ret = 0; > > - bdrv_unfreeze_backing_chain(bs, base); > + bdrv_unfreeze_backing_chain(bs, s->bottom); > s->chain_frozen = false; > > if (bs->backing) { > @@ -112,7 +112,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) > StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); > BlockBackend *blk = s->common.blk; > BlockDriverState *bs = blk_bs(blk); > - BlockDriverState *base = s->base; > + bool enable_cor = !!backing_bs(s->bottom); s/!!/! > int64_t len; > int64_t offset = 0; > uint64_t delay_ns = 0; > @@ -125,6 +125,11 @@ static int coroutine_fn stream_run(Job *job, Error **errp) > return 0; > } > > + if (bs == s->bottom) { > + /* Nothing to stream */ > + return 0; > + } I think, with this "if" covers the previous (if (!bs->backing)), so the previous should be deleted. > + > len = bdrv_getlength(bs); > if (len < 0) { > return len; > @@ -138,7 +143,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) > * backing chain since the copy-on-read operation does not take base into > * account. > */ > - if (!base) { > + if (!enable_cor) { s/!// > bdrv_enable_copy_on_read(bs); > } > > @@ -161,9 +166,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp) > } else if (ret >= 0) { > /* Copy if allocated in the intermediate images. Limit to the > * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE). */ > - ret = bdrv_is_allocated_above(backing_bs(bs), base, > - offset, n, &n); > - > + ret = bdrv_is_allocated_above_inclusive(backing_bs(bs), s->bottom, > + offset, n, &n); > /* Finish early if end of backing file has been reached */ > if (ret == 0 && n == 0) { > n = len - offset; > @@ -200,7 +204,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) > } > } > > - if (!base) { > + if (!enable_cor) { s/!// > bdrv_disable_copy_on_read(bs); > } > > @@ -225,13 +229,14 @@ static const BlockJobDriver stream_job_driver = { > }; > The rest looks good for me, just use bdrv_find_overlay instead of similar loop as Alberto suggested.
On Fri 29 Mar 2019 05:15:43 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >>> job_flags |= JOB_MANUAL_DISMISS; >>> } >>> >>> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >>> + /* Find the bottom node that has the base as its backing image */ >>> + bottom_node = bs; >>> + while ((iter = backing_bs(bottom_node)) != base_bs) { >>> + bottom_node = iter; >>> + } >>> + assert(bottom_node); >>> + >>> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >>> job_flags, has_speed ? speed : 0, on_error, &local_err); >> >> Isn't it simpler to pass 'base' to stream_start() and find the bottom >> node there? (with bdrv_find_overlay()). >> >> I think bottom should be an internal implementation detail of the >> block-stream driver, callers don't need to know about it, or do they? >> > My idea is that we should get rid of base before any yield, and better > do it as soon as possible. But you can do it at the beginning of stream_start() without exposing 'bottom' to the callers which, again, is an implementation detail. Berto
On 01/04/2019 18:44, Alberto Garcia wrote: > On Fri 29 Mar 2019 05:15:43 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>>> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >>>> job_flags |= JOB_MANUAL_DISMISS; >>>> } >>>> >>>> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >>>> + /* Find the bottom node that has the base as its backing image */ >>>> + bottom_node = bs; >>>> + while ((iter = backing_bs(bottom_node)) != base_bs) { >>>> + bottom_node = iter; >>>> + } >>>> + assert(bottom_node); >>>> + >>>> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >>>> job_flags, has_speed ? speed : 0, on_error, &local_err); >>> >>> Isn't it simpler to pass 'base' to stream_start() and find the bottom >>> node there? (with bdrv_find_overlay()). >>> >>> I think bottom should be an internal implementation detail of the >>> block-stream driver, callers don't need to know about it, or do they? >>> >> My idea is that we should get rid of base before any yield, and better >> do it as soon as possible. > > But you can do it at the beginning of stream_start() without exposing > 'bottom' to the callers which, again, is an implementation detail. > > Berto > We have got a latent trouble with the base involved into the process. For instance, if we encounter a filter while searching for the base, we will want to manage it somehow. If the base is identified by the node name rather than by file name, it would be easier. So, we would assign the node name to the base earliest as possible. Another approach suggested by Vladimir supposed to eliminate the base from the interface. It is a sensitive change and we all need to come to an agreement.
02.04.2019 11:51, Andrey Shinkevich wrote: > > > On 01/04/2019 18:44, Alberto Garcia wrote: >> On Fri 29 Mar 2019 05:15:43 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>>>> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >>>>> job_flags |= JOB_MANUAL_DISMISS; >>>>> } >>>>> >>>>> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >>>>> + /* Find the bottom node that has the base as its backing image */ >>>>> + bottom_node = bs; >>>>> + while ((iter = backing_bs(bottom_node)) != base_bs) { >>>>> + bottom_node = iter; >>>>> + } >>>>> + assert(bottom_node); >>>>> + >>>>> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >>>>> job_flags, has_speed ? speed : 0, on_error, &local_err); >>>> >>>> Isn't it simpler to pass 'base' to stream_start() and find the bottom >>>> node there? (with bdrv_find_overlay()). >>>> >>>> I think bottom should be an internal implementation detail of the >>>> block-stream driver, callers don't need to know about it, or do they? >>>> >>> My idea is that we should get rid of base before any yield, and better >>> do it as soon as possible. >> >> But you can do it at the beginning of stream_start() without exposing >> 'bottom' to the callers which, again, is an implementation detail. >> >> Berto >> > > We have got a latent trouble with the base involved into the process. > For instance, if we encounter a filter while searching for the base, > we will want to manage it somehow. If the base is identified by the > node name rather than by file name, it would be easier. So, we would > assign the node name to the base earliest as possible. Another approach > suggested by Vladimir supposed to eliminate the base from the interface. > It is a sensitive change and we all need to come to an agreement. > On the other hand we freeze backing chain in stream_start anyway. So, we'd like to be sure anyway, that there are no yields before stream_start() call.
On Tue 02 Apr 2019 10:51:06 AM CEST, Andrey Shinkevich wrote: >>>>> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >>>>> job_flags |= JOB_MANUAL_DISMISS; >>>>> } >>>>> >>>>> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >>>>> + /* Find the bottom node that has the base as its backing image */ >>>>> + bottom_node = bs; >>>>> + while ((iter = backing_bs(bottom_node)) != base_bs) { >>>>> + bottom_node = iter; >>>>> + } >>>>> + assert(bottom_node); >>>>> + >>>>> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >>>>> job_flags, has_speed ? speed : 0, on_error, &local_err); >>>> >>>> Isn't it simpler to pass 'base' to stream_start() and find the >>>> bottom node there? (with bdrv_find_overlay()). >>>> >>>> I think bottom should be an internal implementation detail of the >>>> block-stream driver, callers don't need to know about it, or do >>>> they? >>>> >>> My idea is that we should get rid of base before any yield, and >>> better do it as soon as possible. >> >> But you can do it at the beginning of stream_start() without exposing >> 'bottom' to the callers which, again, is an implementation detail. > > We have got a latent trouble with the base involved into the process. > For instance, if we encounter a filter while searching for the base, > we will want to manage it somehow. If the base is identified by the > node name rather than by file name, it would be easier. So, we would > assign the node name to the base earliest as possible. There's already the 'base-node' parameter, and converting 'base' into 'base-node' is already done early on by qmp_block_stream() so it doesn't make much difference whether the user passed the former or the latter. > Another approach suggested by Vladimir supposed to eliminate the base > from the interface. It is a sensitive change and we all need to come > to an agreement. Well, replacing 'base' with 'bottom' on the public API is a completely different thing and I'm not sure if it's a good idea. 'base' is used to decide the new backing image after block-stream completes, and its semantics are quite clear. But that does not mean that 'base' must be used throughout the implementation of block-stream. If it turns out that block-stream is best implemented using a 'bottom' node istead of 'base' then the first thing that stream_start() can do is calculate 'bottom' and discard 'base'. Whether you do that at the beginning of stream_start() or at the end of qmp_block_stream() does not make any difference in practice, the end result is going to be the same. The only reason why I'm proposing this is because I don't think 'base' needs to be removed from the public API and therefore I see 'bottom' as simply an implementation detail that doesn't need to be exposed outside stream.c Berto
On 29/03/2019 19:07, Alberto Garcia wrote: > On Fri 29 Mar 2019 02:29:14 PM CET, Andrey Shinkevich wrote: >> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >> job_flags |= JOB_MANUAL_DISMISS; >> } >> >> - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, >> + /* Find the bottom node that has the base as its backing image */ >> + bottom_node = bs; >> + while ((iter = backing_bs(bottom_node)) != base_bs) { >> + bottom_node = iter; >> + } >> + assert(bottom_node); >> + >> + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, >> job_flags, has_speed ? speed : 0, on_error, &local_err); > > Isn't it simpler to pass 'base' to stream_start() and find the bottom > node there? (with bdrv_find_overlay()). > I am going to move the bottom = bdrv_find_overlay() into the stream_start() in v3, which is coming soon. > I think bottom should be an internal implementation detail of the > block-stream driver, callers don't need to know about it, or do they? > > Berto >
diff --git a/block/stream.c b/block/stream.c index c065e99..d4d0737 100644 --- a/block/stream.c +++ b/block/stream.c @@ -31,7 +31,7 @@ enum { typedef struct StreamBlockJob { BlockJob common; - BlockDriverState *base; + BlockDriverState *bottom; BlockdevOnError on_error; char *backing_file_str; bool bs_read_only; @@ -56,7 +56,7 @@ static void stream_abort(Job *job) if (s->chain_frozen) { BlockJob *bjob = &s->common; - bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base); + bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom); } } @@ -65,11 +65,11 @@ static int stream_prepare(Job *job) StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); BlockJob *bjob = &s->common; BlockDriverState *bs = blk_bs(bjob->blk); - BlockDriverState *base = s->base; + BlockDriverState *base = backing_bs(s->bottom); Error *local_err = NULL; int ret = 0; - bdrv_unfreeze_backing_chain(bs, base); + bdrv_unfreeze_backing_chain(bs, s->bottom); s->chain_frozen = false; if (bs->backing) { @@ -112,7 +112,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); BlockBackend *blk = s->common.blk; BlockDriverState *bs = blk_bs(blk); - BlockDriverState *base = s->base; + bool enable_cor = !!backing_bs(s->bottom); int64_t len; int64_t offset = 0; uint64_t delay_ns = 0; @@ -125,6 +125,11 @@ static int coroutine_fn stream_run(Job *job, Error **errp) return 0; } + if (bs == s->bottom) { + /* Nothing to stream */ + return 0; + } + len = bdrv_getlength(bs); if (len < 0) { return len; @@ -138,7 +143,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) * backing chain since the copy-on-read operation does not take base into * account. */ - if (!base) { + if (!enable_cor) { bdrv_enable_copy_on_read(bs); } @@ -161,9 +166,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp) } else if (ret >= 0) { /* Copy if allocated in the intermediate images. Limit to the * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE). */ - ret = bdrv_is_allocated_above(backing_bs(bs), base, - offset, n, &n); - + ret = bdrv_is_allocated_above_inclusive(backing_bs(bs), s->bottom, + offset, n, &n); /* Finish early if end of backing file has been reached */ if (ret == 0 && n == 0) { n = len - offset; @@ -200,7 +204,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) } } - if (!base) { + if (!enable_cor) { bdrv_disable_copy_on_read(bs); } @@ -225,13 +229,14 @@ static const BlockJobDriver stream_job_driver = { }; void stream_start(const char *job_id, BlockDriverState *bs, - BlockDriverState *base, const char *backing_file_str, + BlockDriverState *bottom, const char *backing_file_str, int creation_flags, int64_t speed, BlockdevOnError on_error, Error **errp) { StreamBlockJob *s; BlockDriverState *iter; bool bs_read_only; + int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED; /* Make sure that the image is opened in read-write mode */ bs_read_only = bdrv_is_read_only(bs); @@ -245,37 +250,36 @@ void stream_start(const char *job_id, BlockDriverState *bs, * already have our own plans. Also don't allow resize as the image size is * queried only at the job start and then cached. */ s = block_job_create(job_id, &stream_job_driver, NULL, bs, - BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | - BLK_PERM_GRAPH_MOD, - BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | - BLK_PERM_WRITE, + basic_flags | BLK_PERM_GRAPH_MOD, + basic_flags | BLK_PERM_WRITE, speed, creation_flags, NULL, NULL, errp); if (!s) { goto fail; } - /* Block all intermediate nodes between bs and base, because they will - * disappear from the chain after this operation. The streaming job reads - * every block only once, assuming that it doesn't change, so block writes - * and resizes. */ - for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) { - block_job_add_bdrv(&s->common, "intermediate node", iter, 0, - BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED, - &error_abort); + /* + * Block all intermediate nodes between bs and bottom (inclusive), because + * they will disappear from the chain after this operation. The streaming + * job reads every block only once, assuming that it doesn't change, so + * forbid writes and resizes. + */ + for (iter = bs; iter != bottom; iter = backing_bs(iter)) { + block_job_add_bdrv(&s->common, "intermediate node", backing_bs(iter), + 0, basic_flags, &error_abort); } - if (bdrv_freeze_backing_chain(bs, base, errp) < 0) { + if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) { job_early_fail(&s->common.job); goto fail; } - s->base = base; + s->bottom = bottom; s->backing_file_str = g_strdup(backing_file_str); s->bs_read_only = bs_read_only; s->chain_frozen = true; s->on_error = on_error; - trace_stream_start(bs, base, s); + trace_stream_start(bs, bottom, s); job_start(&s->common.job); return; diff --git a/block/trace-events b/block/trace-events index e6bb5a8..36e7b79 100644 --- a/block/trace-events +++ b/block/trace-events @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of # stream.c stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d" -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p" +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p" # commit.c commit_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d" diff --git a/blockdev.c b/blockdev.c index 4775a07..bc84312 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3164,6 +3164,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, { BlockDriverState *bs, *iter; BlockDriverState *base_bs = NULL; + BlockDriverState *bottom_node = NULL; AioContext *aio_context; Error *local_err = NULL; const char *base_name = NULL; @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, job_flags |= JOB_MANUAL_DISMISS; } - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, + /* Find the bottom node that has the base as its backing image */ + bottom_node = bs; + while ((iter = backing_bs(bottom_node)) != base_bs) { + bottom_node = iter; + } + assert(bottom_node); + + stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name, job_flags, has_speed ? speed : 0, on_error, &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 01e855a..8ab1144 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1019,8 +1019,8 @@ int is_windows_drive(const char *filename); * @job_id: The id of the newly-created job, or %NULL to use the * device name of @bs. * @bs: Block device to operate on. - * @base: Block device that will become the new base, or %NULL to - * flatten the whole backing file chain onto @bs. + * @bottom_node: The intermediate block device right above the new base. + * If base is %NULL, the whole backing file chain is flattened onto @bs. * @backing_file_str: The file name that will be written to @bs as the * the new backing file if the job completes. Ignored if @base is %NULL. * @creation_flags: Flags that control the behavior of the Job lifetime. @@ -1037,7 +1037,7 @@ int is_windows_drive(const char *filename); * BlockDriverState. */ void stream_start(const char *job_id, BlockDriverState *bs, - BlockDriverState *base, const char *backing_file_str, + BlockDriverState *bottom_node, const char *backing_file_str, int creation_flags, int64_t speed, BlockdevOnError on_error, Error **errp); diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index 7891a21..d11e73c 100644 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -859,9 +859,9 @@ class TestBlockdevReopen(iotests.QMPTestCase): device = 'hd0', base_node = 'hd2', speed = 512 * 1024) self.assert_qmp(result, 'return', {}) - # We can't remove hd2 while the stream job is ongoing + # We can remove hd2 while the stream job is ongoing opts['backing']['backing'] = None - self.reopen(opts, {}, "Cannot change 'backing' link from 'hd1' to 'hd2'") + self.reopen(opts, {}) # We can't remove hd1 while the stream job is ongoing opts['backing'] = None
The bottom node is the intermediate block device that has the base as its backing image. It is used instead of the base node while a block stream job is running to avoid dependency on the base that may change due to the parallel jobs. The change may take place due to a filter node as well that is inserted between the base and the intermediate bottom node. It occurs when the base node is the top one for another commit or stream job. Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/stream.c | 56 +++++++++++++++++++++++++---------------------- block/trace-events | 2 +- blockdev.c | 10 ++++++++- include/block/block_int.h | 6 ++--- tests/qemu-iotests/245 | 4 ++-- 5 files changed, 45 insertions(+), 33 deletions(-)