| Message ID | 1c8e6e73084e968514b6522576cfacd1bf69609e.1475757437.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/06/2016 08:02 AM, Alberto Garcia wrote: > This patch makes the 'device' parameter of the 'block-stream' command > accept a node name that is not a root node. > > In addition to that, operation blockers will be checked in all > intermediate nodes between the top and the base node. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockdev.c | 11 +++++++++-- > qapi/block-core.json | 10 +++++++--- > 2 files changed, 16 insertions(+), 5 deletions(-) > > +++ b/qapi/block-core.json > @@ -1469,6 +1469,10 @@ > # with query-block-jobs. The operation can be stopped before it has completed > # using the block-job-cancel command. > # > +# The node that receives the data is called the top image, can be located in > +# any part of the chain (but always above the base image; see below) and can be > +# specified using its device or node name. > +# > # If a base file is specified then sectors are not copied from that base file and > # its backing chain. When streaming completes the image file will have the base > # file as its backing file. This can be used to stream a subset of the backing > @@ -1480,12 +1484,12 @@ > # @job-id: #optional identifier for the newly-created block job. If > # omitted, the device name will be used. (Since 2.7) > # > -# @device: the device name or node-name of a root node > +# @device: the device or node name of the top image > # > # @base: #optional the common backing file name > # > -# @backing-file: #optional The backing file string to write into the active > -# layer. This filename is not validated. > +# @backing-file: #optional The backing file string to write into the top > +# image. This filename is not validated. No change to the actual introspection. What's the easiest way for libvirt to learn if the new style command will work, short of actually trying it to see if it succeeds or fails? (That may be sufficient, but the error message quality can often be improved in libvirt if it is known up front if qemu is new enough rather than taking the 'try and see' approach and getting stuck with qemu's error message)
On Mon 10 Oct 2016 09:09:25 PM CEST, Eric Blake wrote: >> # @job-id: #optional identifier for the newly-created block job. If >> # omitted, the device name will be used. (Since 2.7) >> # >> -# @device: the device name or node-name of a root node >> +# @device: the device or node name of the top image >> # >> # @base: #optional the common backing file name >> # >> -# @backing-file: #optional The backing file string to write into the active >> -# layer. This filename is not validated. >> +# @backing-file: #optional The backing file string to write into the top >> +# image. This filename is not validated. > > No change to the actual introspection. What's the easiest way for > libvirt to learn if the new style command will work, short of actually > trying it to see if it succeeds or fails? (That may be sufficient, > but the error message quality can often be improved in libvirt if it > is known up front if qemu is new enough rather than taking the 'try > and see' approach and getting stuck with qemu's error message) I don't think there's any straightforward way. Some ideas: 1) Try to start a block-stream job that does nothing. When base == backing_bs(device) that's a no-op, but it fails if device is not the root node and intermediate block streaming is not supported. 2) We could add an extra parameter. For example 'base-node', which would be the same as 'base' but would take a node name instead of a file name. 3) QEMU could advertise that feature to the client. This is probably simpler than trying to figure it out from the API. I guess that's the idea of 'qmp_capabilities'? Berto
Am 11.10.2016 um 16:30 hat Alberto Garcia geschrieben: > On Mon 10 Oct 2016 09:09:25 PM CEST, Eric Blake wrote: > >> # @job-id: #optional identifier for the newly-created block job. If > >> # omitted, the device name will be used. (Since 2.7) > >> # > >> -# @device: the device name or node-name of a root node > >> +# @device: the device or node name of the top image > >> # > >> # @base: #optional the common backing file name > >> # > >> -# @backing-file: #optional The backing file string to write into the active > >> -# layer. This filename is not validated. > >> +# @backing-file: #optional The backing file string to write into the top > >> +# image. This filename is not validated. > > > > No change to the actual introspection. What's the easiest way for > > libvirt to learn if the new style command will work, short of actually > > trying it to see if it succeeds or fails? (That may be sufficient, > > but the error message quality can often be improved in libvirt if it > > is known up front if qemu is new enough rather than taking the 'try > > and see' approach and getting stuck with qemu's error message) > > I don't think there's any straightforward way. Some ideas: > > 1) Try to start a block-stream job that does nothing. When base == > backing_bs(device) that's a no-op, but it fails if device is not the > root node and intermediate block streaming is not supported. > > 2) We could add an extra parameter. For example 'base-node', which would > be the same as 'base' but would take a node name instead of a file name. Oh, we still have those filename-based parameters? :-/ Should we introduce a new, clean blockdev-stream command that fixes this and matches the common name pattern? Of course, block-stream vs. blockdev-stream could be a bit confusing, too... > 3) QEMU could advertise that feature to the client. This is probably > simpler than trying to figure it out from the API. I guess that's the > idea of 'qmp_capabilities'? I think that was the idea, though it was never used. If we had used it, I'm not sure how long the capabilities list would be today. :-) Kevin
On 10/11/2016 09:57 AM, Kevin Wolf wrote: > Should we introduce a new, clean blockdev-stream command that fixes this > and matches the common name pattern? Of course, block-stream vs. > blockdev-stream could be a bit confusing, too... > A new command is easy to introspect (query-commands), lets us get rid of cruft, and makes it obvious that we support everything from the get-go. I'm favoring that option, even if it leads to slightly confusing names of the deprecated vs. the new command.
Kevin Wolf <kwolf@redhat.com> writes: > Am 11.10.2016 um 16:30 hat Alberto Garcia geschrieben: [...] >> 3) QEMU could advertise that feature to the client. This is probably >> simpler than trying to figure it out from the API. I guess that's the >> idea of 'qmp_capabilities'? > > I think that was the idea, though it was never used. If we had used it, > I'm not sure how long the capabilities list would be today. :-) QMP capabilities are for changes in the QMP protocol, not for changes in commands, events and types. The protocol has been good enough so far, thus no capabilities.
Eric Blake <eblake@redhat.com> writes: > On 10/11/2016 09:57 AM, Kevin Wolf wrote: >> Should we introduce a new, clean blockdev-stream command that fixes this >> and matches the common name pattern? Of course, block-stream vs. >> blockdev-stream could be a bit confusing, too... >> > > A new command is easy to introspect (query-commands), lets us get rid of > cruft, and makes it obvious that we support everything from the get-go. > I'm favoring that option, even if it leads to slightly confusing names > of the deprecated vs. the new command. Let's take a step back and consider extending old commands vs. adding new commands. A new command is trivial to detect in introspection. Command extensions are not as trivial to detect in introspection. Many extensions are exposed in query-qmp-schema, but not all. Back when QMP was young, Anthony argued for always adding new commands, never extend existing ones. I opposed it, because it would lead to a confusing mess of related commands, unreadable or incomplete documentation, and abysmal test coverage. However, the other extreme is also unwise: we shouldn't shoehorn new functionality into existing commands just because we can. We should ask ourselves questions like these: * Is the extended command still a sane interface? If writing clear documentation for it is hard, it perhaps isn't. Pay special attention to failure modes. Overloaded arguments are prone to confusing errors. * How will the command's users use the extension? If it requires new code paths, a new command may be more convenient for them.
Am 11.10.2016 um 18:50 hat Markus Armbruster geschrieben: > Eric Blake <eblake@redhat.com> writes: > > > On 10/11/2016 09:57 AM, Kevin Wolf wrote: > >> Should we introduce a new, clean blockdev-stream command that fixes this > >> and matches the common name pattern? Of course, block-stream vs. > >> blockdev-stream could be a bit confusing, too... > >> > > > > A new command is easy to introspect (query-commands), lets us get rid of > > cruft, and makes it obvious that we support everything from the get-go. > > I'm favoring that option, even if it leads to slightly confusing names > > of the deprecated vs. the new command. > > Let's take a step back and consider extending old commands vs. adding > new commands. > > A new command is trivial to detect in introspection. > > Command extensions are not as trivial to detect in introspection. Many > extensions are exposed in query-qmp-schema, but not all. > > Back when QMP was young, Anthony argued for always adding new commands, > never extend existing ones. I opposed it, because it would lead to a > confusing mess of related commands, unreadable or incomplete > documentation, and abysmal test coverage. > > However, the other extreme is also unwise: we shouldn't shoehorn new > functionality into existing commands just because we can. We should ask > ourselves questions like these: > > * Is the extended command still a sane interface? If writing clear > documentation for it is hard, it perhaps isn't. Pay special attention > to failure modes. Overloaded arguments are prone to confusing errors. It has never been a sane interface in the first place (identifying a backing file node by its filename). We ended up having two versions of all block job commands anyway (one that creates an image file, and later one that just takes a node-name of an existing node), except for image streaming so far. So it would be consistent (and enable consistent naming for the preferred commands) to have it here, too. > * How will the command's users use the extension? If it requires new > code paths, a new command may be more convenient for them. Kevin
On Tue 11 Oct 2016 06:50:27 PM CEST, Markus Armbruster wrote: > * Is the extended command still a sane interface? If writing clear > documentation for it is hard, it perhaps isn't. Pay special > attention to failure modes. Overloaded arguments are prone to > confusing errors. This is what the current command looks like: { 'command': 'block-stream', 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*backing-file': 'str', '*speed': 'int', '*on-error': 'BlockdevOnError' } } If we decide to add a new command, this is what it could look like: { 'command': 'blockdev-stream', 'data': { '*job-id': 'str', 'top': 'str', '*base': 'str', '*backing-file': 'str', '*speed': 'int', '*on-error': 'BlockdevOnError' } } If we decide to extend the existing command, there's essentially two changes that we have to do: 1) 'device' refers to a device name, it should refer to (or allow) a node name. This is trivial to do, the only problem is that the name of the parameter is not the best. 2) 'base' takes a file name, but we should have a way to pass a node name instead. Overloading here is not an option, we need a new parameter ('base-node' or something like that). 'base' and 'base-node' would be optional but mutually exclusive. { 'command': 'block-stream', 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*base-node': 'str', '*backing-file': 'str', '*speed': 'int', '*on-error': 'BlockdevOnError' } } Considering that a new command would be very similar to the original one (the only problems being an ill-named parameter and an obsolete one), I actually don't think that extending the current command is such a bad idea. But I don't have a strong opinion. Berto
On Tue 11 Oct 2016 06:32:39 PM CEST, Markus Armbruster wrote: >>> 3) QEMU could advertise that feature to the client. This is probably >>> simpler than trying to figure it out from the API. I guess that's >>> the idea of 'qmp_capabilities'? >> >> I think that was the idea, though it was never used. If we had used >> it, I'm not sure how long the capabilities list would be today. :-) > > QMP capabilities are for changes in the QMP protocol, not for changes > in commands, events and types. The protocol has been good enough so > far, thus no capabilities. I see. Wouldn't it make sense in general to have a way to ask QEMU whether some certain feature is supported? Berto
Alberto Garcia <berto@igalia.com> writes: > On Tue 11 Oct 2016 06:32:39 PM CEST, Markus Armbruster wrote: > >>>> 3) QEMU could advertise that feature to the client. This is probably >>>> simpler than trying to figure it out from the API. I guess that's >>>> the idea of 'qmp_capabilities'? >>> >>> I think that was the idea, though it was never used. If we had used >>> it, I'm not sure how long the capabilities list would be today. :-) >> >> QMP capabilities are for changes in the QMP protocol, not for changes >> in commands, events and types. The protocol has been good enough so >> far, thus no capabilities. > > I see. Wouldn't it make sense in general to have a way to ask QEMU > whether some certain feature is supported? Feature bits work fine for interfaces that see relatively little change. Some QEMU interfaces are like that, for instance the QMP protocol itself. Other interfaces, however, change at a rapid pace. If we tried to provide a feature bit for every change there, we'd end up with a huge number of them, almost certainly underdocumented, and most of them not used by any client. We'd probably fail to provide feature bits for some of the interface changes, and have "feature X" bits followed some time later by "feature X, except now it actually works" bits. Instead, we opted for making external interfaces introspectable, so that clients can detect stuff whether we thought of the need to detect it or not. The first interface with decent introspection is QMP: there's query-qmp-schema. Changes are easy to detect there as long as they're *syntactic*: new commands or events, type extensions and so forth. QMP introspection is blind to purely *semantic* changes, say a string argument that can now denote a node-name in addition to a filename. Such changes should be avoided.
Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben: > This patch makes the 'device' parameter of the 'block-stream' command > accept a node name that is not a root node. > > In addition to that, operation blockers will be checked in all > intermediate nodes between the top and the base node. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockdev.c | 11 +++++++++-- > qapi/block-core.json | 10 +++++++--- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index f13fc69..57c8329 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, > bool has_on_error, BlockdevOnError on_error, > Error **errp) > { > - BlockDriverState *bs; > + BlockDriverState *bs, *iter; > BlockDriverState *base_bs = NULL; > AioContext *aio_context; > Error *local_err = NULL; > @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, > on_error = BLOCKDEV_ON_ERROR_REPORT; > } > > - bs = qmp_get_root_bs(device, errp); > + bs = bdrv_lookup_bs(device, device, errp); > if (!bs) { > return; > } > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { > goto out; > } Added a bit more context. This check is redundant now... > if (has_base) { > base_bs = bdrv_find_backing_image(bs, base); > if (base_bs == NULL) { > error_setg(errp, QERR_BASE_NOT_FOUND, base); > goto out; > } > assert(bdrv_get_aio_context(base_bs) == aio_context); > base_name = base; > } > > + /* Check for op blockers in the whole chain between bs and base */ > + for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) { > + if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) { > + goto out; > + } > + } ...because you start with iter = bs here. Another thought I had while looking at the previous few patches is whether all the op blocker checks wouldn't be better moved to the actual block job code (i.e. stream_start in this case). In this case it doesn't matter much because this is the only caller of stream_start(), but for other jobs the situation is more complicated and it would be easy for a caller to forget the checks. Probably also matter for another series, but I wanted to mention it. > + > /* if we are streaming the entire chain, the result will have no backing > * file, and specifying one is therefore an error */ > if (base_bs == NULL && has_backing_file) { Kevin
On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote: >> if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { >> goto out; >> } > > Added a bit more context. > > This check is redundant now... > >> if (has_base) { >> base_bs = bdrv_find_backing_image(bs, base); >> if (base_bs == NULL) { >> error_setg(errp, QERR_BASE_NOT_FOUND, base); >> goto out; >> } >> assert(bdrv_get_aio_context(base_bs) == aio_context); >> base_name = base; >> } >> >> + /* Check for op blockers in the whole chain between bs and base */ >> + for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) { >> + if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) { >> + goto out; >> + } >> + } > > ...because you start with iter = bs here. You're right, I'll fix it. > Another thought I had while looking at the previous few patches is > whether all the op blocker checks wouldn't be better moved to the > actual block job code (i.e. stream_start in this case). I thought about that too. In some cases I don't know if it's a good idea because the qmp_foo_bar() functions do a bit more than simply checking blockers (e.g. blockdev-mirror creates the target image), so you would want to have the checks before that. However doing it in the actual block job code could allow us to do other things. For example: at the moment when we call block-stream we check whether a number of BDSs are blocked (in qmp_block_stream()), and if they're not we proceed to block them (in stream_start()). We could make block_job_add_bdrv() do both things. On the other hand, since the plan is to move to a new block job API maybe it's better not to overdo things now. It's worth considering for the future anyway. Berto
diff --git a/blockdev.c b/blockdev.c index f13fc69..57c8329 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bool has_on_error, BlockdevOnError on_error, Error **errp) { - BlockDriverState *bs; + BlockDriverState *bs, *iter; BlockDriverState *base_bs = NULL; AioContext *aio_context; Error *local_err = NULL; @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, on_error = BLOCKDEV_ON_ERROR_REPORT; } - bs = qmp_get_root_bs(device, errp); + bs = bdrv_lookup_bs(device, device, errp); if (!bs) { return; } @@ -2969,6 +2969,13 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, base_name = base; } + /* Check for op blockers in the whole chain between bs and base */ + for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) { + if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) { + goto out; + } + } + /* if we are streaming the entire chain, the result will have no backing * file, and specifying one is therefore an error */ if (base_bs == NULL && has_backing_file) { diff --git a/qapi/block-core.json b/qapi/block-core.json index 9d797b8..88f4c55 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1469,6 +1469,10 @@ # with query-block-jobs. The operation can be stopped before it has completed # using the block-job-cancel command. # +# The node that receives the data is called the top image, can be located in +# any part of the chain (but always above the base image; see below) and can be +# specified using its device or node name. +# # If a base file is specified then sectors are not copied from that base file and # its backing chain. When streaming completes the image file will have the base # file as its backing file. This can be used to stream a subset of the backing @@ -1480,12 +1484,12 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the device name or node-name of a root node +# @device: the device or node name of the top image # # @base: #optional the common backing file name # -# @backing-file: #optional The backing file string to write into the active -# layer. This filename is not validated. +# @backing-file: #optional The backing file string to write into the top +# image. This filename is not validated. # # If a pathname string is such that it cannot be # resolved by QEMU, that means that subsequent QMP or
This patch makes the 'device' parameter of the 'block-stream' command accept a node name that is not a root node. In addition to that, operation blockers will be checked in all intermediate nodes between the top and the base node. Signed-off-by: Alberto Garcia <berto@igalia.com> --- blockdev.c | 11 +++++++++-- qapi/block-core.json | 10 +++++++--- 2 files changed, 16 insertions(+), 5 deletions(-)