| Message ID | 20201204220758.2879-11-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apply COR-filter to the block-stream permanently | expand |
On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: > The code already don't freeze base node and we try to make it prepared > for the situation when base node is changed during the operation. In > other words, block-stream doesn't own base node. > > Let's introduce a new interface which should replace the current one, > which will in better relations with the code. Specifying bottom node > instead of base, and requiring it to be non-filter gives us the > following benefits: > > - drop difference between above_base and base_overlay, which will be > renamed to just bottom, when old interface dropped > > - clean way to work with parallel streams/commits on the same backing > chain, which otherwise become a problem when we introduce a filter > for stream job > > - cleaner interface. Nobody will surprised the fact that base node may > disappear during block-stream, when there is no word about "base" in > the interface. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 8 +++-- > include/block/block_int.h | 1 + > block/monitor/block-hmp-cmds.c | 3 +- > block/stream.c | 50 +++++++++++++++++++--------- > blockdev.c | 61 ++++++++++++++++++++++++++++------ > 5 files changed, 94 insertions(+), 29 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 04055ef50c..5d6681a35d 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2522,6 +2522,10 @@ > # @base-node: the node name of the backing file. > # It cannot be set if @base is also set. (Since 2.8) > # > +# @bottom: the last node in the chain that should be streamed into > +# top. It cannot be set any of @base, @base-node or @backing-file s/set any/set if any/ But what’s the problem with backing-file? The fact that specifying backing-file means that stream will look for that filename in the backing chain when the job is done (so if you use @bottom, we generally don’t want to rely on the presence of any nodes below it)? (If so, I would have thought that we actually want the user to specify backing-file so we don’t have to look down below @bottom to look for a filename. Perhaps a @backing-fmt parameter would help.) [...] > diff --git a/blockdev.c b/blockdev.c > index 70900f4f77..e0e19db88b 100644 > --- a/blockdev.c > +++ b/blockdev.c [...] > @@ -2551,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, > bdrv_refresh_filename(base_bs); > } > > - /* Check for op blockers in the whole chain between bs and base */ > - for (iter = bs; iter && iter != base_bs; > + if (has_bottom) { > + bottom_bs = bdrv_lookup_bs(NULL, bottom, errp); > + if (!bottom_bs) { > + goto out; > + } > + if (!bottom_bs->drv) { > + error_setg(errp, "Node '%s' is not open", bottom); > + goto out; > + } > + if (bottom_bs->drv->is_filter) { > + error_setg(errp, "Node '%s' is filter, use non-filter node" > + "as 'bottom'", bottom); Missing a space between “node” and “as”. (Also, probably two articles, i.e. “Node '%s' is a filter, use a non-filter node...”.) The rest looks good to me, but I’m withholding my R-b because I haven’t understood why using @bottom precludes giving @backing-file. Max
11.12.2020 19:05, Max Reitz wrote: > On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >> The code already don't freeze base node and we try to make it prepared >> for the situation when base node is changed during the operation. In >> other words, block-stream doesn't own base node. >> >> Let's introduce a new interface which should replace the current one, >> which will in better relations with the code. Specifying bottom node >> instead of base, and requiring it to be non-filter gives us the >> following benefits: >> >> - drop difference between above_base and base_overlay, which will be >> renamed to just bottom, when old interface dropped >> >> - clean way to work with parallel streams/commits on the same backing >> chain, which otherwise become a problem when we introduce a filter >> for stream job >> >> - cleaner interface. Nobody will surprised the fact that base node may >> disappear during block-stream, when there is no word about "base" in >> the interface. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 8 +++-- >> include/block/block_int.h | 1 + >> block/monitor/block-hmp-cmds.c | 3 +- >> block/stream.c | 50 +++++++++++++++++++--------- >> blockdev.c | 61 ++++++++++++++++++++++++++++------ >> 5 files changed, 94 insertions(+), 29 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 04055ef50c..5d6681a35d 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2522,6 +2522,10 @@ >> # @base-node: the node name of the backing file. >> # It cannot be set if @base is also set. (Since 2.8) >> # >> +# @bottom: the last node in the chain that should be streamed into >> +# top. It cannot be set any of @base, @base-node or @backing-file > > s/set any/set if any/ > > But what’s the problem with backing-file? The fact that specifying backing-file means that stream will look for that filename in the backing chain when the job is done (so if you use @bottom, we generally don’t want to rely on the presence of any nodes below it)? I just wanted to deprecate 'backing-file' together with base and base-node as a next step. If user wants to set backing file unrelated to current backing-chain, is it correct at all? It's a direct violation of what's going on, and I doubt that other parts of Qemu working with backing-file are prepared for such situation. User can do it by hand later.. Anyway, we'll have three releases deprecation period for people to come and cry that this is a really needed option, so we can support it later on demand. > > (If so, I would have thought that we actually want the user to specify backing-file so we don’t have to look down below @bottom to look for a filename. Perhaps a @backing-fmt parameter would help.) If we decide that 'backing-file' is really needed, than yes we should require backing-fmt to be specified together with backing-file when using new "bottom" interface. > > [...] > >> diff --git a/blockdev.c b/blockdev.c >> index 70900f4f77..e0e19db88b 100644 >> --- a/blockdev.c >> +++ b/blockdev.c > > [...] > >> @@ -2551,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, >> bdrv_refresh_filename(base_bs); >> } >> - /* Check for op blockers in the whole chain between bs and base */ >> - for (iter = bs; iter && iter != base_bs; >> + if (has_bottom) { >> + bottom_bs = bdrv_lookup_bs(NULL, bottom, errp); >> + if (!bottom_bs) { >> + goto out; >> + } >> + if (!bottom_bs->drv) { >> + error_setg(errp, "Node '%s' is not open", bottom); >> + goto out; >> + } >> + if (bottom_bs->drv->is_filter) { >> + error_setg(errp, "Node '%s' is filter, use non-filter node" >> + "as 'bottom'", bottom); > > Missing a space between “node” and “as”. (Also, probably two articles, i.e. “Node '%s' is a filter, use a non-filter node...”.) > > The rest looks good to me, but I’m withholding my R-b because I haven’t understood why using @bottom precludes giving @backing-file. > > Max >
On 11.12.20 17:50, Vladimir Sementsov-Ogievskiy wrote: > 11.12.2020 19:05, Max Reitz wrote: >> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >>> The code already don't freeze base node and we try to make it prepared >>> for the situation when base node is changed during the operation. In >>> other words, block-stream doesn't own base node. >>> >>> Let's introduce a new interface which should replace the current one, >>> which will in better relations with the code. Specifying bottom node >>> instead of base, and requiring it to be non-filter gives us the >>> following benefits: >>> >>> - drop difference between above_base and base_overlay, which will be >>> renamed to just bottom, when old interface dropped >>> >>> - clean way to work with parallel streams/commits on the same backing >>> chain, which otherwise become a problem when we introduce a filter >>> for stream job >>> >>> - cleaner interface. Nobody will surprised the fact that base node may >>> disappear during block-stream, when there is no word about "base" in >>> the interface. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> qapi/block-core.json | 8 +++-- >>> include/block/block_int.h | 1 + >>> block/monitor/block-hmp-cmds.c | 3 +- >>> block/stream.c | 50 +++++++++++++++++++--------- >>> blockdev.c | 61 ++++++++++++++++++++++++++++------ >>> 5 files changed, 94 insertions(+), 29 deletions(-) >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 04055ef50c..5d6681a35d 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -2522,6 +2522,10 @@ >>> # @base-node: the node name of the backing file. >>> # It cannot be set if @base is also set. (Since 2.8) >>> # >>> +# @bottom: the last node in the chain that should be streamed into >>> +# top. It cannot be set any of @base, @base-node or >>> @backing-file >> >> s/set any/set if any/ >> >> But what’s the problem with backing-file? The fact that specifying >> backing-file means that stream will look for that filename in the >> backing chain when the job is done (so if you use @bottom, we >> generally don’t want to rely on the presence of any nodes below it)? > > I just wanted to deprecate 'backing-file' together with base and > base-node as a next step. If user wants to set backing file unrelated to > current backing-chain, is it correct at all? It's a direct violation of > what's going on, and I doubt that other parts of Qemu working with > backing-file are prepared for such situation. User can do it by hand > later.. Anyway, we'll have three releases deprecation period for people > to come and cry that this is a really needed option, so we can support > it later on demand. > >> >> (If so, I would have thought that we actually want the user to specify >> backing-file so we don’t have to look down below @bottom to look for a >> filename. Perhaps a @backing-fmt parameter would help.) > > If we decide that 'backing-file' is really needed, than yes we should > require backing-fmt to be specified together with backing-file when > using new "bottom" interface. Before I can agree on removing backing-file (or deprecating it), I need to know what it’s actually used for. I actually don’t, though. The only reason I could imagine was because the user wanted to write some string into there that is different from base.filename. (The original commit 13d8cc515df does mention cases like FD passing, where qemu has no idea what an appropriate filename would be (it can only see /dev/fd/*). From that, it does appear to me that it’ll be needed even with @bottom.) Max
11.12.2020 20:24, Max Reitz wrote: > On 11.12.20 17:50, Vladimir Sementsov-Ogievskiy wrote: >> 11.12.2020 19:05, Max Reitz wrote: >>> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >>>> The code already don't freeze base node and we try to make it prepared >>>> for the situation when base node is changed during the operation. In >>>> other words, block-stream doesn't own base node. >>>> >>>> Let's introduce a new interface which should replace the current one, >>>> which will in better relations with the code. Specifying bottom node >>>> instead of base, and requiring it to be non-filter gives us the >>>> following benefits: >>>> >>>> - drop difference between above_base and base_overlay, which will be >>>> renamed to just bottom, when old interface dropped >>>> >>>> - clean way to work with parallel streams/commits on the same backing >>>> chain, which otherwise become a problem when we introduce a filter >>>> for stream job >>>> >>>> - cleaner interface. Nobody will surprised the fact that base node may >>>> disappear during block-stream, when there is no word about "base" in >>>> the interface. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> qapi/block-core.json | 8 +++-- >>>> include/block/block_int.h | 1 + >>>> block/monitor/block-hmp-cmds.c | 3 +- >>>> block/stream.c | 50 +++++++++++++++++++--------- >>>> blockdev.c | 61 ++++++++++++++++++++++++++++------ >>>> 5 files changed, 94 insertions(+), 29 deletions(-) >>>> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 04055ef50c..5d6681a35d 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -2522,6 +2522,10 @@ >>>> # @base-node: the node name of the backing file. >>>> # It cannot be set if @base is also set. (Since 2.8) >>>> # >>>> +# @bottom: the last node in the chain that should be streamed into >>>> +# top. It cannot be set any of @base, @base-node or @backing-file >>> >>> s/set any/set if any/ >>> >>> But what’s the problem with backing-file? The fact that specifying backing-file means that stream will look for that filename in the backing chain when the job is done (so if you use @bottom, we generally don’t want to rely on the presence of any nodes below it)? >> >> I just wanted to deprecate 'backing-file' together with base and base-node as a next step. If user wants to set backing file unrelated to current backing-chain, is it correct at all? It's a direct violation of what's going on, and I doubt that other parts of Qemu working with backing-file are prepared for such situation. User can do it by hand later.. Anyway, we'll have three releases deprecation period for people to come and cry that this is a really needed option, so we can support it later on demand. >> >>> >>> (If so, I would have thought that we actually want the user to specify backing-file so we don’t have to look down below @bottom to look for a filename. Perhaps a @backing-fmt parameter would help.) >> >> If we decide that 'backing-file' is really needed, than yes we should require backing-fmt to be specified together with backing-file when using new "bottom" interface. > Before I can agree on removing backing-file (or deprecating it), I need to know what it’s actually used for. I actually don’t, though. The only reason I could imagine was because the user wanted to write some string into there that is different from base.filename. > > (The original commit 13d8cc515df does mention cases like FD passing, where qemu has no idea what an appropriate filename would be (it can only see /dev/fd/*). From that, it does appear to me that it’ll be needed even with @bottom.) > I should have checked it myself.. That's one more reason for my "RFC: don't store backing filename in qcow2 image".. OK, do you think we can require backing-fmt to be specified if backing-file and bottom are specified? Or allow omitting it and deprecate this thing? We actually already have deprecation message in bdrv_change_backing_file(), and how we are trying to workaround it in block-stream will not work with file descriptors anyway (hmm, and old code works, so, actually 09 is a regression?)
On 11.12.20 18:42, Vladimir Sementsov-Ogievskiy wrote: > 11.12.2020 20:24, Max Reitz wrote: >> On 11.12.20 17:50, Vladimir Sementsov-Ogievskiy wrote: >>> 11.12.2020 19:05, Max Reitz wrote: >>>> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >>>>> The code already don't freeze base node and we try to make it prepared >>>>> for the situation when base node is changed during the operation. In >>>>> other words, block-stream doesn't own base node. >>>>> >>>>> Let's introduce a new interface which should replace the current one, >>>>> which will in better relations with the code. Specifying bottom node >>>>> instead of base, and requiring it to be non-filter gives us the >>>>> following benefits: >>>>> >>>>> - drop difference between above_base and base_overlay, which will be >>>>> renamed to just bottom, when old interface dropped >>>>> >>>>> - clean way to work with parallel streams/commits on the same >>>>> backing >>>>> chain, which otherwise become a problem when we introduce a filter >>>>> for stream job >>>>> >>>>> - cleaner interface. Nobody will surprised the fact that base >>>>> node may >>>>> disappear during block-stream, when there is no word about >>>>> "base" in >>>>> the interface. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> qapi/block-core.json | 8 +++-- >>>>> include/block/block_int.h | 1 + >>>>> block/monitor/block-hmp-cmds.c | 3 +- >>>>> block/stream.c | 50 +++++++++++++++++++--------- >>>>> blockdev.c | 61 >>>>> ++++++++++++++++++++++++++++------ >>>>> 5 files changed, 94 insertions(+), 29 deletions(-) >>>>> >>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>> index 04055ef50c..5d6681a35d 100644 >>>>> --- a/qapi/block-core.json >>>>> +++ b/qapi/block-core.json >>>>> @@ -2522,6 +2522,10 @@ >>>>> # @base-node: the node name of the backing file. >>>>> # It cannot be set if @base is also set. (Since 2.8) >>>>> # >>>>> +# @bottom: the last node in the chain that should be streamed into >>>>> +# top. It cannot be set any of @base, @base-node or >>>>> @backing-file >>>> >>>> s/set any/set if any/ >>>> >>>> But what’s the problem with backing-file? The fact that specifying >>>> backing-file means that stream will look for that filename in the >>>> backing chain when the job is done (so if you use @bottom, we >>>> generally don’t want to rely on the presence of any nodes below it)? >>> >>> I just wanted to deprecate 'backing-file' together with base and >>> base-node as a next step. If user wants to set backing file unrelated >>> to current backing-chain, is it correct at all? It's a direct >>> violation of what's going on, and I doubt that other parts of Qemu >>> working with backing-file are prepared for such situation. User can >>> do it by hand later.. Anyway, we'll have three releases deprecation >>> period for people to come and cry that this is a really needed >>> option, so we can support it later on demand. >>> >>>> >>>> (If so, I would have thought that we actually want the user to >>>> specify backing-file so we don’t have to look down below @bottom to >>>> look for a filename. Perhaps a @backing-fmt parameter would help.) >>> >>> If we decide that 'backing-file' is really needed, than yes we should >>> require backing-fmt to be specified together with backing-file when >>> using new "bottom" interface. >> Before I can agree on removing backing-file (or deprecating it), I >> need to know what it’s actually used for. I actually don’t, though. >> The only reason I could imagine was because the user wanted to write >> some string into there that is different from base.filename. >> >> (The original commit 13d8cc515df does mention cases like FD passing, >> where qemu has no idea what an appropriate filename would be (it can >> only see /dev/fd/*). From that, it does appear to me that it’ll be >> needed even with @bottom.) >> > > I should have checked it myself.. That's one more reason for my "RFC: > don't store backing filename in qcow2 image".. > > OK, do you think we can require backing-fmt to be specified if > backing-file and bottom are specified? Sure. > Or allow omitting it and > deprecate this thing? We actually already have deprecation message in > bdrv_change_backing_file(), and how we are trying to workaround it in > block-stream will not work with file descriptors anyway (hmm, and old > code works, so, actually 09 is a regression?) I think requiring backing-fmt for bottom + backing-file would be the most simple and clean way, hopefully saving us some headaches. Max
diff --git a/qapi/block-core.json b/qapi/block-core.json index 04055ef50c..5d6681a35d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2522,6 +2522,10 @@ # @base-node: the node name of the backing file. # It cannot be set if @base is also set. (Since 2.8) # +# @bottom: the last node in the chain that should be streamed into +# top. It cannot be set any of @base, @base-node or @backing-file +# is set. It cannot be filter node. (Since 6.0) +# # @backing-file: The backing file string to write into the top # image. This filename is not validated. # @@ -2576,8 +2580,8 @@ ## { 'command': 'block-stream', 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', - '*base-node': 'str', '*backing-file': 'str', '*speed': 'int', - '*on-error': 'BlockdevOnError', + '*base-node': 'str', '*backing-file': 'str', '*bottom': 'str', + '*speed': 'int', '*on-error': 'BlockdevOnError', '*filter-node-name': 'str', '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } diff --git a/include/block/block_int.h b/include/block/block_int.h index 247e166ab6..b13154edbf 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1152,6 +1152,7 @@ int is_windows_drive(const char *filename); */ void stream_start(const char *job_id, BlockDriverState *bs, BlockDriverState *base, const char *backing_file_str, + BlockDriverState *bottom, int creation_flags, int64_t speed, BlockdevOnError on_error, const char *filter_node_name, diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index e8a58f326e..afd75ab628 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -507,7 +507,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) int64_t speed = qdict_get_try_int(qdict, "speed", 0); qmp_block_stream(true, device, device, base != NULL, base, false, NULL, - false, NULL, qdict_haskey(qdict, "speed"), speed, true, + false, NULL, false, NULL, + qdict_haskey(qdict, "speed"), speed, true, BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false, false, false, &error); diff --git a/block/stream.c b/block/stream.c index c208393c34..a2744d07fe 100644 --- a/block/stream.c +++ b/block/stream.c @@ -237,6 +237,7 @@ static const BlockJobDriver stream_job_driver = { void stream_start(const char *job_id, BlockDriverState *bs, BlockDriverState *base, const char *backing_file_str, + BlockDriverState *bottom, int creation_flags, int64_t speed, BlockdevOnError on_error, const char *filter_node_name, @@ -246,25 +247,42 @@ void stream_start(const char *job_id, BlockDriverState *bs, BlockDriverState *iter; bool bs_read_only; int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED; - BlockDriverState *base_overlay = bdrv_find_overlay(bs, base); + BlockDriverState *base_overlay; BlockDriverState *above_base; - if (!base_overlay) { - error_setg(errp, "'%s' is not in the backing chain of '%s'", - base->node_name, bs->node_name); - return; - } + assert(!(base && bottom)); + assert(!(backing_file_str && bottom)); + + if (bottom) { + /* + * New simple interface. The code is written in terms of old interface + * with @base parameter (still, it doesn't freeze link to base, so in + * this mean old code is correct for new interface). So, for now, just + * emulate base_overlay and above_base. Still, when old interface + * finally removed, we should refactor code to use only "bottom", but + * not "*base*" things. + */ + assert(!bottom->drv->is_filter); + base_overlay = above_base = bottom; + } else { + base_overlay = bdrv_find_overlay(bs, base); + if (!base_overlay) { + error_setg(errp, "'%s' is not in the backing chain of '%s'", + base->node_name, bs->node_name); + return; + } - /* - * Find the node directly above @base. @base_overlay is a COW overlay, so - * it must have a bdrv_cow_child(), but it is the immediate overlay of - * @base, so between the two there can only be filters. - */ - above_base = base_overlay; - if (bdrv_cow_bs(above_base) != base) { - above_base = bdrv_cow_bs(above_base); - while (bdrv_filter_bs(above_base) != base) { - above_base = bdrv_filter_bs(above_base); + /* + * Find the node directly above @base. @base_overlay is a COW overlay, + * so it must have a bdrv_cow_child(), but it is the immediate overlay + * of @base, so between the two there can only be filters. + */ + above_base = base_overlay; + if (bdrv_cow_bs(above_base) != base) { + above_base = bdrv_cow_bs(above_base); + while (bdrv_filter_bs(above_base) != base) { + above_base = bdrv_filter_bs(above_base); + } } } diff --git a/blockdev.c b/blockdev.c index 70900f4f77..e0e19db88b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2497,6 +2497,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bool has_base, const char *base, bool has_base_node, const char *base_node, bool has_backing_file, const char *backing_file, + bool has_bottom, const char *bottom, bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, bool has_filter_node_name, const char *filter_node_name, @@ -2504,16 +2505,37 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bool has_auto_dismiss, bool auto_dismiss, Error **errp) { - BlockDriverState *bs, *iter; + BlockDriverState *bs, *iter, *iter_end; BlockDriverState *base_bs = NULL; + BlockDriverState *bottom_bs = NULL; AioContext *aio_context; Error *local_err = NULL; int job_flags = JOB_DEFAULT; + struct { + bool a; + const char *a_name; + bool b; + const char *b_name; + } restricted_pairs[] = { + {has_base, "base", has_base_node, "base-node"}, + {has_bottom, "bottom", has_base, "base"}, + {has_bottom, "bottom", has_base_node, "base-node"}, + {has_bottom, "bottom", has_backing_file, "backing-file"}, + {0} + }, *rp = restricted_pairs; if (!has_on_error) { on_error = BLOCKDEV_ON_ERROR_REPORT; } + for ( ; rp->a_name; rp++) { + if (rp->a && rp->b) { + error_setg(errp, "'%s' and '%s' cannot be specified " + "at the same time", rp->a_name, rp->b_name); + return; + } + } + bs = bdrv_lookup_bs(device, device, errp); if (!bs) { return; @@ -2522,12 +2544,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (has_base && has_base_node) { - error_setg(errp, "'base' and 'base-node' cannot be specified " - "at the same time"); - goto out; - } - if (has_base) { base_bs = bdrv_find_backing_image(bs, base); if (base_bs == NULL) { @@ -2551,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bdrv_refresh_filename(base_bs); } - /* Check for op blockers in the whole chain between bs and base */ - for (iter = bs; iter && iter != base_bs; + if (has_bottom) { + bottom_bs = bdrv_lookup_bs(NULL, bottom, errp); + if (!bottom_bs) { + goto out; + } + if (!bottom_bs->drv) { + error_setg(errp, "Node '%s' is not open", bottom); + goto out; + } + if (bottom_bs->drv->is_filter) { + error_setg(errp, "Node '%s' is filter, use non-filter node" + "as 'bottom'", bottom); + goto out; + } + if (!bdrv_chain_contains(bs, bottom_bs)) { + error_setg(errp, "Node '%s' is not in a chain starting from '%s'", + bottom, device); + goto out; + } + assert(bdrv_get_aio_context(bottom_bs) == aio_context); + } + + /* + * Check for op blockers in the whole chain between bs and base (or bottom) + */ + iter_end = has_bottom ? bdrv_filter_or_cow_bs(bottom_bs) : base_bs; + for (iter = bs; iter && iter != iter_end; iter = bdrv_filter_or_cow_bs(iter)) { if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) { @@ -2576,7 +2617,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, } stream_start(has_job_id ? job_id : NULL, bs, base_bs, backing_file, - job_flags, has_speed ? speed : 0, on_error, + bottom_bs, job_flags, has_speed ? speed : 0, on_error, filter_node_name, &local_err); if (local_err) { error_propagate(errp, local_err);
The code already don't freeze base node and we try to make it prepared for the situation when base node is changed during the operation. In other words, block-stream doesn't own base node. Let's introduce a new interface which should replace the current one, which will in better relations with the code. Specifying bottom node instead of base, and requiring it to be non-filter gives us the following benefits: - drop difference between above_base and base_overlay, which will be renamed to just bottom, when old interface dropped - clean way to work with parallel streams/commits on the same backing chain, which otherwise become a problem when we introduce a filter for stream job - cleaner interface. Nobody will surprised the fact that base node may disappear during block-stream, when there is no word about "base" in the interface. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 8 +++-- include/block/block_int.h | 1 + block/monitor/block-hmp-cmds.c | 3 +- block/stream.c | 50 +++++++++++++++++++--------- blockdev.c | 61 ++++++++++++++++++++++++++++------ 5 files changed, 94 insertions(+), 29 deletions(-)