block: extend BLOCK_IO_ERROR event with nospace indicator

Management software, such as RHEV's vdsm, want to be able to allocate disk space on demand. The basic use case is to start a VM with a small disk and then the disk is enlarged when QEMU hits a ENOSPC condition.

To this end, the management software has to be notified when QEMU encounters ENOSPC. The solution implemented by this commit is simple:
it extends the BLOCK_IO_ERROR with a 'nospace' key, which is true when QEMU is stopped due to ENOSPC.

Note that support for querying this event is already present in query-block by means of the 'io-status' key. Also, the new 'nospace' BLOCK_IO_ERROR field shares the same semantics with 'io-status', which basically means that werror= has to be set to either 'stop' or 'enospc' to enable 'nospace'.

Finally, this commit also updates the 'io-status' key doc in the schema with a list of supported device models.

Signed-off-by Luiz Capitulino <[email protected]>

---

Management software, such as RHEV's vdsm, want to be able to allocate disk space on demand. The basic use case is to start a VM with a small disk and then the disk is enlarged when QEMU hits a ENOSPC condition.

To this end, the management software has to be notified when QEMU encounters ENOSPC. The solution implemented by this commit is simple:
it extends the BLOCK_IO_ERROR with a 'nospace' key, which is true when QEMU is stopped due to ENOSPC.

Note that support for querying this event is already present in query-block by means of the 'io-status' key. Also, the new 'nospace' BLOCK_IO_ERROR field shares the same semantics with 'io-status', which basically means that werror= has to be set to either 'stop' or 'enospc' to enable 'nospace'.

Finally, this commit also updates the 'io-status' key doc in the schema with a list of supported device models.

Signed-off-by Luiz Capitulino <[email protected]>

---

Three important observations:

1. We've talked with oVirt and OpenStack folks. oVirt folks say that this implementation is enough for their use-case. OpenStack don't need this feature

2. While testing this with a raw image on a (smaller) ext2 file mounted via the loopback device, I get half "Invalid argument" I/O errors and half "No space" errors". This means that half of the BLOCK_IO_ERROR events that are emitted for this test-case will have nospace=false and the other half nospace=true. I don't know why I'm getting those "Invalid argument" errors, can anyone of the block layer comment on this? I don't get that with a qcow2 image (I get nospace=true for all events)

3. I think this should go via block tree

 block.c              | 22 ++++++++++++++--------
 qapi/block-core.json |  8 +++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

block.c +14 -8

index 1df13ac..b334e35 100644
@@ -3632,6 +3632,18 @@ BlockErrorAction bdrv_get_error_action(BlockDriverState *bs, bool is_read, int e
     }
 }
 
+static void send_qmp_error_event(BlockDriverState *bs,
+                                 BlockErrorAction action,
+                                 bool is_read, int error)
+{
+    BlockErrorAction ac;
+
+    ac = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
+    qapi_event_send_block_io_error(bdrv_get_device_name(bs), ac, action,
+                                   bdrv_iostatus_is_enabled(bs),
+                                   error == ENOSPC, &error_abort);
+}
+
 /* This is done by device models because, while the block layer knows
  * about the error, it does not know whether an operation comes from
  * the device or the block layer (from a job, for example).

@@ -3657,16 +3669,10 @@ void bdrv_error_action(BlockDriverState *bs, BlockErrorAction action,
          * also ensures that the STOP/RESUME pair of events is emitted.
          */
         qemu_system_vmstop_request_prepare();
-        qapi_event_send_block_io_error(bdrv_get_device_name(bs),
-                                       is_read ? IO_OPERATION_TYPE_READ :
-                                       IO_OPERATION_TYPE_WRITE,
-                                       action, &error_abort);
+        send_qmp_error_event(bs, action, is_read, error);
         qemu_system_vmstop_request(RUN_STATE_IO_ERROR);
     } else {
-        qapi_event_send_block_io_error(bdrv_get_device_name(bs),
-                                       is_read ? IO_OPERATION_TYPE_READ :
-                                       IO_OPERATION_TYPE_WRITE,
-                                       action, &error_abort);
+        send_qmp_error_event(bs, action, is_read, error);
     }
 }
 

qapi/block-core.json +7 -1

index fb74c56..567e0a6 100644
@@ -336,6 +336,7 @@
 #
 # @io-status: #optional @BlockDeviceIoStatus. Only present if the device
 #             supports it and the VM is configured to stop on errors
+#             (supported device models: virtio-blk, ide, scsi-disk)
 #
 # @inserted: #optional @BlockDeviceInfo describing the device if media is
 #            present

@@ -1569,6 +1570,11 @@
 #
 # @action: action that has been taken
 #
+# @nospace: #optional true if I/O error was caused due to a no-space
+#           condition. This key is only present if query-block's
+#           io-status is present, please see query-block documentation
+#           for more information (since: 2.2)
+#
 # Note: If action is "stop", a STOP event will eventually follow the
 # BLOCK_IO_ERROR event
 #

@@ -1576,7 +1582,7 @@
 ##
 { 'event': 'BLOCK_IO_ERROR',
   'data': { 'device': 'str', 'operation': 'IoOperationType',
-            'action': 'BlockErrorAction' } }
+            'action': 'BlockErrorAction', '*nospace': 'bool' } }
 
 ##
 # @BLOCK_JOB_COMPLETED

Quoted 15 lines · On Fri, 29 Aug 2014 16:07:27 -0400 Luiz Capitulino <[email protected]>…

Management software, such as RHEV's vdsm, want to be able to allocate disk space on demand. The basic use case is to start a VM with a small disk and then the disk is enlarged when QEMU hits a ENOSPC condition.

To this end, the management software has to be notified when QEMU encounters ENOSPC. The solution implemented by this commit is simple:
it extends the BLOCK_IO_ERROR with a 'nospace' key, which is true when QEMU is stopped due to ENOSPC.

Note that support for querying this event is already present in query-block by means of the 'io-status' key. Also, the new 'nospace' BLOCK_IO_ERROR field shares the same semantics with 'io-status', which basically means that werror= has to be set to either 'stop' or 'enospc' to enable 'nospace'.

Quoted 4 lines

Finally, this commit also updates the 'io-status' key doc in the schema with a list of supported device models.

Signed-off-by Luiz Capitulino <[email protected]>

Kevin, are you going to take this via block layer tree?

Quoted 8 lines

---

Three important observations:

1. We've talked with oVirt and OpenStack folks. oVirt folks say that this implementation is enough for their use-case. OpenStack don't need this feature

Quoted 8 lines

2. While testing this with a raw image on a (smaller) ext2 file mounted via the loopback device, I get half "Invalid argument" I/O errors and half "No space" errors". This means that half of the BLOCK_IO_ERROR events that are emitted for this test-case will have nospace=false and the other half nospace=true. I don't know why I'm getting those "Invalid argument" errors, can anyone of the block layer comment on this? I don't get that with a qcow2 image (I get nospace=true for all events)

Quoted 5 lines

3. I think this should go via block tree

 block.c              | 22 ++++++++++++++--------
 qapi/block-core.json |  8 +++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

Am 08.09.2014 um 16:42 hat Luiz Capitulino geschrieben:

Quoted 21 lines

Quoted 18 lines · On Fri, 29 Aug 2014 16:07:27 -0400 Luiz Capitulino <[email protected]>…

Management software, such as RHEV's vdsm, want to be able to allocate disk space on demand. The basic use case is to start a VM with a small disk and then the disk is enlarged when QEMU hits a ENOSPC condition.

To this end, the management software has to be notified when QEMU encounters ENOSPC. The solution implemented by this commit is simple:
it extends the BLOCK_IO_ERROR with a 'nospace' key, which is true when QEMU is stopped due to ENOSPC.

Note that support for querying this event is already present in query-block by means of the 'io-status' key. Also, the new 'nospace' BLOCK_IO_ERROR field shares the same semantics with 'io-status', which basically means that werror= has to be set to either 'stop' or 'enospc' to enable 'nospace'.

Finally, this commit also updates the 'io-status' key doc in the schema with a list of supported device models.

Quoted 3 lines

Quoted 1 line

Signed-off-by Luiz Capitulino <[email protected]>

Kevin, are you going to take this via block layer tree?

Yes, thanks, I've applied it now.

What was our conclusion wrt the human-readable strerror() string for debugging? Didn't we want to add that as well?

Quoted 9 lines

Quoted 9 lines

---

Three important observations:

1. We've talked with oVirt and OpenStack folks. oVirt folks say that this implementation is enough for their use-case. OpenStack don't need this feature

Quoted 8 lines

Quoted 8 lines

2. While testing this with a raw image on a (smaller) ext2 file mounted via the loopback device, I get half "Invalid argument" I/O errors and half "No space" errors". This means that half of the BLOCK_IO_ERROR events that are emitted for this test-case will have nospace=false and the other half nospace=true. I don't know why I'm getting those "Invalid argument" errors, can anyone of the block layer comment on this? I don't get that with a qcow2 image (I get nospace=true for all events)

Sounds familiar, but I never got around to debugging. Would probably be worth some digging where the EINVAL comes from.

Quoted 5 lines

Quoted 5 lines

3. I think this should go via block tree

 block.c              | 22 ++++++++++++++--------
 qapi/block-core.json |  8 +++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

Kevin

Quoted 26 lines · On Mon, 8 Sep 2014 17:33:18 +0200 Kevin Wolf <[email protected]> wrote:

Am 08.09.2014 um 16:42 hat Luiz Capitulino geschrieben:

Quoted 24 lines

Quoted 15 lines · On Fri, 29 Aug 2014 16:07:27 -0400 Luiz Capitulino <[email protected]>…

Management software, such as RHEV's vdsm, want to be able to allocate disk space on demand. The basic use case is to start a VM with a small disk and then the disk is enlarged when QEMU hits a ENOSPC condition.

To this end, the management software has to be notified when QEMU encounters ENOSPC. The solution implemented by this commit is simple:
it extends the BLOCK_IO_ERROR with a 'nospace' key, which is true when QEMU is stopped due to ENOSPC.

Note that support for querying this event is already present in query-block by means of the 'io-status' key. Also, the new 'nospace' BLOCK_IO_ERROR field shares the same semantics with 'io-status', which basically means that werror= has to be set to either 'stop' or 'enospc' to enable 'nospace'.

Quoted 4 lines

Finally, this commit also updates the 'io-status' key doc in the schema with a list of supported device models.

Signed-off-by Luiz Capitulino <[email protected]>

Kevin, are you going to take this via block layer tree?

Quoted 4 lines

Yes, thanks, I've applied it now.

What was our conclusion wrt the human-readable strerror() string for debugging? Didn't we want to add that as well?

I can do it on top of this patch. So, just adding a new field for this is fine?

Quoted 6 lines

Quoted 3 lines

What was our conclusion wrt the human-readable strerror() string for debugging? Didn't we want to add that as well?

I can do it on top of this patch. So, just adding a new field for this is fine?

I think so. Perhaps we should give it an 'x-' name to make clear that it's a debugging help and not supposed to be parsed by management tools. Or would that be abuse of that namespace?

The alternative solution (or actually we could do both) would be to store it somewhere in bs and put it into query-block.

Quoted 10 lines · Kevin Wolf

Quoted 6 lines

Quoted 3 lines

What was our conclusion wrt the human-readable strerror() string for debugging? Didn't we want to add that as well?

I can do it on top of this patch. So, just adding a new field for this is fine?

I think so. Perhaps we should give it an 'x-' name to make clear that it's a debugging help and not supposed to be parsed by management tools. Or would that be abuse of that namespace?

I think using x- would be okay for the namespace, but am not sure we need to go that far. We already have other fields without x- that are documented as human-readable only; for example, CommandLineParameterInfo has:

# @help: #optional human readable text string, not suitable for parsing.

or in our events, QUORUM_REPORT_BAD has:

# @error: #optional, error message. Only present on failure. This field
# contains a human-readable error message. There are no
semantics other
# than that the block layer reported an error and clients should not
# try to interpret the error string.

So I'd be fine with something as simple as 'message' or 'error'.

Quoted 1 line

Quoted 2 lines

The alternative solution (or actually we could do both) would be to store it somewhere in bs and put it into query-block.

Enhancing query-block in addition to the event makes sense, if it is easy enough to do. At this point, we are talking about debugging aids, so as long as they are documented appropriately, I won't be too fussy.

Quoted 3 lines

Enhancing query-block in addition to the event makes sense, if it is easy enough to do. At this point, we are talking about debugging aids, so as long as they are documented appropriately, I won't be too fussy.

OK, but I'm wondering if we need to add the string field to both, BLOCK_IO_ERROR and query-block, or only to one to the other.

In my opinion, we should only add it to BLOCK_IO_ERROR if libvirt is going to consume. Otherwise, it makes more sense to add it to query-block because that's where we'll meet the user.

Btw, by "consume" I mean read it and make it available to libvirt clients so that they can print it to their users. If we don't want libvirt to consume that field then I think we should only add it to query-block and info block.

Quoted 8 lines

Quoted 4 lines

Enhancing query-block in addition to the event makes sense, if it is easy enough to do. At this point, we are talking about debugging aids, so as long as they are documented appropriately, I won't be too fussy.

OK, but I'm wondering if we need to add the string field to both, BLOCK_IO_ERROR and query-block, or only to one to the other.

Quoted 8 lines

In my opinion, we should only add it to BLOCK_IO_ERROR if libvirt is going to consume. Otherwise, it makes more sense to add it to query-block because that's where we'll meet the user.

Btw, by "consume" I mean read it and make it available to libvirt clients so that they can print it to their users. If we don't want libvirt to consume that field then I think we should only add it to query-block and info block.

[For those not aware, qemu built for downstream RHEL already has an error string in the __com.redhat_ namespace; we're trying to figure out what upstream should have so that downstream doesn't have to perpetually maintain an extension]

Downstream libvirt does not currently consume any error string. With downstream qemu, the _only_ way to get the error string in the event is to parse libvirt logs, or use upstream libvirt with its backdoor of 'virsh qemu-monitor-event' (through the explicitly unsupported libvirt-qemu.so) to get at the raw event information. Changing libvirt to expose such an error string to the end user would require a new libvirt event number (the existing libvirt event is not extensible), so existing clients would not be able to get at the information without being recompiled to a new libvirt.

Since the whole point of this field is for debugging, I think that it is sufficient to add it to JUST query-block, and not to the event. That is, if the app on top of libvirt gets an error, in the common case, they won't care about the message (it won't change how they act), and in the debug case, a developer trying to learn more about what happened can do their own query-block directly (via 'virsh qemu-monitor-command', also in libvirt-qemu.so) rather than trying to wire up libvirt to pass through an error string through a new event.

Am 09.09.2014 um 14:53 hat Eric Blake geschrieben:

Quoted 33 lines · Luiz Capitulino

Quoted 8 lines

Quoted 4 lines

Enhancing query-block in addition to the event makes sense, if it is easy enough to do. At this point, we are talking about debugging aids, so as long as they are documented appropriately, I won't be too fussy.

OK, but I'm wondering if we need to add the string field to both, BLOCK_IO_ERROR and query-block, or only to one to the other.

Quoted 8 lines

In my opinion, we should only add it to BLOCK_IO_ERROR if libvirt is going to consume. Otherwise, it makes more sense to add it to query-block because that's where we'll meet the user.

Btw, by "consume" I mean read it and make it available to libvirt clients so that they can print it to their users. If we don't want libvirt to consume that field then I think we should only add it to query-block and info block.

[For those not aware, qemu built for downstream RHEL already has an error string in the __com.redhat_ namespace; we're trying to figure out what upstream should have so that downstream doesn't have to perpetually maintain an extension]

Downstream libvirt does not currently consume any error string. With downstream qemu, the _only_ way to get the error string in the event is to parse libvirt logs, or use upstream libvirt with its backdoor of 'virsh qemu-monitor-event' (through the explicitly unsupported libvirt-qemu.so) to get at the raw event information. Changing libvirt to expose such an error string to the end user would require a new libvirt event number (the existing libvirt event is not extensible), so existing clients would not be able to get at the information without being recompiled to a new libvirt.

Quoted 8 lines

Since the whole point of this field is for debugging, I think that it is sufficient to add it to JUST query-block, and not to the event. That is, if the app on top of libvirt gets an error, in the common case, they won't care about the message (it won't change how they act), and in the debug case, a developer trying to learn more about what happened can do their own query-block directly (via 'virsh qemu-monitor-command', also in libvirt-qemu.so) rather than trying to wire up libvirt to pass through an error string through a new event.

You mention libvirt logs and I think that's an important point: If we move it to query-block, will we still get a log entry so that we can check this after the fact when only a log file is attached to a bug report, but we don't have a running guest?