[Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Luiz Capitulino]

This series adds support to the block layer to keep track of devices'
I/O status. That information is also made available in QMP and HMP.

The goal here is to allow management applications that miss the
BLOCK_IO_ERROR event to able to query the VM to determine if any device has
caused the VM to stop and which device caused it.

Please, note that this series depends on the following series:

 o [PATCH v3 0/8]: Introduce the RunState type
 o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html

And to be able to properly test it you'll also need:

 o [PATCH 0/3] qcow2/coroutine fixes
 o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html

Here's an HMP example:

  (qemu) info status 
  VM status: paused (io-error)
  (qemu) info block
  ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
  ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
  ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
  floppy0: removable=1 locked=0 [not inserted]
  sd0: removable=1 locked=0 [not inserted]

The "info status" command shows that the VM is stopped due to an I/O error.
By issuing "info block" it's possible to determine that the 'ide0-hd1' device
caused the error, which turns out to be due to no space.

 block.c         |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 block.h         |    9 +++++++++
 block_int.h     |    1 +
 hw/ide/core.c   |    2 ++
 hw/scsi-disk.c  |    2 ++
 hw/virtio-blk.c |    2 ++
 monitor.c       |    6 ++++++
 qmp-commands.hx |    5 +++++
 8 files changed, 77 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

This commit adds support to the BlockDriverState type to keep track
of the devices' I/O status.

There are three possible status: BDRV_IOS_OK (no error so far),
BDRV_IOS_ENOSPC (no space error) and BDRV_IOS_FAILED (any other error).
The distinction between no space and other errors is important because
a management application may want to watch for no space in order to
extend the host's underline device and put the VM to run again.

Qemu devices supporting the I/O status feature have to enable it
explicitly by calling bdrv_iostatus_enable() _and_ have to be
configured to stop the VM on errors (ie. werror=stop|enospc or
rerror=stop).

In case of multiple errors being triggered in sequence only the first
one is stored. The I/O status is set back to BDRV_IOS_OK when the
'cont' command is issued.

Next commits will add support to some devices and extend the
query-block/info block commands with the I/O status information.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c     |   32 ++++++++++++++++++++++++++++++++
 block.h     |    9 +++++++++
 block_int.h |    1 +
 monitor.c   |    6 ++++++
 4 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 03a21d8..c54caf2 100644
--- a/block.c
+++ b/block.c
@@ -220,6 +220,7 @@ BlockDriverState *bdrv_new(const char *device_name)
     if (device_name[0] != '\0') {
         QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
     }
+    bs->iostatus = BDRV_IOS_INVAL;
     return bs;
 }
 
@@ -3165,6 +3166,37 @@ int bdrv_in_use(BlockDriverState *bs)
     return bs->in_use;
 }
 
+void bdrv_iostatus_enable(BlockDriverState *bs)
+{
+    assert(bs->iostatus == BDRV_IOS_INVAL);
+    bs->iostatus = BDRV_IOS_OK;
+}
+
+/* The I/O status is only enabled if the drive explicitly
+ * enables it _and_ the VM is configured to stop on errors */
+bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
+{
+    return (bs->iostatus != BDRV_IOS_INVAL &&
+           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
+            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
+            bs->on_read_error == BLOCK_ERR_STOP_ANY));
+}
+
+void bdrv_iostatus_reset(BlockDriverState *bs)
+{
+    if (bdrv_iostatus_is_enabled(bs)) {
+        bs->iostatus = BDRV_IOS_OK;
+    }
+}
+
+void bdrv_iostatus_update(BlockDriverState *bs, int error)
+{
+    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
+        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
+                                                BDRV_IOS_FAILED;
+    }
+}
+
 void
 bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
         enum BlockAcctType type)
diff --git a/block.h b/block.h
index 3ac0b94..24914e0 100644
--- a/block.h
+++ b/block.h
@@ -51,6 +51,15 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockMonEventAction;
 
+typedef enum {
+    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
+    BDRV_IOS_MAX
+} BlockIOStatus;
+
+void bdrv_iostatus_enable(BlockDriverState *bs);
+void bdrv_iostatus_reset(BlockDriverState *bs);
+bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
+void bdrv_iostatus_update(BlockDriverState *bs, int error);
 void bdrv_mon_event(const BlockDriverState *bdrv,
                     BlockMonEventAction action, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
diff --git a/block_int.h b/block_int.h
index 8a72b80..600801d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -203,6 +203,7 @@ struct BlockDriverState {
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
     BlockErrorAction on_read_error, on_write_error;
+    BlockIOStatus iostatus;
     char device_name[32];
     unsigned long *dirty_bitmap;
     int64_t dirty_count;
diff --git a/monitor.c b/monitor.c
index 9807005..69f1b3c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1302,6 +1302,11 @@ struct bdrv_iterate_context {
     int err;
 };
 
+static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
+{
+    bdrv_iostatus_reset(bs);
+}
+
 /**
  * do_cont(): Resume emulation.
  */
@@ -1318,6 +1323,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
+    bdrv_iterate(iostatus_bdrv_it, NULL);
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err) {
-- 
1.7.7.rc0.72.g4b5ea

[Qemu-devel] [PATCH 2/6] virtio: Support I/O status

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/virtio-blk.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 8a33720..bd81586 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -78,6 +78,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         s->rq = req;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RSTATE_IO_ERROR);
+        bdrv_iostatus_update(s->bs, error);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_acct_done(s->bs, &req->acct);
@@ -602,6 +603,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
     bdrv_set_change_cb(s->bs, virtio_blk_change_cb, s);
     s->bs->buffer_alignment = conf->logical_block_size;
 
+    bdrv_iostatus_enable(s->bs);
     add_boot_device_path(conf->bootindex, dev, "/disk@0,0");
 
     return &s->vdev;
-- 
1.7.7.rc0.72.g4b5ea

[Qemu-devel] [PATCH 3/6] ide: Support I/O status

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/ide/core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 41f2679..16b422f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -527,6 +527,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->error_status = op;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RSTATE_IO_ERROR);
+        bdrv_iostatus_update(s->bs, error);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
             dma_buf_commit(s, 0);
@@ -1801,6 +1802,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
     }
 
     ide_reset(s);
+    bdrv_iostatus_enable(bs);
     bdrv_set_removable(bs, s->drive_kind == IDE_CD);
     return 0;
 }
-- 
1.7.7.rc0.72.g4b5ea

[Qemu-devel] [PATCH 4/6] scsi: Support I/O status

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/scsi-disk.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 9f451bc..85b1ce0 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -193,6 +193,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
 
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RSTATE_IO_ERROR);
+        bdrv_iostatus_update(s->bs, error);
     } else {
         switch (error) {
         case ENOMEM:
@@ -1168,6 +1169,7 @@ static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type)
 
     s->qdev.type = scsi_type;
     qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
+    bdrv_iostatus_enable(s->bs);
     bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
     return 0;
-- 
1.7.7.rc0.72.g4b5ea

[Qemu-devel] [PATCH 5/6] QMP: query-status: Add 'io-status' key

[Luiz Capitulino]

Contains the I/O status for the given device. The key is only present
if the device supports it and the VM is configured to stop on errors.

Please, check the documentation being added in this commit for more
information.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c         |   15 ++++++++++++++-
 qmp-commands.hx |    5 +++++
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index c54caf2..77579b6 100644
--- a/block.c
+++ b/block.c
@@ -1865,6 +1865,12 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
     qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
 }
 
+static const char *const io_status_name[BDRV_IOS_MAX] = {
+    [BDRV_IOS_OK] = "ok",
+    [BDRV_IOS_FAILED] = "failed",
+    [BDRV_IOS_ENOSPC] = "nospace",
+};
+
 void bdrv_info(Monitor *mon, QObject **ret_data)
 {
     QList *bs_list;
@@ -1874,15 +1880,16 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         QObject *bs_obj;
+        QDict *bs_dict;
 
         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
                                     "'removable': %i, 'locked': %i }",
                                     bs->device_name, bs->removable,
                                     bs->locked);
+        bs_dict = qobject_to_qdict(bs_obj);
 
         if (bs->drv) {
             QObject *obj;
-            QDict *bs_dict = qobject_to_qdict(bs_obj);
 
             obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
                                      "'encrypted': %i }",
@@ -1897,6 +1904,12 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
             qdict_put_obj(bs_dict, "inserted", obj);
         }
+
+        if (bdrv_iostatus_is_enabled(bs)) {
+            qdict_put(bs_dict, "io-status",
+                      qstring_from_str(io_status_name[bs->iostatus]));
+        }
+
         qlist_append_obj(bs_list, bs_obj);
     }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1fbda8c..c045043 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1143,6 +1143,9 @@ Each json-object contain the following:
                                 "tftp", "vdi", "vmdk", "vpc", "vvfat"
          - "backing_file": backing file name (json-string, optional)
          - "encrypted": true if encrypted, false otherwise (json-bool)
+- "io-status": I/O operation status, only present if the device supports it
+               and the VM is configured to stop on errors (json_string,optional)
+             - Possible values: "ok", "failed", "nospace"
 
 Example:
 
@@ -1150,6 +1153,7 @@ Example:
 <- {
       "return":[
          {
+            "io-status": "ok",
             "device":"ide0-hd0",
             "locked":false,
             "removable":false,
@@ -1162,6 +1166,7 @@ Example:
             "type":"unknown"
          },
          {
+            "io-status": "ok",
             "device":"ide1-cd0",
             "locked":false,
             "removable":true,
-- 
1.7.7.rc0.72.g4b5ea

[Qemu-devel] [PATCH 6/6] HMP: Print 'io-status' information

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 77579b6..ac9e913 100644
--- a/block.c
+++ b/block.c
@@ -1840,6 +1840,10 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
         monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
     }
 
+    if (qdict_haskey(bs_dict, "io-status")) {
+        monitor_printf(mon, " io-status=%s", qdict_get_str(bs_dict, "io-status"));
+    }
+
     if (qdict_haskey(bs_dict, "inserted")) {
         QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
 
-- 
1.7.7.rc0.72.g4b5ea

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Luiz Capitulino]

On Thu,  1 Sep 2011 15:37:49 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> This series adds support to the block layer to keep track of devices'
> I/O status. That information is also made available in QMP and HMP.

ping?

I'll have to rebase it anyway, but I'd like to know if the general approach
is acceptable.

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Kevin Wolf]

Am 01.09.2011 20:37, schrieb Luiz Capitulino:
> This series adds support to the block layer to keep track of devices'
> I/O status. That information is also made available in QMP and HMP.
> 
> The goal here is to allow management applications that miss the
> BLOCK_IO_ERROR event to able to query the VM to determine if any device has
> caused the VM to stop and which device caused it.
> 
> Please, note that this series depends on the following series:
> 
>  o [PATCH v3 0/8]: Introduce the RunState type
>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
> 
> And to be able to properly test it you'll also need:
> 
>  o [PATCH 0/3] qcow2/coroutine fixes
>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
> 
> Here's an HMP example:
> 
>   (qemu) info status 
>   VM status: paused (io-error)
>   (qemu) info block
>   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
>   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
>   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
>   floppy0: removable=1 locked=0 [not inserted]
>   sd0: removable=1 locked=0 [not inserted]
> 
> The "info status" command shows that the VM is stopped due to an I/O error.
> By issuing "info block" it's possible to determine that the 'ide0-hd1' device
> caused the error, which turns out to be due to no space.

Looks like I didn't reply here yet?

I still don't quite like that the devices are involved, but their part
is minimal and it makes the implementation much easier, so for me that's
acceptable.

Kevin

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Luiz Capitulino]

On Mon, 19 Sep 2011 15:40:06 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 01.09.2011 20:37, schrieb Luiz Capitulino:
> > This series adds support to the block layer to keep track of devices'
> > I/O status. That information is also made available in QMP and HMP.
> > 
> > The goal here is to allow management applications that miss the
> > BLOCK_IO_ERROR event to able to query the VM to determine if any device has
> > caused the VM to stop and which device caused it.
> > 
> > Please, note that this series depends on the following series:
> > 
> >  o [PATCH v3 0/8]: Introduce the RunState type
> >  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
> > 
> > And to be able to properly test it you'll also need:
> > 
> >  o [PATCH 0/3] qcow2/coroutine fixes
> >  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
> > 
> > Here's an HMP example:
> > 
> >   (qemu) info status 
> >   VM status: paused (io-error)
> >   (qemu) info block
> >   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
> >   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
> >   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
> >   floppy0: removable=1 locked=0 [not inserted]
> >   sd0: removable=1 locked=0 [not inserted]
> > 
> > The "info status" command shows that the VM is stopped due to an I/O error.
> > By issuing "info block" it's possible to determine that the 'ide0-hd1' device
> > caused the error, which turns out to be due to no space.
> 
> Looks like I didn't reply here yet?

No, you didn't.

> I still don't quite like that the devices are involved, but their part
> is minimal and it makes the implementation much easier, so for me that's
> acceptable.

Suggestions on better ways of implementing this are welcome! :)

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Kevin Wolf]

Am 19.09.2011 16:09, schrieb Luiz Capitulino:
> On Mon, 19 Sep 2011 15:40:06 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 01.09.2011 20:37, schrieb Luiz Capitulino:
>>> This series adds support to the block layer to keep track of devices'
>>> I/O status. That information is also made available in QMP and HMP.
>>>
>>> The goal here is to allow management applications that miss the
>>> BLOCK_IO_ERROR event to able to query the VM to determine if any device has
>>> caused the VM to stop and which device caused it.
>>>
>>> Please, note that this series depends on the following series:
>>>
>>>  o [PATCH v3 0/8]: Introduce the RunState type
>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
>>>
>>> And to be able to properly test it you'll also need:
>>>
>>>  o [PATCH 0/3] qcow2/coroutine fixes
>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
>>>
>>> Here's an HMP example:
>>>
>>>   (qemu) info status 
>>>   VM status: paused (io-error)
>>>   (qemu) info block
>>>   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
>>>   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
>>>   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
>>>   floppy0: removable=1 locked=0 [not inserted]
>>>   sd0: removable=1 locked=0 [not inserted]
>>>
>>> The "info status" command shows that the VM is stopped due to an I/O error.
>>> By issuing "info block" it's possible to determine that the 'ide0-hd1' device
>>> caused the error, which turns out to be due to no space.
>>
>> Looks like I didn't reply here yet?
> 
> No, you didn't.
> 
>> I still don't quite like that the devices are involved, but their part
>> is minimal and it makes the implementation much easier, so for me that's
>> acceptable.
> 
> Suggestions on better ways of implementing this are welcome! :)

I don't have one. :-)

Surely it would be possible to do everything in block.c, but I see that
this would make things much more complicated (would need to get an AIO
callback added to the chain that can save the error code). It's not
worth the trouble.

Kevin

[Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

This commit adds support to the BlockDriverState type to keep track
of devices' I/O status.

There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
(no space error) and BDRV_IOS_FAILED (any other error). The distinction
between no space and other errors is important because a management
application may want to watch for no space in order to extend the
space assigned to the VM and put it to run again.

Qemu devices supporting the I/O status feature have to enable it
explicitly by calling bdrv_iostatus_enable() _and_ have to be
configured to stop the VM on errors (ie. werror=stop|enospc or
rerror=stop).

In case of multiple errors being triggered in sequence only the first
one is stored. The I/O status is always reset to BDRV_IOS_OK when the
'cont' command is issued.

Next commits will add support to some devices and extend the
query-block/info block commands to return the I/O status information.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c     |   32 ++++++++++++++++++++++++++++++++
 block.h     |    9 +++++++++
 block_int.h |    1 +
 monitor.c   |    6 ++++++
 4 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index e3fe97f..fbd90b4 100644
--- a/block.c
+++ b/block.c
@@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
     if (device_name[0] != '\0') {
         QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
     }
+    bs->iostatus = BDRV_IOS_INVAL;
     return bs;
 }
 
@@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
     return bs->in_use;
 }
 
+void bdrv_iostatus_enable(BlockDriverState *bs)
+{
+    assert(bs->iostatus == BDRV_IOS_INVAL);
+    bs->iostatus = BDRV_IOS_OK;
+}
+
+/* The I/O status is only enabled if the drive explicitly
+ * enables it _and_ the VM is configured to stop on errors */
+bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
+{
+    return (bs->iostatus != BDRV_IOS_INVAL &&
+           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
+            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
+            bs->on_read_error == BLOCK_ERR_STOP_ANY));
+}
+
+void bdrv_iostatus_reset(BlockDriverState *bs)
+{
+    if (bdrv_iostatus_is_enabled(bs)) {
+        bs->iostatus = BDRV_IOS_OK;
+    }
+}
+
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
+{
+    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
+        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
+                                                BDRV_IOS_FAILED;
+    }
+}
+
 void
 bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
         enum BlockAcctType type)
diff --git a/block.h b/block.h
index 16bfa0a..de74af0 100644
--- a/block.h
+++ b/block.h
@@ -77,6 +77,15 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockMonEventAction;
 
+typedef enum {
+    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
+    BDRV_IOS_MAX
+} BlockIOStatus;
+
+void bdrv_iostatus_enable(BlockDriverState *bs);
+void bdrv_iostatus_reset(BlockDriverState *bs);
+bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
 void bdrv_mon_event(const BlockDriverState *bdrv,
                     BlockMonEventAction action, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
diff --git a/block_int.h b/block_int.h
index 8c3b863..f2f4f2d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -199,6 +199,7 @@ struct BlockDriverState {
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
     BlockErrorAction on_read_error, on_write_error;
+    BlockIOStatus iostatus;
     char device_name[32];
     unsigned long *dirty_bitmap;
     int64_t dirty_count;
diff --git a/monitor.c b/monitor.c
index 8ec2c5e..88d8228 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1304,6 +1304,11 @@ struct bdrv_iterate_context {
     int err;
 };
 
+static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
+{
+    bdrv_iostatus_reset(bs);
+}
+
 /**
  * do_cont(): Resume emulation.
  */
@@ -1320,6 +1325,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
+    bdrv_iterate(iostatus_bdrv_it, NULL);
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err) {
-- 
1.7.7.rc0.72.g4b5ea

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Zhi Yong Wu]

On Fri, Sep 23, 2011 at 2:19 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> This commit adds support to the BlockDriverState type to keep track
> of devices' I/O status.
>
> There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
> (no space error) and BDRV_IOS_FAILED (any other error). The distinction
> between no space and other errors is important because a management
> application may want to watch for no space in order to extend the
> space assigned to the VM and put it to run again.
>
> Qemu devices supporting the I/O status feature have to enable it
> explicitly by calling bdrv_iostatus_enable() _and_ have to be
> configured to stop the VM on errors (ie. werror=stop|enospc or
> rerror=stop).
>
> In case of multiple errors being triggered in sequence only the first
> one is stored. The I/O status is always reset to BDRV_IOS_OK when the
> 'cont' command is issued.
>
> Next commits will add support to some devices and extend the
> query-block/info block commands to return the I/O status information.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c     |   32 ++++++++++++++++++++++++++++++++
>  block.h     |    9 +++++++++
>  block_int.h |    1 +
>  monitor.c   |    6 ++++++
>  4 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index e3fe97f..fbd90b4 100644
> --- a/block.c
> +++ b/block.c
> @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>     if (device_name[0] != '\0') {
>         QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>     }
> +    bs->iostatus = BDRV_IOS_INVAL;
>     return bs;
>  }
bs->iostatus is set to BDRV_IOS_INVAL only in bdrv_new(), if the drive
is opened and closed repeatly, how about the field?
Moreover, it seems that it has not been reset when the drive is closed
via bdrv_close().

>
> @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
>     return bs->in_use;
>  }
>
> +void bdrv_iostatus_enable(BlockDriverState *bs)
> +{
> +    assert(bs->iostatus == BDRV_IOS_INVAL);
> +    bs->iostatus = BDRV_IOS_OK;
> +}
> +
> +/* The I/O status is only enabled if the drive explicitly
> + * enables it _and_ the VM is configured to stop on errors */
> +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
> +{
> +    return (bs->iostatus != BDRV_IOS_INVAL &&
> +           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
> +            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
> +            bs->on_read_error == BLOCK_ERR_STOP_ANY));
> +}
> +
> +void bdrv_iostatus_reset(BlockDriverState *bs)
> +{
> +    if (bdrv_iostatus_is_enabled(bs)) {
> +        bs->iostatus = BDRV_IOS_OK;
> +    }
> +}
> +
> +void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
> +{
> +    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
> +        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
> +                                                BDRV_IOS_FAILED;
> +    }
> +}
> +
>  void
>  bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
>         enum BlockAcctType type)
> diff --git a/block.h b/block.h
> index 16bfa0a..de74af0 100644
> --- a/block.h
> +++ b/block.h
> @@ -77,6 +77,15 @@ typedef enum {
>     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>  } BlockMonEventAction;
>
> +typedef enum {
> +    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
> +    BDRV_IOS_MAX
> +} BlockIOStatus;
> +
> +void bdrv_iostatus_enable(BlockDriverState *bs);
> +void bdrv_iostatus_reset(BlockDriverState *bs);
> +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
> +void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
>  void bdrv_mon_event(const BlockDriverState *bdrv,
>                     BlockMonEventAction action, int is_read);
>  void bdrv_info_print(Monitor *mon, const QObject *data);
> diff --git a/block_int.h b/block_int.h
> index 8c3b863..f2f4f2d 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -199,6 +199,7 @@ struct BlockDriverState {
>        drivers. They are not used by the block driver */
>     int cyls, heads, secs, translation;
>     BlockErrorAction on_read_error, on_write_error;
> +    BlockIOStatus iostatus;
>     char device_name[32];
>     unsigned long *dirty_bitmap;
>     int64_t dirty_count;
> diff --git a/monitor.c b/monitor.c
> index 8ec2c5e..88d8228 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1304,6 +1304,11 @@ struct bdrv_iterate_context {
>     int err;
>  };
>
> +static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> +{
> +    bdrv_iostatus_reset(bs);
> +}
> +
>  /**
>  * do_cont(): Resume emulation.
>  */
> @@ -1320,6 +1325,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>         return -1;
>     }
>
> +    bdrv_iterate(iostatus_bdrv_it, NULL);
>     bdrv_iterate(encrypted_bdrv_it, &context);
>     /* only resume the vm if all keys are set and valid */
>     if (!context.err) {
> --
> 1.7.7.rc0.72.g4b5ea
>
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This commit adds support to the BlockDriverState type to keep track
> of devices' I/O status.
>
> There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
> (no space error) and BDRV_IOS_FAILED (any other error). The distinction
> between no space and other errors is important because a management
> application may want to watch for no space in order to extend the
> space assigned to the VM and put it to run again.
>
> Qemu devices supporting the I/O status feature have to enable it
> explicitly by calling bdrv_iostatus_enable() _and_ have to be
> configured to stop the VM on errors (ie. werror=stop|enospc or
> rerror=stop).
>
> In case of multiple errors being triggered in sequence only the first
> one is stored. The I/O status is always reset to BDRV_IOS_OK when the
> 'cont' command is issued.
>
> Next commits will add support to some devices and extend the
> query-block/info block commands to return the I/O status information.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c     |   32 ++++++++++++++++++++++++++++++++
>  block.h     |    9 +++++++++
>  block_int.h |    1 +
>  monitor.c   |    6 ++++++
>  4 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index e3fe97f..fbd90b4 100644
> --- a/block.c
> +++ b/block.c
> @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>      if (device_name[0] != '\0') {
>          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>      }
> +    bs->iostatus = BDRV_IOS_INVAL;
>      return bs;
>  }
>  
> @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
>      return bs->in_use;
>  }
>  
> +void bdrv_iostatus_enable(BlockDriverState *bs)
> +{
> +    assert(bs->iostatus == BDRV_IOS_INVAL);
> +    bs->iostatus = BDRV_IOS_OK;
> +}

bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
that do tracking declare that by calling this function during
initialization.  Enables tracking if the BDS has suitable on_write_error
and on_read_error.

If a device gets hot unplugged, tracking remains enabled.  If the BDS
then gets reused with a device that doesn't do tracking, I/O status
becomes incorrect.  Can't happen right now, because we automatically
delete the BDS on hot unplug, but it's a trap.

Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().

> +
> +/* The I/O status is only enabled if the drive explicitly
> + * enables it _and_ the VM is configured to stop on errors */
> +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
> +{
> +    return (bs->iostatus != BDRV_IOS_INVAL &&
> +           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
> +            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
> +            bs->on_read_error == BLOCK_ERR_STOP_ANY));
> +}
> +
> +void bdrv_iostatus_reset(BlockDriverState *bs)
> +{
> +    if (bdrv_iostatus_is_enabled(bs)) {
> +        bs->iostatus = BDRV_IOS_OK;
> +    }
> +}
> +
> +void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
> +{
> +    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
> +        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
> +                                                BDRV_IOS_FAILED;
> +    }
> +}
> +

abs(error) feels... unusual.

If you want to guard against callers passing wrongly signed values, why
not simply assert(error >= 0)?

>  void
>  bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
>          enum BlockAcctType type)
> diff --git a/block.h b/block.h
> index 16bfa0a..de74af0 100644
> --- a/block.h
> +++ b/block.h
> @@ -77,6 +77,15 @@ typedef enum {
>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>  } BlockMonEventAction;
>  
> +typedef enum {
> +    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
> +    BDRV_IOS_MAX
> +} BlockIOStatus;

Why is this in block.h?

> +
> +void bdrv_iostatus_enable(BlockDriverState *bs);
> +void bdrv_iostatus_reset(BlockDriverState *bs);
> +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
> +void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
>  void bdrv_mon_event(const BlockDriverState *bdrv,
>                      BlockMonEventAction action, int is_read);
>  void bdrv_info_print(Monitor *mon, const QObject *data);
> diff --git a/block_int.h b/block_int.h
> index 8c3b863..f2f4f2d 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -199,6 +199,7 @@ struct BlockDriverState {
>         drivers. They are not used by the block driver */
>      int cyls, heads, secs, translation;
>      BlockErrorAction on_read_error, on_write_error;
> +    BlockIOStatus iostatus;
>      char device_name[32];
>      unsigned long *dirty_bitmap;
>      int64_t dirty_count;
> diff --git a/monitor.c b/monitor.c
> index 8ec2c5e..88d8228 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1304,6 +1304,11 @@ struct bdrv_iterate_context {
>      int err;
>  };
>  
> +static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> +{
> +    bdrv_iostatus_reset(bs);
> +}
> +
>  /**
>   * do_cont(): Resume emulation.
>   */
> @@ -1320,6 +1325,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          return -1;
>      }
>  
> +    bdrv_iterate(iostatus_bdrv_it, NULL);
>      bdrv_iterate(encrypted_bdrv_it, &context);
>      /* only resume the vm if all keys are set and valid */
>      if (!context.err) {

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> This commit adds support to the BlockDriverState type to keep track
>> of devices' I/O status.
>>
>> There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
>> (no space error) and BDRV_IOS_FAILED (any other error). The distinction
>> between no space and other errors is important because a management
>> application may want to watch for no space in order to extend the
>> space assigned to the VM and put it to run again.
>>
>> Qemu devices supporting the I/O status feature have to enable it
>> explicitly by calling bdrv_iostatus_enable() _and_ have to be
>> configured to stop the VM on errors (ie. werror=stop|enospc or
>> rerror=stop).
>>
>> In case of multiple errors being triggered in sequence only the first
>> one is stored. The I/O status is always reset to BDRV_IOS_OK when the
>> 'cont' command is issued.
>>
>> Next commits will add support to some devices and extend the
>> query-block/info block commands to return the I/O status information.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  block.c     |   32 ++++++++++++++++++++++++++++++++
>>  block.h     |    9 +++++++++
>>  block_int.h |    1 +
>>  monitor.c   |    6 ++++++
>>  4 files changed, 48 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e3fe97f..fbd90b4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>>      if (device_name[0] != '\0') {
>>          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>>      }
>> +    bs->iostatus = BDRV_IOS_INVAL;
>>      return bs;
>>  }
>>  
>> @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
>>      return bs->in_use;
>>  }
>>  
>> +void bdrv_iostatus_enable(BlockDriverState *bs)
>> +{
>> +    assert(bs->iostatus == BDRV_IOS_INVAL);
>> +    bs->iostatus = BDRV_IOS_OK;
>> +}
>
> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
> that do tracking declare that by calling this function during
> initialization.  Enables tracking if the BDS has suitable on_write_error
> and on_read_error.
>
> If a device gets hot unplugged, tracking remains enabled.  If the BDS
> then gets reused with a device that doesn't do tracking, I/O status
> becomes incorrect.  Can't happen right now, because we automatically
> delete the BDS on hot unplug, but it's a trap.

And if the BDS gets reused with a device that does tracking, the
assertion above fails.

> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().

[...]

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 19.09.2011 16:09, schrieb Luiz Capitulino:
>> On Mon, 19 Sep 2011 15:40:06 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>> 
>>> Am 01.09.2011 20:37, schrieb Luiz Capitulino:
>>>> This series adds support to the block layer to keep track of devices'
>>>> I/O status. That information is also made available in QMP and HMP.
>>>>
>>>> The goal here is to allow management applications that miss the
>>>> BLOCK_IO_ERROR event to able to query the VM to determine if any device has
>>>> caused the VM to stop and which device caused it.
>>>>
>>>> Please, note that this series depends on the following series:
>>>>
>>>>  o [PATCH v3 0/8]: Introduce the RunState type
>>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
>>>>
>>>> And to be able to properly test it you'll also need:
>>>>
>>>>  o [PATCH 0/3] qcow2/coroutine fixes
>>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
>>>>
>>>> Here's an HMP example:
>>>>
>>>>   (qemu) info status 
>>>>   VM status: paused (io-error)
>>>>   (qemu) info block
>>>>   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
>>>>   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
>>>>   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
>>>>   floppy0: removable=1 locked=0 [not inserted]
>>>>   sd0: removable=1 locked=0 [not inserted]
>>>>
>>>> The "info status" command shows that the VM is stopped due to an I/O error.
>>>> By issuing "info block" it's possible to determine that the 'ide0-hd1' device
>>>> caused the error, which turns out to be due to no space.
>>>
>>> Looks like I didn't reply here yet?
>> 
>> No, you didn't.
>> 
>>> I still don't quite like that the devices are involved, but their part
>>> is minimal and it makes the implementation much easier, so for me that's
>>> acceptable.
>> 
>> Suggestions on better ways of implementing this are welcome! :)
>
> I don't have one. :-)
>
> Surely it would be possible to do everything in block.c, but I see that
> this would make things much more complicated (would need to get an AIO
> callback added to the chain that can save the error code). It's not
> worth the trouble.

The block layer necessarily detects errors in a huge number of places.

It also has a rich and complex interface to device models, with error
reporting in many places.

virtio-blk, ide and scsi-disk each have a single error handler to handle
block layer errors.

For better or worse, these are the only block layer error "chokepoints"
we have now, and therefore the only easy places to set BDS I/O status.
But they're still wrong.

The block layer shouldn't require device models to tell it what it
already knows.

Now, I don't want to block your series just because it adds this wart.
But the wart should be documented in the code.

Also document that any device model implementing werror=stop|enospc or
rerror=stop must update I/O status.  Extra points if you find a way to
enforce it instead.

Apropos enforcing.  Currently, -drive accepts any werror and rerror
action with if={ide,virtio,scsi,none}.  We rely on device models not
implementing an action to check and fail during initialization.
scsi-generic and the floppy devices do.  All the other qdevified devices
don't, and that's broken.

Are werror and rerror really host state?

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Kevin Wolf]

Am 23.09.2011 10:55, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 19.09.2011 16:09, schrieb Luiz Capitulino:
>>> On Mon, 19 Sep 2011 15:40:06 +0200
>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>
>>>> Am 01.09.2011 20:37, schrieb Luiz Capitulino:
>>>>> This series adds support to the block layer to keep track of devices'
>>>>> I/O status. That information is also made available in QMP and HMP.
>>>>>
>>>>> The goal here is to allow management applications that miss the
>>>>> BLOCK_IO_ERROR event to able to query the VM to determine if any device has
>>>>> caused the VM to stop and which device caused it.
>>>>>
>>>>> Please, note that this series depends on the following series:
>>>>>
>>>>>  o [PATCH v3 0/8]: Introduce the RunState type
>>>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
>>>>>
>>>>> And to be able to properly test it you'll also need:
>>>>>
>>>>>  o [PATCH 0/3] qcow2/coroutine fixes
>>>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
>>>>>
>>>>> Here's an HMP example:
>>>>>
>>>>>   (qemu) info status 
>>>>>   VM status: paused (io-error)
>>>>>   (qemu) info block
>>>>>   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
>>>>>   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
>>>>>   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
>>>>>   floppy0: removable=1 locked=0 [not inserted]
>>>>>   sd0: removable=1 locked=0 [not inserted]
>>>>>
>>>>> The "info status" command shows that the VM is stopped due to an I/O error.
>>>>> By issuing "info block" it's possible to determine that the 'ide0-hd1' device
>>>>> caused the error, which turns out to be due to no space.
>>>>
>>>> Looks like I didn't reply here yet?
>>>
>>> No, you didn't.
>>>
>>>> I still don't quite like that the devices are involved, but their part
>>>> is minimal and it makes the implementation much easier, so for me that's
>>>> acceptable.
>>>
>>> Suggestions on better ways of implementing this are welcome! :)
>>
>> I don't have one. :-)
>>
>> Surely it would be possible to do everything in block.c, but I see that
>> this would make things much more complicated (would need to get an AIO
>> callback added to the chain that can save the error code). It's not
>> worth the trouble.
> 
> The block layer necessarily detects errors in a huge number of places.
> 
> It also has a rich and complex interface to device models, with error
> reporting in many places.
> 
> virtio-blk, ide and scsi-disk each have a single error handler to handle
> block layer errors.
> 
> For better or worse, these are the only block layer error "chokepoints"
> we have now, and therefore the only easy places to set BDS I/O status.
> But they're still wrong.
> 
> The block layer shouldn't require device models to tell it what it
> already knows.
> 
> Now, I don't want to block your series just because it adds this wart.
> But the wart should be documented in the code.
> 
> Also document that any device model implementing werror=stop|enospc or
> rerror=stop must update I/O status.  Extra points if you find a way to
> enforce it instead.
> 
> Apropos enforcing.  Currently, -drive accepts any werror and rerror
> action with if={ide,virtio,scsi,none}.  We rely on device models not
> implementing an action to check and fail during initialization.
> scsi-generic and the floppy devices do.  All the other qdevified devices
> don't, and that's broken.
> 
> Are werror and rerror really host state?

If you want to convince me of the opposite, show me the real device that
implements them.

In fact, for the implementation of rerror/werror the very same thing is
true as you say about I/O status updates. Other than being easy to
implement, there is really no reason to do it in the devices. The block
layer could just as well stop the VM and queue the requests for
resubmission when the VM is restarted. Of course, you can't keep
migration compatibility when doing such a fundamental change to the
design, but if you take the host state thing serious, this is what you
should do.

Kevin

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.09.2011 10:55, schrieb Markus Armbruster:
[...]
>> Apropos enforcing.  Currently, -drive accepts any werror and rerror
>> action with if={ide,virtio,scsi,none}.  We rely on device models not
>> implementing an action to check and fail during initialization.
>> scsi-generic and the floppy devices do.  All the other qdevified devices
>> don't, and that's broken.
>> 
>> Are werror and rerror really host state?
>
> If you want to convince me of the opposite, show me the real device that
> implements them.

It's an honest question.

> In fact, for the implementation of rerror/werror the very same thing is
> true as you say about I/O status updates. Other than being easy to
> implement, there is really no reason to do it in the devices. The block
> layer could just as well stop the VM and queue the requests for
> resubmission when the VM is restarted. Of course, you can't keep
> migration compatibility when doing such a fundamental change to the
> design, but if you take the host state thing serious, this is what you
> should do.

Good answer.  I'd love to have it spelled out in the code.

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Luiz Capitulino]

On Fri, 23 Sep 2011 10:55:39 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 19.09.2011 16:09, schrieb Luiz Capitulino:
> >> On Mon, 19 Sep 2011 15:40:06 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >> 
> >>> Am 01.09.2011 20:37, schrieb Luiz Capitulino:
> >>>> This series adds support to the block layer to keep track of devices'
> >>>> I/O status. That information is also made available in QMP and HMP.
> >>>>
> >>>> The goal here is to allow management applications that miss the
> >>>> BLOCK_IO_ERROR event to able to query the VM to determine if any device has
> >>>> caused the VM to stop and which device caused it.
> >>>>
> >>>> Please, note that this series depends on the following series:
> >>>>
> >>>>  o [PATCH v3 0/8]: Introduce the RunState type
> >>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
> >>>>
> >>>> And to be able to properly test it you'll also need:
> >>>>
> >>>>  o [PATCH 0/3] qcow2/coroutine fixes
> >>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
> >>>>
> >>>> Here's an HMP example:
> >>>>
> >>>>   (qemu) info status 
> >>>>   VM status: paused (io-error)
> >>>>   (qemu) info block
> >>>>   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
> >>>>   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
> >>>>   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
> >>>>   floppy0: removable=1 locked=0 [not inserted]
> >>>>   sd0: removable=1 locked=0 [not inserted]
> >>>>
> >>>> The "info status" command shows that the VM is stopped due to an I/O error.
> >>>> By issuing "info block" it's possible to determine that the 'ide0-hd1' device
> >>>> caused the error, which turns out to be due to no space.
> >>>
> >>> Looks like I didn't reply here yet?
> >> 
> >> No, you didn't.
> >> 
> >>> I still don't quite like that the devices are involved, but their part
> >>> is minimal and it makes the implementation much easier, so for me that's
> >>> acceptable.
> >> 
> >> Suggestions on better ways of implementing this are welcome! :)
> >
> > I don't have one. :-)
> >
> > Surely it would be possible to do everything in block.c, but I see that
> > this would make things much more complicated (would need to get an AIO
> > callback added to the chain that can save the error code). It's not
> > worth the trouble.
> 
> The block layer necessarily detects errors in a huge number of places.
> 
> It also has a rich and complex interface to device models, with error
> reporting in many places.
> 
> virtio-blk, ide and scsi-disk each have a single error handler to handle
> block layer errors.
> 
> For better or worse, these are the only block layer error "chokepoints"
> we have now, and therefore the only easy places to set BDS I/O status.
> But they're still wrong.
> 
> The block layer shouldn't require device models to tell it what it
> already knows.
> 
> Now, I don't want to block your series just because it adds this wart.
> But the wart should be documented in the code.

Is something like:

 /* XXX: this should be implemented in the block layer */

good enough?

> Also document that any device model implementing werror=stop|enospc or
> rerror=stop must update I/O status.  Extra points if you find a way to
> enforce it instead.

Why? It's probably a nice thing because we can know which device caused
the VM to stop, but I don't see it as a must have (it's probably a
no brainer to update the I/O status though).

> 
> Apropos enforcing.  Currently, -drive accepts any werror and rerror
> action with if={ide,virtio,scsi,none}.  We rely on device models not
> implementing an action to check and fail during initialization.
> scsi-generic and the floppy devices do.  All the other qdevified devices
> don't, and that's broken.

I think I can fix that, but this series doesn't depend on that, right?
(in the meaning that we could merge this before getting that problem fixed).

> 
> Are werror and rerror really host state?
> 

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

On Fri, 23 Sep 2011 09:51:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This commit adds support to the BlockDriverState type to keep track
> > of devices' I/O status.
> >
> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction
> > between no space and other errors is important because a management
> > application may want to watch for no space in order to extend the
> > space assigned to the VM and put it to run again.
> >
> > Qemu devices supporting the I/O status feature have to enable it
> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
> > configured to stop the VM on errors (ie. werror=stop|enospc or
> > rerror=stop).
> >
> > In case of multiple errors being triggered in sequence only the first
> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
> > 'cont' command is issued.
> >
> > Next commits will add support to some devices and extend the
> > query-block/info block commands to return the I/O status information.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  block.c     |   32 ++++++++++++++++++++++++++++++++
> >  block.h     |    9 +++++++++
> >  block_int.h |    1 +
> >  monitor.c   |    6 ++++++
> >  4 files changed, 48 insertions(+), 0 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index e3fe97f..fbd90b4 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
> >      if (device_name[0] != '\0') {
> >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> >      }
> > +    bs->iostatus = BDRV_IOS_INVAL;
> >      return bs;
> >  }
> >  
> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
> >      return bs->in_use;
> >  }
> >  
> > +void bdrv_iostatus_enable(BlockDriverState *bs)
> > +{
> > +    assert(bs->iostatus == BDRV_IOS_INVAL);
> > +    bs->iostatus = BDRV_IOS_OK;
> > +}
> 
> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
> that do tracking declare that by calling this function during
> initialization.  Enables tracking if the BDS has suitable on_write_error
> and on_read_error.
> 
> If a device gets hot unplugged, tracking remains enabled.  If the BDS
> then gets reused with a device that doesn't do tracking, I/O status
> becomes incorrect.  Can't happen right now, because we automatically
> delete the BDS on hot unplug, but it's a trap.
> 
> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().

Ok, and in bdrv_close() as Zhi Yong said.

> > +
> > +/* The I/O status is only enabled if the drive explicitly
> > + * enables it _and_ the VM is configured to stop on errors */
> > +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
> > +{
> > +    return (bs->iostatus != BDRV_IOS_INVAL &&
> > +           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
> > +            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
> > +            bs->on_read_error == BLOCK_ERR_STOP_ANY));
> > +}
> > +
> > +void bdrv_iostatus_reset(BlockDriverState *bs)
> > +{
> > +    if (bdrv_iostatus_is_enabled(bs)) {
> > +        bs->iostatus = BDRV_IOS_OK;
> > +    }
> > +}
> > +
> > +void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
> > +{
> > +    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
> > +        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
> > +                                                BDRV_IOS_FAILED;
> > +    }
> > +}
> > +
> 
> abs(error) feels... unusual.
> 
> If you want to guard against callers passing wrongly signed values, why
> not simply assert(error >= 0)?

Yes. Actually, I thought that there were existing devices that could pass
a positive value (while others passed a negative one), but by taking a look
at the code now it seems that all supported devices pass a negative value,
so your suggestion makes sense.

> 
> >  void
> >  bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
> >          enum BlockAcctType type)
> > diff --git a/block.h b/block.h
> > index 16bfa0a..de74af0 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -77,6 +77,15 @@ typedef enum {
> >      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> >  } BlockMonEventAction;
> >  
> > +typedef enum {
> > +    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
> > +    BDRV_IOS_MAX
> > +} BlockIOStatus;
> 
> Why is this in block.h?

I followed BlockErrorAction, you suggest block_int.h?

> 
> > +
> > +void bdrv_iostatus_enable(BlockDriverState *bs);
> > +void bdrv_iostatus_reset(BlockDriverState *bs);
> > +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
> > +void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
> >  void bdrv_mon_event(const BlockDriverState *bdrv,
> >                      BlockMonEventAction action, int is_read);
> >  void bdrv_info_print(Monitor *mon, const QObject *data);
> > diff --git a/block_int.h b/block_int.h
> > index 8c3b863..f2f4f2d 100644
> > --- a/block_int.h
> > +++ b/block_int.h
> > @@ -199,6 +199,7 @@ struct BlockDriverState {
> >         drivers. They are not used by the block driver */
> >      int cyls, heads, secs, translation;
> >      BlockErrorAction on_read_error, on_write_error;
> > +    BlockIOStatus iostatus;
> >      char device_name[32];
> >      unsigned long *dirty_bitmap;
> >      int64_t dirty_count;
> > diff --git a/monitor.c b/monitor.c
> > index 8ec2c5e..88d8228 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1304,6 +1304,11 @@ struct bdrv_iterate_context {
> >      int err;
> >  };
> >  
> > +static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> > +{
> > +    bdrv_iostatus_reset(bs);
> > +}
> > +
> >  /**
> >   * do_cont(): Resume emulation.
> >   */
> > @@ -1320,6 +1325,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >          return -1;
> >      }
> >  
> > +    bdrv_iterate(iostatus_bdrv_it, NULL);
> >      bdrv_iterate(encrypted_bdrv_it, &context);
> >      /* only resume the vm if all keys are set and valid */
> >      if (!context.err) {
> 

Re: [Qemu-devel] [PATCH v1 0/6]: block: Add I/O status support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 23 Sep 2011 10:55:39 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 19.09.2011 16:09, schrieb Luiz Capitulino:
>> >> On Mon, 19 Sep 2011 15:40:06 +0200
>> >> Kevin Wolf <kwolf@redhat.com> wrote:
>> >> 
>> >>> Am 01.09.2011 20:37, schrieb Luiz Capitulino:
>> >>>> This series adds support to the block layer to keep track of devices'
>> >>>> I/O status. That information is also made available in QMP and HMP.
>> >>>>
>> >>>> The goal here is to allow management applications that miss the
>> >>>> BLOCK_IO_ERROR event to able to query the VM to determine if any device has
>> >>>> caused the VM to stop and which device caused it.
>> >>>>
>> >>>> Please, note that this series depends on the following series:
>> >>>>
>> >>>>  o [PATCH v3 0/8]: Introduce the RunState type
>> >>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00118.html
>> >>>>
>> >>>> And to be able to properly test it you'll also need:
>> >>>>
>> >>>>  o [PATCH 0/3] qcow2/coroutine fixes
>> >>>>  o http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00074.html
>> >>>>
>> >>>> Here's an HMP example:
>> >>>>
>> >>>>   (qemu) info status 
>> >>>>   VM status: paused (io-error)
>> >>>>   (qemu) info block
>> >>>>   ide0-hd0: removable=0 io-status=ok file=disks/test2.img ro=0 drv=qcow2 encrypted=0
>> >>>>   ide0-hd1: removable=0 io-status=nospace file=/dev/vg_doriath/kvmtest ro=0 drv=qcow2 encrypted=0
>> >>>>   ide1-cd0: removable=1 locked=0 io-status=ok [not inserted]
>> >>>>   floppy0: removable=1 locked=0 [not inserted]
>> >>>>   sd0: removable=1 locked=0 [not inserted]
>> >>>>
>> >>>> The "info status" command shows that the VM is stopped due to an I/O error.
>> >>>> By issuing "info block" it's possible to determine that the 'ide0-hd1' device
>> >>>> caused the error, which turns out to be due to no space.
>> >>>
>> >>> Looks like I didn't reply here yet?
>> >> 
>> >> No, you didn't.
>> >> 
>> >>> I still don't quite like that the devices are involved, but their part
>> >>> is minimal and it makes the implementation much easier, so for me that's
>> >>> acceptable.
>> >> 
>> >> Suggestions on better ways of implementing this are welcome! :)
>> >
>> > I don't have one. :-)
>> >
>> > Surely it would be possible to do everything in block.c, but I see that
>> > this would make things much more complicated (would need to get an AIO
>> > callback added to the chain that can save the error code). It's not
>> > worth the trouble.
>> 
>> The block layer necessarily detects errors in a huge number of places.
>> 
>> It also has a rich and complex interface to device models, with error
>> reporting in many places.
>> 
>> virtio-blk, ide and scsi-disk each have a single error handler to handle
>> block layer errors.
>> 
>> For better or worse, these are the only block layer error "chokepoints"
>> we have now, and therefore the only easy places to set BDS I/O status.
>> But they're still wrong.
>> 
>> The block layer shouldn't require device models to tell it what it
>> already knows.
>> 
>> Now, I don't want to block your series just because it adds this wart.
>> But the wart should be documented in the code.
>
> Is something like:
>
>  /* XXX: this should be implemented in the block layer */
>
> good enough?

Add a brief "why", and it's perfect.

>> Also document that any device model implementing werror=stop|enospc or
>> rerror=stop must update I/O status.  Extra points if you find a way to
>> enforce it instead.
>
> Why? It's probably a nice thing because we can know which device caused
> the VM to stop, but I don't see it as a must have (it's probably a
> no brainer to update the I/O status though).

I misread and thought it would lie in that case, but it's actually
silent then (bdrv_iostatus_is_enabled() returns false).  Thus, it's not
a must.

I'd still be tempted to make it one, so clients can rely on io-status
after error stop.  Can't see why anyone would want to do stop on error
in a device model without also setting I/O status.  But use your own
judgement.

>> Apropos enforcing.  Currently, -drive accepts any werror and rerror
>> action with if={ide,virtio,scsi,none}.  We rely on device models not
>> implementing an action to check and fail during initialization.
>> scsi-generic and the floppy devices do.  All the other qdevified devices
>> don't, and that's broken.
>
> I think I can fix that, but this series doesn't depend on that, right?
> (in the meaning that we could merge this before getting that problem fixed).

Right.

[...]

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 23 Sep 2011 09:51:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > This commit adds support to the BlockDriverState type to keep track
>> > of devices' I/O status.
>> >
>> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
>> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction
>> > between no space and other errors is important because a management
>> > application may want to watch for no space in order to extend the
>> > space assigned to the VM and put it to run again.
>> >
>> > Qemu devices supporting the I/O status feature have to enable it
>> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
>> > configured to stop the VM on errors (ie. werror=stop|enospc or
>> > rerror=stop).
>> >
>> > In case of multiple errors being triggered in sequence only the first
>> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
>> > 'cont' command is issued.
>> >
>> > Next commits will add support to some devices and extend the
>> > query-block/info block commands to return the I/O status information.
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  block.c     |   32 ++++++++++++++++++++++++++++++++
>> >  block.h     |    9 +++++++++
>> >  block_int.h |    1 +
>> >  monitor.c   |    6 ++++++
>> >  4 files changed, 48 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/block.c b/block.c
>> > index e3fe97f..fbd90b4 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>> >      if (device_name[0] != '\0') {
>> >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>> >      }
>> > +    bs->iostatus = BDRV_IOS_INVAL;
>> >      return bs;
>> >  }
>> >  
>> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
>> >      return bs->in_use;
>> >  }
>> >  
>> > +void bdrv_iostatus_enable(BlockDriverState *bs)
>> > +{
>> > +    assert(bs->iostatus == BDRV_IOS_INVAL);
>> > +    bs->iostatus = BDRV_IOS_OK;
>> > +}
>> 
>> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
>> that do tracking declare that by calling this function during
>> initialization.  Enables tracking if the BDS has suitable on_write_error
>> and on_read_error.
>> 
>> If a device gets hot unplugged, tracking remains enabled.  If the BDS
>> then gets reused with a device that doesn't do tracking, I/O status
>> becomes incorrect.  Can't happen right now, because we automatically
>> delete the BDS on hot unplug, but it's a trap.
>> 
>> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().
>
> Ok, and in bdrv_close() as Zhi Yong said.

Disabling in bdrv_close() makes the status go away on eject.  Makes some
sense, in a way.  Also complicates the simple rule "status persists from
stop to cont".  Hmm, consider the following race:

1. Management app sends eject command

2. qemu gets read error on the same drive, VM stops, I/O status set,
QEVENT_BLOCK_IO_ERROR sent to management app.

3. qemu receives eject command, bdrv_close() resets I/O status

4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out
which drive caused it.

If 2 happens before 3, I/O status is lost.

>> > +
>> > +/* The I/O status is only enabled if the drive explicitly
>> > + * enables it _and_ the VM is configured to stop on errors */
>> > +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
>> > +{
>> > +    return (bs->iostatus != BDRV_IOS_INVAL &&
>> > +           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
>> > +            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
>> > +            bs->on_read_error == BLOCK_ERR_STOP_ANY));
>> > +}
>> > +
>> > +void bdrv_iostatus_reset(BlockDriverState *bs)
>> > +{
>> > +    if (bdrv_iostatus_is_enabled(bs)) {
>> > +        bs->iostatus = BDRV_IOS_OK;
>> > +    }
>> > +}
>> > +
>> > +void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
>> > +{
>> > +    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
>> > +        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
>> > +                                                BDRV_IOS_FAILED;
>> > +    }
>> > +}
>> > +
>> 
>> abs(error) feels... unusual.
>> 
>> If you want to guard against callers passing wrongly signed values, why
>> not simply assert(error >= 0)?
>
> Yes. Actually, I thought that there were existing devices that could pass
> a positive value (while others passed a negative one), but by taking a look
> at the code now it seems that all supported devices pass a negative value,
> so your suggestion makes sense.
>
>> 
>> >  void
>> >  bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
>> >          enum BlockAcctType type)
>> > diff --git a/block.h b/block.h
>> > index 16bfa0a..de74af0 100644
>> > --- a/block.h
>> > +++ b/block.h
>> > @@ -77,6 +77,15 @@ typedef enum {
>> >      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>> >  } BlockMonEventAction;
>> >  
>> > +typedef enum {
>> > +    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
>> > +    BDRV_IOS_MAX
>> > +} BlockIOStatus;
>> 
>> Why is this in block.h?
>
> I followed BlockErrorAction, you suggest block_int.h?

I guess I'd put it in block_int.h, just because I can.  No biggie,
though.

[...]

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

On Fri, 23 Sep 2011 17:36:53 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 23 Sep 2011 09:51:24 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > This commit adds support to the BlockDriverState type to keep track
> >> > of devices' I/O status.
> >> >
> >> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
> >> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction
> >> > between no space and other errors is important because a management
> >> > application may want to watch for no space in order to extend the
> >> > space assigned to the VM and put it to run again.
> >> >
> >> > Qemu devices supporting the I/O status feature have to enable it
> >> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
> >> > configured to stop the VM on errors (ie. werror=stop|enospc or
> >> > rerror=stop).
> >> >
> >> > In case of multiple errors being triggered in sequence only the first
> >> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
> >> > 'cont' command is issued.
> >> >
> >> > Next commits will add support to some devices and extend the
> >> > query-block/info block commands to return the I/O status information.
> >> >
> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > ---
> >> >  block.c     |   32 ++++++++++++++++++++++++++++++++
> >> >  block.h     |    9 +++++++++
> >> >  block_int.h |    1 +
> >> >  monitor.c   |    6 ++++++
> >> >  4 files changed, 48 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/block.c b/block.c
> >> > index e3fe97f..fbd90b4 100644
> >> > --- a/block.c
> >> > +++ b/block.c
> >> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
> >> >      if (device_name[0] != '\0') {
> >> >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> >> >      }
> >> > +    bs->iostatus = BDRV_IOS_INVAL;
> >> >      return bs;
> >> >  }
> >> >  
> >> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
> >> >      return bs->in_use;
> >> >  }
> >> >  
> >> > +void bdrv_iostatus_enable(BlockDriverState *bs)
> >> > +{
> >> > +    assert(bs->iostatus == BDRV_IOS_INVAL);
> >> > +    bs->iostatus = BDRV_IOS_OK;
> >> > +}
> >> 
> >> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
> >> that do tracking declare that by calling this function during
> >> initialization.  Enables tracking if the BDS has suitable on_write_error
> >> and on_read_error.
> >> 
> >> If a device gets hot unplugged, tracking remains enabled.  If the BDS
> >> then gets reused with a device that doesn't do tracking, I/O status
> >> becomes incorrect.  Can't happen right now, because we automatically
> >> delete the BDS on hot unplug, but it's a trap.
> >> 
> >> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().
> >
> > Ok, and in bdrv_close() as Zhi Yong said.
> 
> Disabling in bdrv_close() makes the status go away on eject.  Makes some
> sense, in a way.  Also complicates the simple rule "status persists from
> stop to cont".  Hmm, consider the following race:
> 
> 1. Management app sends eject command
> 
> 2. qemu gets read error on the same drive, VM stops, I/O status set,
> QEVENT_BLOCK_IO_ERROR sent to management app.
> 
> 3. qemu receives eject command, bdrv_close() resets I/O status
> 
> 4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out
> which drive caused it.
> 
> If 2 happens before 3, I/O status is lost.

Honestly speaking, I didn't intend to make it persistent on eject. Do we really
want to support this? I find it a bit confusing to have the I/O status on
something that has no media. In that case we should only return the io-status
info if the media is inserted.

This way, if the VM stops due to an I/O error and the media is ejected, a mngt
application would:

 1. Issue the query-block and find no guilt
 2. Issue cont
 3. The VM would execute normally

Of course that we have to be clear about it in the documentation and a
mngt app has to be prepared to deal with it.

> >> > +
> >> > +/* The I/O status is only enabled if the drive explicitly
> >> > + * enables it _and_ the VM is configured to stop on errors */
> >> > +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
> >> > +{
> >> > +    return (bs->iostatus != BDRV_IOS_INVAL &&
> >> > +           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
> >> > +            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
> >> > +            bs->on_read_error == BLOCK_ERR_STOP_ANY));
> >> > +}
> >> > +
> >> > +void bdrv_iostatus_reset(BlockDriverState *bs)
> >> > +{
> >> > +    if (bdrv_iostatus_is_enabled(bs)) {
> >> > +        bs->iostatus = BDRV_IOS_OK;
> >> > +    }
> >> > +}
> >> > +
> >> > +void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
> >> > +{
> >> > +    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
> >> > +        bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
> >> > +                                                BDRV_IOS_FAILED;
> >> > +    }
> >> > +}
> >> > +
> >> 
> >> abs(error) feels... unusual.
> >> 
> >> If you want to guard against callers passing wrongly signed values, why
> >> not simply assert(error >= 0)?
> >
> > Yes. Actually, I thought that there were existing devices that could pass
> > a positive value (while others passed a negative one), but by taking a look
> > at the code now it seems that all supported devices pass a negative value,
> > so your suggestion makes sense.
> >
> >> 
> >> >  void
> >> >  bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
> >> >          enum BlockAcctType type)
> >> > diff --git a/block.h b/block.h
> >> > index 16bfa0a..de74af0 100644
> >> > --- a/block.h
> >> > +++ b/block.h
> >> > @@ -77,6 +77,15 @@ typedef enum {
> >> >      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> >> >  } BlockMonEventAction;
> >> >  
> >> > +typedef enum {
> >> > +    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
> >> > +    BDRV_IOS_MAX
> >> > +} BlockIOStatus;
> >> 
> >> Why is this in block.h?
> >
> > I followed BlockErrorAction, you suggest block_int.h?
> 
> I guess I'd put it in block_int.h, just because I can.  No biggie,
> though.
> 
> [...]
> 

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 23 Sep 2011 17:36:53 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 23 Sep 2011 09:51:24 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > This commit adds support to the BlockDriverState type to keep track
>> >> > of devices' I/O status.
>> >> >
>> >> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
>> >> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction
>> >> > between no space and other errors is important because a management
>> >> > application may want to watch for no space in order to extend the
>> >> > space assigned to the VM and put it to run again.
>> >> >
>> >> > Qemu devices supporting the I/O status feature have to enable it
>> >> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
>> >> > configured to stop the VM on errors (ie. werror=stop|enospc or
>> >> > rerror=stop).
>> >> >
>> >> > In case of multiple errors being triggered in sequence only the first
>> >> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
>> >> > 'cont' command is issued.
>> >> >
>> >> > Next commits will add support to some devices and extend the
>> >> > query-block/info block commands to return the I/O status information.
>> >> >
>> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> > ---
>> >> >  block.c     |   32 ++++++++++++++++++++++++++++++++
>> >> >  block.h     |    9 +++++++++
>> >> >  block_int.h |    1 +
>> >> >  monitor.c   |    6 ++++++
>> >> >  4 files changed, 48 insertions(+), 0 deletions(-)
>> >> >
>> >> > diff --git a/block.c b/block.c
>> >> > index e3fe97f..fbd90b4 100644
>> >> > --- a/block.c
>> >> > +++ b/block.c
>> >> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>> >> >      if (device_name[0] != '\0') {
>> >> >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>> >> >      }
>> >> > +    bs->iostatus = BDRV_IOS_INVAL;
>> >> >      return bs;
>> >> >  }
>> >> >  
>> >> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
>> >> >      return bs->in_use;
>> >> >  }
>> >> >  
>> >> > +void bdrv_iostatus_enable(BlockDriverState *bs)
>> >> > +{
>> >> > +    assert(bs->iostatus == BDRV_IOS_INVAL);
>> >> > +    bs->iostatus = BDRV_IOS_OK;
>> >> > +}
>> >> 
>> >> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
>> >> that do tracking declare that by calling this function during
>> >> initialization.  Enables tracking if the BDS has suitable on_write_error
>> >> and on_read_error.
>> >> 
>> >> If a device gets hot unplugged, tracking remains enabled.  If the BDS
>> >> then gets reused with a device that doesn't do tracking, I/O status
>> >> becomes incorrect.  Can't happen right now, because we automatically
>> >> delete the BDS on hot unplug, but it's a trap.
>> >> 
>> >> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().
>> >
>> > Ok, and in bdrv_close() as Zhi Yong said.
>> 
>> Disabling in bdrv_close() makes the status go away on eject.  Makes some
>> sense, in a way.  Also complicates the simple rule "status persists from
>> stop to cont".  Hmm, consider the following race:
>> 
>> 1. Management app sends eject command
>> 
>> 2. qemu gets read error on the same drive, VM stops, I/O status set,
>> QEVENT_BLOCK_IO_ERROR sent to management app.
>> 
>> 3. qemu receives eject command, bdrv_close() resets I/O status
>> 
>> 4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out
>> which drive caused it.
>> 
>> If 2 happens before 3, I/O status is lost.
>
> Honestly speaking, I didn't intend to make it persistent on eject. Do we really
> want to support this? I find it a bit confusing to have the I/O status on
> something that has no media. In that case we should only return the io-status
> info if the media is inserted.
>
> This way, if the VM stops due to an I/O error and the media is ejected, a mngt
> application would:
>
>  1. Issue the query-block and find no guilt
>  2. Issue cont
>  3. The VM would execute normally
>
> Of course that we have to be clear about it in the documentation and a
> mngt app has to be prepared to deal with it.

The management app may well want to know that we hit errors on the
media, even when the error happens close to an eject.  In particular,
they probably want to alert their users on write errors.  Just because
you eject a medium doesn't mean you're no longer interested in the data
on it ;)

[...]

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

On Mon, 26 Sep 2011 11:34:34 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 23 Sep 2011 17:36:53 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 23 Sep 2011 09:51:24 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > This commit adds support to the BlockDriverState type to keep track
> >> >> > of devices' I/O status.
> >> >> >
> >> >> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
> >> >> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction
> >> >> > between no space and other errors is important because a management
> >> >> > application may want to watch for no space in order to extend the
> >> >> > space assigned to the VM and put it to run again.
> >> >> >
> >> >> > Qemu devices supporting the I/O status feature have to enable it
> >> >> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
> >> >> > configured to stop the VM on errors (ie. werror=stop|enospc or
> >> >> > rerror=stop).
> >> >> >
> >> >> > In case of multiple errors being triggered in sequence only the first
> >> >> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
> >> >> > 'cont' command is issued.
> >> >> >
> >> >> > Next commits will add support to some devices and extend the
> >> >> > query-block/info block commands to return the I/O status information.
> >> >> >
> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> >> > ---
> >> >> >  block.c     |   32 ++++++++++++++++++++++++++++++++
> >> >> >  block.h     |    9 +++++++++
> >> >> >  block_int.h |    1 +
> >> >> >  monitor.c   |    6 ++++++
> >> >> >  4 files changed, 48 insertions(+), 0 deletions(-)
> >> >> >
> >> >> > diff --git a/block.c b/block.c
> >> >> > index e3fe97f..fbd90b4 100644
> >> >> > --- a/block.c
> >> >> > +++ b/block.c
> >> >> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
> >> >> >      if (device_name[0] != '\0') {
> >> >> >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> >> >> >      }
> >> >> > +    bs->iostatus = BDRV_IOS_INVAL;
> >> >> >      return bs;
> >> >> >  }
> >> >> >  
> >> >> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
> >> >> >      return bs->in_use;
> >> >> >  }
> >> >> >  
> >> >> > +void bdrv_iostatus_enable(BlockDriverState *bs)
> >> >> > +{
> >> >> > +    assert(bs->iostatus == BDRV_IOS_INVAL);
> >> >> > +    bs->iostatus = BDRV_IOS_OK;
> >> >> > +}
> >> >> 
> >> >> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
> >> >> that do tracking declare that by calling this function during
> >> >> initialization.  Enables tracking if the BDS has suitable on_write_error
> >> >> and on_read_error.
> >> >> 
> >> >> If a device gets hot unplugged, tracking remains enabled.  If the BDS
> >> >> then gets reused with a device that doesn't do tracking, I/O status
> >> >> becomes incorrect.  Can't happen right now, because we automatically
> >> >> delete the BDS on hot unplug, but it's a trap.
> >> >> 
> >> >> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().
> >> >
> >> > Ok, and in bdrv_close() as Zhi Yong said.
> >> 
> >> Disabling in bdrv_close() makes the status go away on eject.  Makes some
> >> sense, in a way.  Also complicates the simple rule "status persists from
> >> stop to cont".  Hmm, consider the following race:
> >> 
> >> 1. Management app sends eject command
> >> 
> >> 2. qemu gets read error on the same drive, VM stops, I/O status set,
> >> QEVENT_BLOCK_IO_ERROR sent to management app.
> >> 
> >> 3. qemu receives eject command, bdrv_close() resets I/O status
> >> 
> >> 4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out
> >> which drive caused it.
> >> 
> >> If 2 happens before 3, I/O status is lost.
> >
> > Honestly speaking, I didn't intend to make it persistent on eject. Do we really
> > want to support this? I find it a bit confusing to have the I/O status on
> > something that has no media. In that case we should only return the io-status
> > info if the media is inserted.
> >
> > This way, if the VM stops due to an I/O error and the media is ejected, a mngt
> > application would:
> >
> >  1. Issue the query-block and find no guilt
> >  2. Issue cont
> >  3. The VM would execute normally
> >
> > Of course that we have to be clear about it in the documentation and a
> > mngt app has to be prepared to deal with it.
> 
> The management app may well want to know that we hit errors on the
> media, even when the error happens close to an eject.  In particular,
> they probably want to alert their users on write errors.  Just because
> you eject a medium doesn't mean you're no longer interested in the data
> on it ;)

Ok, so I'll have to reset the I/O status on bdrv_open() too, so that
a new inserted media won't contain the previous media status.

Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 26 Sep 2011 11:34:34 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 23 Sep 2011 17:36:53 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Fri, 23 Sep 2011 09:51:24 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> 
>> >> >> > This commit adds support to the BlockDriverState type to keep track
>> >> >> > of devices' I/O status.
>> >> >> >
>> >> >> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
>> >> >> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction
>> >> >> > between no space and other errors is important because a management
>> >> >> > application may want to watch for no space in order to extend the
>> >> >> > space assigned to the VM and put it to run again.
>> >> >> >
>> >> >> > Qemu devices supporting the I/O status feature have to enable it
>> >> >> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
>> >> >> > configured to stop the VM on errors (ie. werror=stop|enospc or
>> >> >> > rerror=stop).
>> >> >> >
>> >> >> > In case of multiple errors being triggered in sequence only the first
>> >> >> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
>> >> >> > 'cont' command is issued.
>> >> >> >
>> >> >> > Next commits will add support to some devices and extend the
>> >> >> > query-block/info block commands to return the I/O status information.
>> >> >> >
>> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> >> > ---
>> >> >> >  block.c     |   32 ++++++++++++++++++++++++++++++++
>> >> >> >  block.h     |    9 +++++++++
>> >> >> >  block_int.h |    1 +
>> >> >> >  monitor.c   |    6 ++++++
>> >> >> >  4 files changed, 48 insertions(+), 0 deletions(-)
>> >> >> >
>> >> >> > diff --git a/block.c b/block.c
>> >> >> > index e3fe97f..fbd90b4 100644
>> >> >> > --- a/block.c
>> >> >> > +++ b/block.c
>> >> >> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>> >> >> >      if (device_name[0] != '\0') {
>> >> >> >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>> >> >> >      }
>> >> >> > +    bs->iostatus = BDRV_IOS_INVAL;
>> >> >> >      return bs;
>> >> >> >  }
>> >> >> >  
>> >> >> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
>> >> >> >      return bs->in_use;
>> >> >> >  }
>> >> >> >  
>> >> >> > +void bdrv_iostatus_enable(BlockDriverState *bs)
>> >> >> > +{
>> >> >> > +    assert(bs->iostatus == BDRV_IOS_INVAL);
>> >> >> > +    bs->iostatus = BDRV_IOS_OK;
>> >> >> > +}
>> >> >> 
>> >> >> bdrv_new() creates the BDS with I/O status tracking disabled.  Devices
>> >> >> that do tracking declare that by calling this function during
>> >> >> initialization.  Enables tracking if the BDS has suitable on_write_error
>> >> >> and on_read_error.
>> >> >> 
>> >> >> If a device gets hot unplugged, tracking remains enabled.  If the BDS
>> >> >> then gets reused with a device that doesn't do tracking, I/O status
>> >> >> becomes incorrect.  Can't happen right now, because we automatically
>> >> >> delete the BDS on hot unplug, but it's a trap.
>> >> >> 
>> >> >> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().
>> >> >
>> >> > Ok, and in bdrv_close() as Zhi Yong said.
>> >> 
>> >> Disabling in bdrv_close() makes the status go away on eject.  Makes some
>> >> sense, in a way.  Also complicates the simple rule "status persists from
>> >> stop to cont".  Hmm, consider the following race:
>> >> 
>> >> 1. Management app sends eject command
>> >> 
>> >> 2. qemu gets read error on the same drive, VM stops, I/O status set,
>> >> QEVENT_BLOCK_IO_ERROR sent to management app.
>> >> 
>> >> 3. qemu receives eject command, bdrv_close() resets I/O status
>> >> 
>> >> 4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out
>> >> which drive caused it.
>> >> 
>> >> If 2 happens before 3, I/O status is lost.
>> >
>> > Honestly speaking, I didn't intend to make it persistent on eject. Do we really
>> > want to support this? I find it a bit confusing to have the I/O status on
>> > something that has no media. In that case we should only return the io-status
>> > info if the media is inserted.
>> >
>> > This way, if the VM stops due to an I/O error and the media is ejected, a mngt
>> > application would:
>> >
>> >  1. Issue the query-block and find no guilt
>> >  2. Issue cont
>> >  3. The VM would execute normally
>> >
>> > Of course that we have to be clear about it in the documentation and a
>> > mngt app has to be prepared to deal with it.
>> 
>> The management app may well want to know that we hit errors on the
>> media, even when the error happens close to an eject.  In particular,
>> they probably want to alert their users on write errors.  Just because
>> you eject a medium doesn't mean you're no longer interested in the data
>> on it ;)
>
> Ok, so I'll have to reset the I/O status on bdrv_open() too, so that
> a new inserted media won't contain the previous media status.

Same race with "change" (which calls bdrv_close() and bdrv_open())
instead of "eject" (which only calls bdrv_close()).

I figure we better define the problem away, like this: I/O status is
what made the VM stop.  It's valid from VM stop to VM continue or drive
delete, whatever comes first.  In particular, media change doesn't
affect it.

The only alternative I can see right now is making media change fail
while I/O status isn't okay, but that "cure" looks far worse than the
disease, and I'm not at all sure it's sufficient.

Two more instances of the race, both caused by destroying the drive (and
thus I/O status) while it's being used by a device (and thus prone to
getting I/O status set):

1. Device hot unplug while drive is not empty

Possible fix: stop auto-destroying drives on device unplug.  Then a
management app can unplug the device, wait until it's gone, then delete
the drive.  Safe, because I/O status can't change anymore once the
device detached from the drive.

2. drive_del of non-empty drive while a device is attached.

Possible fix: split into detach and delete.

[Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

This commit adds support to the BlockDriverState type to keep track
of devices' I/O status.

There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC
(no space error) and BDRV_IOS_FAILED (any other error). The distinction
between no space and other errors is important because a management
application may want to watch for no space in order to extend the
space assigned to the VM and put it to run again.

Qemu devices supporting the I/O status feature have to enable it
explicitly by calling bdrv_iostatus_enable() _and_ have to be
configured to stop the VM on errors (ie. werror=stop|enospc or
rerror=stop).

In case of multiple errors being triggered in sequence only the first
one is stored. The I/O status is always reset to BDRV_IOS_OK when the
'cont' command is issued.

Next commits will add support to some devices and extend the
query-block/info block commands to return the I/O status information.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c     |   40 ++++++++++++++++++++++++++++++++++++++++
 block.h     |   10 ++++++++++
 block_int.h |    1 +
 monitor.c   |    6 ++++++
 4 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index e3fe97f..96b047b 100644
--- a/block.c
+++ b/block.c
@@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name)
     if (device_name[0] != '\0') {
         QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
     }
+    bdrv_iostatus_disable(bs);
     return bs;
 }
 
@@ -770,6 +771,7 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
         return -EBUSY;
     }
     bs->dev = dev;
+    bdrv_iostatus_reset(bs);
     return 0;
 }
 
@@ -3181,6 +3183,44 @@ int bdrv_in_use(BlockDriverState *bs)
     return bs->in_use;
 }
 
+void bdrv_iostatus_enable(BlockDriverState *bs)
+{
+    bs->iostatus = BDRV_IOS_OK;
+}
+
+/* The I/O status is only enabled if the drive explicitly
+ * enables it _and_ the VM is configured to stop on errors */
+bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
+{
+    return (bs->iostatus != BDRV_IOS_INVAL &&
+           (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
+            bs->on_write_error == BLOCK_ERR_STOP_ANY    ||
+            bs->on_read_error == BLOCK_ERR_STOP_ANY));
+}
+
+void bdrv_iostatus_disable(BlockDriverState *bs)
+{
+    bs->iostatus = BDRV_IOS_INVAL;
+}
+
+void bdrv_iostatus_reset(BlockDriverState *bs)
+{
+    if (bdrv_iostatus_is_enabled(bs)) {
+        bs->iostatus = BDRV_IOS_OK;
+    }
+}
+
+/* XXX: Today this is set by device models because it makes the implementation
+   quite simple. However, the block layer knows about the error, so it's
+   possible to implement this without device models being involved */
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
+{
+    if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
+        assert(error >= 0);
+        bs->iostatus = error == ENOSPC ? BDRV_IOS_ENOSPC : BDRV_IOS_FAILED;
+    }
+}
+
 void
 bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes,
         enum BlockAcctType type)
diff --git a/block.h b/block.h
index 16bfa0a..e77988e 100644
--- a/block.h
+++ b/block.h
@@ -77,6 +77,16 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockMonEventAction;
 
+typedef enum {
+    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
+    BDRV_IOS_MAX
+} BlockIOStatus;
+
+void bdrv_iostatus_enable(BlockDriverState *bs);
+void bdrv_iostatus_reset(BlockDriverState *bs);
+void bdrv_iostatus_disable(BlockDriverState *bs);
+bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
 void bdrv_mon_event(const BlockDriverState *bdrv,
                     BlockMonEventAction action, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
diff --git a/block_int.h b/block_int.h
index 8c3b863..f2f4f2d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -199,6 +199,7 @@ struct BlockDriverState {
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
     BlockErrorAction on_read_error, on_write_error;
+    BlockIOStatus iostatus;
     char device_name[32];
     unsigned long *dirty_bitmap;
     int64_t dirty_count;
diff --git a/monitor.c b/monitor.c
index 8ec2c5e..88d8228 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1304,6 +1304,11 @@ struct bdrv_iterate_context {
     int err;
 };
 
+static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
+{
+    bdrv_iostatus_reset(bs);
+}
+
 /**
  * do_cont(): Resume emulation.
  */
@@ -1320,6 +1325,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
+    bdrv_iterate(iostatus_bdrv_it, NULL);
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err) {
-- 
1.7.7.rc0.72.g4b5ea