[Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Luiz Capitulino]

Roughly speaking, thin provisioning is a feature where the VM is started with
a small disk and space is allocated on demand.

It's already possible for QMP clients to implement this feature by using
the BLOCK_IO_ERROR event. However, the event can be missed. When this
happens QMP clients need a way to query if any block device has hit a
no space condition.

This is what this series is about: it extends the query-block command to
contain the disk's I/O status.

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

 1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
 2. [PATCH 0/7]: Introduce the QemuState type (by me)

changelog
---------

v2

- Split the series in two (first part already sent)
- Rebased on top of Markus's block layer cleanup series and using BlockDevOps
- Only make the 'io-status' key available if the VM is stopped


 block.c           |   38 ++++++++++++++++++++++++++++++++++++++
 block.h           |   11 +++++++++++
 hw/ide/core.c     |   16 ++++++++++++++++
 hw/ide/internal.h |    2 ++
 hw/scsi-disk.c    |   18 ++++++++++++++++++
 hw/virtio-blk.c   |   14 ++++++++++++++
 qemu-tool.c       |    6 ++++++
 qmp-commands.hx   |    3 +++
 8 files changed, 108 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH 1/5] block: Introduce get_iostatus() device model operation

[Luiz Capitulino]

It returns the status of the last executed I/O operation, which can be:

 o OK (success)
 o ENOSPC (no space)
 o FAILED (other kinds of failures such as EIO)

We make a distiction between ENOSPC and other kinds of failures because
QMP clients can use ENOSPC to implement a feature where the VM is
started with a small disk and space is allocated on demand.

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

diff --git a/block.c b/block.c
index d391836..c86e144 100644
--- a/block.c
+++ b/block.c
@@ -28,6 +28,7 @@
 #include "block_int.h"
 #include "module.h"
 #include "qemu-objects.h"
+#include "sysemu.h"
 
 #ifdef CONFIG_BSD
 #include <sys/types.h>
@@ -790,6 +791,14 @@ int bdrv_dev_is_medium_locked(BlockDriverState *bs)
     return 0;
 }
 
+BlockDevIOStatus bdrv_dev_iostatus(BlockDriverState *bs)
+{
+    if (bs->dev_ops && bs->dev_ops->get_iostatus) {
+        return bs->dev_ops->get_iostatus(bs->dev_opaque);
+    }
+    return BDRV_IOS_INVAL;
+}
+
 /*
  * Run consistency checks on an image
  *
@@ -1724,6 +1733,16 @@ void bdrv_mon_event(const BlockDriverState *bdrv,
     qobject_decref(data);
 }
 
+BlockDevIOStatus bdrv_iostatus_from_error(int error)
+{
+    if (!error) {
+        return BDRV_IOS_OK;
+    } else {
+        return (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
+                                        BDRV_IOS_FAILED;
+    }
+}
+
 static void bdrv_print_dict(QObject *obj, void *opaque)
 {
     QDict *bs_dict;
diff --git a/block.h b/block.h
index 1157eed..27413ae 100644
--- a/block.h
+++ b/block.h
@@ -27,6 +27,10 @@ typedef struct QEMUSnapshotInfo {
     uint64_t vm_clock_nsec; /* VM clock relative to boot */
 } QEMUSnapshotInfo;
 
+typedef enum {
+    BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
+} BlockDevIOStatus;
+
 /* Callbacks for block device models */
 typedef struct BlockDevOps {
     /*
@@ -51,6 +55,11 @@ typedef struct BlockDevOps {
      * Runs when the size changed (e.g. monitor command block_resize)
      */
     void (*resize_cb)(void *opaque);
+    /*
+     * Returns the status of the last I/O operation. Likely to be useful
+     * only when the VM is stopped.
+     */
+    BlockDevIOStatus (*get_iostatus)(void *opaque);
 } BlockDevOps;
 
 #define BDRV_O_RDWR        0x0002
@@ -78,6 +87,7 @@ typedef enum {
 
 void bdrv_mon_event(const BlockDriverState *bdrv,
                     BlockMonEventAction action, int is_read);
+BlockDevIOStatus bdrv_iostatus_from_error(int error);
 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);
@@ -107,6 +117,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
 int bdrv_dev_has_removable_media(BlockDriverState *bs);
 bool bdrv_dev_is_medium_ejected(BlockDriverState *bs);
 int bdrv_dev_is_medium_locked(BlockDriverState *bs);
+BlockDevIOStatus bdrv_dev_iostatus(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 2/5] QMP/HMP: Add the 'io-status' field to query-block and info block

[Luiz Capitulino]

This field is only available if the device model implements the
get_iostatus() operation and the VM is stopped.

It contains the I/O status before the VM being stopped.

Please, also note that it's necessary to add qemu_is_running() to
qemu-tool.c so that we can use it in block.c.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c         |   19 +++++++++++++++++++
 qemu-tool.c     |    6 ++++++
 qmp-commands.hx |    3 +++
 3 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index c86e144..41e4f89 100644
--- a/block.c
+++ b/block.c
@@ -1760,6 +1760,10 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
     if (qdict_get_try_bool(bs_dict, "ejected", false)) {
         monitor_printf(mon, " ejected");
     }
+    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"));
 
@@ -1785,6 +1789,12 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
     qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
 }
 
+static const char *const block_iostatus_name[] = {
+    [BDRV_IOS_OK] = "ok",
+    [BDRV_IOS_FAILED] = "failed",
+    [BDRV_IOS_ENOSPC] = "nospace",
+};
+
 void bdrv_info(Monitor *mon, QObject **ret_data)
 {
     QList *bs_list;
@@ -1822,6 +1832,15 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
             qdict_put_obj(bs_dict, "inserted", obj);
         }
+
+        if (!qemu_is_running()) {
+            BlockDevIOStatus iostatus = bdrv_dev_iostatus(bs);
+            if (iostatus != BDRV_IOS_INVAL) {
+                qdict_put(bs_dict, "io-status",
+                        qstring_from_str(block_iostatus_name[iostatus]));
+            }
+        }
+
         qlist_append_obj(bs_list, bs_obj);
     }
 
diff --git a/qemu-tool.c b/qemu-tool.c
index 41e5c41..9381c56 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -15,6 +15,7 @@
 #include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-log.h"
+#include "sysemu.h"
 
 #include <sys/time.h>
 
@@ -96,3 +97,8 @@ int64_t qemu_get_clock_ns(QEMUClock *clock)
 {
     return 0;
 }
+
+int qemu_is_running(void)
+{
+    return 0;
+}
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b141221..1007ec6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1132,6 +1132,9 @@ Each json-object contain the following:
 - "removable": true if the device is removable, false otherwise (json-bool)
 - "locked": true if the device is locked, false otherwise (json-bool)
 - "ejected": present and true if the device ejected its media (json-bool)
+- "io-status": contains the status of the last I/O operation. It's only
+   present if the device supports it and if the Virtual Machine is stopped
+   (json-bool, optional). Possible values: "ok", "nospace", "failed"
 - "inserted": only present if the device is inserted, it is a json-object
    containing the following:
          - "file": device file name (json-string)
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 3/5] virtio-blk: Support I/O status

[Luiz Capitulino]

TODO: migration

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

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 265c795..a008956 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -29,6 +29,7 @@ typedef struct VirtIOBlock
     QEMUBH *bh;
     BlockConf *conf;
     char *serial;
+    BlockDevIOStatus iostatus;
     unsigned short sector_mask;
     DeviceState *qdev;
 } VirtIOBlock;
@@ -90,9 +91,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
 static void virtio_blk_rw_complete(void *opaque, int ret)
 {
     VirtIOBlockReq *req = opaque;
+    VirtIOBlock *s = req->dev;
 
     trace_virtio_blk_rw_complete(req, ret);
 
+    s->iostatus = bdrv_iostatus_from_error(ret);
     if (ret) {
         int is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
@@ -105,7 +108,9 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 static void virtio_blk_flush_complete(void *opaque, int ret)
 {
     VirtIOBlockReq *req = opaque;
+    VirtIOBlock *s = req->dev;
 
+    s->iostatus = bdrv_iostatus_from_error(ret);
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, 0)) {
             return;
@@ -535,8 +540,16 @@ static void virtio_blk_resize(void *opaque)
     virtio_notify_config(&s->vdev);
 }
 
+static BlockDevIOStatus virtio_blk_iostatus(void *opaque)
+{
+    VirtIOBlock *s = opaque;
+
+    return s->iostatus;
+}
+
 static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
+    .get_iostatus = virtio_blk_iostatus,
 };
 
 VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
@@ -575,6 +588,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
     s->conf = conf;
     s->serial = *serial;
     s->rq = NULL;
+    s->iostatus = BDRV_IOS_INVAL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
 
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 4/5] ide: Support I/O status

[Luiz Capitulino]

TODO: migration

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index ffb6500..3f075a1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -58,6 +58,11 @@ static const int smart_attributes[][12] = {
 static int ide_handle_rw_error(IDEState *s, int error, int op);
 static void ide_dummy_transfer_stop(IDEState *s);
 
+static void ide_update_iostatus(IDEState *s, int error)
+{
+    s->iostatus = bdrv_iostatus_from_error(error);
+}
+
 static void padstr(char *str, const char *src, int len)
 {
     int i, v;
@@ -474,6 +479,7 @@ void ide_sector_read(IDEState *s)
         if (n > s->req_nb_sectors)
             n = s->req_nb_sectors;
         ret = bdrv_read(s->bs, sector_num, s->io_buffer, n);
+        ide_update_iostatus(s, ret);
         if (ret != 0) {
             if (ide_handle_rw_error(s, -ret,
                 BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ))
@@ -544,6 +550,7 @@ void ide_dma_cb(void *opaque, int ret)
     int64_t sector_num;
 
 handle_rw_error:
+    ide_update_iostatus(s, ret);
     if (ret < 0) {
         int op = BM_STATUS_DMA_RETRY;
 
@@ -643,6 +650,7 @@ void ide_sector_write(IDEState *s)
         n = s->req_nb_sectors;
     ret = bdrv_write(s->bs, sector_num, s->io_buffer, n);
 
+    ide_update_iostatus(s, ret);
     if (ret != 0) {
         if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY))
             return;
@@ -678,6 +686,7 @@ static void ide_flush_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
 
+    ide_update_iostatus(s, ret);
     if (ret < 0) {
         /* XXX: What sector number to set here? */
         if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
@@ -1780,10 +1789,16 @@ static bool ide_cd_is_medium_locked(void *opaque)
     return ((IDEState *)opaque)->tray_locked;
 }
 
+static BlockDevIOStatus ide_iostatus(void *opaque)
+{
+    return ((IDEState *)opaque)->iostatus;
+}
+
 static const BlockDevOps ide_cd_block_ops = {
     .change_media_cb = ide_cd_change_cb,
     .is_medium_ejected = ide_cd_medium_ejected,
     .is_medium_locked = ide_cd_is_medium_locked,
+    .get_iostatus = ide_iostatus,
 };
 
 int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
@@ -1843,6 +1858,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
     } else {
         pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
     }
+    s->iostatus = BDRV_IOS_INVAL;
 
     ide_reset(s);
     if (s->drive_kind == IDE_CD) {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 2feae25..3a9a2cf 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -480,6 +480,8 @@ struct IDEState {
     uint8_t *smart_selftest_data;
     /* AHCI */
     int ncq_queues;
+
+    BlockDevIOStatus iostatus;
 };
 
 struct IDEDMAOps {
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 5/5] scsi-disk: Support I/O status

[Luiz Capitulino]

TODO: migration

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

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index db2d5a3..e4b57a7 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -77,11 +77,17 @@ struct SCSIDiskState
     bool tray_open;
     bool tray_locked;
     SCSIDriveKind drive_kind;
+    BlockDevIOStatus iostatus;
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
 static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf);
 
+static void scsi_update_iostatus(SCSIDiskState *s, int error)
+{
+    s->iostatus = bdrv_iostatus_from_error(error);
+}
+
 static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint32_t lun, void *hba_private)
 {
@@ -139,10 +145,12 @@ static void scsi_cancel_io(SCSIRequest *req)
 static void scsi_read_complete(void * opaque, int ret)
 {
     SCSIDiskReq *r = (SCSIDiskReq *)opaque;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     int n;
 
     r->req.aiocb = NULL;
 
+    scsi_update_iostatus(s, ret);
     if (ret) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
             return;
@@ -247,11 +255,13 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
 static void scsi_write_complete(void * opaque, int ret)
 {
     SCSIDiskReq *r = (SCSIDiskReq *)opaque;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     uint32_t len;
     uint32_t n;
 
     r->req.aiocb = NULL;
 
+    scsi_update_iostatus(s, ret);
     if (ret) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_WRITE)) {
             return;
@@ -940,6 +950,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
 	break;
     case SYNCHRONIZE_CACHE:
         ret = bdrv_flush(s->bs);
+        scsi_update_iostatus(s, ret);
         if (ret < 0) {
             if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
                 return -1;
@@ -1298,10 +1309,16 @@ static bool scsi_cd_is_medium_locked(void *opaque)
     return ((SCSIDiskState *)opaque)->tray_locked;
 }
 
+static BlockDevIOStatus scsi_iostatus(void *opaque)
+{
+    return ((SCSIDiskState *)opaque)->iostatus;
+}
+
 static const BlockDevOps scsi_cd_block_ops = {
     .change_media_cb = scsi_cd_change_media_cb,
     .is_medium_ejected = scsi_cd_is_medium_ejected,
     .is_medium_locked = scsi_cd_is_medium_locked,
+    .get_iostatus = scsi_iostatus,
 };
 
 static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
@@ -1347,6 +1364,7 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
     s->cluster_size = s->qdev.blocksize / 512;
     bdrv_set_buffer_alignment(s->bs, s->qdev.blocksize);
 
+    s->iostatus = BDRV_IOS_INVAL;
     s->qdev.type = TYPE_DISK;
     qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
     if (kind == SCSI_CD) {
-- 
1.7.6.396.ge0613

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Kevin Wolf]

Am 03.08.2011 17:19, schrieb Luiz Capitulino:
> Roughly speaking, thin provisioning is a feature where the VM is started with
> a small disk and space is allocated on demand.
> 
> It's already possible for QMP clients to implement this feature by using
> the BLOCK_IO_ERROR event. However, the event can be missed. When this
> happens QMP clients need a way to query if any block device has hit a
> no space condition.
> 
> This is what this series is about: it extends the query-block command to
> contain the disk's I/O status.
> 
> Please, note that this series depends on the following two other series:
> 
>  1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
>  2. [PATCH 0/7]: Introduce the QemuState type (by me)

It feels strange that device models are involved with this. The block
layer already knows the error number, it just tells the device model so
that it can ask it back. Wouldn't it be easier to store it in the
BlockDriverState?

Also, as I already commented on v1, I think it's a bad idea to let
requests that complete later overwrite the error code with "success".
Quite possible in an ENOSPC case, I think.

Kevin

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Luiz Capitulino]

On Wed, 03 Aug 2011 17:39:04 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 03.08.2011 17:19, schrieb Luiz Capitulino:
> > Roughly speaking, thin provisioning is a feature where the VM is started with
> > a small disk and space is allocated on demand.
> > 
> > It's already possible for QMP clients to implement this feature by using
> > the BLOCK_IO_ERROR event. However, the event can be missed. When this
> > happens QMP clients need a way to query if any block device has hit a
> > no space condition.
> > 
> > This is what this series is about: it extends the query-block command to
> > contain the disk's I/O status.
> > 
> > Please, note that this series depends on the following two other series:
> > 
> >  1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
> >  2. [PATCH 0/7]: Introduce the QemuState type (by me)
> 
> It feels strange that device models are involved with this. The block
> layer already knows the error number, it just tells the device model so
> that it can ask it back. Wouldn't it be easier to store it in the
> BlockDriverState?

My first version did it, but looking at it now maybe I did it wrong,
because I was setting the iostatus in the devices (instead of setting
it in the block layer itself). Here's an example:

 http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00421.html

But I'm not sure what the best place is. I'm hoping that you and/or
Markus can help me here. Also note that Markus already told us what
he thinks:

"""
Actually, this is a symptom of the midlayer disease.  I suspect things
would be simpler if we hold the status in its rightful owner, the device
model.  Need a getter for it.  I'm working on a patch series that moves
misplaced state out of the block layer into device models and block
drivers, and a I/O status getter will fit in easily there.
"""

 http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00937.html

Can you guys get in agreement?
 
> Also, as I already commented on v1, I think it's a bad idea to let
> requests that complete later overwrite the error code with "success".
> Quite possible in an ENOSPC case, I think.

Really? I'm under the assumption that all pending I/O will be completed
as soon as the vm is stopped. Am I wrong?

What we need here is a way to "reset" the io-status field. That's, the vm will
stop due to ENOSPC, the mngt application will handle it and put the vm to run
again. At this point the io-status has to be reset otherwise it will contain
stale data.

By doing the overwrite this "reset" is done automatically as soon as the
vm is put to run again and doesn't hit the same (or other) errors.

The other option would be to allow the mngt application to manually reset
the field.

More ideas?

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Kevin Wolf]

Am 03.08.2011 20:08, schrieb Luiz Capitulino:
> On Wed, 03 Aug 2011 17:39:04 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 03.08.2011 17:19, schrieb Luiz Capitulino:
>>> Roughly speaking, thin provisioning is a feature where the VM is started with
>>> a small disk and space is allocated on demand.
>>>
>>> It's already possible for QMP clients to implement this feature by using
>>> the BLOCK_IO_ERROR event. However, the event can be missed. When this
>>> happens QMP clients need a way to query if any block device has hit a
>>> no space condition.
>>>
>>> This is what this series is about: it extends the query-block command to
>>> contain the disk's I/O status.
>>>
>>> Please, note that this series depends on the following two other series:
>>>
>>>  1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
>>>  2. [PATCH 0/7]: Introduce the QemuState type (by me)
>>
>> It feels strange that device models are involved with this. The block
>> layer already knows the error number, it just tells the device model so
>> that it can ask it back. Wouldn't it be easier to store it in the
>> BlockDriverState?
> 
> My first version did it, but looking at it now maybe I did it wrong,
> because I was setting the iostatus in the devices (instead of setting
> it in the block layer itself). Here's an example:
> 
>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00421.html
> 
> But I'm not sure what the best place is. I'm hoping that you and/or
> Markus can help me here. Also note that Markus already told us what
> he thinks:
> 
> """
> Actually, this is a symptom of the midlayer disease.  I suspect things
> would be simpler if we hold the status in its rightful owner, the device
> model.  Need a getter for it.  I'm working on a patch series that moves
> misplaced state out of the block layer into device models and block
> drivers, and a I/O status getter will fit in easily there.
> """
> 
>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00937.html
> 
> Can you guys get in agreement?

I think you can argue either way, but the main point is that we should
clearly assign it to either host or guest state, but not involve both sides.

For me, an ENOSPC is clearly host state, so I would prefer to see it
done in the block layer. An I/O status property of a device I would
expect to provide whatever the guest sees and that's not an ENOSPC.

>> Also, as I already commented on v1, I think it's a bad idea to let
>> requests that complete later overwrite the error code with "success".
>> Quite possible in an ENOSPC case, I think.
> 
> Really? I'm under the assumption that all pending I/O will be completed
> as soon as the vm is stopped. Am I wrong?

Yes, vm_stop() will flush all requests. And of course this means that
any request that completes successfully during this flush will reset the
error value.

> What we need here is a way to "reset" the io-status field. That's, the vm will
> stop due to ENOSPC, the mngt application will handle it and put the vm to run
> again. At this point the io-status has to be reset otherwise it will contain
> stale data.
> 
> By doing the overwrite this "reset" is done automatically as soon as the
> vm is put to run again and doesn't hit the same (or other) errors.
> 
> The other option would be to allow the mngt application to manually reset
> the field.
> 
> More ideas?

Maybe we could do it automatically in 'cont'. Not sure if this is too
much magic, but if the VM isn't stopped you can't do anything useful
with the value anyway.

Kevin

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.08.2011 20:08, schrieb Luiz Capitulino:
>> On Wed, 03 Aug 2011 17:39:04 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>> 
>>> Am 03.08.2011 17:19, schrieb Luiz Capitulino:
>>>> Roughly speaking, thin provisioning is a feature where the VM is started with
>>>> a small disk and space is allocated on demand.
>>>>
>>>> It's already possible for QMP clients to implement this feature by using
>>>> the BLOCK_IO_ERROR event. However, the event can be missed. When this
>>>> happens QMP clients need a way to query if any block device has hit a
>>>> no space condition.
>>>>
>>>> This is what this series is about: it extends the query-block command to
>>>> contain the disk's I/O status.
>>>>
>>>> Please, note that this series depends on the following two other series:
>>>>
>>>>  1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
>>>>  2. [PATCH 0/7]: Introduce the QemuState type (by me)
>>>
>>> It feels strange that device models are involved with this. The block
>>> layer already knows the error number, it just tells the device model so
>>> that it can ask it back. Wouldn't it be easier to store it in the
>>> BlockDriverState?
>> 
>> My first version did it, but looking at it now maybe I did it wrong,
>> because I was setting the iostatus in the devices (instead of setting
>> it in the block layer itself). Here's an example:
>> 
>>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00421.html
>> 
>> But I'm not sure what the best place is. I'm hoping that you and/or
>> Markus can help me here. Also note that Markus already told us what
>> he thinks:
>> 
>> """
>> Actually, this is a symptom of the midlayer disease.  I suspect things
>> would be simpler if we hold the status in its rightful owner, the device
>> model.  Need a getter for it.  I'm working on a patch series that moves
>> misplaced state out of the block layer into device models and block
>> drivers, and a I/O status getter will fit in easily there.
>> """
>> 
>>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00937.html
>> 
>> Can you guys get in agreement?
>
> I think you can argue either way, but the main point is that we should
> clearly assign it to either host or guest state, but not involve both sides.

Indeed.

> For me, an ENOSPC is clearly host state, so I would prefer to see it
> done in the block layer. An I/O status property of a device I would
> expect to provide whatever the guest sees and that's not an ENOSPC.

If it can be done in the block layer without involving device models,
and the status is not guest visible, then it's most probably host state,
and I retract the stupid things I said earlier :)

>>> Also, as I already commented on v1, I think it's a bad idea to let
>>> requests that complete later overwrite the error code with "success".
>>> Quite possible in an ENOSPC case, I think.
>> 
>> Really? I'm under the assumption that all pending I/O will be completed
>> as soon as the vm is stopped. Am I wrong?
>
> Yes, vm_stop() will flush all requests. And of course this means that
> any request that completes successfully during this flush will reset the
> error value.

Looks like a show stopper.

What if multiple different errors occur?  Do we need a set of errors, or
should the first one stick, or should the most important one (definition
needed) stick?

>> What we need here is a way to "reset" the io-status field. That's, the vm will
>> stop due to ENOSPC, the mngt application will handle it and put the vm to run
>> again. At this point the io-status has to be reset otherwise it will contain
>> stale data.
>> 
>> By doing the overwrite this "reset" is done automatically as soon as the
>> vm is put to run again and doesn't hit the same (or other) errors.
>> 
>> The other option would be to allow the mngt application to manually reset
>> the field.
>> 
>> More ideas?
>
> Maybe we could do it automatically in 'cont'. Not sure if this is too
> much magic, but if the VM isn't stopped you can't do anything useful
> with the value anyway.

Sounds workable to me.

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Luiz Capitulino]

On Thu, 04 Aug 2011 11:19:31 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 03.08.2011 20:08, schrieb Luiz Capitulino:
> >> On Wed, 03 Aug 2011 17:39:04 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >> 
> >>> Am 03.08.2011 17:19, schrieb Luiz Capitulino:
> >>>> Roughly speaking, thin provisioning is a feature where the VM is started with
> >>>> a small disk and space is allocated on demand.
> >>>>
> >>>> It's already possible for QMP clients to implement this feature by using
> >>>> the BLOCK_IO_ERROR event. However, the event can be missed. When this
> >>>> happens QMP clients need a way to query if any block device has hit a
> >>>> no space condition.
> >>>>
> >>>> This is what this series is about: it extends the query-block command to
> >>>> contain the disk's I/O status.
> >>>>
> >>>> Please, note that this series depends on the following two other series:
> >>>>
> >>>>  1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
> >>>>  2. [PATCH 0/7]: Introduce the QemuState type (by me)
> >>>
> >>> It feels strange that device models are involved with this. The block
> >>> layer already knows the error number, it just tells the device model so
> >>> that it can ask it back. Wouldn't it be easier to store it in the
> >>> BlockDriverState?
> >> 
> >> My first version did it, but looking at it now maybe I did it wrong,
> >> because I was setting the iostatus in the devices (instead of setting
> >> it in the block layer itself). Here's an example:
> >> 
> >>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00421.html
> >> 
> >> But I'm not sure what the best place is. I'm hoping that you and/or
> >> Markus can help me here. Also note that Markus already told us what
> >> he thinks:
> >> 
> >> """
> >> Actually, this is a symptom of the midlayer disease.  I suspect things
> >> would be simpler if we hold the status in its rightful owner, the device
> >> model.  Need a getter for it.  I'm working on a patch series that moves
> >> misplaced state out of the block layer into device models and block
> >> drivers, and a I/O status getter will fit in easily there.
> >> """
> >> 
> >>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00937.html
> >> 
> >> Can you guys get in agreement?
> >
> > I think you can argue either way, but the main point is that we should
> > clearly assign it to either host or guest state, but not involve both sides.
> 
> Indeed.
> 
> > For me, an ENOSPC is clearly host state, so I would prefer to see it
> > done in the block layer. An I/O status property of a device I would
> > expect to provide whatever the guest sees and that's not an ENOSPC.
> 
> If it can be done in the block layer without involving device models,
> and the status is not guest visible, then it's most probably host state,
> and I retract the stupid things I said earlier :)

ACK.

It's also important to note that I'm considering to do what Christoph
suggested in the other thread: only set ENOSPC. Perhaps, only when it's
set to be reported by the werror option.

This would simplify things immensely. I just have to be sure it would
be sufficient for the use-case.

> 
> >>> Also, as I already commented on v1, I think it's a bad idea to let
> >>> requests that complete later overwrite the error code with "success".
> >>> Quite possible in an ENOSPC case, I think.
> >> 
> >> Really? I'm under the assumption that all pending I/O will be completed
> >> as soon as the vm is stopped. Am I wrong?
> >
> > Yes, vm_stop() will flush all requests. And of course this means that
> > any request that completes successfully during this flush will reset the
> > error value.
> 
> Looks like a show stopper.
> 
> What if multiple different errors occur?  Do we need a set of errors, or
> should the first one stick, or should the most important one (definition
> needed) stick?

Good point. I'd choose to stick with the first one, but this is not an issue
if we implement the idea described above.

> >> What we need here is a way to "reset" the io-status field. That's, the vm will
> >> stop due to ENOSPC, the mngt application will handle it and put the vm to run
> >> again. At this point the io-status has to be reset otherwise it will contain
> >> stale data.
> >> 
> >> By doing the overwrite this "reset" is done automatically as soon as the
> >> vm is put to run again and doesn't hit the same (or other) errors.
> >> 
> >> The other option would be to allow the mngt application to manually reset
> >> the field.
> >> 
> >> More ideas?
> >
> > Maybe we could do it automatically in 'cont'. Not sure if this is too
> > much magic, but if the VM isn't stopped you can't do anything useful
> > with the value anyway.
> 
> Sounds workable to me.

I thought about doing this too, but I feared I'd be coupling unrelated things.

Maybe the block layer could register a vm state handler which resets the
field for all BSs (at vm_start() time)?

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Christoph Hellwig]

> It's already possible for QMP clients to implement this feature by using
> the BLOCK_IO_ERROR event. However, the event can be missed. When this
> happens QMP clients need a way to query if any block device has hit a
> no space condition.

We have a mode where we stop the VM when it hits ENOSPC, and that is
the only reliably way to handle it.  One that ENOSPC gets returned
to the guest we don't know that it was an ENOSPC, and the guest will
usually simply shut the filesystem down.  This could be worked around
by implementing full T10 logical provisioning support, but very few
guests actually support it properly.  And it won't work with ide disks
at all, and for virtio we'd have to implement it first.

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Luiz Capitulino]

On Wed, 3 Aug 2011 18:31:31 +0200
Christoph Hellwig <hch@lst.de> wrote:

> > It's already possible for QMP clients to implement this feature by using
> > the BLOCK_IO_ERROR event. However, the event can be missed. When this
> > happens QMP clients need a way to query if any block device has hit a
> > no space condition.
> 
> We have a mode where we stop the VM when it hits ENOSPC, and that is
> the only reliably way to handle it. 

This series just complements that mode by allowing a mngt application
to query which device triggered the ENOSPC. Well, that's my intention :)

> One that ENOSPC gets returned
> to the guest we don't know that it was an ENOSPC, and the guest will
> usually simply shut the filesystem down.  This could be worked around
> by implementing full T10 logical provisioning support, but very few
> guests actually support it properly.  And it won't work with ide disks
> at all, and for virtio we'd have to implement it first.

Seems interesting, but the current solution is guest independent.

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Christoph Hellwig]

On Wed, Aug 03, 2011 at 03:11:38PM -0300, Luiz Capitulino wrote:
> > > It's already possible for QMP clients to implement this feature by using
> > > the BLOCK_IO_ERROR event. However, the event can be missed. When this
> > > happens QMP clients need a way to query if any block device has hit a
> > > no space condition.
> > 
> > We have a mode where we stop the VM when it hits ENOSPC, and that is
> > the only reliably way to handle it. 
> 
> This series just complements that mode by allowing a mngt application
> to query which device triggered the ENOSPC. Well, that's my intention :)

Oh, so you only plan to use the QMP command post-mortem?  That makes a lot
more sense.  But relying on that fact should also help to simplify the
implementation and make it more robust by storing the error code from
the same place that we stop them VM.

Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Luiz Capitulino]

On Wed, 3 Aug 2011 20:26:30 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Wed, Aug 03, 2011 at 03:11:38PM -0300, Luiz Capitulino wrote:
> > > > It's already possible for QMP clients to implement this feature by using
> > > > the BLOCK_IO_ERROR event. However, the event can be missed. When this
> > > > happens QMP clients need a way to query if any block device has hit a
> > > > no space condition.
> > > 
> > > We have a mode where we stop the VM when it hits ENOSPC, and that is
> > > the only reliably way to handle it. 
> > 
> > This series just complements that mode by allowing a mngt application
> > to query which device triggered the ENOSPC. Well, that's my intention :)
> 
> Oh, so you only plan to use the QMP command post-mortem?  That makes a lot
> more sense.  

Yes. Calling it 'proper thin provisioning support' was too ambitious from
my part.

> But relying on that fact should also help to simplify the
> implementation and make it more robust by storing the error code from
> the same place that we stop them VM.

Yes, I think I'll do that.