[RFC,3/3] QMP: extend BLOCK_IO_ERROR event with no-space indicator
diff mbox

Message ID 1406121438-23083-4-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

Luiz Capitulino July 23, 2014, 1:17 p.m. UTC
Management software, such as OpenStack and 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 quering this event is already present in
query-block by means of the 'io-status' key and that 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'.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c              | 22 ++++++++++++++--------
 qapi/block-core.json |  7 ++++++-
 2 files changed, 20 insertions(+), 9 deletions(-)

Comments

Eric Blake Aug. 5, 2014, 12:19 p.m. UTC | #1
On 07/23/2014 07:17 AM, Luiz Capitulino wrote:
> Management software, such as OpenStack and 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.

I'd still like feedback from OpenStack or vdsm folks stating what they
do with this information, if a bool for ENOSPC is good enough.  In RHEV,
qemu was patched to provide a downstream-only enum of ENOSPC, EIO,
EPERM, and a catch-all for all other errors; if anyone cared about the
distinction between EIO/EPERM and all others, providing a bool for
ENOSPC is a loss of information.  On the other hand, as far as I can
tell, management above libvirt really cared only about ENOSPC (time to
grow the underlying disk) vs. all other errors (tell the user their
guest is hosed), in which case this simpler proposal seems fine to me.
The problem is that since I don't know the upper layer use case, I don't
know if we are painting ourselves into a corner by providing a bool that
we'd have to maintain forever, if we also end up having to provide
EIO/EPERM distinctions.

> 
> 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 quering this event is already present in
> query-block by means of the 'io-status' key and that 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'.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c              | 22 ++++++++++++++--------
>  qapi/block-core.json |  7 ++++++-
>  2 files changed, 20 insertions(+), 9 deletions(-)

Codewise, this looks correct.  Design-wise, I'd still like more input,
so I'm reluctant to give R-b without discussion.
Luiz Capitulino Aug. 14, 2014, 1:07 p.m. UTC | #2
On Tue, 05 Aug 2014 06:19:27 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/23/2014 07:17 AM, Luiz Capitulino wrote:
> > Management software, such as OpenStack and 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.
> 
> I'd still like feedback from OpenStack or vdsm folks stating what they
> do with this information, if a bool for ENOSPC is good enough.

You're right, that's the most important item for this series. Do you know
whom I should contact from OpenStack?

Patch
diff mbox

diff --git a/block.c b/block.c
index 8cf519b..566ef56 100644
--- a/block.c
+++ b/block.c
@@ -3596,6 +3596,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).
@@ -3621,16 +3633,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);
     }
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1069679..d659165 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1534,6 +1534,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
 #
@@ -1541,7 +1546,7 @@ 
 ##
 { 'event': 'BLOCK_IO_ERROR',
   'data': { 'device': 'str', 'operation': 'IoOperationType',
-            'action': 'BlockErrorAction' } }
+            'action': 'BlockErrorAction', '*nospace': 'bool' } }
 
 ##
 # @BLOCK_JOB_COMPLETED