[Qemu-devel] [PATCH 0/2] Fix immediate error handling on synthetic aios

[Qemu-devel] [PATCH 0/2] Fix immediate error handling on synthetic aios

[Avi Kivity]

The block layer can signal aio errors in two ways: by calling the completion
function with a negative status code, or by returning a NULL aiocb.  Two of
the synthetic aio implementations (the DMA API and the vectored aio bouncing)
fail to handle the second case correctly, resulting in crashes when
cancellation of a timed out request is attempted.

I think the long term fix is to have the block layer signal errors in just
one way -- by invoking the completion handler with a negative return code --
but in the meanwhile, here are patches that add the required error checking.

Avi Kivity (2):
  Fix vectored aio bounce handling immediate errors
  Fix DMA API when handling an immediate error from block layer

 block.c       |    5 +++++
 dma-helpers.c |   27 +++++++++++++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/2] Fix vectored aio bounce handling immediate errors

[Avi Kivity]

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 fd09dff..6479072 100644
--- a/block.c
+++ b/block.c
@@ -1306,6 +1306,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.1.1

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

[Avi Kivity]

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

[Qemu-devel] Re: [PATCH 0/2] Fix immediate error handling on synthetic aios

[Anthony Liguori]

Avi Kivity wrote:
> The block layer can signal aio errors in two ways: by calling the completion
> function with a negative status code, or by returning a NULL aiocb.  Two of
> the synthetic aio implementations (the DMA API and the vectored aio bouncing)
> fail to handle the second case correctly, resulting in crashes when
> cancellation of a timed out request is attempted.
>
> I think the long term fix is to have the block layer signal errors in just
> one way -- by invoking the completion handler with a negative return code --
> but in the meanwhile, here are patches that add the required error checking.
>
> Avi Kivity (2):
>   Fix vectored aio bounce handling immediate errors
>   Fix DMA API when handling an immediate error from block layer
>
>  block.c       |    5 +++++
>  dma-helpers.c |   27 +++++++++++++++++++++------
>  2 files changed, 26 insertions(+), 6 deletions(-)
>   

Applied all.  Thanks.

Regards,

Anthony Liguori