[Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Frediano Ziglio]

These patches mostly cleanup some AIO code using coroutines.
These patches apply to Kevin's repository, branch coroutine-block.
Mostly they use stack instead of allocated AIO structure.

Frediano Ziglio (5):
  qcow: allocate QCowAIOCB structure using stack
  qcow: QCowAIOCB field cleanup
  qcow: move some blocks of code to avoid useless variable
    initialization
  avoid dandling pointers
  qcow: small optimization initializing QCowAIOCB

 block/qcow.c  |  210 +++++++++++++++++++++++++--------------------------------
 block/qcow2.c |   38 +++-------
 2 files changed, 102 insertions(+), 146 deletions(-)

[Qemu-devel] [RFC PATCH 1/5] qcow: allocate QCowAIOCB structure using stack

[Frediano Ziglio]

instead of calling qemi_aio_get use stack

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
 block/qcow.c  |   52 ++++++++++++++++------------------------------------
 block/qcow2.c |   38 +++++++++++---------------------------
 2 files changed, 27 insertions(+), 63 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 6447c2a..d19ef04 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -503,28 +503,12 @@ typedef struct QCowAIOCB {
     BlockDriverAIOCB *hd_aiocb;
 } QCowAIOCB;
 
-static void qcow_aio_cancel(BlockDriverAIOCB *blockacb)
-{
-    QCowAIOCB *acb = container_of(blockacb, QCowAIOCB, common);
-    if (acb->hd_aiocb)
-        bdrv_aio_cancel(acb->hd_aiocb);
-    qemu_aio_release(acb);
-}
-
-static AIOPool qcow_aio_pool = {
-    .aiocb_size         = sizeof(QCowAIOCB),
-    .cancel             = qcow_aio_cancel,
-};
-
 static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
-        int is_write)
+        int is_write, QCowAIOCB *acb)
 {
-    QCowAIOCB *acb;
-
-    acb = qemu_aio_get(&qcow_aio_pool, bs, NULL, NULL);
-    if (!acb)
-        return NULL;
+    memset(acb, 0, sizeof(*acb));
+    acb->common.bs = bs;
     acb->hd_aiocb = NULL;
     acb->sector_num = sector_num;
     acb->qiov = qiov;
@@ -543,9 +527,8 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
     return acb;
 }
 
-static int qcow_aio_read_cb(void *opaque)
+static int qcow_aio_read_cb(QCowAIOCB *acb)
 {
-    QCowAIOCB *acb = opaque;
     BlockDriverState *bs = acb->common.bs;
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
@@ -634,29 +617,27 @@ static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
                          int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVQcowState *s = bs->opaque;
-    QCowAIOCB *acb;
+    QCowAIOCB acb;
     int ret;
 
-    acb = qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 0);
+    qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 0, &acb);
 
     qemu_co_mutex_lock(&s->lock);
     do {
-        ret = qcow_aio_read_cb(acb);
+        ret = qcow_aio_read_cb(&acb);
     } while (ret > 0);
     qemu_co_mutex_unlock(&s->lock);
 
-    if (acb->qiov->niov > 1) {
-        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
-        qemu_vfree(acb->orig_buf);
+    if (acb.qiov->niov > 1) {
+        qemu_iovec_from_buffer(acb.qiov, acb.orig_buf, acb.qiov->size);
+        qemu_vfree(acb.orig_buf);
     }
-    qemu_aio_release(acb);
 
     return ret;
 }
 
-static int qcow_aio_write_cb(void *opaque)
+static int qcow_aio_write_cb(QCowAIOCB *acb)
 {
-    QCowAIOCB *acb = opaque;
     BlockDriverState *bs = acb->common.bs;
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
@@ -714,23 +695,22 @@ static int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVQcowState *s = bs->opaque;
-    QCowAIOCB *acb;
+    QCowAIOCB acb;
     int ret;
 
     s->cluster_cache_offset = -1; /* disable compressed cache */
 
-    acb = qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 1);
+    qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 1, &acb);
 
     qemu_co_mutex_lock(&s->lock);
     do {
-        ret = qcow_aio_write_cb(acb);
+        ret = qcow_aio_write_cb(&acb);
     } while (ret > 0);
     qemu_co_mutex_unlock(&s->lock);
 
-    if (acb->qiov->niov > 1) {
-        qemu_vfree(acb->orig_buf);
+    if (acb.qiov->niov > 1) {
+        qemu_vfree(acb.orig_buf);
     }
-    qemu_aio_release(acb);
 
     return ret;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index f07d550..edc068e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -388,17 +388,6 @@ typedef struct QCowAIOCB {
     QLIST_ENTRY(QCowAIOCB) next_depend;
 } QCowAIOCB;
 
-static void qcow2_aio_cancel(BlockDriverAIOCB *blockacb)
-{
-    QCowAIOCB *acb = container_of(blockacb, QCowAIOCB, common);
-    qemu_aio_release(acb);
-}
-
-static AIOPool qcow2_aio_pool = {
-    .aiocb_size         = sizeof(QCowAIOCB),
-    .cancel             = qcow2_aio_cancel,
-};
-
 /*
  * Returns 0 when the request is completed successfully, 1 when there is still
  * a part left to do and -errno in error cases.
@@ -528,13 +517,10 @@ static int qcow2_aio_read_cb(QCowAIOCB *acb)
 static QCowAIOCB *qcow2_aio_setup(BlockDriverState *bs, int64_t sector_num,
                                   QEMUIOVector *qiov, int nb_sectors,
                                   BlockDriverCompletionFunc *cb,
-                                  void *opaque, int is_write)
+                                  void *opaque, int is_write, QCowAIOCB *acb)
 {
-    QCowAIOCB *acb;
-
-    acb = qemu_aio_get(&qcow2_aio_pool, bs, cb, opaque);
-    if (!acb)
-        return NULL;
+    memset(acb, 0, sizeof(*acb));
+    acb->common.bs = bs;
     acb->sector_num = sector_num;
     acb->qiov = qiov;
     acb->is_write = is_write;
@@ -554,19 +540,18 @@ static int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVQcowState *s = bs->opaque;
-    QCowAIOCB *acb;
+    QCowAIOCB acb;
     int ret;
 
-    acb = qcow2_aio_setup(bs, sector_num, qiov, nb_sectors, NULL, NULL, 0);
+    qcow2_aio_setup(bs, sector_num, qiov, nb_sectors, NULL, NULL, 0, &acb);
 
     qemu_co_mutex_lock(&s->lock);
     do {
-        ret = qcow2_aio_read_cb(acb);
+        ret = qcow2_aio_read_cb(&acb);
     } while (ret > 0);
     qemu_co_mutex_unlock(&s->lock);
 
-    qemu_iovec_destroy(&acb->hd_qiov);
-    qemu_aio_release(acb);
+    qemu_iovec_destroy(&acb.hd_qiov);
 
     return ret;
 }
@@ -670,20 +655,19 @@ static int qcow2_co_writev(BlockDriverState *bs,
                            QEMUIOVector *qiov)
 {
     BDRVQcowState *s = bs->opaque;
-    QCowAIOCB *acb;
+    QCowAIOCB acb;
     int ret;
 
-    acb = qcow2_aio_setup(bs, sector_num, qiov, nb_sectors, NULL, NULL, 1);
+    qcow2_aio_setup(bs, sector_num, qiov, nb_sectors, NULL, NULL, 1, &acb);
     s->cluster_cache_offset = -1; /* disable compressed cache */
 
     qemu_co_mutex_lock(&s->lock);
     do {
-        ret = qcow2_aio_write_cb(acb);
+        ret = qcow2_aio_write_cb(&acb);
     } while (ret > 0);
     qemu_co_mutex_unlock(&s->lock);
 
-    qemu_iovec_destroy(&acb->hd_qiov);
-    qemu_aio_release(acb);
+    qemu_iovec_destroy(&acb.hd_qiov);
 
     return ret;
 }
-- 
1.7.1

[Qemu-devel] [RFC PATCH 2/5] qcow: QCowAIOCB field cleanup

[Frediano Ziglio]

remove unused field from this structure and put some of them in qcow_aio_read_cb and qcow_aio_write_cb

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
 block/qcow.c |  133 +++++++++++++++++++++++++++-------------------------------
 1 files changed, 62 insertions(+), 71 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index d19ef04..cd1f9e3 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -487,20 +487,12 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
 #endif
 
 typedef struct QCowAIOCB {
-    BlockDriverAIOCB common;
+    BlockDriverState *bs;
     int64_t sector_num;
     QEMUIOVector *qiov;
     uint8_t *buf;
     void *orig_buf;
     int nb_sectors;
-    int n;
-    uint64_t cluster_offset;
-    uint8_t *cluster_data;
-    struct iovec hd_iov;
-    bool is_write;
-    QEMUBH *bh;
-    QEMUIOVector hd_qiov;
-    BlockDriverAIOCB *hd_aiocb;
 } QCowAIOCB;
 
 static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
@@ -508,11 +500,9 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
         int is_write, QCowAIOCB *acb)
 {
     memset(acb, 0, sizeof(*acb));
-    acb->common.bs = bs;
-    acb->hd_aiocb = NULL;
+    acb->bs = bs;
     acb->sector_num = sector_num;
     acb->qiov = qiov;
-    acb->is_write = is_write;
 
     if (qiov->niov > 1) {
         acb->buf = acb->orig_buf = qemu_blockalign(bs, qiov->size);
@@ -522,37 +512,36 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
         acb->buf = (uint8_t *)qiov->iov->iov_base;
     }
     acb->nb_sectors = nb_sectors;
-    acb->n = 0;
-    acb->cluster_offset = 0;
     return acb;
 }
 
 static int qcow_aio_read_cb(QCowAIOCB *acb)
 {
-    BlockDriverState *bs = acb->common.bs;
+    BlockDriverState *bs = acb->bs;
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
-    int ret;
-
-    acb->hd_aiocb = NULL;
+    int ret, n = 0;
+    uint64_t cluster_offset = 0;
+    struct iovec hd_iov;
+    QEMUIOVector hd_qiov;
 
  redo:
     /* post process the read buffer */
-    if (!acb->cluster_offset) {
+    if (!cluster_offset) {
         /* nothing to do */
-    } else if (acb->cluster_offset & QCOW_OFLAG_COMPRESSED) {
+    } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
         /* nothing to do */
     } else {
         if (s->crypt_method) {
             encrypt_sectors(s, acb->sector_num, acb->buf, acb->buf,
-                            acb->n, 0,
+                            n, 0,
                             &s->aes_decrypt_key);
         }
     }
 
-    acb->nb_sectors -= acb->n;
-    acb->sector_num += acb->n;
-    acb->buf += acb->n * 512;
+    acb->nb_sectors -= n;
+    acb->sector_num += n;
+    acb->buf += n * 512;
 
     if (acb->nb_sectors == 0) {
         /* request completed */
@@ -560,57 +549,57 @@ static int qcow_aio_read_cb(QCowAIOCB *acb)
     }
 
     /* prepare next AIO request */
-    acb->cluster_offset = get_cluster_offset(bs, acb->sector_num << 9,
+    cluster_offset = get_cluster_offset(bs, acb->sector_num << 9,
                                              0, 0, 0, 0);
     index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
-    acb->n = s->cluster_sectors - index_in_cluster;
-    if (acb->n > acb->nb_sectors)
-        acb->n = acb->nb_sectors;
+    n = s->cluster_sectors - index_in_cluster;
+    if (n > acb->nb_sectors)
+        n = acb->nb_sectors;
 
-    if (!acb->cluster_offset) {
+    if (!cluster_offset) {
         if (bs->backing_hd) {
             /* read from the base image */
-            acb->hd_iov.iov_base = (void *)acb->buf;
-            acb->hd_iov.iov_len = acb->n * 512;
-            qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
+            hd_iov.iov_base = (void *)acb->buf;
+            hd_iov.iov_len = n * 512;
+            qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
             qemu_co_mutex_unlock(&s->lock);
             ret = bdrv_co_readv(bs->backing_hd, acb->sector_num,
-                                acb->n, &acb->hd_qiov);
+                                n, &hd_qiov);
             qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
                 return -EIO;
             }
         } else {
             /* Note: in this case, no need to wait */
-            memset(acb->buf, 0, 512 * acb->n);
+            memset(acb->buf, 0, 512 * n);
             goto redo;
         }
-    } else if (acb->cluster_offset & QCOW_OFLAG_COMPRESSED) {
+    } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
         /* add AIO support for compressed blocks ? */
-        if (decompress_cluster(bs, acb->cluster_offset) < 0) {
+        if (decompress_cluster(bs, cluster_offset) < 0) {
             return -EIO;
         }
         memcpy(acb->buf,
-               s->cluster_cache + index_in_cluster * 512, 512 * acb->n);
+               s->cluster_cache + index_in_cluster * 512, 512 * n);
         goto redo;
     } else {
-        if ((acb->cluster_offset & 511) != 0) {
+        if ((cluster_offset & 511) != 0) {
             return -EIO;
         }
-        acb->hd_iov.iov_base = (void *)acb->buf;
-        acb->hd_iov.iov_len = acb->n * 512;
-        qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
+        hd_iov.iov_base = (void *)acb->buf;
+        hd_iov.iov_len = n * 512;
+        qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
         qemu_co_mutex_unlock(&s->lock);
         ret = bdrv_co_readv(bs->file,
-                            (acb->cluster_offset >> 9) + index_in_cluster,
-                            acb->n, &acb->hd_qiov);
+                            (cluster_offset >> 9) + index_in_cluster,
+                            n, &hd_qiov);
         qemu_co_mutex_lock(&s->lock);
         if (ret < 0) {
             return ret;
         }
     }
 
-    return 1;
+    goto redo;
 }
 
 static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -623,9 +612,7 @@ static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 0, &acb);
 
     qemu_co_mutex_lock(&s->lock);
-    do {
-        ret = qcow_aio_read_cb(&acb);
-    } while (ret > 0);
+    ret = qcow_aio_read_cb(&acb);
     qemu_co_mutex_unlock(&s->lock);
 
     if (acb.qiov->niov > 1) {
@@ -638,18 +625,20 @@ static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
 
 static int qcow_aio_write_cb(QCowAIOCB *acb)
 {
-    BlockDriverState *bs = acb->common.bs;
+    BlockDriverState *bs = acb->bs;
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
     uint64_t cluster_offset;
     const uint8_t *src_buf;
-    int ret;
-
-    acb->hd_aiocb = NULL;
+    int ret, n = 0;
+    uint8_t *cluster_data = NULL;
+    struct iovec hd_iov;
+    QEMUIOVector hd_qiov;
 
-    acb->nb_sectors -= acb->n;
-    acb->sector_num += acb->n;
-    acb->buf += acb->n * 512;
+  redo:
+    acb->nb_sectors -= n;
+    acb->sector_num += n;
+    acb->buf += n * 512;
 
     if (acb->nb_sectors == 0) {
         /* request completed */
@@ -657,38 +646,42 @@ static int qcow_aio_write_cb(QCowAIOCB *acb)
     }
 
     index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
-    acb->n = s->cluster_sectors - index_in_cluster;
-    if (acb->n > acb->nb_sectors)
-        acb->n = acb->nb_sectors;
+    n = s->cluster_sectors - index_in_cluster;
+    if (n > acb->nb_sectors)
+        n = acb->nb_sectors;
     cluster_offset = get_cluster_offset(bs, acb->sector_num << 9, 1, 0,
                                         index_in_cluster,
-                                        index_in_cluster + acb->n);
+                                        index_in_cluster + n);
     if (!cluster_offset || (cluster_offset & 511) != 0) {
         return -EIO;
     }
     if (s->crypt_method) {
-        if (!acb->cluster_data) {
-            acb->cluster_data = qemu_mallocz(s->cluster_size);
+        if (!cluster_data) {
+            cluster_data = qemu_mallocz(s->cluster_size);
         }
-        encrypt_sectors(s, acb->sector_num, acb->cluster_data, acb->buf,
-                        acb->n, 1, &s->aes_encrypt_key);
-        src_buf = acb->cluster_data;
+        encrypt_sectors(s, acb->sector_num, cluster_data, acb->buf,
+                        n, 1, &s->aes_encrypt_key);
+        src_buf = cluster_data;
     } else {
         src_buf = acb->buf;
     }
 
-    acb->hd_iov.iov_base = (void *)src_buf;
-    acb->hd_iov.iov_len = acb->n * 512;
-    qemu_iovec_init_external(&acb->hd_qiov, &acb->hd_iov, 1);
+    hd_iov.iov_base = (void *)src_buf;
+    hd_iov.iov_len = n * 512;
+    qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
     qemu_co_mutex_unlock(&s->lock);
     ret = bdrv_co_writev(bs->file,
                          (cluster_offset >> 9) + index_in_cluster,
-                         acb->n, &acb->hd_qiov);
+                         n, &hd_qiov);
+    if (cluster_data) {
+        free(cluster_data);
+        cluster_data = NULL;
+    }
     qemu_co_mutex_lock(&s->lock);
     if (ret < 0) {
         return ret;
     }
-    return 1;
+    goto redo;
 }
 
 static int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
@@ -703,9 +696,7 @@ static int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
     qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 1, &acb);
 
     qemu_co_mutex_lock(&s->lock);
-    do {
-        ret = qcow_aio_write_cb(&acb);
-    } while (ret > 0);
+    ret = qcow_aio_write_cb(&acb);
     qemu_co_mutex_unlock(&s->lock);
 
     if (acb.qiov->niov > 1) {
-- 
1.7.1

[Qemu-devel] [RFC PATCH 3/5] qcow: move some blocks of code to avoid useless variable initialization

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
 block/qcow.c |   53 ++++++++++++++++++++++++++---------------------------
 1 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index cd1f9e3..8ccd7d7 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -520,35 +520,18 @@ static int qcow_aio_read_cb(QCowAIOCB *acb)
     BlockDriverState *bs = acb->bs;
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
-    int ret, n = 0;
-    uint64_t cluster_offset = 0;
+    int ret, n;
+    uint64_t cluster_offset;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
 
  redo:
-    /* post process the read buffer */
-    if (!cluster_offset) {
-        /* nothing to do */
-    } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
-        /* nothing to do */
-    } else {
-        if (s->crypt_method) {
-            encrypt_sectors(s, acb->sector_num, acb->buf, acb->buf,
-                            n, 0,
-                            &s->aes_decrypt_key);
-        }
-    }
-
-    acb->nb_sectors -= n;
-    acb->sector_num += n;
-    acb->buf += n * 512;
-
     if (acb->nb_sectors == 0) {
         /* request completed */
         return 0;
     }
 
-    /* prepare next AIO request */
+    /* prepare next request */
     cluster_offset = get_cluster_offset(bs, acb->sector_num << 9,
                                              0, 0, 0, 0);
     index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
@@ -572,7 +555,6 @@ static int qcow_aio_read_cb(QCowAIOCB *acb)
         } else {
             /* Note: in this case, no need to wait */
             memset(acb->buf, 0, 512 * n);
-            goto redo;
         }
     } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
         /* add AIO support for compressed blocks ? */
@@ -581,7 +563,6 @@ static int qcow_aio_read_cb(QCowAIOCB *acb)
         }
         memcpy(acb->buf,
                s->cluster_cache + index_in_cluster * 512, 512 * n);
-        goto redo;
     } else {
         if ((cluster_offset & 511) != 0) {
             return -EIO;
@@ -599,6 +580,23 @@ static int qcow_aio_read_cb(QCowAIOCB *acb)
         }
     }
 
+    /* post process the read buffer */
+    if (!cluster_offset) {
+        /* nothing to do */
+    } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
+        /* nothing to do */
+    } else {
+        if (s->crypt_method) {
+            encrypt_sectors(s, acb->sector_num, acb->buf, acb->buf,
+                            n, 0,
+                            &s->aes_decrypt_key);
+        }
+    }
+
+    acb->nb_sectors -= n;
+    acb->sector_num += n;
+    acb->buf += n * 512;
+
     goto redo;
 }
 
@@ -630,16 +628,12 @@ static int qcow_aio_write_cb(QCowAIOCB *acb)
     int index_in_cluster;
     uint64_t cluster_offset;
     const uint8_t *src_buf;
-    int ret, n = 0;
+    int ret, n;
     uint8_t *cluster_data = NULL;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
 
   redo:
-    acb->nb_sectors -= n;
-    acb->sector_num += n;
-    acb->buf += n * 512;
-
     if (acb->nb_sectors == 0) {
         /* request completed */
         return 0;
@@ -681,6 +675,11 @@ static int qcow_aio_write_cb(QCowAIOCB *acb)
     if (ret < 0) {
         return ret;
     }
+
+    acb->nb_sectors -= n;
+    acb->sector_num += n;
+    acb->buf += n * 512;
+
     goto redo;
 }
 
-- 
1.7.1

[Qemu-devel] [RFC PATCH 4/5] avoid dandling pointers

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
 block/qcow.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 8ccd7d7..007fb57 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -616,6 +616,7 @@ static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     if (acb.qiov->niov > 1) {
         qemu_iovec_from_buffer(acb.qiov, acb.orig_buf, acb.qiov->size);
         qemu_vfree(acb.orig_buf);
+        acb.orig_buf = NULL;
     }
 
     return ret;
@@ -700,6 +701,7 @@ static int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
 
     if (acb.qiov->niov > 1) {
         qemu_vfree(acb.orig_buf);
+        acb.orig_buf = NULL;
     }
 
     return ret;
-- 
1.7.1

[Qemu-devel] [RFC PATCH 5/5] qcow: small optimization initializing QCowAIOCB

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
 block/qcow.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 007fb57..8fd1ee5 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -499,7 +499,6 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         int is_write, QCowAIOCB *acb)
 {
-    memset(acb, 0, sizeof(*acb));
     acb->bs = bs;
     acb->sector_num = sector_num;
     acb->qiov = qiov;
@@ -509,6 +508,7 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
         if (is_write)
             qemu_iovec_to_buffer(qiov, acb->buf);
     } else {
+        acb->orig_buf = NULL;
         acb->buf = (uint8_t *)qiov->iov->iov_base;
     }
     acb->nb_sectors = nb_sectors;
-- 
1.7.1

Re: [Qemu-devel] [RFC PATCH 2/5] qcow: QCowAIOCB field cleanup

[Kevin Wolf]

Am 20.07.2011 15:56, schrieb Frediano Ziglio:
> remove unused field from this structure and put some of them in qcow_aio_read_cb and qcow_aio_write_cb
> 
> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>

>          qemu_co_mutex_lock(&s->lock);
>          if (ret < 0) {
>              return ret;
>          }
>      }
>  
> -    return 1;
> +    goto redo;
>  }
>  
>  static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
> @@ -623,9 +612,7 @@ static int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>      qcow_aio_setup(bs, sector_num, qiov, nb_sectors, 0, &acb);
>  
>      qemu_co_mutex_lock(&s->lock);
> -    do {
> -        ret = qcow_aio_read_cb(&acb);
> -    } while (ret > 0);
> +    ret = qcow_aio_read_cb(&acb);
>      qemu_co_mutex_unlock(&s->lock);
>  
>      if (acb.qiov->niov > 1) {

What was wrong with the loop and return 1?

I think the old state was closer to what it should look like:
qcow_aio_read_cb should become a function that reads as much as it can
from a given offset to a given pointer and returns how much that was.
qcow_co_readv should contain the loop that calls it with the right
parameters.

Kevin

Re: [Qemu-devel] [RFC PATCH 4/5] avoid dandling pointers

[Kevin Wolf]

Am 20.07.2011 15:56, schrieb Frediano Ziglio:
> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
> ---
>  block/qcow.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Subject needs a "qcow: ..."

Kevin

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Kevin Wolf]

Am 20.07.2011 15:56, schrieb Frediano Ziglio:
> These patches mostly cleanup some AIO code using coroutines.
> These patches apply to Kevin's repository, branch coroutine-block.
> Mostly they use stack instead of allocated AIO structure.
> 
> Frediano Ziglio (5):
>   qcow: allocate QCowAIOCB structure using stack
>   qcow: QCowAIOCB field cleanup
>   qcow: move some blocks of code to avoid useless variable
>     initialization
>   avoid dandling pointers
>   qcow: small optimization initializing QCowAIOCB
> 
>  block/qcow.c  |  210 +++++++++++++++++++++++++--------------------------------
>  block/qcow2.c |   38 +++-------
>  2 files changed, 102 insertions(+), 146 deletions(-)

Most of it looks good now. Did you include the "RFC" in the subject just
because the coroutine work is in RFC state, too, or did you intend to
tell me that I shouldn't merge yet?

Kevin

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Frediano Ziglio]

2011/7/22 Kevin Wolf <kwolf@redhat.com>:
> Am 20.07.2011 15:56, schrieb Frediano Ziglio:
>> These patches mostly cleanup some AIO code using coroutines.
>> These patches apply to Kevin's repository, branch coroutine-block.
>> Mostly they use stack instead of allocated AIO structure.
>>
>> Frediano Ziglio (5):
>>   qcow: allocate QCowAIOCB structure using stack
>>   qcow: QCowAIOCB field cleanup
>>   qcow: move some blocks of code to avoid useless variable
>>     initialization
>>   avoid dandling pointers
>>   qcow: small optimization initializing QCowAIOCB
>>
>>  block/qcow.c  |  210 +++++++++++++++++++++++++--------------------------------
>>  block/qcow2.c |   38 +++-------
>>  2 files changed, 102 insertions(+), 146 deletions(-)
>
> Most of it looks good now. Did you include the "RFC" in the subject just
> because the coroutine work is in RFC state, too, or did you intend to
> tell me that I shouldn't merge yet?
>
> Kevin
>

As these patches are first quite big patches I send (typo or small
fixes do not counts) I just want to mark that I could write something
really wrong. Just a way to avoid somebody having to send more patches
and get more attention. Some projects are quite prone to merge even
not that fine ones. I prefer to have some (a bit) pedantic comments
and a real fix/improve.

Now I removed the RFC from last update. The main reason is that I
found your qemu-iotests repository which, I think should be merged to
main repository, but it's just my opinion.
Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error.

I must say there are a lot of small hidden things that a developer
should know about Qemu, for instance
- mailing list follow some LKML rules as CC ML and send to maintainer
to get more attention
- you can use scripts/checkpatch.pl to check your patches before send

I still have also to understand how to use git format-patch/send-email
correctly and "fluently" :)

Frediano

Re: [Qemu-devel] [RFC PATCH 4/5] avoid dandling pointers

[Frediano Ziglio]

2011/7/22 Kevin Wolf <kwolf@redhat.com>:
> Am 20.07.2011 15:56, schrieb Frediano Ziglio:
>> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
>> ---
>>  block/qcow.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> Subject needs a "qcow: ..."
>
> Kevin
>

Yes, now I removed that patch as with argument on stack it just make
few sense...

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Kevin Wolf]

Am 22.07.2011 11:26, schrieb Frediano Ziglio:
> 2011/7/22 Kevin Wolf <kwolf@redhat.com>:
>> Am 20.07.2011 15:56, schrieb Frediano Ziglio:
>>> These patches mostly cleanup some AIO code using coroutines.
>>> These patches apply to Kevin's repository, branch coroutine-block.
>>> Mostly they use stack instead of allocated AIO structure.
>>>
>>> Frediano Ziglio (5):
>>>   qcow: allocate QCowAIOCB structure using stack
>>>   qcow: QCowAIOCB field cleanup
>>>   qcow: move some blocks of code to avoid useless variable
>>>     initialization
>>>   avoid dandling pointers
>>>   qcow: small optimization initializing QCowAIOCB
>>>
>>>  block/qcow.c  |  210 +++++++++++++++++++++++++--------------------------------
>>>  block/qcow2.c |   38 +++-------
>>>  2 files changed, 102 insertions(+), 146 deletions(-)
>>
>> Most of it looks good now. Did you include the "RFC" in the subject just
>> because the coroutine work is in RFC state, too, or did you intend to
>> tell me that I shouldn't merge yet?
>>
>> Kevin
>>
> 
> As these patches are first quite big patches I send (typo or small
> fixes do not counts) I just want to mark that I could write something
> really wrong. Just a way to avoid somebody having to send more patches
> and get more attention. Some projects are quite prone to merge even
> not that fine ones. I prefer to have some (a bit) pedantic comments
> and a real fix/improve.
> 
> Now I removed the RFC from last update. The main reason is that I
> found your qemu-iotests repository which, I think should be merged to
> main repository, but it's just my opinion.
> Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error.

Yup, you're right, I must have messed it up. Care to fix it or should I
look into it?

> I must say there are a lot of small hidden things that a developer
> should know about Qemu, for instance
> - mailing list follow some LKML rules as CC ML and send to maintainer
> to get more attention
> - you can use scripts/checkpatch.pl to check your patches before send
> 
> I still have also to understand how to use git format-patch/send-email
> correctly and "fluently" :)

That's true. Maybe you can update the wiki page with your findings?

The one suggestion I have for your use of git format-patch is
--subject-prefix="PATCH v3", so I don't get confused by the different
versions ;-)

Kevin

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Stefan Hajnoczi]

On Fri, Jul 22, 2011 at 11:10 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 22.07.2011 11:26, schrieb Frediano Ziglio:
>> - you can use scripts/checkpatch.pl to check your patches before send

I updated the SubmitAPatch wiki earlier this week.

Stefan

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Frediano Ziglio]

2011/7/22 Stefan Hajnoczi <stefanha@gmail.com>:
> On Fri, Jul 22, 2011 at 11:10 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 22.07.2011 11:26, schrieb Frediano Ziglio:
>>> - you can use scripts/checkpatch.pl to check your patches before send
>
> I updated the SubmitAPatch wiki earlier this week.
>
> Stefan
>

Good, now wiki is working (it seems somebody is attacking Qemu
sites... yesterday the ML).

http://git.qemu.org/git/qemu.git/ is not working so all links give 404.

I added some notes, yes checkpatch was already in the page.

About git commands to send multiple patches, I use

  git format-patch --cover-letter -s -M origin/original_branch_name
--subject-prefix='PATCH vXX' -o outgoing/

edit manually cover letter and

  git send-email --to='maintainer@domain' --cc='qemu-devel@nongnu.org'
outgoing/*

are these command correct or there is a better way?

Frediano

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Kevin Wolf]

Am 22.07.2011 15:24, schrieb Frediano Ziglio:
> 2011/7/22 Stefan Hajnoczi <stefanha@gmail.com>:
>> On Fri, Jul 22, 2011 at 11:10 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 22.07.2011 11:26, schrieb Frediano Ziglio:
>>>> - you can use scripts/checkpatch.pl to check your patches before send
>>
>> I updated the SubmitAPatch wiki earlier this week.
>>
>> Stefan
>>
> 
> Good, now wiki is working (it seems somebody is attacking Qemu
> sites... yesterday the ML).
> 
> http://git.qemu.org/git/qemu.git/ is not working so all links give 404.
> 
> I added some notes, yes checkpatch was already in the page.
> 
> About git commands to send multiple patches, I use
> 
>   git format-patch --cover-letter -s -M origin/original_branch_name
> --subject-prefix='PATCH vXX' -o outgoing/
> 
> edit manually cover letter and
> 
>   git send-email --to='maintainer@domain' --cc='qemu-devel@nongnu.org'
> outgoing/*
> 
> are these command correct or there is a better way?

Looks correct to me. I usually use -s already for commits, so I don't
need it in format-patch, and I don't use -M (maybe I should), but
otherwise it's more or less the same as I use.

Kevin

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Gerd Hoffmann]

   Hi,

>    git send-email --to='maintainer@domain' --cc='qemu-devel@nongnu.org'
> outgoing/*

It is a good idea to make the last argument "outgoing/0*.patch", 
otherwise you risk to send out the "0000-cover-letter.patch~" editor 
backup file ...

Otherwise the procedure looks fine.

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Frediano Ziglio]

Il giorno ven, 22/07/2011 alle 12.10 +0200, Kevin Wolf ha scritto: 
> Am 22.07.2011 11:26, schrieb Frediano Ziglio:
> > 2011/7/22 Kevin Wolf <kwolf@redhat.com>:
> >> Am 20.07.2011 15:56, schrieb Frediano Ziglio:
> >>> These patches mostly cleanup some AIO code using coroutines.
> >>> These patches apply to Kevin's repository, branch coroutine-block.
> >>> Mostly they use stack instead of allocated AIO structure.
> >>>
> >>> Frediano Ziglio (5):
> >>>   qcow: allocate QCowAIOCB structure using stack
> >>>   qcow: QCowAIOCB field cleanup
> >>>   qcow: move some blocks of code to avoid useless variable
> >>>     initialization
> >>>   avoid dandling pointers
> >>>   qcow: small optimization initializing QCowAIOCB
> >>>
> >>>  block/qcow.c  |  210 +++++++++++++++++++++++++--------------------------------
> >>>  block/qcow2.c |   38 +++-------
> >>>  2 files changed, 102 insertions(+), 146 deletions(-)
> >>
> >> Most of it looks good now. Did you include the "RFC" in the subject just
> >> because the coroutine work is in RFC state, too, or did you intend to
> >> tell me that I shouldn't merge yet?
> >>
> >> Kevin
> >>
> > 
> > As these patches are first quite big patches I send (typo or small
> > fixes do not counts) I just want to mark that I could write something
> > really wrong. Just a way to avoid somebody having to send more patches
> > and get more attention. Some projects are quite prone to merge even
> > not that fine ones. I prefer to have some (a bit) pedantic comments
> > and a real fix/improve.
> > 
> > Now I removed the RFC from last update. The main reason is that I
> > found your qemu-iotests repository which, I think should be merged to
> > main repository, but it's just my opinion.
> > Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error.
> 
> Yup, you're right, I must have messed it up. Care to fix it or should I
> look into it?
> 

Care but I don't know if I'll have time before Thursday. However I found
the problem, really strange. bdrv_read returns <0 for errors 0 for
success and... bytes read on partial read! Now a qcow image of 128m is
560 bytes so when you read sector 1 you get 48 which is not a problem
for qcow code. But if you replace bdrv_read with a bdrv_co_readv (your
latest patch on coroutine-block) bdrv_co_readv return -EINVAL on partial
read.

Frediano

Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup

[Kevin Wolf]

Am 22.07.2011 22:09, schrieb Frediano Ziglio:
> Il giorno ven, 22/07/2011 alle 12.10 +0200, Kevin Wolf ha scritto: 
>> Am 22.07.2011 11:26, schrieb Frediano Ziglio:
>>> 2011/7/22 Kevin Wolf <kwolf@redhat.com>:
>>>> Am 20.07.2011 15:56, schrieb Frediano Ziglio:
>>>>> These patches mostly cleanup some AIO code using coroutines.
>>>>> These patches apply to Kevin's repository, branch coroutine-block.
>>>>> Mostly they use stack instead of allocated AIO structure.
>>>>>
>>>>> Frediano Ziglio (5):
>>>>>   qcow: allocate QCowAIOCB structure using stack
>>>>>   qcow: QCowAIOCB field cleanup
>>>>>   qcow: move some blocks of code to avoid useless variable
>>>>>     initialization
>>>>>   avoid dandling pointers
>>>>>   qcow: small optimization initializing QCowAIOCB
>>>>>
>>>>>  block/qcow.c  |  210 +++++++++++++++++++++++++--------------------------------
>>>>>  block/qcow2.c |   38 +++-------
>>>>>  2 files changed, 102 insertions(+), 146 deletions(-)
>>>>
>>>> Most of it looks good now. Did you include the "RFC" in the subject just
>>>> because the coroutine work is in RFC state, too, or did you intend to
>>>> tell me that I shouldn't merge yet?
>>>>
>>>> Kevin
>>>>
>>>
>>> As these patches are first quite big patches I send (typo or small
>>> fixes do not counts) I just want to mark that I could write something
>>> really wrong. Just a way to avoid somebody having to send more patches
>>> and get more attention. Some projects are quite prone to merge even
>>> not that fine ones. I prefer to have some (a bit) pedantic comments
>>> and a real fix/improve.
>>>
>>> Now I removed the RFC from last update. The main reason is that I
>>> found your qemu-iotests repository which, I think should be merged to
>>> main repository, but it's just my opinion.
>>> Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error.
>>
>> Yup, you're right, I must have messed it up. Care to fix it or should I
>> look into it?
>>
> 
> Care but I don't know if I'll have time before Thursday. However I found
> the problem, really strange. bdrv_read returns <0 for errors 0 for
> success and... bytes read on partial read! Now a qcow image of 128m is
> 560 bytes so when you read sector 1 you get 48 which is not a problem
> for qcow code. But if you replace bdrv_read with a bdrv_co_readv (your
> latest patch on coroutine-block) bdrv_co_readv return -EINVAL on partial
> read.

Oh, that one. I think I have a fix for it somewhere, I'll include it in
my series.

Kevin