[Qemu-devel] [STABLE][PATCH 0/3] Rebase aio dma cancellation work

[Qemu-devel] [STABLE][PATCH 0/3] Rebase aio dma cancellation work

[Ryan Harper]

Rebased Avi's aio patch series for dma cancellation to stable branch.  Cursory
tested Linux 64-bit guest and WinXP SP2.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

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

[Ryan Harper]

Re-based against qemu-stable 0.10 branch

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>

diff --git a/block.c b/block.c
index 3f262fa..1aa3de5 100644
--- a/block.c
+++ b/block.c
@@ -141,6 +141,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;
 }
@@ -1443,14 +1444,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);
 }
 
 
@@ -1568,18 +1567,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;
@@ -1587,12 +1593,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 c6af1c6..e581637 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;
@@ -90,7 +96,7 @@ struct BlockDriver {
     /* to control generic scsi devices */
     int (*bdrv_ioctl)(BlockDriverState *bs, unsigned long int req, void *buf);
 
-    BlockDriverAIOCB *free_aiocb;
+    AIOPool aio_pool;
     struct BlockDriver *next;
 };
 
@@ -140,6 +146,7 @@ struct BlockDriverState {
 };
 
 struct BlockDriverAIOCB {
+    AIOPool *pool;
     BlockDriverState *bs;
     BlockDriverCompletionFunc *cb;
     void *opaque;
@@ -148,8 +155,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;

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

[Ryan Harper]

Rebased on qemu-stable.

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

Signed-off-by: Avi Kivity <avi@redhat.com>

diff --git a/block.c b/block.c
index 1aa3de5..465a09f 100644
--- a/block.c
+++ b/block.c
@@ -41,6 +41,8 @@
 #define SECTOR_BITS 9
 #define SECTOR_SIZE (1 << SECTOR_BITS)
 
+static AIOPool vectored_aio_pool;
+
 typedef struct BlockDriverAIOCBSync {
     BlockDriverAIOCB common;
     QEMUBH *bh;
@@ -1333,6 +1335,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;
@@ -1355,7 +1364,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;
@@ -1444,11 +1454,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);
 }
 
@@ -1550,6 +1555,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

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

[Ryan Harper]

Rebased on qemu-stable.

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>

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 54ae45b..d6d8307 100644
--- a/vl.c
+++ b/vl.c
@@ -5485,6 +5485,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 */
 

[Qemu-devel] Re: [STABLE][PATCH 0/3] Rebase aio dma cancellation work

[Anthony Liguori]

Ryan Harper wrote:
> Rebased Avi's aio patch series for dma cancellation to stable branch.  Cursory
> tested Linux 64-bit guest and WinXP SP2.
>
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
>   

Avi,

Are the first 3 of your original 5 patches what's suitable for stable?

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [STABLE][PATCH 0/3] Rebase aio dma cancellation work

[Avi Kivity]

Anthony Liguori wrote:
> Ryan Harper wrote:
>> Rebased Avi's aio patch series for dma cancellation to stable 
>> branch.  Cursory
>> tested Linux 64-bit guest and WinXP SP2.
>>
>> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
>>   
>
> Avi,
>
> Are the first 3 of your original 5 patches what's suitable for stable?
>

Yes.

Of course, the intermediate state wasn't tested as well as the complete 
patchset, but I don't think there's any issue lurking in there.

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

[Qemu-devel] [STABLE][PATCH 0/4] Rebase more aio dma cancellation work

[Mark McLoughlin]

Hi,
        We've a report of F11 qemu segfaulting when using read-only
images here:

   https://bugzilla.redhat.com/497170

        Looking at the current code in stable, there is clearly some
borkage:

  - No error handling in bdrv_aio_rw_vector()

  - No error handling in dma_bdrv_cb()/dma_bdrv_io()

  - VectorTranslationState is leaked by bdrv_aio_rw_vector_cb()

  - dma_aio_cancel() is cancelling the wrong aio

       The rest of Avi's aio cancellation patches actually fixes all
these problems. Please apply to stable.

Cheers,
Mark.

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

[Mark McLoughlin]

From: Avi Kivity <avi@redhat.com>

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 b12318f..689ea37 100644
--- a/block.c
+++ b/block.c
@@ -1332,31 +1332,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,
@@ -1368,11 +1369,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;
@@ -1384,7 +1383,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,
@@ -1560,7 +1559,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.0.6

[Qemu-devel] [STABLE][PATCH 2/4] Move block dma helpers aiocb to store dma state

[Mark McLoughlin]

From: Avi Kivity <avi@redhat.com>

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.0.6

[Qemu-devel] [STABLE][PATCH 3/4] Fix vectored aio bounce handling immediate errors

[Mark McLoughlin]

From: Avi Kivity <avi@redhat.com>

If a bounced vectored aio fails immediately (the inner aio submission
returning NULL) then the bounce handler erronously returns an aio
request which will never be completed (and which crashes when cancelled).

Fix by detecting that the inner request has failed and propagating the
error.

Signed-off-by: Avi Kivity <avi@redhat.com>

---
 block.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 689ea37..b68a8da 100644
--- a/block.c
+++ b/block.c
@@ -1383,6 +1383,11 @@ 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);
     }
+    if (!s->aiocb) {
+        qemu_vfree(s->bounce);
+        qemu_aio_release(s);
+        return NULL;
+    }
     return &s->common;
 }
 
-- 
1.6.0.6

[Qemu-devel] [STABLE][PATCH 4/4] Fix DMA API when handling an immediate error from block layer

[Mark McLoughlin]

From: Avi Kivity <avi@redhat.com>

The block layer may signal an immediate error on an asynchronous request
by returning NULL.  The DMA API did not handle this correctly, returning
an AIO request which would never complete (and which would crash if
cancelled).

Fix by detecting the failure and propagating it.

Signed-off-by: Avi Kivity <avi@redhat.com>

---
 dma-helpers.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 96a120c..1469e34 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -70,20 +70,26 @@ static void continue_after_map_failure(void *opaque)
     qemu_bh_schedule(dbs->bh);
 }
 
-static void dma_bdrv_cb(void *opaque, int ret)
+static void dma_bdrv_unmap(DMAAIOCB *dbs)
 {
-    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,
                                   dbs->iov.iov[i].iov_len, !dbs->is_write,
                                   dbs->iov.iov[i].iov_len);
     }
+}
+
+void dma_bdrv_cb(void *opaque, int ret)
+{
+    DMAAIOCB *dbs = (DMAAIOCB *)opaque;
+    target_phys_addr_t cur_addr, cur_len;
+    void *mem;
+
+    dbs->acb = NULL;
+    dbs->sector_num += dbs->iov.size / 512;
+    dma_bdrv_unmap(dbs);
     qemu_iovec_reset(&dbs->iov);
 
     if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
@@ -119,6 +125,11 @@ static void dma_bdrv_cb(void *opaque, int ret)
         dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
     }
+    if (!dbs->acb) {
+        dma_bdrv_unmap(dbs);
+        qemu_iovec_destroy(&dbs->iov);
+        return;
+    }
 }
 
 static BlockDriverAIOCB *dma_bdrv_io(
@@ -138,6 +149,10 @@ static BlockDriverAIOCB *dma_bdrv_io(
     dbs->bh = NULL;
     qemu_iovec_init(&dbs->iov, sg->nsg);
     dma_bdrv_cb(dbs, 0);
+    if (!dbs->acb) {
+        qemu_aio_release(dbs);
+        return NULL;
+    }
     return &dbs->common;
 }
 
-- 
1.6.0.6

[Qemu-devel] Re: [STABLE][PATCH 0/4] Rebase more aio dma cancellation work

[Avi Kivity]

Mark McLoughlin wrote:
> Hi,
>         We've a report of F11 qemu segfaulting when using read-only
> images here:
>
>    https://bugzilla.redhat.com/497170
>
>         Looking at the current code in stable, there is clearly some
> borkage:
>
>   - No error handling in bdrv_aio_rw_vector()
>
>   - No error handling in dma_bdrv_cb()/dma_bdrv_io()
>
>   - VectorTranslationState is leaked by bdrv_aio_rw_vector_cb()
>
>   - dma_aio_cancel() is cancelling the wrong aio
>
>        The rest of Avi's aio cancellation patches actually fixes all
> these problems. Please apply to stable.
>   

It's a little sad that we have to apply such a non trivial patchset to 
stable, but I see no way around it.  At least it has seen extensive 
testing in the main branch and in qemu-kvm.git.

(Mark, you have to add your signoff if you're submitting a patch, even 
if you've done no changes to it at all)

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

Re: [Qemu-devel] [STABLE][PATCH 1/4] Use vectored aiocb storage to store vector translation state

[Mark McLoughlin]

On Thu, 2009-05-14 at 11:56 +0100, Mark McLoughlin wrote:
> From: Avi Kivity <avi@redhat.com>
> 
> 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>

Signed-off-by: Mark McLoughlin <markmc@redhat.com>

> ---
>  block.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b12318f..689ea37 100644
> --- a/block.c
> +++ b/block.c
> @@ -1332,31 +1332,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,
> @@ -1368,11 +1369,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;
> @@ -1384,7 +1383,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,
> @@ -1560,7 +1559,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);

Re: [Qemu-devel] [STABLE][PATCH 2/4] Move block dma helpers aiocb to store dma state

[Mark McLoughlin]

On Thu, 2009-05-14 at 11:56 +0100, Mark McLoughlin wrote:
> From: Avi Kivity <avi@redhat.com>
> 
> Use the dedicated dma aiocb to store intermediate state for dma block
> transactions.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Signed-off-by: Mark McLoughlin <markmc@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);
>  }

Re: [Qemu-devel] [STABLE][PATCH 3/4] Fix vectored aio bounce handling immediate errors

[Mark McLoughlin]

On Thu, 2009-05-14 at 11:56 +0100, Mark McLoughlin wrote:
> From: Avi Kivity <avi@redhat.com>
> 
> If a bounced vectored aio fails immediately (the inner aio submission
> returning NULL) then the bounce handler erronously returns an aio
> request which will never be completed (and which crashes when cancelled).
> 
> Fix by detecting that the inner request has failed and propagating the
> error.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Signed-off-by: Mark McLoughlin <markmc@redhat.com>

> ---
>  block.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 689ea37..b68a8da 100644
> --- a/block.c
> +++ b/block.c
> @@ -1383,6 +1383,11 @@ 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);
>      }
> +    if (!s->aiocb) {
> +        qemu_vfree(s->bounce);
> +        qemu_aio_release(s);
> +        return NULL;
> +    }
>      return &s->common;
>  }
>  

Re: [Qemu-devel] [STABLE][PATCH 4/4] Fix DMA API when handling an immediate error from block layer

[Mark McLoughlin]

On Thu, 2009-05-14 at 11:56 +0100, Mark McLoughlin wrote:
> From: Avi Kivity <avi@redhat.com>
> 
> The block layer may signal an immediate error on an asynchronous request
> by returning NULL.  The DMA API did not handle this correctly, returning
> an AIO request which would never complete (and which would crash if
> cancelled).
> 
> Fix by detecting the failure and propagating it.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Signed-off-by: Mark McLoughlin <markmc@redhat.com>

> ---
>  dma-helpers.c |   27 +++++++++++++++++++++------
>  1 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 96a120c..1469e34 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -70,20 +70,26 @@ static void continue_after_map_failure(void *opaque)
>      qemu_bh_schedule(dbs->bh);
>  }
>  
> -static void dma_bdrv_cb(void *opaque, int ret)
> +static void dma_bdrv_unmap(DMAAIOCB *dbs)
>  {
> -    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,
>                                    dbs->iov.iov[i].iov_len, !dbs->is_write,
>                                    dbs->iov.iov[i].iov_len);
>      }
> +}
> +
> +void dma_bdrv_cb(void *opaque, int ret)
> +{
> +    DMAAIOCB *dbs = (DMAAIOCB *)opaque;
> +    target_phys_addr_t cur_addr, cur_len;
> +    void *mem;
> +
> +    dbs->acb = NULL;
> +    dbs->sector_num += dbs->iov.size / 512;
> +    dma_bdrv_unmap(dbs);
>      qemu_iovec_reset(&dbs->iov);
>  
>      if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
> @@ -119,6 +125,11 @@ static void dma_bdrv_cb(void *opaque, int ret)
>          dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
>                                    dbs->iov.size / 512, dma_bdrv_cb, dbs);
>      }
> +    if (!dbs->acb) {
> +        dma_bdrv_unmap(dbs);
> +        qemu_iovec_destroy(&dbs->iov);
> +        return;
> +    }
>  }
>  
>  static BlockDriverAIOCB *dma_bdrv_io(
> @@ -138,6 +149,10 @@ static BlockDriverAIOCB *dma_bdrv_io(
>      dbs->bh = NULL;
>      qemu_iovec_init(&dbs->iov, sg->nsg);
>      dma_bdrv_cb(dbs, 0);
> +    if (!dbs->acb) {
> +        qemu_aio_release(dbs);
> +        return NULL;
> +    }
>      return &dbs->common;
>  }
>  

Re: [Qemu-devel] [STABLE][PATCH 0/4] Rebase more aio dma cancellation work

[Anthony Liguori]

Mark McLoughlin wrote:
> Hi,
>         We've a report of F11 qemu segfaulting when using read-only
> images here:
>
>    https://bugzilla.redhat.com/497170
>
>         Looking at the current code in stable, there is clearly some
> borkage:
>
>   - No error handling in bdrv_aio_rw_vector()
>
>   - No error handling in dma_bdrv_cb()/dma_bdrv_io()
>
>   - VectorTranslationState is leaked by bdrv_aio_rw_vector_cb()
>
>   - dma_aio_cancel() is cancelling the wrong aio
>
>        The rest of Avi's aio cancellation patches actually fixes all
> these problems. Please apply to stable.
>   

Thanks Mark.

I actually cherry picked the patches directly.  This is less error prone.

Regards,

Anthony Liguori