[Qemu-devel] [PATCH 0/7] block: convert VDI image format to coroutines

[Qemu-devel] [PATCH 0/7] block: convert VDI image format to coroutines

[Paolo Bonzini]

Conversion to coroutines simplifies the code and removes the need to
duplicate common features of the block layer.  Each step in the conversion
is detailed in the corresponding commit message.

Tested with qemu-iotests.

Paolo Bonzini (7):
  vdi: basic conversion to coroutines
  vdi: move end-of-I/O handling at the end
  vdi: merge aio_read_cb and aio_write_cb into callers
  vdi: move aiocb fields to locals
  vdi: leave bounce buffering to block layer
  vdi: do not create useless iovecs
  vdi: change goto to loop

 block/vdi.c          |  421 +++++++++++++-------------------------------------
 2 files changed, 108 insertions(+), 317 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/7] vdi: basic conversion to coroutines

[Paolo Bonzini]

Even a basic conversion changing the bdrv_aio_readv/bdrv_aio_writev calls
to bdrv_co_readv/bdrv_co_writev, and callbacks to goto statements can
eliminate a lot of code.  This is because error handling is simplified
and indirections through bottom halves can go away.

After this patch, I/O to the underlying file already happens via
coroutines, but the code still looks a lot like if asynchronous I/O was
being used.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c |  158 ++++++++++++++---------------------------------------------
 1 files changed, 37 insertions(+), 121 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6a0011f..35edc90 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -160,7 +160,6 @@ typedef struct {
     void *orig_buf;
     bool is_write;
     int header_modified;
-    BlockDriverAIOCB *hd_aiocb;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
     QEMUBH *bh;
@@ -489,33 +488,19 @@ static int coroutine_fn vdi_co_is_allocated(BlockDriverState *bs,
     return VDI_IS_ALLOCATED(bmap_entry);
 }
 
-static void vdi_aio_cancel(BlockDriverAIOCB *blockacb)
-{
-    /* TODO: This code is untested. How can I get it executed? */
-    VdiAIOCB *acb = container_of(blockacb, VdiAIOCB, common);
-    logout("\n");
-    if (acb->hd_aiocb) {
-        bdrv_aio_cancel(acb->hd_aiocb);
-    }
-    qemu_aio_release(acb);
-}
-
 static AIOPool vdi_aio_pool = {
     .aiocb_size = sizeof(VdiAIOCB),
-    .cancel = vdi_aio_cancel,
 };
 
 static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
-        QEMUIOVector *qiov, int nb_sectors,
-        BlockDriverCompletionFunc *cb, void *opaque, int is_write)
+        QEMUIOVector *qiov, int nb_sectors, int is_write)
 {
     VdiAIOCB *acb;
 
     logout("%p, %" PRId64 ", %p, %d, %p, %p, %d\n",
-           bs, sector_num, qiov, nb_sectors, cb, opaque, is_write);
+           bs, sector_num, qiov, nb_sectors, is_write);
 
-    acb = qemu_aio_get(&vdi_aio_pool, bs, cb, opaque);
-    acb->hd_aiocb = NULL;
+    acb = qemu_aio_get(&vdi_aio_pool, bs, NULL, NULL);
     acb->sector_num = sector_num;
     acb->qiov = qiov;
     acb->is_write = is_write;
@@ -538,42 +523,7 @@ static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
     return acb;
 }
 
-static int vdi_schedule_bh(QEMUBHFunc *cb, VdiAIOCB *acb)
-{
-    logout("\n");
-
-    if (acb->bh) {
-        return -EIO;
-    }
-
-    acb->bh = qemu_bh_new(cb, acb);
-    if (!acb->bh) {
-        return -EIO;
-    }
-
-    qemu_bh_schedule(acb->bh);
-
-    return 0;
-}
-
-static void vdi_aio_read_cb(void *opaque, int ret);
-static void vdi_aio_write_cb(void *opaque, int ret);
-
-static void vdi_aio_rw_bh(void *opaque)
-{
-    VdiAIOCB *acb = opaque;
-    logout("\n");
-    qemu_bh_delete(acb->bh);
-    acb->bh = NULL;
-
-    if (acb->is_write) {
-        vdi_aio_write_cb(opaque, 0);
-    } else {
-        vdi_aio_read_cb(opaque, 0);
-    }
-}
-
-static void vdi_aio_read_cb(void *opaque, int ret)
+static int vdi_aio_read_cb(void *opaque, int ret)
 {
     VdiAIOCB *acb = opaque;
     BlockDriverState *bs = acb->common.bs;
@@ -585,12 +535,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
 
     logout("%u sectors read\n", acb->n_sectors);
 
-    acb->hd_aiocb = NULL;
-
-    if (ret < 0) {
-        goto done;
-    }
-
+restart:
     acb->nb_sectors -= acb->n_sectors;
 
     if (acb->nb_sectors == 0) {
@@ -618,10 +563,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
     if (!VDI_IS_ALLOCATED(bmap_entry)) {
         /* Block not allocated, return zeros, no need to wait. */
         memset(acb->buf, 0, n_sectors * SECTOR_SIZE);
-        ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
-        if (ret < 0) {
-            goto done;
-        }
+        ret = 0;
     } else {
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
@@ -629,41 +571,34 @@ static void vdi_aio_read_cb(void *opaque, int ret)
         acb->hd_iov.iov_base = (void *)acb->buf;
         acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
         qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-        acb->hd_aiocb = bdrv_aio_readv(bs->file, offset, &acb->hd_qiov,
-                                       n_sectors, vdi_aio_read_cb, acb);
+        ret = bdrv_co_readv(bs->file, offset, n_sectors, &acb->hd_qiov);
+    }
+    if (ret >= 0) {
+        goto restart;
     }
-    return;
+
 done:
     if (acb->qiov->niov > 1) {
         qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
         qemu_vfree(acb->orig_buf);
     }
-    acb->common.cb(acb->common.opaque, ret);
     qemu_aio_release(acb);
+    return ret;
 }
 
-static BlockDriverAIOCB *vdi_aio_readv(BlockDriverState *bs,
-        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
-        BlockDriverCompletionFunc *cb, void *opaque)
+static int vdi_co_readv(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
     VdiAIOCB *acb;
     int ret;
 
     logout("\n");
-    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
-    ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
-    if (ret < 0) {
-        if (acb->qiov->niov > 1) {
-            qemu_vfree(acb->orig_buf);
-        }
-        qemu_aio_release(acb);
-        return NULL;
-    }
-
-    return &acb->common;
+    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 0);
+    ret = vdi_aio_read_cb(acb, 0);
+    return ret;
 }
 
-static void vdi_aio_write_cb(void *opaque, int ret)
+static int vdi_aio_write_cb(void *opaque, int ret)
 {
     VdiAIOCB *acb = opaque;
     BlockDriverState *bs = acb->common.bs;
@@ -673,12 +608,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
     uint32_t sector_in_block;
     uint32_t n_sectors;
 
-    acb->hd_aiocb = NULL;
-
-    if (ret < 0) {
-        goto done;
-    }
-
+restart:
     acb->nb_sectors -= acb->n_sectors;
     acb->sector_num += acb->n_sectors;
     acb->buf += acb->n_sectors * SECTOR_SIZE;
@@ -686,6 +616,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
     if (acb->nb_sectors == 0) {
         logout("finished data write\n");
         acb->n_sectors = 0;
+        ret = 0;
         if (acb->header_modified) {
             VdiHeader *header = acb->block_buffer;
             logout("now writing modified header\n");
@@ -696,10 +627,9 @@ static void vdi_aio_write_cb(void *opaque, int ret)
             acb->hd_iov.iov_base = acb->block_buffer;
             acb->hd_iov.iov_len = SECTOR_SIZE;
             qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-            acb->hd_aiocb = bdrv_aio_writev(bs->file, 0, &acb->hd_qiov, 1,
-                                            vdi_aio_write_cb, acb);
-            return;
-        } else if (VDI_IS_ALLOCATED(acb->bmap_first)) {
+            ret = bdrv_co_writev(bs->file, 0, 1, &acb->hd_qiov);
+        }
+        if (ret >= 0 && VDI_IS_ALLOCATED(acb->bmap_first)) {
             /* One or more new blocks were allocated. */
             uint64_t offset;
             uint32_t bmap_first;
@@ -722,11 +652,8 @@ static void vdi_aio_write_cb(void *opaque, int ret)
             qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
             logout("will write %u block map sectors starting from entry %u\n",
                    n_sectors, bmap_first);
-            acb->hd_aiocb = bdrv_aio_writev(bs->file, offset, &acb->hd_qiov,
-                                            n_sectors, vdi_aio_write_cb, acb);
-            return;
+            ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
         }
-        ret = 0;
         goto done;
     }
 
@@ -772,9 +699,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
         acb->hd_iov.iov_base = (void *)block;
         acb->hd_iov.iov_len = s->block_size;
         qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-        acb->hd_aiocb = bdrv_aio_writev(bs->file, offset,
-                                        &acb->hd_qiov, s->block_sectors,
-                                        vdi_aio_write_cb, acb);
+        ret = bdrv_co_writev(bs->file, offset, s->block_sectors, &acb->hd_qiov);
     } else {
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
@@ -782,39 +707,30 @@ static void vdi_aio_write_cb(void *opaque, int ret)
         acb->hd_iov.iov_base = (void *)acb->buf;
         acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
         qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-        acb->hd_aiocb = bdrv_aio_writev(bs->file, offset, &acb->hd_qiov,
-                                        n_sectors, vdi_aio_write_cb, acb);
+        ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
+    }
+    if (ret >= 0) {
+        goto restart;
     }
-
-    return;
 
 done:
     if (acb->qiov->niov > 1) {
         qemu_vfree(acb->orig_buf);
     }
-    acb->common.cb(acb->common.opaque, ret);
     qemu_aio_release(acb);
+    return ret;
 }
 
-static BlockDriverAIOCB *vdi_aio_writev(BlockDriverState *bs,
-        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
-        BlockDriverCompletionFunc *cb, void *opaque)
+static int vdi_co_writev(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
     VdiAIOCB *acb;
     int ret;
 
     logout("\n");
-    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
-    ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
-    if (ret < 0) {
-        if (acb->qiov->niov > 1) {
-            qemu_vfree(acb->orig_buf);
-        }
-        qemu_aio_release(acb);
-        return NULL;
-    }
-
-    return &acb->common;
+    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 1);
+    ret = vdi_aio_write_cb(acb, 0);
+    return ret;
 }
 
 static int vdi_create(const char *filename, QEMUOptionParameter *options)
@@ -973,9 +889,9 @@ static BlockDriver bdrv_vdi = {
     .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
-    .bdrv_aio_readv = vdi_aio_readv,
+    .bdrv_co_readv = vdi_co_readv,
 #if defined(CONFIG_VDI_WRITE)
-    .bdrv_aio_writev = vdi_aio_writev,
+    .bdrv_co_writev = vdi_co_writev,
 #endif
 
     .bdrv_get_info = vdi_get_info,
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/7] vdi: move end-of-I/O handling at the end

[Paolo Bonzini]

The next step is to take code that only triggers after the first operation,
and move it at the end of vdi_aio_read_cb and vdi_aio_write_cb.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c |  123 +++++++++++++++++++++++++++--------------------------------
 1 files changed, 56 insertions(+), 67 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 35edc90..4b780db 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -533,20 +533,7 @@ static int vdi_aio_read_cb(void *opaque, int ret)
     uint32_t sector_in_block;
     uint32_t n_sectors;
 
-    logout("%u sectors read\n", acb->n_sectors);
-
 restart:
-    acb->nb_sectors -= acb->n_sectors;
-
-    if (acb->nb_sectors == 0) {
-        /* request completed */
-        ret = 0;
-        goto done;
-    }
-
-    acb->sector_num += acb->n_sectors;
-    acb->buf += acb->n_sectors * SECTOR_SIZE;
-
     block_index = acb->sector_num / s->block_sectors;
     sector_in_block = acb->sector_num % s->block_sectors;
     n_sectors = s->block_sectors - sector_in_block;
@@ -573,11 +560,16 @@ restart:
         qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
         ret = bdrv_co_readv(bs->file, offset, n_sectors, &acb->hd_qiov);
     }
-    if (ret >= 0) {
+    logout("%u sectors read\n", acb->n_sectors);
+
+    acb->nb_sectors -= acb->n_sectors;
+    acb->sector_num += acb->n_sectors;
+    acb->buf += acb->n_sectors * SECTOR_SIZE;
+
+    if (ret >= 0 && acb->nb_sectors > 0) {
         goto restart;
     }
 
-done:
     if (acb->qiov->niov > 1) {
         qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
         qemu_vfree(acb->orig_buf);
@@ -609,56 +601,6 @@ static int vdi_aio_write_cb(void *opaque, int ret)
     uint32_t n_sectors;
 
 restart:
-    acb->nb_sectors -= acb->n_sectors;
-    acb->sector_num += acb->n_sectors;
-    acb->buf += acb->n_sectors * SECTOR_SIZE;
-
-    if (acb->nb_sectors == 0) {
-        logout("finished data write\n");
-        acb->n_sectors = 0;
-        ret = 0;
-        if (acb->header_modified) {
-            VdiHeader *header = acb->block_buffer;
-            logout("now writing modified header\n");
-            assert(VDI_IS_ALLOCATED(acb->bmap_first));
-            *header = s->header;
-            vdi_header_to_le(header);
-            acb->header_modified = 0;
-            acb->hd_iov.iov_base = acb->block_buffer;
-            acb->hd_iov.iov_len = SECTOR_SIZE;
-            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-            ret = bdrv_co_writev(bs->file, 0, 1, &acb->hd_qiov);
-        }
-        if (ret >= 0 && VDI_IS_ALLOCATED(acb->bmap_first)) {
-            /* One or more new blocks were allocated. */
-            uint64_t offset;
-            uint32_t bmap_first;
-            uint32_t bmap_last;
-            g_free(acb->block_buffer);
-            acb->block_buffer = NULL;
-            bmap_first = acb->bmap_first;
-            bmap_last = acb->bmap_last;
-            logout("now writing modified block map entry %u...%u\n",
-                   bmap_first, bmap_last);
-            /* Write modified sectors from block map. */
-            bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
-            bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
-            n_sectors = bmap_last - bmap_first + 1;
-            offset = s->bmap_sector + bmap_first;
-            acb->bmap_first = VDI_UNALLOCATED;
-            acb->hd_iov.iov_base = (void *)((uint8_t *)&s->bmap[0] +
-                                            bmap_first * SECTOR_SIZE);
-            acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-            logout("will write %u block map sectors starting from entry %u\n",
-                   n_sectors, bmap_first);
-            ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
-        }
-        goto done;
-    }
-
-    logout("%u sectors written\n", acb->n_sectors);
-
     block_index = acb->sector_num / s->block_sectors;
     sector_in_block = acb->sector_num % s->block_sectors;
     n_sectors = s->block_sectors - sector_in_block;
@@ -709,11 +651,58 @@ restart:
         qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
         ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
     }
-    if (ret >= 0) {
+
+    acb->nb_sectors -= acb->n_sectors;
+    acb->sector_num += acb->n_sectors;
+    acb->buf += acb->n_sectors * SECTOR_SIZE;
+
+    logout("%u sectors written\n", acb->n_sectors);
+    if (ret >= 0 && acb->nb_sectors > 0) {
         goto restart;
     }
 
-done:
+    logout("finished data write\n");
+    if (ret >= 0) {
+        ret = 0;
+        if (acb->header_modified) {
+            VdiHeader *header = acb->block_buffer;
+            logout("now writing modified header\n");
+            assert(VDI_IS_ALLOCATED(acb->bmap_first));
+            *header = s->header;
+            vdi_header_to_le(header);
+            acb->header_modified = 0;
+            acb->hd_iov.iov_base = acb->block_buffer;
+            acb->hd_iov.iov_len = SECTOR_SIZE;
+            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
+            ret = bdrv_co_writev(bs->file, 0, 1, &acb->hd_qiov);
+        }
+        if (ret >= 0 && VDI_IS_ALLOCATED(acb->bmap_first)) {
+            /* One or more new blocks were allocated. */
+            uint64_t offset;
+            uint32_t bmap_first;
+            uint32_t bmap_last;
+            g_free(acb->block_buffer);
+            acb->block_buffer = NULL;
+            bmap_first = acb->bmap_first;
+            bmap_last = acb->bmap_last;
+            logout("now writing modified block map entry %u...%u\n",
+                   bmap_first, bmap_last);
+            /* Write modified sectors from block map. */
+            bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
+            bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
+            n_sectors = bmap_last - bmap_first + 1;
+            offset = s->bmap_sector + bmap_first;
+            acb->bmap_first = VDI_UNALLOCATED;
+            acb->hd_iov.iov_base = (void *)((uint8_t *)&s->bmap[0] +
+                                            bmap_first * SECTOR_SIZE);
+            acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
+            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
+            logout("will write %u block map sectors starting from entry %u\n",
+                   n_sectors, bmap_first);
+            ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
+        }
+    }
+
     if (acb->qiov->niov > 1) {
         qemu_vfree(acb->orig_buf);
     }
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/7] vdi: merge aio_read_cb and aio_write_cb into callers

[Paolo Bonzini]

Now inline the former AIO callbacks into vdi_co_readv and vdi_co_writev.
While many cleanups are possible, the code now really looks synchronous.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c |   40 ++++++++++++----------------------------
 1 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 4b780db..9e5b169 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -523,15 +523,19 @@ static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
     return acb;
 }
 
-static int vdi_aio_read_cb(void *opaque, int ret)
+static int vdi_co_readv(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
-    VdiAIOCB *acb = opaque;
-    BlockDriverState *bs = acb->common.bs;
+    VdiAIOCB *acb;
     BDRVVdiState *s = bs->opaque;
     uint32_t bmap_entry;
     uint32_t block_index;
     uint32_t sector_in_block;
     uint32_t n_sectors;
+    int ret;
+
+    logout("\n");
+    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 0);
 
 restart:
     block_index = acb->sector_num / s->block_sectors;
@@ -578,27 +582,19 @@ restart:
     return ret;
 }
 
-static int vdi_co_readv(BlockDriverState *bs,
+static int vdi_co_writev(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
 {
     VdiAIOCB *acb;
-    int ret;
-
-    logout("\n");
-    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 0);
-    ret = vdi_aio_read_cb(acb, 0);
-    return ret;
-}
-
-static int vdi_aio_write_cb(void *opaque, int ret)
-{
-    VdiAIOCB *acb = opaque;
-    BlockDriverState *bs = acb->common.bs;
     BDRVVdiState *s = bs->opaque;
     uint32_t bmap_entry;
     uint32_t block_index;
     uint32_t sector_in_block;
     uint32_t n_sectors;
+    int ret;
+
+    logout("\n");
+    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 1);
 
 restart:
     block_index = acb->sector_num / s->block_sectors;
@@ -710,18 +706,6 @@ restart:
     return ret;
 }
 
-static int vdi_co_writev(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
-{
-    VdiAIOCB *acb;
-    int ret;
-
-    logout("\n");
-    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 1);
-    ret = vdi_aio_write_cb(acb, 0);
-    return ret;
-}
-
 static int vdi_create(const char *filename, QEMUOptionParameter *options)
 {
     int fd;
-- 
1.7.7.6

[Qemu-devel] [PATCH 4/7] vdi: move aiocb fields to locals

[Paolo Bonzini]

Most of the AIOCB really holds local variables that need to persist
across callback invocation.  It can go away now.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c          |  162 ++++++++++++++++++++------------------------------
 2 files changed, 65 insertions(+), 101 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 9e5b169..682583c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -145,24 +145,8 @@ void uuid_unparse(const uuid_t uu, char *out)
 
 typedef struct {
     BlockDriverAIOCB common;
-    int64_t sector_num;
-    QEMUIOVector *qiov;
     uint8_t *buf;
-    /* Total number of sectors. */
-    int nb_sectors;
-    /* Number of sectors for current AIO. */
-    int n_sectors;
-    /* New allocated block map entry. */
-    uint32_t bmap_first;
-    uint32_t bmap_last;
-    /* Buffer for new allocated block. */
-    void *block_buffer;
     void *orig_buf;
-    bool is_write;
-    int header_modified;
-    struct iovec hd_iov;
-    QEMUIOVector hd_qiov;
-    QEMUBH *bh;
 } VdiAIOCB;
 
 typedef struct {
@@ -492,34 +476,20 @@ static AIOPool vdi_aio_pool = {
     .aiocb_size = sizeof(VdiAIOCB),
 };
 
-static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
-        QEMUIOVector *qiov, int nb_sectors, int is_write)
+static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov)
 {
     VdiAIOCB *acb;
 
-    logout("%p, %" PRId64 ", %p, %d, %p, %p, %d\n",
-           bs, sector_num, qiov, nb_sectors, is_write);
+    logout("%p, %p\n", bs, qiov);
 
     acb = qemu_aio_get(&vdi_aio_pool, bs, NULL, NULL);
-    acb->sector_num = sector_num;
-    acb->qiov = qiov;
-    acb->is_write = is_write;
 
     if (qiov->niov > 1) {
         acb->buf = qemu_blockalign(bs, qiov->size);
         acb->orig_buf = acb->buf;
-        if (is_write) {
-            qemu_iovec_to_buffer(qiov, acb->buf);
-        }
     } else {
         acb->buf = (uint8_t *)qiov->iov->iov_base;
     }
-    acb->nb_sectors = nb_sectors;
-    acb->n_sectors = 0;
-    acb->bmap_first = VDI_UNALLOCATED;
-    acb->bmap_last = VDI_UNALLOCATED;
-    acb->block_buffer = NULL;
-    acb->header_modified = 0;
     return acb;
 }
 
@@ -532,24 +503,25 @@ static int vdi_co_readv(BlockDriverState *bs,
     uint32_t block_index;
     uint32_t sector_in_block;
     uint32_t n_sectors;
+    struct iovec hd_iov;
+    QEMUIOVector hd_qiov;
     int ret;
 
     logout("\n");
-    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 0);
+    acb = vdi_aio_setup(bs, qiov);
 
 restart:
-    block_index = acb->sector_num / s->block_sectors;
-    sector_in_block = acb->sector_num % s->block_sectors;
+    block_index = sector_num / s->block_sectors;
+    sector_in_block = sector_num % s->block_sectors;
     n_sectors = s->block_sectors - sector_in_block;
-    if (n_sectors > acb->nb_sectors) {
-        n_sectors = acb->nb_sectors;
+    if (n_sectors > nb_sectors) {
+        n_sectors = nb_sectors;
     }
 
     logout("will read %u sectors starting at sector %" PRIu64 "\n",
-           n_sectors, acb->sector_num);
+           n_sectors, sector_num);
 
     /* prepare next AIO request */
-    acb->n_sectors = n_sectors;
     bmap_entry = le32_to_cpu(s->bmap[block_index]);
     if (!VDI_IS_ALLOCATED(bmap_entry)) {
         /* Block not allocated, return zeros, no need to wait. */
@@ -559,23 +531,23 @@ restart:
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
                           sector_in_block;
-        acb->hd_iov.iov_base = (void *)acb->buf;
-        acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-        qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-        ret = bdrv_co_readv(bs->file, offset, n_sectors, &acb->hd_qiov);
+        hd_iov.iov_base = (void *)acb->buf;
+        hd_iov.iov_len = n_sectors * SECTOR_SIZE;
+        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+        ret = bdrv_co_readv(bs->file, offset, n_sectors, &hd_qiov);
     }
-    logout("%u sectors read\n", acb->n_sectors);
+    logout("%u sectors read\n", n_sectors);
 
-    acb->nb_sectors -= acb->n_sectors;
-    acb->sector_num += acb->n_sectors;
-    acb->buf += acb->n_sectors * SECTOR_SIZE;
+    nb_sectors -= n_sectors;
+    sector_num += n_sectors;
+    acb->buf += n_sectors * SECTOR_SIZE;
 
-    if (ret >= 0 && acb->nb_sectors > 0) {
+    if (ret >= 0 && nb_sectors > 0) {
         goto restart;
     }
 
-    if (acb->qiov->niov > 1) {
-        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
+    if (acb->orig_buf) {
+        qemu_iovec_from_buffer(qiov, acb->orig_buf, qiov->size);
         qemu_vfree(acb->orig_buf);
     }
     qemu_aio_release(acb);
@@ -591,96 +563,93 @@ static int vdi_co_writev(BlockDriverState *bs,
     uint32_t block_index;
     uint32_t sector_in_block;
     uint32_t n_sectors;
+    uint32_t bmap_first;
+    uint32_t bmap_last;
+    struct iovec hd_iov;
+    QEMUIOVector hd_qiov;
+    uint8_t *block = NULL;
     int ret;
 
     logout("\n");
-    acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, 1);
+    acb = vdi_aio_setup(bs, qiov);
+    if (acb->orig_buf) {
+        qemu_iovec_to_buffer(qiov, acb->buf);
+    }
 
 restart:
-    block_index = acb->sector_num / s->block_sectors;
-    sector_in_block = acb->sector_num % s->block_sectors;
+    block_index = sector_num / s->block_sectors;
+    sector_in_block = sector_num % s->block_sectors;
     n_sectors = s->block_sectors - sector_in_block;
-    if (n_sectors > acb->nb_sectors) {
-        n_sectors = acb->nb_sectors;
+    if (n_sectors > nb_sectors) {
+        n_sectors = nb_sectors;
     }
 
     logout("will write %u sectors starting at sector %" PRIu64 "\n",
-           n_sectors, acb->sector_num);
+           n_sectors, sector_num);
 
     /* prepare next AIO request */
-    acb->n_sectors = n_sectors;
     bmap_entry = le32_to_cpu(s->bmap[block_index]);
     if (!VDI_IS_ALLOCATED(bmap_entry)) {
         /* Allocate new block and write to it. */
         uint64_t offset;
-        uint8_t *block;
         bmap_entry = s->header.blocks_allocated;
         s->bmap[block_index] = cpu_to_le32(bmap_entry);
         s->header.blocks_allocated++;
         offset = s->header.offset_data / SECTOR_SIZE +
                  (uint64_t)bmap_entry * s->block_sectors;
-        block = acb->block_buffer;
         if (block == NULL) {
             block = g_malloc(s->block_size);
-            acb->block_buffer = block;
-            acb->bmap_first = block_index;
-            assert(!acb->header_modified);
-            acb->header_modified = 1;
+            bmap_first = block_index;
         }
-        acb->bmap_last = block_index;
+        bmap_last = block_index;
         /* Copy data to be written to new block and zero unused parts. */
         memset(block, 0, sector_in_block * SECTOR_SIZE);
         memcpy(block + sector_in_block * SECTOR_SIZE,
                acb->buf, n_sectors * SECTOR_SIZE);
         memset(block + (sector_in_block + n_sectors) * SECTOR_SIZE, 0,
                (s->block_sectors - n_sectors - sector_in_block) * SECTOR_SIZE);
-        acb->hd_iov.iov_base = (void *)block;
-        acb->hd_iov.iov_len = s->block_size;
-        qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-        ret = bdrv_co_writev(bs->file, offset, s->block_sectors, &acb->hd_qiov);
+        hd_iov.iov_base = (void *)block;
+        hd_iov.iov_len = s->block_size;
+        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+        ret = bdrv_co_writev(bs->file, offset, s->block_sectors, &hd_qiov);
     } else {
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
                           sector_in_block;
-        acb->hd_iov.iov_base = (void *)acb->buf;
-        acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-        qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-        ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
+        hd_iov.iov_base = (void *)acb->buf;
+        hd_iov.iov_len = n_sectors * SECTOR_SIZE;
+        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+        ret = bdrv_co_writev(bs->file, offset, n_sectors, &hd_qiov);
     }
 
-    acb->nb_sectors -= acb->n_sectors;
-    acb->sector_num += acb->n_sectors;
-    acb->buf += acb->n_sectors * SECTOR_SIZE;
+    nb_sectors -= n_sectors;
+    sector_num += n_sectors;
+    acb->buf += n_sectors * SECTOR_SIZE;
 
-    logout("%u sectors written\n", acb->n_sectors);
-    if (ret >= 0 && acb->nb_sectors > 0) {
+    logout("%u sectors written\n", n_sectors);
+    if (ret >= 0 && nb_sectors > 0) {
         goto restart;
     }
 
     logout("finished data write\n");
     if (ret >= 0) {
         ret = 0;
-        if (acb->header_modified) {
-            VdiHeader *header = acb->block_buffer;
+        if (block) {
+            VdiHeader *header = (VdiHeader *) block;
             logout("now writing modified header\n");
-            assert(VDI_IS_ALLOCATED(acb->bmap_first));
+            assert(VDI_IS_ALLOCATED(bmap_first));
             *header = s->header;
             vdi_header_to_le(header);
-            acb->header_modified = 0;
-            acb->hd_iov.iov_base = acb->block_buffer;
-            acb->hd_iov.iov_len = SECTOR_SIZE;
-            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
-            ret = bdrv_co_writev(bs->file, 0, 1, &acb->hd_qiov);
+            hd_iov.iov_base = block;
+            hd_iov.iov_len = SECTOR_SIZE;
+            qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+            ret = bdrv_co_writev(bs->file, 0, 1, &hd_qiov);
         }
-        if (ret >= 0 && VDI_IS_ALLOCATED(acb->bmap_first)) {
+        g_free(block);
+        block = NULL;
+        if (ret >= 0 && VDI_IS_ALLOCATED(bmap_first)) {
             /* One or more new blocks were allocated. */
             uint64_t offset;
-            uint32_t bmap_first;
-            uint32_t bmap_last;
-            g_free(acb->block_buffer);
-            acb->block_buffer = NULL;
-            bmap_first = acb->bmap_first;
-            bmap_last = acb->bmap_last;
             logout("now writing modified block map entry %u...%u\n",
                    bmap_first, bmap_last);
             /* Write modified sectors from block map. */
@@ -688,18 +657,17 @@ restart:
             bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
             n_sectors = bmap_last - bmap_first + 1;
             offset = s->bmap_sector + bmap_first;
-            acb->bmap_first = VDI_UNALLOCATED;
-            acb->hd_iov.iov_base = (void *)((uint8_t *)&s->bmap[0] +
+            hd_iov.iov_base = (void *)((uint8_t *)&s->bmap[0] +
                                             bmap_first * SECTOR_SIZE);
-            acb->hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
+            hd_iov.iov_len = n_sectors * SECTOR_SIZE;
+            qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
             logout("will write %u block map sectors starting from entry %u\n",
                    n_sectors, bmap_first);
-            ret = bdrv_co_writev(bs->file, offset, n_sectors, &acb->hd_qiov);
+            ret = bdrv_co_writev(bs->file, offset, n_sectors, &hd_qiov);
         }
     }
 
-    if (acb->qiov->niov > 1) {
+    if (acb->orig_buf) {
         qemu_vfree(acb->orig_buf);
     }
     qemu_aio_release(acb);
-- 
1.7.7.6

[Qemu-devel] [PATCH 5/7] vdi: leave bounce buffering to block layer

[Paolo Bonzini]

vdi.c really works as if it implemented bdrv_read and bdrv_write.
However, because only vector I/O is supported by the asynchronous
callbacks, it went through extra pain to bounce-buffer the I/O.
With the conversion to coroutines bdrv_read and bdrv_write are now
asynchronous, so they can be handled by the block layer now that the
format is coroutine-based.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c |   68 ++++++++++------------------------------------------------
 1 files changed, 12 insertions(+), 56 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 682583c..5070f91 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -144,12 +144,6 @@ void uuid_unparse(const uuid_t uu, char *out)
 #endif
 
 typedef struct {
-    BlockDriverAIOCB common;
-    uint8_t *buf;
-    void *orig_buf;
-} VdiAIOCB;
-
-typedef struct {
     char text[0x40];
     uint32_t signature;
     uint32_t version;
@@ -472,31 +466,9 @@ static int coroutine_fn vdi_co_is_allocated(BlockDriverState *bs,
     return VDI_IS_ALLOCATED(bmap_entry);
 }
 
-static AIOPool vdi_aio_pool = {
-    .aiocb_size = sizeof(VdiAIOCB),
-};
-
-static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov)
+static int vdi_co_read(BlockDriverState *bs,
+        int64_t sector_num, uint8_t *buf, int nb_sectors)
 {
-    VdiAIOCB *acb;
-
-    logout("%p, %p\n", bs, qiov);
-
-    acb = qemu_aio_get(&vdi_aio_pool, bs, NULL, NULL);
-
-    if (qiov->niov > 1) {
-        acb->buf = qemu_blockalign(bs, qiov->size);
-        acb->orig_buf = acb->buf;
-    } else {
-        acb->buf = (uint8_t *)qiov->iov->iov_base;
-    }
-    return acb;
-}
-
-static int vdi_co_readv(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
-{
-    VdiAIOCB *acb;
     BDRVVdiState *s = bs->opaque;
     uint32_t bmap_entry;
     uint32_t block_index;
@@ -508,7 +479,6 @@ static int vdi_co_readv(BlockDriverState *bs,
     int ret;
 
     logout("\n");
-    acb = vdi_aio_setup(bs, qiov);
 
 restart:
     block_index = sector_num / s->block_sectors;
@@ -525,13 +495,13 @@ restart:
     bmap_entry = le32_to_cpu(s->bmap[block_index]);
     if (!VDI_IS_ALLOCATED(bmap_entry)) {
         /* Block not allocated, return zeros, no need to wait. */
-        memset(acb->buf, 0, n_sectors * SECTOR_SIZE);
+        memset(buf, 0, n_sectors * SECTOR_SIZE);
         ret = 0;
     } else {
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
                           sector_in_block;
-        hd_iov.iov_base = (void *)acb->buf;
+        hd_iov.iov_base = (void *)buf;
         hd_iov.iov_len = n_sectors * SECTOR_SIZE;
         qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
         ret = bdrv_co_readv(bs->file, offset, n_sectors, &hd_qiov);
@@ -540,24 +510,18 @@ restart:
 
     nb_sectors -= n_sectors;
     sector_num += n_sectors;
-    acb->buf += n_sectors * SECTOR_SIZE;
+    buf += n_sectors * SECTOR_SIZE;
 
     if (ret >= 0 && nb_sectors > 0) {
         goto restart;
     }
 
-    if (acb->orig_buf) {
-        qemu_iovec_from_buffer(qiov, acb->orig_buf, qiov->size);
-        qemu_vfree(acb->orig_buf);
-    }
-    qemu_aio_release(acb);
     return ret;
 }
 
-static int vdi_co_writev(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+static int vdi_co_write(BlockDriverState *bs,
+        int64_t sector_num, const uint8_t *buf, int nb_sectors)
 {
-    VdiAIOCB *acb;
     BDRVVdiState *s = bs->opaque;
     uint32_t bmap_entry;
     uint32_t block_index;
@@ -571,10 +535,6 @@ static int vdi_co_writev(BlockDriverState *bs,
     int ret;
 
     logout("\n");
-    acb = vdi_aio_setup(bs, qiov);
-    if (acb->orig_buf) {
-        qemu_iovec_to_buffer(qiov, acb->buf);
-    }
 
 restart:
     block_index = sector_num / s->block_sectors;
@@ -605,7 +565,7 @@ restart:
         /* Copy data to be written to new block and zero unused parts. */
         memset(block, 0, sector_in_block * SECTOR_SIZE);
         memcpy(block + sector_in_block * SECTOR_SIZE,
-               acb->buf, n_sectors * SECTOR_SIZE);
+               buf, n_sectors * SECTOR_SIZE);
         memset(block + (sector_in_block + n_sectors) * SECTOR_SIZE, 0,
                (s->block_sectors - n_sectors - sector_in_block) * SECTOR_SIZE);
         hd_iov.iov_base = (void *)block;
@@ -616,7 +576,7 @@ restart:
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
                           sector_in_block;
-        hd_iov.iov_base = (void *)acb->buf;
+        hd_iov.iov_base = (void *)buf;
         hd_iov.iov_len = n_sectors * SECTOR_SIZE;
         qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
         ret = bdrv_co_writev(bs->file, offset, n_sectors, &hd_qiov);
@@ -624,7 +584,7 @@ restart:
 
     nb_sectors -= n_sectors;
     sector_num += n_sectors;
-    acb->buf += n_sectors * SECTOR_SIZE;
+    buf += n_sectors * SECTOR_SIZE;
 
     logout("%u sectors written\n", n_sectors);
     if (ret >= 0 && nb_sectors > 0) {
@@ -667,10 +627,6 @@ restart:
         }
     }
 
-    if (acb->orig_buf) {
-        qemu_vfree(acb->orig_buf);
-    }
-    qemu_aio_release(acb);
     return ret;
 }
 
@@ -830,9 +786,9 @@ static BlockDriver bdrv_vdi = {
     .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
-    .bdrv_co_readv = vdi_co_readv,
+    .bdrv_read = vdi_co_read,
 #if defined(CONFIG_VDI_WRITE)
-    .bdrv_co_writev = vdi_co_writev,
+    .bdrv_write = vdi_co_write,
 #endif
 
     .bdrv_get_info = vdi_get_info,
-- 
1.7.7.6

[Qemu-devel] [PATCH 6/7] vdi: do not create useless iovecs

[Paolo Bonzini]

Reads and writes to the underlying file can also occur with the simple
non-vectored I/O interfaces.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c |   79 ++++++++++++++++++++++++----------------------------------
 1 files changed, 33 insertions(+), 46 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 5070f91..c192c7e 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -474,8 +474,6 @@ static int vdi_co_read(BlockDriverState *bs,
     uint32_t block_index;
     uint32_t sector_in_block;
     uint32_t n_sectors;
-    struct iovec hd_iov;
-    QEMUIOVector hd_qiov;
     int ret;
 
     logout("\n");
@@ -501,10 +499,7 @@ restart:
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
                           sector_in_block;
-        hd_iov.iov_base = (void *)buf;
-        hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
-        ret = bdrv_co_readv(bs->file, offset, n_sectors, &hd_qiov);
+        ret = bdrv_read(bs->file, offset, buf, n_sectors);
     }
     logout("%u sectors read\n", n_sectors);
 
@@ -529,8 +524,6 @@ static int vdi_co_write(BlockDriverState *bs,
     uint32_t n_sectors;
     uint32_t bmap_first;
     uint32_t bmap_last;
-    struct iovec hd_iov;
-    QEMUIOVector hd_qiov;
     uint8_t *block = NULL;
     int ret;
 
@@ -568,18 +561,12 @@ restart:
                buf, n_sectors * SECTOR_SIZE);
         memset(block + (sector_in_block + n_sectors) * SECTOR_SIZE, 0,
                (s->block_sectors - n_sectors - sector_in_block) * SECTOR_SIZE);
-        hd_iov.iov_base = (void *)block;
-        hd_iov.iov_len = s->block_size;
-        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
-        ret = bdrv_co_writev(bs->file, offset, s->block_sectors, &hd_qiov);
+        ret = bdrv_write(bs->file, offset, block, s->block_sectors);
     } else {
         uint64_t offset = s->header.offset_data / SECTOR_SIZE +
                           (uint64_t)bmap_entry * s->block_sectors +
                           sector_in_block;
-        hd_iov.iov_base = (void *)buf;
-        hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
-        ret = bdrv_co_writev(bs->file, offset, n_sectors, &hd_qiov);
+        ret = bdrv_write(bs->file, offset, buf, n_sectors);
     }
 
     nb_sectors -= n_sectors;
@@ -592,39 +579,39 @@ restart:
     }
 
     logout("finished data write\n");
-    if (ret >= 0) {
-        ret = 0;
-        if (block) {
-            VdiHeader *header = (VdiHeader *) block;
-            logout("now writing modified header\n");
-            assert(VDI_IS_ALLOCATED(bmap_first));
-            *header = s->header;
-            vdi_header_to_le(header);
-            hd_iov.iov_base = block;
-            hd_iov.iov_len = SECTOR_SIZE;
-            qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
-            ret = bdrv_co_writev(bs->file, 0, 1, &hd_qiov);
-        }
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (block) {
+        /* One or more new blocks were allocated. */
+        VdiHeader *header = (VdiHeader *) block;
+        uint8_t *base;
+        uint64_t offset;
+
+        logout("now writing modified header\n");
+        assert(VDI_IS_ALLOCATED(bmap_first));
+        *header = s->header;
+        vdi_header_to_le(header);
+        ret = bdrv_write(bs->file, 0, block, 1);
         g_free(block);
         block = NULL;
-        if (ret >= 0 && VDI_IS_ALLOCATED(bmap_first)) {
-            /* One or more new blocks were allocated. */
-            uint64_t offset;
-            logout("now writing modified block map entry %u...%u\n",
-                   bmap_first, bmap_last);
-            /* Write modified sectors from block map. */
-            bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
-            bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
-            n_sectors = bmap_last - bmap_first + 1;
-            offset = s->bmap_sector + bmap_first;
-            hd_iov.iov_base = (void *)((uint8_t *)&s->bmap[0] +
-                                            bmap_first * SECTOR_SIZE);
-            hd_iov.iov_len = n_sectors * SECTOR_SIZE;
-            qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
-            logout("will write %u block map sectors starting from entry %u\n",
-                   n_sectors, bmap_first);
-            ret = bdrv_co_writev(bs->file, offset, n_sectors, &hd_qiov);
+
+        if (ret < 0) {
+            return ret;
         }
+
+        logout("now writing modified block map entry %u...%u\n",
+               bmap_first, bmap_last);
+        /* Write modified sectors from block map. */
+        bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
+        bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
+        n_sectors = bmap_last - bmap_first + 1;
+        offset = s->bmap_sector + bmap_first;
+        base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
+        logout("will write %u block map sectors starting from entry %u\n",
+               n_sectors, bmap_first);
+        ret = bdrv_write(bs->file, offset, base, n_sectors);
     }
 
     return ret;
-- 
1.7.7.6

[Qemu-devel] [PATCH 7/7] vdi: change goto to loop

[Paolo Bonzini]

Finally reindent all code and change goto statements to a loop.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c |  141 ++++++++++++++++++++++++++++------------------------------
 1 files changed, 68 insertions(+), 73 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index c192c7e..41ec137 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -474,41 +474,38 @@ static int vdi_co_read(BlockDriverState *bs,
     uint32_t block_index;
     uint32_t sector_in_block;
     uint32_t n_sectors;
-    int ret;
+    int ret = 0;
 
     logout("\n");
 
-restart:
-    block_index = sector_num / s->block_sectors;
-    sector_in_block = sector_num % s->block_sectors;
-    n_sectors = s->block_sectors - sector_in_block;
-    if (n_sectors > nb_sectors) {
-        n_sectors = nb_sectors;
-    }
-
-    logout("will read %u sectors starting at sector %" PRIu64 "\n",
-           n_sectors, sector_num);
+    while (ret >= 0 && nb_sectors > 0) {
+        block_index = sector_num / s->block_sectors;
+        sector_in_block = sector_num % s->block_sectors;
+        n_sectors = s->block_sectors - sector_in_block;
+        if (n_sectors > nb_sectors) {
+            n_sectors = nb_sectors;
+        }
 
-    /* prepare next AIO request */
-    bmap_entry = le32_to_cpu(s->bmap[block_index]);
-    if (!VDI_IS_ALLOCATED(bmap_entry)) {
-        /* Block not allocated, return zeros, no need to wait. */
-        memset(buf, 0, n_sectors * SECTOR_SIZE);
-        ret = 0;
-    } else {
-        uint64_t offset = s->header.offset_data / SECTOR_SIZE +
-                          (uint64_t)bmap_entry * s->block_sectors +
-                          sector_in_block;
-        ret = bdrv_read(bs->file, offset, buf, n_sectors);
-    }
-    logout("%u sectors read\n", n_sectors);
+        logout("will read %u sectors starting at sector %" PRIu64 "\n",
+               n_sectors, sector_num);
 
-    nb_sectors -= n_sectors;
-    sector_num += n_sectors;
-    buf += n_sectors * SECTOR_SIZE;
+        /* prepare next AIO request */
+        bmap_entry = le32_to_cpu(s->bmap[block_index]);
+        if (!VDI_IS_ALLOCATED(bmap_entry)) {
+            /* Block not allocated, return zeros, no need to wait. */
+            memset(buf, 0, n_sectors * SECTOR_SIZE);
+            ret = 0;
+        } else {
+            uint64_t offset = s->header.offset_data / SECTOR_SIZE +
+                              (uint64_t)bmap_entry * s->block_sectors +
+                              sector_in_block;
+            ret = bdrv_read(bs->file, offset, buf, n_sectors);
+        }
+        logout("%u sectors read\n", n_sectors);
 
-    if (ret >= 0 && nb_sectors > 0) {
-        goto restart;
+        nb_sectors -= n_sectors;
+        sector_num += n_sectors;
+        buf += n_sectors * SECTOR_SIZE;
     }
 
     return ret;
@@ -525,57 +522,55 @@ static int vdi_co_write(BlockDriverState *bs,
     uint32_t bmap_first;
     uint32_t bmap_last;
     uint8_t *block = NULL;
-    int ret;
+    int ret = 0;
 
     logout("\n");
 
-restart:
-    block_index = sector_num / s->block_sectors;
-    sector_in_block = sector_num % s->block_sectors;
-    n_sectors = s->block_sectors - sector_in_block;
-    if (n_sectors > nb_sectors) {
-        n_sectors = nb_sectors;
-    }
-
-    logout("will write %u sectors starting at sector %" PRIu64 "\n",
-           n_sectors, sector_num);
+    while (ret >= 0 && nb_sectors > 0) {
+        block_index = sector_num / s->block_sectors;
+        sector_in_block = sector_num % s->block_sectors;
+        n_sectors = s->block_sectors - sector_in_block;
+        if (n_sectors > nb_sectors) {
+            n_sectors = nb_sectors;
+        }
 
-    /* prepare next AIO request */
-    bmap_entry = le32_to_cpu(s->bmap[block_index]);
-    if (!VDI_IS_ALLOCATED(bmap_entry)) {
-        /* Allocate new block and write to it. */
-        uint64_t offset;
-        bmap_entry = s->header.blocks_allocated;
-        s->bmap[block_index] = cpu_to_le32(bmap_entry);
-        s->header.blocks_allocated++;
-        offset = s->header.offset_data / SECTOR_SIZE +
-                 (uint64_t)bmap_entry * s->block_sectors;
-        if (block == NULL) {
-            block = g_malloc(s->block_size);
-            bmap_first = block_index;
+        logout("will write %u sectors starting at sector %" PRIu64 "\n",
+               n_sectors, sector_num);
+
+        /* prepare next AIO request */
+        bmap_entry = le32_to_cpu(s->bmap[block_index]);
+        if (!VDI_IS_ALLOCATED(bmap_entry)) {
+            /* Allocate new block and write to it. */
+            uint64_t offset;
+            bmap_entry = s->header.blocks_allocated;
+            s->bmap[block_index] = cpu_to_le32(bmap_entry);
+            s->header.blocks_allocated++;
+            offset = s->header.offset_data / SECTOR_SIZE +
+                     (uint64_t)bmap_entry * s->block_sectors;
+            if (block == NULL) {
+                block = g_malloc(s->block_size);
+                bmap_first = block_index;
+            }
+            bmap_last = block_index;
+            /* Copy data to be written to new block and zero unused parts. */
+            memset(block, 0, sector_in_block * SECTOR_SIZE);
+            memcpy(block + sector_in_block * SECTOR_SIZE,
+                   buf, n_sectors * SECTOR_SIZE);
+            memset(block + (sector_in_block + n_sectors) * SECTOR_SIZE, 0,
+                   (s->block_sectors - n_sectors - sector_in_block) * SECTOR_SIZE);
+            ret = bdrv_write(bs->file, offset, block, s->block_sectors);
+        } else {
+            uint64_t offset = s->header.offset_data / SECTOR_SIZE +
+                              (uint64_t)bmap_entry * s->block_sectors +
+                              sector_in_block;
+            ret = bdrv_write(bs->file, offset, buf, n_sectors);
         }
-        bmap_last = block_index;
-        /* Copy data to be written to new block and zero unused parts. */
-        memset(block, 0, sector_in_block * SECTOR_SIZE);
-        memcpy(block + sector_in_block * SECTOR_SIZE,
-               buf, n_sectors * SECTOR_SIZE);
-        memset(block + (sector_in_block + n_sectors) * SECTOR_SIZE, 0,
-               (s->block_sectors - n_sectors - sector_in_block) * SECTOR_SIZE);
-        ret = bdrv_write(bs->file, offset, block, s->block_sectors);
-    } else {
-        uint64_t offset = s->header.offset_data / SECTOR_SIZE +
-                          (uint64_t)bmap_entry * s->block_sectors +
-                          sector_in_block;
-        ret = bdrv_write(bs->file, offset, buf, n_sectors);
-    }
 
-    nb_sectors -= n_sectors;
-    sector_num += n_sectors;
-    buf += n_sectors * SECTOR_SIZE;
+        nb_sectors -= n_sectors;
+        sector_num += n_sectors;
+        buf += n_sectors * SECTOR_SIZE;
 
-    logout("%u sectors written\n", n_sectors);
-    if (ret >= 0 && nb_sectors > 0) {
-        goto restart;
+        logout("%u sectors written\n", n_sectors);
     }
 
     logout("finished data write\n");
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 0/7] block: convert VDI image format to coroutines

[Stefan Weil]

Am 05.03.2012 18:40, schrieb Paolo Bonzini:
> Conversion to coroutines simplifies the code and removes the need to
> duplicate common features of the block layer. Each step in the conversion
> is detailed in the corresponding commit message.
>
> Tested with qemu-iotests.
>
> Paolo Bonzini (7):
> vdi: basic conversion to coroutines
> vdi: move end-of-I/O handling at the end
> vdi: merge aio_read_cb and aio_write_cb into callers
> vdi: move aiocb fields to locals
> vdi: leave bounce buffering to block layer
> vdi: do not create useless iovecs
> vdi: change goto to loop
>
> block/vdi.c | 421 +++++++++++++-------------------------------------
> 2 files changed, 108 insertions(+), 317 deletions(-)

Acked-by: Stefan Weil <sw@weilnetz.de>

Kevin, could you please add Paolo's patches to your block queue?

I did not review each single patch in detail, but the resulting
code looks sane and much better (and shorter) than the current code
(a little like my original code before I had to add asynchronous
I/O support).

As Paolo already tested with qemu-iotests, I did not re-run those
tests.

Thanks,
Stefan W.

Re: [Qemu-devel] [PATCH 0/7] block: convert VDI image format to coroutines

[Kevin Wolf]

Am 15.03.2012 22:28, schrieb Stefan Weil:
> Am 05.03.2012 18:40, schrieb Paolo Bonzini:
>> Conversion to coroutines simplifies the code and removes the need to
>> duplicate common features of the block layer. Each step in the conversion
>> is detailed in the corresponding commit message.
>>
>> Tested with qemu-iotests.
>>
>> Paolo Bonzini (7):
>> vdi: basic conversion to coroutines
>> vdi: move end-of-I/O handling at the end
>> vdi: merge aio_read_cb and aio_write_cb into callers
>> vdi: move aiocb fields to locals
>> vdi: leave bounce buffering to block layer
>> vdi: do not create useless iovecs
>> vdi: change goto to loop
>>
>> block/vdi.c | 421 +++++++++++++-------------------------------------
>> 2 files changed, 108 insertions(+), 317 deletions(-)
> 
> Acked-by: Stefan Weil <sw@weilnetz.de>
> 
> Kevin, could you please add Paolo's patches to your block queue?

Yes, I was just waiting for your Acked-by.

Thanks, applied all patches to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 5/7] vdi: leave bounce buffering to block layer

[Kevin Wolf]

Am 05.03.2012 18:40, schrieb Paolo Bonzini:
> vdi.c really works as if it implemented bdrv_read and bdrv_write.
> However, because only vector I/O is supported by the asynchronous
> callbacks, it went through extra pain to bounce-buffer the I/O.
> With the conversion to coroutines bdrv_read and bdrv_write are now
> asynchronous, so they can be handled by the block layer now that the
> format is coroutine-based.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

In the long run, the right thing to do would be to convert VDI to deal
with vectored I/O, of course. Stefan, maybe you want to do that on top.
It's not that hard, I did it for qcow2 a while ago.

Kevin