Support streaming to an intermediate layer

Hello,

I added support to the Block Stream API for streaming to an intermediate layer.

I followed the proposed API from the wiki, which simply adds an additional 'top' parameter to block-stream specifying the image that data is written to:

Quoted 3 lines

I followed the proposed API from the wiki, which simply adds an additional 'top' parameter to block-stream specifying the image that data is written to:

How does one learn whether qemu is new enough to support this mode? Until we add QMP introspection, learning whether an optional parameter exists requires attempting the command and seeing a different error depending on whether the argument is recognized.

This adds the 'top' parameter to the 'block-stream' QMP command and checks that its value is valid before passing it to stream_start().

Signed-off-by Alberto Garcia <[email protected]>

---
 blockdev.c                | 19 +++++++++++++++----
 hmp.c                     |  2 +-
 include/qapi/qmp/qerror.h |  3 +++
 qapi/block-core.json      | 18 ++++++++++++++----
 qmp-commands.hx           |  2 +-
 5 files changed, 34 insertions(+), 10 deletions(-)

Quoted 4 lines · Eric Blake

Quoted 3 lines

I followed the proposed API from the wiki, which simply adds an additional 'top' parameter to block-stream specifying the image that data is written to:

Quoted 4 lines

How does one learn whether qemu is new enough to support this mode? Until we add QMP introspection, learning whether an optional parameter exists requires attempting the command and seeing a different error depending on whether the argument is recognized.

Isn't it possible to just check the QEMU version number?

If not, what's the recommended way to do it?

1. implement QAPI introspection (we've been dreaming about this since qemu 1.5 days), then the caller just queries to see if the version of QMP has the optional 'top' parameter.

2. implement the new feature as a new command (then the existing 'query-commands' becomes an easy probe; but we have code duplication with existing commands)

3. guarantee that we have sane error messages for bogus commands used as the probe. If {"execute":"block-stream", "data":{"device":"no-such","top":"bogus"}} gives a reliable DeviceNotFound error for qemu that supports optional 'top', and a reliable GenericError about an unknown argument 'top' in older qemu

blockdev.c void qmp_block_stream(const char *device, · line 2133

Quoted 6 lines

blockdev.c +6 -0

+    if (has_top) {
+        top_bs = bdrv_find_backing_image(bs, top);
+        if (top_bs == NULL) {
+            error_set(errp, QERR_TOP_NOT_FOUND, top);
+            goto out;
+        }

If I understand correctly, bdrv_find_backing_image has problems for backing nodes that don't have a file name. Given our shift towards node names, I think we really want to target node names rather than file names when specifying which node we will use as the top bound receiving the stream operations.

include/qapi/qmp/qerror.h void qerror_report_err(Error *err); · line 132

Quoted 3 lines

include/qapi/qmp/qerror.h +3 -0

+#define QERR_TOP_NOT_FOUND \
+    ERROR_CLASS_GENERIC_ERROR, "Top '%s' not found"
+

Please don't. Just use error_setg() at the right place with the direct message (existing QERR_ macros are a legacy holdover, and we shouldn't be creating more of them).

Quoted 8 lines

blockdev.c void qmp_block_stream(const char *device, · line 2133

Quoted 7 lines

blockdev.c +6 -0

+    if (has_top) {
+        top_bs = bdrv_find_backing_image(bs, top);
+        if (top_bs == NULL) {
+            error_set(errp, QERR_TOP_NOT_FOUND, top);
+            goto out;
+        }

Quoted 5 lines

If I understand correctly, bdrv_find_backing_image has problems for backing nodes that don't have a file name. Given our shift towards node names, I think we really want to target node names rather than file names when specifying which node we will use as the top bound receiving the stream operations.

Sure I can change that, but note that the 'base' parameter also receives a file name and uses bdrv_find_backing_image, so I guess it makes sense to change it in both sides.

Am 23.02.2015 um 13:23 hat Alberto Garcia geschrieben:

Quoted 15 lines · Eric Blake

Quoted 13 lines

blockdev.c void qmp_block_stream(const char *device, · line 2133

Quoted 7 lines

blockdev.c +6 -0

+    if (has_top) {
+        top_bs = bdrv_find_backing_image(bs, top);
+        if (top_bs == NULL) {
+            error_set(errp, QERR_TOP_NOT_FOUND, top);
+            goto out;
+        }

If I understand correctly, bdrv_find_backing_image has problems for backing nodes that don't have a file name. Given our shift towards node names, I think we really want to target node names rather than file names when specifying which node we will use as the top bound receiving the stream operations.

Quoted 3 lines

Sure I can change that, but note that the 'base' parameter also receives a file name and uses bdrv_find_backing_image, so I guess it makes sense to change it in both sides.

Yes, using the file name for identifying nodes was a mistake. We're going to replace all occurrences of it sooner or later. Not sure if someone is actively working on this currently - Markus?

For your patch series, I think it's good enough to use node names for the new parameter. Converting old parameters is a separate issue.

This adds a new 'top' parameter to stream_start(), that specifies the block device where the data will be written. The image is changed to read-write mode during the streaming and back to read-only afterwards.

This also unblocks the stream operation in backing files.

Signed-off-by Alberto Garcia <[email protected]>

This adds the 'top' parameter to the 'block-stream' QMP command and checks that its value is valid before passing it to stream_start().

Signed-off-by Alberto Garcia <[email protected]>

qapi/block-core.json +11 -2

@@ -1013,6 +1013,9 @@
 # with query-block-jobs.  The operation can be stopped before it has completed
 # using the block-job-cancel command.
 #
+# Data is copied to the top image, which defaults to the active layer if no other
+# file is selected.
+#
 # 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

@@ -1025,8 +1028,14 @@
 #
 # @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.
+# @top:    #optional Top image, only sectors below this image are streamed
+#                    into it.
+#
+#                    If not specified, the top image is the active layer.
+#                    (Since 2.3)
+#
+# @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

The bs parameter is now only used for the following things:

1. As the default for top

2. For error handling: Any errors are reported for bs, even though they are actually for top. Is this correct behaviour? It looks questionable to me.

3. As the BDS that owns the job

My question is whether we can't simply call stream_start() with an intermediate node as bs instead of introducing a new parameter. I'm not completely sure about the consequences of 3., i.e. moving ownership of a block job to some BDS somewhere down the chain, but otherwise it should be possible and seems cleaner.

qapi/block-core.json line 1066

Quoted 6 lines

{ 'command': 'block-stream',

qapi/block-core.json +3 -2

-  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+            '*backing-file': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError' } }

A better interface would probably be to allow node names for 'device' and leave everything else as it is.

There is no point in specifying some root node as 'device' that isn't actually involved in the operation; worse, it isn't even possible in the general case because 'top' could have multiple users/parents.

Quoted 4 lines

The bs parameter is now only used for the following things:

1. As the default for top

Right.

Quoted 3 lines

2. For error handling: Any errors are reported for bs, even though they are actually for top. Is this correct behaviour? It looks questionable to me.

Hmm... I guess you mean when calling block_job_error_action(), I probably overlooked that.

Quoted 7 lines

3. As the BDS that owns the job

My question is whether we can't simply call stream_start() with an intermediate node as bs instead of introducing a new parameter. I'm not completely sure about the consequences of 3., i.e. moving ownership of a block job to some BDS somewhere down the chain, but otherwise it should be possible and seems cleaner.

We can, that was actually the first thing I tried and it does work, but then I noticed that other parts of the code would need changes, e.g. qmp_query_block_jobs() must be modified to iterate over all nodes of each device.

Since I was also not sure about the consequences of such a change I opted for the conservative approach.

Quoted 9 lines

Quoted 8 lines

3. As the BDS that owns the job

My question is whether we can't simply call stream_start() with an intermediate node as bs instead of introducing a new parameter. I'm not completely sure about the consequences of 3., i.e. moving ownership of a block job to some BDS somewhere down the chain, but otherwise it should be possible and seems cleaner.

Quoted 2 lines

Since I was also not sure about the consequences of such a change I opted for the conservative approach.

I see. I'm worried about the external API. If we let the root node own the job, we may paint ourselves into a corner with respect to nodes with multiple users and therefore multiple possible root nodes.

qapi/block-core.json line 1066

Quoted 6 lines

{ 'command': 'block-stream',

qapi/block-core.json +3 -2

-  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+            '*backing-file': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError' } }

There is no point in specifying some root node as 'device' that isn't actually involved in the operation; worse, it isn't even possible in the general case because 'top' could have multiple users/parents.

A better interface would probably be to allow node names for 'device' and leave everything else as it is.

My question is whether we can't simply call stream_start() with an intermediate node as bs instead of introducing a new parameter. I'm not completely sure about the consequences of 3., i.e. moving ownership of a block job to some BDS somewhere down the chain, but otherwise it should be possible and seems cleaner.

Quoted 8 lines

qapi/block-core.json line 1066

Quoted 7 lines

{ 'command': 'block-stream',

qapi/block-core.json +3 -2

-  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+            '*backing-file': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError' } }

Quoted 7 lines

There is no point in specifying some root node as 'device' that isn't actually involved in the operation; worse, it isn't even possible in the general case because 'top' could have multiple users/parents.

A better interface would probably be to allow node names for 'device' and leave everything else as it is.

Ok, I changed the code and it does make the implementation simpler.

One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like "Device '' is busy".

I can use node names instead, but they are also not guaranteed to exist. I heard there was a plan to auto-generate names, and searching the archives I found this patch by Jeff Cody:

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg04057.html

But it seems that it was never merged?

If we are going to have a scenario where a parameter can mean either a device or a node name, we need a clear way to identify that node.

Quoted 6 lines

One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like "Device '' is busy".

I can use node names instead, but they are also not guaranteed to exist.

My first thought was "then make it 'Source/Target device is busy' without mentioning any name". In the context of any given command, it would still be clear which BDS is meant.

On the other hand, having an owner BDS for a block job is considered a mistake meanwhile because there is no clear rule which BDS to pick when the job involves more than one.

In fact, without tying a job to a BDS, it could be just a background job instead of specifically a block job.

The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because 1) it might not exist and 2) existing libvirt versions expect a device name.

So I see several alternatives:

a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility.

b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous.

c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option.

d) Create a new query command that returns a different data structure.

I would opt for a) or b), but I'd like to hear if you have a different opinion.

Regarding the 'block-stream' command, I think the current option to reuse the 'device' parameter to refer to either a device or a node name is ok, so I'll go forward with that one.

Quoted 4 lines

On the other hand, having an owner BDS for a block job is considered a mistake meanwhile because there is no clear rule which BDS to pick when the job involves more than one.

Does it really matter as long as all the operations blockers are correctly set?

Quoted 2 lines

In fact, without tying a job to a BDS, it could be just a background job instead of specifically a block job.

I don't understand what you mean by this.

Berto

Quoted 11 lines

Quoted 7 lines

One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like "Device '' is busy".

I can use node names instead, but they are also not guaranteed to exist.

My first thought was "then make it 'Source/Target device is busy' without mentioning any name". In the context of any given command, it would still be clear which BDS is meant.

There's a related problem that I discussed on IRC with Kevin and Eric but that I think needs further deliberation.

The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because 1) it might not exist and 2) existing libvirt versions expect a device name.

So I see several alternatives:

a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility.

b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous.

c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option.

d) Create a new query command that returns a different data structure.

I would opt for a) or b), but I'd like to hear if you have a different opinion.

Regarding the 'block-stream' command, I think the current option to reuse the 'device' parameter to refer to either a device or a node name is ok, so I'll go forward with that one.

Quoted 3 lines

On the other hand, having an owner BDS for a block job is considered a mistake meanwhile because there is no clear rule which BDS to pick when the job involves more than one.

Does it really matter as long as all the operations blockers are correctly set?

Quoted 2 lines

In fact, without tying a job to a BDS, it could be just a background job instead of specifically a block job.

I don't understand what you mean by this.

Quoted 13 lines

The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because
1) it might not exist and 2) existing libvirt versions expect a device name.

So I see several alternatives:

a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility.

Quoted 5 lines

b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous.

If libvirt is new enough to create the block job via node name instead of device name, then it is also new enough to expect a node name instead of device name in the returned job information.

Quoted 5 lines

c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option.

If you're going to reuse 'device' on the creation, then reuse it on the reporting.

Quoted 5 lines

d) Create a new query command that returns a different data structure.

I would opt for a) or b), but I'd like to hear if you have a different opinion.

I'm kind of leaning towards b).

Quoted 3 lines

One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like "Device '' is busy".

My first thought was "then make it 'Source/Target device is busy' without mentioning any name". In the context of any given command, it would still be clear which BDS is meant. In fact, I have argued before that mentioning the device name in an error to a command that refers to a specific device is redundant and should be avoided.

The problem here is that it's not stream_start() that generates the error, but block_job_create(), which doesn't know which role it's bs argument has for the block job. So it can't decide whether to say "source device", "target device" or something completely different.