[Qemu-devel] [PATCH v0 0/5]: BLOCK_IO_ERROR QMP event

[Qemu-devel] [PATCH v0 0/5]: BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

 Hi,

 This series adds the BLOCK_IO_ERROR event libvirt guys have requested,
I have made some improvements after Kevin's feedback and hope it's in better
shape now.

 The only small issue is that I couldn't get a read error. I've followed Kevin's
advices wrt NFS, but got only write errors...

 I've tested with ide and virtio.

 Thanks.

[Qemu-devel] [PATCH 1/5] QMP: BLOCK_IO_ERROR event handling

[Luiz Capitulino]

This commit adds the basic definitions for the BLOCK_IO_ERROR
event, but actual event emission will be introduced by the
next commits.

NOTE: Adding a small reference in QMP/qmp-events.txt, but this
file is wrong and will be replaced by proper documentation shortly.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |    7 +++++++
 monitor.c          |    3 +++
 monitor.h          |    1 +
 3 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index dc48ccc..7886192 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -43,3 +43,10 @@ Data: 'server' and 'client' keys with the same keys as 'query-vnc'.
 
 Description: Issued when the VNC session is made active.
 Data: 'server' and 'client' keys with the same keys as 'query-vnc'.
+
+7 BLOCK_IO_ERROR
+----------------
+
+Description: Issued when a disk I/O error occurs
+Data: 'device' (device name), 'action' (action to be taken),
+      'operation' ("read" or "write")
diff --git a/monitor.c b/monitor.c
index fb7c572..6e688ac 100644
--- a/monitor.c
+++ b/monitor.c
@@ -378,6 +378,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_VNC_DISCONNECTED:
             event_name = "VNC_DISCONNECTED";
             break;
+        case QEVENT_BLOCK_IO_ERROR:
+            event_name = "BLOCK_IO_ERROR";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index b0f9270..e35f1e4 100644
--- a/monitor.h
+++ b/monitor.h
@@ -23,6 +23,7 @@ typedef enum MonitorEvent {
     QEVENT_VNC_CONNECTED,
     QEVENT_VNC_INITIALIZED,
     QEVENT_VNC_DISCONNECTED,
+    QEVENT_BLOCK_IO_ERROR,
     QEVENT_MAX,
 } MonitorEvent;
 
-- 
1.6.6

[Qemu-devel] Re: [PATCH 1/5] QMP: BLOCK_IO_ERROR event handling

[Kevin Wolf]

Am 02.02.2010 22:10, schrieb Luiz Capitulino:
> This commit adds the basic definitions for the BLOCK_IO_ERROR
> event, but actual event emission will be introduced by the
> next commits.
> 
> NOTE: Adding a small reference in QMP/qmp-events.txt, but this
> file is wrong and will be replaced by proper documentation shortly.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  QMP/qmp-events.txt |    7 +++++++
>  monitor.c          |    3 +++
>  monitor.h          |    1 +
>  3 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index dc48ccc..7886192 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -43,3 +43,10 @@ Data: 'server' and 'client' keys with the same keys as 'query-vnc'.
>  
>  Description: Issued when the VNC session is made active.
>  Data: 'server' and 'client' keys with the same keys as 'query-vnc'.
> +
> +7 BLOCK_IO_ERROR
> +----------------
> +
> +Description: Issued when a disk I/O error occurs
> +Data: 'device' (device name), 'action' (action to be taken),
> +      'operation' ("read" or "write")

I think we should document what allowed values for action are and what
their meaning is. And isn't it actually "action that has been taken"?

Kevin

[Qemu-devel] Re: [PATCH 1/5] QMP: BLOCK_IO_ERROR event handling

[Luiz Capitulino]

On Wed, 03 Feb 2010 10:18:37 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 02.02.2010 22:10, schrieb Luiz Capitulino:
> > This commit adds the basic definitions for the BLOCK_IO_ERROR
> > event, but actual event emission will be introduced by the
> > next commits.
> > 
> > NOTE: Adding a small reference in QMP/qmp-events.txt, but this
> > file is wrong and will be replaced by proper documentation shortly.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  QMP/qmp-events.txt |    7 +++++++
> >  monitor.c          |    3 +++
> >  monitor.h          |    1 +
> >  3 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index dc48ccc..7886192 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -43,3 +43,10 @@ Data: 'server' and 'client' keys with the same keys as 'query-vnc'.
> >  
> >  Description: Issued when the VNC session is made active.
> >  Data: 'server' and 'client' keys with the same keys as 'query-vnc'.
> > +
> > +7 BLOCK_IO_ERROR
> > +----------------
> > +
> > +Description: Issued when a disk I/O error occurs
> > +Data: 'device' (device name), 'action' (action to be taken),
> > +      'operation' ("read" or "write")
> 
> I think we should document what allowed values for action are and what
> their meaning is. And isn't it actually "action that has been taken"?

 Right, sometimes I don't know how to document this stuff
because this file is going to be replaced soon...

[Qemu-devel] [PATCH 2/5] block: BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

This commit introduces the bdrv_mon_event() function, which
should be called by block subsystems (eg. IDE) when a I/O
error occurs, so that an QMP event is emitted.

The following information is currently provided in the event:

- device name
- operation (ie. "read" or "write")
- action taken (eg. "stop")

Event example:

{ "event": "BLOCK_IO_ERROR",
    "data": { "device": "ide0-hd1",
              "operation": "write",
              "action": "stop" },
    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c |   29 +++++++++++++++++++++++++++++
 block.h |    6 ++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..2913124 100644
--- a/block.c
+++ b/block.c
@@ -1164,6 +1164,35 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
 }
 
+void bdrv_mon_event(const BlockDriverState *bdrv,
+                    BlockMonEventAction action, int is_read)
+{
+    QObject *data;
+    const char *action_str;
+
+    switch (action) {
+    case BDRV_ACTION_REPORT:
+        action_str = "report";
+        break;
+    case BDRV_ACTION_IGNORE:
+        action_str = "ignore";
+        break;
+    case BDRV_ACTION_STOP:
+        action_str = "stop";
+        break;
+    default:
+        abort();
+    }
+
+    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, 'operation': %s }",
+                              bdrv->device_name,
+                              action_str,
+                              is_read ? "read" : "write");
+    monitor_protocol_event(QEVENT_BLOCK_IO_ERROR, data);
+
+    qobject_decref(data);
+}
+
 static void bdrv_print_dict(QObject *obj, void *opaque)
 {
     QDict *bs_dict;
diff --git a/block.h b/block.h
index ecf66c5..a834300 100644
--- a/block.h
+++ b/block.h
@@ -44,6 +44,12 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_SECTOR_SIZE   (1 << BDRV_SECTOR_BITS)
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1);
 
+typedef enum {
+    BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
+} BlockMonEventAction;
+
+void bdrv_mon_event(const BlockDriverState *bdrv,
+                    BlockMonEventAction action, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
 void bdrv_info(Monitor *mon, QObject **ret_data);
 void bdrv_stats_print(Monitor *mon, const QObject *data);
-- 
1.6.6

[Qemu-devel] [PATCH 3/5] ide: Generate BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

Just call bdrv_mon_event() in the right place.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b6643e8..603e537 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     int is_read = (op & BM_STATUS_RETRY_READ);
     BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
 
-    if (action == BLOCK_ERR_IGNORE)
+    if (action == BLOCK_ERR_IGNORE) {
+        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
+    }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
         s->bus->bmdma->unit = s->unit;
         s->bus->bmdma->status |= op;
         vm_stop(0);
+        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
             dma_buf_commit(s, 0);
@@ -495,6 +498,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         } else {
             ide_rw_error(s);
         }
+        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
-- 
1.6.6

Re: [Qemu-devel] [PATCH 3/5] ide: Generate BLOCK_IO_ERROR QMP event

[Christoph Hellwig]

On Tue, Feb 02, 2010 at 07:10:11PM -0200, Luiz Capitulino wrote:
> Just call bdrv_mon_event() in the right place.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hw/ide/core.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b6643e8..603e537 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>      int is_read = (op & BM_STATUS_RETRY_READ);
>      BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
>  
> -    if (action == BLOCK_ERR_IGNORE)
> +    if (action == BLOCK_ERR_IGNORE) {
> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
>          return 0;
> +    }
>  
>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>              || action == BLOCK_ERR_STOP_ANY) {
>          s->bus->bmdma->unit = s->unit;
>          s->bus->bmdma->status |= op;
>          vm_stop(0);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);

Why isn't the event directly sent from drive_get_on_error?  Having
to opencode this in every driver is a sure way to make sure it's
going to be broken somewhere.

Re: [Qemu-devel] [PATCH 3/5] ide: Generate BLOCK_IO_ERROR QMP event

[Kevin Wolf]

Am 03.02.2010 09:31, schrieb Christoph Hellwig:
> On Tue, Feb 02, 2010 at 07:10:11PM -0200, Luiz Capitulino wrote:
>> Just call bdrv_mon_event() in the right place.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  hw/ide/core.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index b6643e8..603e537 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>>      int is_read = (op & BM_STATUS_RETRY_READ);
>>      BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
>>  
>> -    if (action == BLOCK_ERR_IGNORE)
>> +    if (action == BLOCK_ERR_IGNORE) {
>> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
>>          return 0;
>> +    }
>>  
>>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>>              || action == BLOCK_ERR_STOP_ANY) {
>>          s->bus->bmdma->unit = s->unit;
>>          s->bus->bmdma->status |= op;
>>          vm_stop(0);
>> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> 
> Why isn't the event directly sent from drive_get_on_error?  Having
> to opencode this in every driver is a sure way to make sure it's
> going to be broken somewhere.

Because drive_get_on_error isn't an event handler and shouldn't have any
side effects. It might be called anywhere. And it doesn't know the error
code, so it can't even decide if the VM has stopped or not.

Maybe we could look at writing a generic handle_rw_error function for
all block devices. They look pretty much the same in every driver, even
without the monitor event.

Kevin

Re: [Qemu-devel] [PATCH 3/5] ide: Generate BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

On Wed, 03 Feb 2010 10:10:59 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 03.02.2010 09:31, schrieb Christoph Hellwig:
> > On Tue, Feb 02, 2010 at 07:10:11PM -0200, Luiz Capitulino wrote:
> >> Just call bdrv_mon_event() in the right place.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >>  hw/ide/core.c |    6 +++++-
> >>  1 files changed, 5 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >> index b6643e8..603e537 100644
> >> --- a/hw/ide/core.c
> >> +++ b/hw/ide/core.c
> >> @@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> >>      int is_read = (op & BM_STATUS_RETRY_READ);
> >>      BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
> >>  
> >> -    if (action == BLOCK_ERR_IGNORE)
> >> +    if (action == BLOCK_ERR_IGNORE) {
> >> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
> >>          return 0;
> >> +    }
> >>  
> >>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
> >>              || action == BLOCK_ERR_STOP_ANY) {
> >>          s->bus->bmdma->unit = s->unit;
> >>          s->bus->bmdma->status |= op;
> >>          vm_stop(0);
> >> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > 
> > Why isn't the event directly sent from drive_get_on_error?  Having
> > to opencode this in every driver is a sure way to make sure it's
> > going to be broken somewhere.
> 
> Because drive_get_on_error isn't an event handler and shouldn't have any
> side effects. It might be called anywhere. And it doesn't know the error
> code, so it can't even decide if the VM has stopped or not.
> 
> Maybe we could look at writing a generic handle_rw_error function for
> all block devices. They look pretty much the same in every driver, even
> without the monitor event.

 Any design in mind? I could try this later (preferably after the
event series is merged).

 What if block devices register the following callbacks somewhere:

- handle_rw_error_ignore()
- handle_rw_error_stop()
- handle_rw_error_report()

 But then they'd have to trigger their call by calling, say,
block_handle_rw_error() from the error site.

[Qemu-devel] [PATCH 4/5] scsi: Generate BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

Just call bdrv_mon_event() in the right place.

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

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b34fbaa..1285122 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -182,16 +182,20 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int error)
     BlockInterfaceErrorAction action =
         drive_get_on_error(s->qdev.dinfo->bdrv, 0);
 
-    if (action == BLOCK_ERR_IGNORE)
+    if (action == BLOCK_ERR_IGNORE) {
+        bdrv_mon_event(s->qdev.dinfo->bdrv, BDRV_ACTION_IGNORE, 0);
         return 0;
+    }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
         r->status |= SCSI_REQ_STATUS_RETRY;
         vm_stop(0);
+        bdrv_mon_event(s->qdev.dinfo->bdrv, BDRV_ACTION_STOP, 0);
     } else {
         scsi_command_complete(r, CHECK_CONDITION,
                 HARDWARE_ERROR);
+        bdrv_mon_event(s->qdev.dinfo->bdrv, BDRV_ACTION_REPORT, 0);
     }
 
     return 1;
-- 
1.6.6

[Qemu-devel] [PATCH 5/5] virtio-blk: Generate BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

Just call bdrv_mon_event() in the right place.

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

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 037a79c..75adbec 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -105,16 +105,20 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         drive_get_on_error(req->dev->bs, is_read);
     VirtIOBlock *s = req->dev;
 
-    if (action == BLOCK_ERR_IGNORE)
+    if (action == BLOCK_ERR_IGNORE) {
+        bdrv_mon_event(req->dev->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
+    }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
         req->next = s->rq;
         s->rq = req;
         vm_stop(0);
+        bdrv_mon_event(req->dev->bs, BDRV_ACTION_STOP, is_read);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
+        bdrv_mon_event(req->dev->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
-- 
1.6.6

[Qemu-devel] Re: [PATCH v0 0/5]: BLOCK_IO_ERROR QMP event

[Kevin Wolf]

Am 02.02.2010 22:10, schrieb Luiz Capitulino:
>  Hi,
> 
>  This series adds the BLOCK_IO_ERROR event libvirt guys have requested,
> I have made some improvements after Kevin's feedback and hope it's in better
> shape now.
> 
>  The only small issue is that I couldn't get a read error. I've followed Kevin's
> advices wrt NFS, but got only write errors...

I should have explained in more detail what I'm doing to produce a read
error... I boot the system from a local image that won't go away when
NFS is down. I add another image (can be empty) stored on NFS and with
cache=off. Then I turn NFS off and start something like a dd if=/dev/vdb
of=/dev/null iflag=direct. This way you produce only reads.

Kevin