[Qemu-devel] [PATCH 0/5] Fix aio cancellation with synthetic aiocbs

[Qemu-devel] [PATCH 0/5] Fix aio cancellation with synthetic aiocbs

[Avi Kivity]

The block layer will currently break with synthetic aiocbs, since it will
forward the cancellation request to the block format driver, which did not
generate the synthetic aiocbs.

This patchset fixes the problem by associating a cancellation method
with synthetic aiocbs through a aio pool data structure.  This structure
contains information common to a class of aiocbs - size, cancellation method,
and free list.

Avi Kivity (5):
  Refactor aio callback allocation to use an aiocb pool
  Convert vectored aio emulation to use a dedicated pool
  Implement cancellation method for dma async I/O
  Use vectored aiocb storage to store vector translation state
  Move block dma helpers aiocb to store dma state

 block.c       |   79 +++++++++++++++++++++++++++++++++++---------------------
 block_int.h   |   14 +++++++++-
 dma-helpers.c |   44 +++++++++++++++++++++----------
 dma.h         |    1 +
 vl.c          |    1 +
 5 files changed, 94 insertions(+), 45 deletions(-)

[Qemu-devel] [PATCH 1/5] Refactor aio callback allocation to use an aiocb pool

[Avi Kivity]

Move the AIOCB allocation code to use a dedicate structure, AIOPool.  AIOCB
specific information, such as the AIOCB size and cancellation routine, is
moved into the pool.

At present, there is exactly one pool per block format driver, maintaining
the status quo.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c     |   42 +++++++++++++++++++++++++++---------------
 block_int.h |   14 +++++++++++++-
 2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 135fbe6..ad16ee4 100644
--- a/block.c
+++ b/block.c
@@ -147,6 +147,7 @@ static void bdrv_register(BlockDriver *bdrv)
         bdrv->bdrv_read = bdrv_read_em;
         bdrv->bdrv_write = bdrv_write_em;
     }
+    aio_pool_init(&bdrv->aio_pool, bdrv->aiocb_size, bdrv->bdrv_aio_cancel);
     bdrv->next = first_drv;
     first_drv = bdrv;
 }
@@ -1371,14 +1372,12 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
 
 void bdrv_aio_cancel(BlockDriverAIOCB *acb)
 {
-    BlockDriver *drv = acb->bs->drv;
-
     if (acb->cb == bdrv_aio_rw_vector_cb) {
         VectorTranslationState *s = acb->opaque;
         acb = s->aiocb;
     }
 
-    drv->bdrv_aio_cancel(acb);
+    acb->pool->cancel(acb);
 }
 
 
@@ -1496,18 +1495,25 @@ void bdrv_init(void)
     bdrv_register(&bdrv_nbd);
 }
 
-void *qemu_aio_get(BlockDriverState *bs, BlockDriverCompletionFunc *cb,
-                   void *opaque)
+void aio_pool_init(AIOPool *pool, int aiocb_size,
+                   void (*cancel)(BlockDriverAIOCB *acb))
+{
+    pool->aiocb_size = aiocb_size;
+    pool->cancel = cancel;
+    pool->free_aiocb = NULL;
+}
+
+void *qemu_aio_get_pool(AIOPool *pool, BlockDriverState *bs,
+                        BlockDriverCompletionFunc *cb, void *opaque)
 {
-    BlockDriver *drv;
     BlockDriverAIOCB *acb;
 
-    drv = bs->drv;
-    if (drv->free_aiocb) {
-        acb = drv->free_aiocb;
-        drv->free_aiocb = acb->next;
+    if (pool->free_aiocb) {
+        acb = pool->free_aiocb;
+        pool->free_aiocb = acb->next;
     } else {
-        acb = qemu_mallocz(drv->aiocb_size);
+        acb = qemu_mallocz(pool->aiocb_size);
+        acb->pool = pool;
     }
     acb->bs = bs;
     acb->cb = cb;
@@ -1515,12 +1521,18 @@ void *qemu_aio_get(BlockDriverState *bs, BlockDriverCompletionFunc *cb,
     return acb;
 }
 
+void *qemu_aio_get(BlockDriverState *bs, BlockDriverCompletionFunc *cb,
+                   void *opaque)
+{
+    return qemu_aio_get_pool(&bs->drv->aio_pool, bs, cb, opaque);
+}
+
 void qemu_aio_release(void *p)
 {
-    BlockDriverAIOCB *acb = p;
-    BlockDriver *drv = acb->bs->drv;
-    acb->next = drv->free_aiocb;
-    drv->free_aiocb = acb;
+    BlockDriverAIOCB *acb = (BlockDriverAIOCB *)p;
+    AIOPool *pool = acb->pool;
+    acb->next = pool->free_aiocb;
+    pool->free_aiocb = acb;
 }
 
 /**************************************************************/
diff --git a/block_int.h b/block_int.h
index d3c81fa..8b4c5fe 100644
--- a/block_int.h
+++ b/block_int.h
@@ -30,6 +30,12 @@
 #define BLOCK_FLAG_COMPRESS	2
 #define BLOCK_FLAG_COMPAT6	4
 
+typedef struct AIOPool {
+    void (*cancel)(BlockDriverAIOCB *acb);
+    int aiocb_size;
+    BlockDriverAIOCB *free_aiocb;
+} AIOPool;
+
 struct BlockDriver {
     const char *format_name;
     int instance_size;
@@ -91,7 +97,7 @@ struct BlockDriver {
                                            BlockDriverCompletionFunc *cb,
                                            void *opaque);
 
-    BlockDriverAIOCB *free_aiocb;
+    AIOPool aio_pool;
     struct BlockDriver *next;
 };
 
@@ -141,6 +147,7 @@ struct BlockDriverState {
 };
 
 struct BlockDriverAIOCB {
+    AIOPool *pool;
     BlockDriverState *bs;
     BlockDriverCompletionFunc *cb;
     void *opaque;
@@ -149,8 +156,13 @@ struct BlockDriverAIOCB {
 
 void get_tmp_filename(char *filename, int size);
 
+void aio_pool_init(AIOPool *pool, int aiocb_size,
+                   void (*cancel)(BlockDriverAIOCB *acb));
+
 void *qemu_aio_get(BlockDriverState *bs, BlockDriverCompletionFunc *cb,
                    void *opaque);
+void *qemu_aio_get_pool(AIOPool *pool, BlockDriverState *bs,
+                        BlockDriverCompletionFunc *cb, void *opaque);
 void qemu_aio_release(void *p);
 
 extern BlockDriverState *bdrv_first;
-- 
1.6.1.1

[Qemu-devel] [PATCH 2/5] Convert vectored aio emulation to use a dedicated pool

[Avi Kivity]

This allows us to remove a hack in the vectored aio cancellation code.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index ad16ee4..fe83585 100644
--- a/block.c
+++ b/block.c
@@ -47,6 +47,8 @@
 #define SECTOR_BITS 9
 #define SECTOR_SIZE (1 << SECTOR_BITS)
 
+static AIOPool vectored_aio_pool;
+
 typedef struct BlockDriverAIOCBSync {
     BlockDriverAIOCB common;
     QEMUBH *bh;
@@ -1261,6 +1263,13 @@ typedef struct VectorTranslationState {
     BlockDriverAIOCB *this_aiocb;
 } VectorTranslationState;
 
+static void bdrv_aio_cancel_vector(BlockDriverAIOCB *acb)
+{
+    VectorTranslationState *s = acb->opaque;
+
+    bdrv_aio_cancel(s->aiocb);
+}
+
 static void bdrv_aio_rw_vector_cb(void *opaque, int ret)
 {
     VectorTranslationState *s = opaque;
@@ -1283,7 +1292,8 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
 
 {
     VectorTranslationState *s = qemu_mallocz(sizeof(*s));
-    BlockDriverAIOCB *aiocb = qemu_aio_get(bs, cb, opaque);
+    BlockDriverAIOCB *aiocb = qemu_aio_get_pool(&vectored_aio_pool, bs,
+                                                cb, opaque);
 
     s->this_aiocb = aiocb;
     s->iov = iov;
@@ -1372,11 +1382,6 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
 
 void bdrv_aio_cancel(BlockDriverAIOCB *acb)
 {
-    if (acb->cb == bdrv_aio_rw_vector_cb) {
-        VectorTranslationState *s = acb->opaque;
-        acb = s->aiocb;
-    }
-
     acb->pool->cancel(acb);
 }
 
@@ -1478,6 +1483,9 @@ static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
 
 void bdrv_init(void)
 {
+    aio_pool_init(&vectored_aio_pool, sizeof(BlockDriverAIOCB),
+                  bdrv_aio_cancel_vector);
+
     bdrv_register(&bdrv_raw);
     bdrv_register(&bdrv_host_device);
 #ifndef _WIN32
-- 
1.6.1.1

Re: [Qemu-devel] [PATCH 2/5] Convert vectored aio emulation to use a dedicated pool

[Christoph Hellwig]

On Mon, Mar 16, 2009 at 03:11:08PM +0200, Avi Kivity wrote:
> This allows us to remove a hack in the vectored aio cancellation code.

Note that patch my switch to the aio_readv/writev methods will get rid of
this whole double aiocb indirection in a very natural way.

Re: [Qemu-devel] [PATCH 2/5] Convert vectored aio emulation to use a dedicated pool

[Avi Kivity]

Christoph Hellwig wrote:
> On Mon, Mar 16, 2009 at 03:11:08PM +0200, Avi Kivity wrote:
>   
>> This allows us to remove a hack in the vectored aio cancellation code.
>>     
>
> Note that patch my switch to the aio_readv/writev methods will get rid of
> this whole double aiocb indirection in a very natural way.
>   

Right, when you don't use a synthetic aiocb, the need goes away.

We'll still have one for the block-dma-helpers, due to the need to 
fragment a request when bounce resources are exhausted.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/5] Convert vectored aio emulation to use a dedicated pool

[Christoph Hellwig]

On Mon, Mar 16, 2009 at 03:39:30PM +0200, Avi Kivity wrote:
> Right, when you don't use a synthetic aiocb, the need goes away.
> 
> We'll still have one for the block-dma-helpers, due to the need to 
> fragment a request when bounce resources are exhausted.

The pools might also help with the independent scsi-generic
implementation, by not requiring it to be tied to the block layer
while still beeing able to use posix-aio-compat.c.  Still need to sort
out the setup issue, but we might just keep a dummy BlockDriverState
for it for now.

[Qemu-devel] [PATCH 3/5] Implement cancellation method for dma async I/O

[Avi Kivity]

Move the dma helpers to a private aio pool, and implement a cancellation
method for them.  Should prevent issues when cancelling I/O while dma is
in progress.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 dma-helpers.c |   15 ++++++++++++++-
 dma.h         |    1 +
 vl.c          |    1 +
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index b2ade19..19fa4f0 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -10,6 +10,8 @@
 #include "dma.h"
 #include "block_int.h"
 
+static AIOPool dma_aio_pool;
+
 void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
 {
     qsg->sg = qemu_malloc(alloc_hint * sizeof(ScatterGatherEntry));
@@ -126,7 +128,7 @@ static BlockDriverAIOCB *dma_bdrv_io(
     DMABlockState *dbs = qemu_malloc(sizeof(*dbs));
 
     dbs->bs = bs;
-    dbs->acb = qemu_aio_get(bs, cb, opaque);
+    dbs->acb = qemu_aio_get_pool(&dma_aio_pool, bs, cb, opaque);
     dbs->sg = sg;
     dbs->sector_num = sector_num;
     dbs->sg_cur_index = 0;
@@ -153,3 +155,14 @@ BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
     return dma_bdrv_io(bs, sg, sector, cb, opaque, 1);
 }
 
+static void dma_aio_cancel(BlockDriverAIOCB *acb)
+{
+    DMABlockState *dbs = (DMABlockState *)acb->opaque;
+
+    bdrv_aio_cancel(dbs->acb);
+}
+
+void dma_helper_init(void)
+{
+    aio_pool_init(&dma_aio_pool, sizeof(BlockDriverAIOCB), dma_aio_cancel);
+}
diff --git a/dma.h b/dma.h
index d596717..7d2ab45 100644
--- a/dma.h
+++ b/dma.h
@@ -37,5 +37,6 @@ BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
 BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
                                  QEMUSGList *sg, uint64_t sector,
                                  BlockDriverCompletionFunc *cb, void *opaque);
+void dma_helper_init(void);
 
 #endif
diff --git a/vl.c b/vl.c
index 2adfebf..4016c37 100644
--- a/vl.c
+++ b/vl.c
@@ -5517,6 +5517,7 @@ int main(int argc, char **argv, char **envp)
     cpu_exec_init_all(tb_size * 1024 * 1024);
 
     bdrv_init();
+    dma_helper_init();
 
     /* we always create the cdrom drive, even if no disk is there */
 
-- 
1.6.1.1

[Qemu-devel] [PATCH 4/5] Use vectored aiocb storage to store vector translation state

[Avi Kivity]

Now that we have a dedicated acb pool for vector translation acbs, we can
store the vector translation state in the acbs instead of in an external
structure.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index fe83585..fd09dff 100644
--- a/block.c
+++ b/block.c
@@ -1255,31 +1255,32 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
 /**************************************************************/
 /* async I/Os */
 
-typedef struct VectorTranslationState {
+typedef struct VectorTranslationAIOCB {
+    BlockDriverAIOCB common;
     QEMUIOVector *iov;
     uint8_t *bounce;
     int is_write;
     BlockDriverAIOCB *aiocb;
-    BlockDriverAIOCB *this_aiocb;
-} VectorTranslationState;
+} VectorTranslationAIOCB;
 
-static void bdrv_aio_cancel_vector(BlockDriverAIOCB *acb)
+static void bdrv_aio_cancel_vector(BlockDriverAIOCB *_acb)
 {
-    VectorTranslationState *s = acb->opaque;
+    VectorTranslationAIOCB *acb
+        = container_of(_acb, VectorTranslationAIOCB, common);
 
-    bdrv_aio_cancel(s->aiocb);
+    bdrv_aio_cancel(acb->aiocb);
 }
 
 static void bdrv_aio_rw_vector_cb(void *opaque, int ret)
 {
-    VectorTranslationState *s = opaque;
+    VectorTranslationAIOCB *s = (VectorTranslationAIOCB *)opaque;
 
     if (!s->is_write) {
         qemu_iovec_from_buffer(s->iov, s->bounce, s->iov->size);
     }
     qemu_vfree(s->bounce);
-    s->this_aiocb->cb(s->this_aiocb->opaque, ret);
-    qemu_aio_release(s->this_aiocb);
+    s->common.cb(s->common.opaque, ret);
+    qemu_aio_release(s);
 }
 
 static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
@@ -1291,11 +1292,9 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
                                             int is_write)
 
 {
-    VectorTranslationState *s = qemu_mallocz(sizeof(*s));
-    BlockDriverAIOCB *aiocb = qemu_aio_get_pool(&vectored_aio_pool, bs,
-                                                cb, opaque);
+    VectorTranslationAIOCB *s = qemu_aio_get_pool(&vectored_aio_pool, bs,
+                                                  cb, opaque);
 
-    s->this_aiocb = aiocb;
     s->iov = iov;
     s->bounce = qemu_memalign(512, nb_sectors * 512);
     s->is_write = is_write;
@@ -1307,7 +1306,7 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
         s->aiocb = bdrv_aio_read(bs, sector_num, s->bounce, nb_sectors,
                                  bdrv_aio_rw_vector_cb, s);
     }
-    return aiocb;
+    return &s->common;
 }
 
 BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
@@ -1483,7 +1482,7 @@ static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
 
 void bdrv_init(void)
 {
-    aio_pool_init(&vectored_aio_pool, sizeof(BlockDriverAIOCB),
+    aio_pool_init(&vectored_aio_pool, sizeof(VectorTranslationAIOCB),
                   bdrv_aio_cancel_vector);
 
     bdrv_register(&bdrv_raw);
-- 
1.6.1.1

[Qemu-devel] [PATCH 5/5] Move block dma helpers aiocb to store dma state

[Avi Kivity]

Use the dedicated dma aiocb to store intermediate state for dma block
transactions.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 dma-helpers.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 19fa4f0..96a120c 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -39,6 +39,7 @@ void qemu_sglist_destroy(QEMUSGList *qsg)
 }
 
 typedef struct {
+    BlockDriverAIOCB common;
     BlockDriverState *bs;
     BlockDriverAIOCB *acb;
     QEMUSGList *sg;
@@ -48,13 +49,13 @@ typedef struct {
     target_phys_addr_t sg_cur_byte;
     QEMUIOVector iov;
     QEMUBH *bh;
-} DMABlockState;
+} DMAAIOCB;
 
 static void dma_bdrv_cb(void *opaque, int ret);
 
 static void reschedule_dma(void *opaque)
 {
-    DMABlockState *dbs = (DMABlockState *)opaque;
+    DMAAIOCB *dbs = (DMAAIOCB *)opaque;
 
     qemu_bh_delete(dbs->bh);
     dbs->bh = NULL;
@@ -63,7 +64,7 @@ static void reschedule_dma(void *opaque)
 
 static void continue_after_map_failure(void *opaque)
 {
-    DMABlockState *dbs = (DMABlockState *)opaque;
+    DMAAIOCB *dbs = (DMAAIOCB *)opaque;
 
     dbs->bh = qemu_bh_new(reschedule_dma, dbs);
     qemu_bh_schedule(dbs->bh);
@@ -71,11 +72,12 @@ static void continue_after_map_failure(void *opaque)
 
 static void dma_bdrv_cb(void *opaque, int ret)
 {
-    DMABlockState *dbs = (DMABlockState *)opaque;
+    DMAAIOCB *dbs = (DMAAIOCB *)opaque;
     target_phys_addr_t cur_addr, cur_len;
     void *mem;
     int i;
 
+    dbs->acb = NULL;
     dbs->sector_num += dbs->iov.size / 512;
     for (i = 0; i < dbs->iov.niov; ++i) {
         cpu_physical_memory_unmap(dbs->iov.iov[i].iov_base,
@@ -85,10 +87,9 @@ static void dma_bdrv_cb(void *opaque, int ret)
     qemu_iovec_reset(&dbs->iov);
 
     if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
-        dbs->acb->cb(dbs->acb->opaque, ret);
+        dbs->common.cb(dbs->common.opaque, ret);
         qemu_iovec_destroy(&dbs->iov);
-        qemu_aio_release(dbs->acb);
-        qemu_free(dbs);
+        qemu_aio_release(dbs);
         return;
     }
 
@@ -112,11 +113,11 @@ static void dma_bdrv_cb(void *opaque, int ret)
     }
 
     if (dbs->is_write) {
-        bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
-                        dbs->iov.size / 512, dma_bdrv_cb, dbs);
+        dbs->acb = bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
+                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
     } else {
-        bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
-                       dbs->iov.size / 512, dma_bdrv_cb, dbs);
+        dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
+                                  dbs->iov.size / 512, dma_bdrv_cb, dbs);
     }
 }
 
@@ -125,10 +126,10 @@ static BlockDriverAIOCB *dma_bdrv_io(
     BlockDriverCompletionFunc *cb, void *opaque,
     int is_write)
 {
-    DMABlockState *dbs = qemu_malloc(sizeof(*dbs));
+    DMAAIOCB *dbs =  qemu_aio_get_pool(&dma_aio_pool, bs, cb, opaque);
 
+    dbs->acb = NULL;
     dbs->bs = bs;
-    dbs->acb = qemu_aio_get_pool(&dma_aio_pool, bs, cb, opaque);
     dbs->sg = sg;
     dbs->sector_num = sector_num;
     dbs->sg_cur_index = 0;
@@ -137,7 +138,7 @@ static BlockDriverAIOCB *dma_bdrv_io(
     dbs->bh = NULL;
     qemu_iovec_init(&dbs->iov, sg->nsg);
     dma_bdrv_cb(dbs, 0);
-    return dbs->acb;
+    return &dbs->common;
 }
 
 
@@ -157,12 +158,14 @@ BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
 
 static void dma_aio_cancel(BlockDriverAIOCB *acb)
 {
-    DMABlockState *dbs = (DMABlockState *)acb->opaque;
+    DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
 
-    bdrv_aio_cancel(dbs->acb);
+    if (dbs->acb) {
+        bdrv_aio_cancel(dbs->acb);
+    }
 }
 
 void dma_helper_init(void)
 {
-    aio_pool_init(&dma_aio_pool, sizeof(BlockDriverAIOCB), dma_aio_cancel);
+    aio_pool_init(&dma_aio_pool, sizeof(DMAAIOCB), dma_aio_cancel);
 }
-- 
1.6.1.1

[Qemu-devel] Re: [PATCH 0/5] Fix aio cancellation with synthetic aiocbs

[Anthony Liguori]

Avi Kivity wrote:
> The block layer will currently break with synthetic aiocbs, since it will
> forward the cancellation request to the block format driver, which did not
> generate the synthetic aiocbs.
>
> This patchset fixes the problem by associating a cancellation method
> with synthetic aiocbs through a aio pool data structure.  This structure
> contains information common to a class of aiocbs - size, cancellation method,
> and free list.
>
> Avi Kivity (5):
>   Refactor aio callback allocation to use an aiocb pool
>   Convert vectored aio emulation to use a dedicated pool
>   Implement cancellation method for dma async I/O
>   Use vectored aiocb storage to store vector translation state
>   Move block dma helpers aiocb to store dma state
>
>  block.c       |   79 +++++++++++++++++++++++++++++++++++---------------------
>  block_int.h   |   14 +++++++++-
>  dma-helpers.c |   44 +++++++++++++++++++++----------
>  dma.h         |    1 +
>  vl.c          |    1 +
>  5 files changed, 94 insertions(+), 45 deletions(-)
>   

Applied all.  Thanks.

Regards,

Anthony Liguori