[Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback

[Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback

[Kevin Wolf]

This series fixes some cases of block drivers calling AIO callbacks too early.
It fixes the IDE assertion failure reported by Luiz (in error cases, the DMA
status, including acb, could first be reset in the callback and only then be
set by the caller, resulting in a dangling acb and wrong status register value).

Kevin Wolf (3):
  qcow2: Avoid direct AIO callback
  qcow: Avoid direct AIO callback
  vdi: Avoid direct AIO callback

 block/qcow.c  |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qcow2.c |   39 +++++++++++++++++++++++++++++--------
 block/vdi.c   |   40 +++++++++++++++++++++++++++++++++++---
 3 files changed, 122 insertions(+), 15 deletions(-)

-- 
1.7.5.2

[Qemu-devel] [PATCH 1/3] qcow2: Avoid direct AIO callback

[Kevin Wolf]

bdrv_aio_* must not call the callback before returning to its caller. In qcow2,
this could happen in some error cases. This starts the real requests processing
in a BH to avoid this situation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 30b6692..890cf2c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -378,6 +378,7 @@ typedef struct QCowAIOCB {
     uint64_t bytes_done;
     uint64_t cluster_offset;
     uint8_t *cluster_data;
+    bool is_write;
     BlockDriverAIOCB *hd_aiocb;
     QEMUIOVector hd_qiov;
     QEMUBH *bh;
@@ -399,12 +400,19 @@ static AIOPool qcow2_aio_pool = {
 };
 
 static void qcow2_aio_read_cb(void *opaque, int ret);
-static void qcow2_aio_read_bh(void *opaque)
+static void qcow2_aio_write_cb(void *opaque, int ret);
+
+static void qcow2_aio_rw_bh(void *opaque)
 {
     QCowAIOCB *acb = opaque;
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
-    qcow2_aio_read_cb(opaque, 0);
+
+    if (acb->is_write) {
+        qcow2_aio_write_cb(opaque, 0);
+    } else {
+        qcow2_aio_read_cb(opaque, 0);
+    }
 }
 
 static int qcow2_schedule_bh(QEMUBHFunc *cb, QCowAIOCB *acb)
@@ -493,14 +501,14 @@ static void qcow2_aio_read_cb(void *opaque, int ret)
                     goto done;
                 }
             } else {
-                ret = qcow2_schedule_bh(qcow2_aio_read_bh, acb);
+                ret = qcow2_schedule_bh(qcow2_aio_rw_bh, acb);
                 if (ret < 0)
                     goto done;
             }
         } else {
             /* Note: in this case, no need to wait */
             qemu_iovec_memset(&acb->hd_qiov, 0, 512 * acb->cur_nr_sectors);
-            ret = qcow2_schedule_bh(qcow2_aio_read_bh, acb);
+            ret = qcow2_schedule_bh(qcow2_aio_rw_bh, acb);
             if (ret < 0)
                 goto done;
         }
@@ -515,7 +523,7 @@ static void qcow2_aio_read_cb(void *opaque, int ret)
             s->cluster_cache + index_in_cluster * 512,
             512 * acb->cur_nr_sectors);
 
-        ret = qcow2_schedule_bh(qcow2_aio_read_bh, acb);
+        ret = qcow2_schedule_bh(qcow2_aio_rw_bh, acb);
         if (ret < 0)
             goto done;
     } else {
@@ -572,6 +580,7 @@ static QCowAIOCB *qcow2_aio_setup(BlockDriverState *bs, int64_t sector_num,
     acb->hd_aiocb = NULL;
     acb->sector_num = sector_num;
     acb->qiov = qiov;
+    acb->is_write = is_write;
 
     qemu_iovec_init(&acb->hd_qiov, qiov->niov);
 
@@ -591,17 +600,22 @@ static BlockDriverAIOCB *qcow2_aio_readv(BlockDriverState *bs,
                                          void *opaque)
 {
     QCowAIOCB *acb;
+    int ret;
 
     acb = qcow2_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
     if (!acb)
         return NULL;
 
-    qcow2_aio_read_cb(acb, 0);
+    ret = qcow2_schedule_bh(qcow2_aio_rw_bh, acb);
+    if (ret < 0) {
+        qemu_iovec_destroy(&acb->hd_qiov);
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
-static void qcow2_aio_write_cb(void *opaque, int ret);
-
 static void run_dependent_requests(QCowL2Meta *m)
 {
     QCowAIOCB *req;
@@ -724,6 +738,7 @@ static BlockDriverAIOCB *qcow2_aio_writev(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     QCowAIOCB *acb;
+    int ret;
 
     s->cluster_cache_offset = -1; /* disable compressed cache */
 
@@ -731,7 +746,13 @@ static BlockDriverAIOCB *qcow2_aio_writev(BlockDriverState *bs,
     if (!acb)
         return NULL;
 
-    qcow2_aio_write_cb(acb, 0);
+    ret = qcow2_schedule_bh(qcow2_aio_rw_bh, acb);
+    if (ret < 0) {
+        qemu_iovec_destroy(&acb->hd_qiov);
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
-- 
1.7.5.2

[Qemu-devel] [PATCH 2/3] qcow: Avoid direct AIO callback

[Kevin Wolf]

bdrv_aio_* must not call the callback before returning to its caller. In qcow,
this could happen in some error cases. This starts the real requests processing
in a BH to avoid this situation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index a26c886..227b104 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -496,6 +496,8 @@ typedef struct QCowAIOCB {
     uint64_t cluster_offset;
     uint8_t *cluster_data;
     struct iovec hd_iov;
+    bool is_write;
+    QEMUBH *bh;
     QEMUIOVector hd_qiov;
     BlockDriverAIOCB *hd_aiocb;
 } QCowAIOCB;
@@ -525,6 +527,8 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
     acb->hd_aiocb = NULL;
     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);
         if (is_write)
@@ -538,6 +542,38 @@ static QCowAIOCB *qcow_aio_setup(BlockDriverState *bs,
     return acb;
 }
 
+static void qcow_aio_read_cb(void *opaque, int ret);
+static void qcow_aio_write_cb(void *opaque, int ret);
+
+static void qcow_aio_rw_bh(void *opaque)
+{
+    QCowAIOCB *acb = opaque;
+    qemu_bh_delete(acb->bh);
+    acb->bh = NULL;
+
+    if (acb->is_write) {
+        qcow_aio_write_cb(opaque, 0);
+    } else {
+        qcow_aio_read_cb(opaque, 0);
+    }
+}
+
+static int qcow_schedule_bh(QEMUBHFunc *cb, QCowAIOCB *acb)
+{
+    if (acb->bh) {
+        return -EIO;
+    }
+
+    acb->bh = qemu_bh_new(cb, acb);
+    if (!acb->bh) {
+        return -EIO;
+    }
+
+    qemu_bh_schedule(acb->bh);
+
+    return 0;
+}
+
 static void qcow_aio_read_cb(void *opaque, int ret)
 {
     QCowAIOCB *acb = opaque;
@@ -640,12 +676,21 @@ static BlockDriverAIOCB *qcow_aio_readv(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     QCowAIOCB *acb;
+    int ret;
 
     acb = qcow_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
     if (!acb)
         return NULL;
 
-    qcow_aio_read_cb(acb, 0);
+    ret = qcow_schedule_bh(qcow_aio_rw_bh, acb);
+    if (ret < 0) {
+        if (acb->qiov->niov > 1) {
+            qemu_vfree(acb->orig_buf);
+        }
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
@@ -725,6 +770,7 @@ static BlockDriverAIOCB *qcow_aio_writev(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     QCowAIOCB *acb;
+    int ret;
 
     s->cluster_cache_offset = -1; /* disable compressed cache */
 
@@ -733,7 +779,15 @@ static BlockDriverAIOCB *qcow_aio_writev(BlockDriverState *bs,
         return NULL;
 
 
-    qcow_aio_write_cb(acb, 0);
+    ret = qcow_schedule_bh(qcow_aio_rw_bh, acb);
+    if (ret < 0) {
+        if (acb->qiov->niov > 1) {
+            qemu_vfree(acb->orig_buf);
+        }
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
-- 
1.7.5.2

[Qemu-devel] [PATCH 3/3] vdi: Avoid direct AIO callback

[Kevin Wolf]

bdrv_aio_* must not call the callback before returning to its caller. In vdi,
this could happen in some error cases. This starts the real requests processing
in a BH to avoid this situation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vdi.c |   40 ++++++++++++++++++++++++++++++++++++----
 1 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 4c9e201..dfde5df 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -152,6 +152,7 @@ typedef struct {
     /* Buffer for new allocated block. */
     void *block_buffer;
     void *orig_buf;
+    bool is_write;
     int header_modified;
     BlockDriverAIOCB *hd_aiocb;
     struct iovec hd_iov;
@@ -504,6 +505,8 @@ static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
         acb->hd_aiocb = NULL;
         acb->sector_num = sector_num;
         acb->qiov = qiov;
+        acb->is_write = is_write;
+
         if (qiov->niov > 1) {
             acb->buf = qemu_blockalign(bs, qiov->size);
             acb->orig_buf = acb->buf;
@@ -542,14 +545,20 @@ static int vdi_schedule_bh(QEMUBHFunc *cb, VdiAIOCB *acb)
 }
 
 static void vdi_aio_read_cb(void *opaque, int ret);
+static void vdi_aio_write_cb(void *opaque, int ret);
 
-static void vdi_aio_read_bh(void *opaque)
+static void vdi_aio_rw_bh(void *opaque)
 {
     VdiAIOCB *acb = opaque;
     logout("\n");
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
-    vdi_aio_read_cb(opaque, 0);
+
+    if (acb->is_write) {
+        vdi_aio_write_cb(opaque, 0);
+    } else {
+        vdi_aio_read_cb(opaque, 0);
+    }
 }
 
 static void vdi_aio_read_cb(void *opaque, int ret)
@@ -597,7 +606,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
     if (bmap_entry == VDI_UNALLOCATED) {
         /* Block not allocated, return zeros, no need to wait. */
         memset(acb->buf, 0, n_sectors * SECTOR_SIZE);
-        ret = vdi_schedule_bh(vdi_aio_read_bh, acb);
+        ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
         if (ret < 0) {
             goto done;
         }
@@ -630,11 +639,23 @@ static BlockDriverAIOCB *vdi_aio_readv(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     VdiAIOCB *acb;
+    int ret;
+
     logout("\n");
     acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
     if (!acb) {
         return NULL;
     }
+
+    ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
+    if (ret < 0) {
+        if (acb->qiov->niov > 1) {
+            qemu_vfree(acb->orig_buf);
+        }
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     vdi_aio_read_cb(acb, 0);
     return &acb->common;
 }
@@ -789,12 +810,23 @@ static BlockDriverAIOCB *vdi_aio_writev(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     VdiAIOCB *acb;
+    int ret;
+
     logout("\n");
     acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
     if (!acb) {
         return NULL;
     }
-    vdi_aio_write_cb(acb, 0);
+
+    ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
+    if (ret < 0) {
+        if (acb->qiov->niov > 1) {
+            qemu_vfree(acb->orig_buf);
+        }
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
-- 
1.7.5.2

Re: [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback

[Luiz Capitulino]

On Tue,  7 Jun 2011 16:18:30 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> This series fixes some cases of block drivers calling AIO callbacks too early.
> It fixes the IDE assertion failure reported by Luiz (in error cases, the DMA
> status, including acb, could first be reset in the callback and only then be
> set by the caller, resulting in a dangling acb and wrong status register value).

This fixes the reported bug, thanks.

I know this is a different subject, but I'm still unable to use the host cdrom
if the -snapshot flag is passed, I think the idea of ignoring the flag for a
read-only device would fix this, no?

> 
> Kevin Wolf (3):
>   qcow2: Avoid direct AIO callback
>   qcow: Avoid direct AIO callback
>   vdi: Avoid direct AIO callback
> 
>  block/qcow.c  |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  block/qcow2.c |   39 +++++++++++++++++++++++++++++--------
>  block/vdi.c   |   40 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 122 insertions(+), 15 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback

[Kevin Wolf]

Am 10.06.2011 17:32, schrieb Luiz Capitulino:
> On Tue,  7 Jun 2011 16:18:30 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> This series fixes some cases of block drivers calling AIO callbacks too early.
>> It fixes the IDE assertion failure reported by Luiz (in error cases, the DMA
>> status, including acb, could first be reset in the callback and only then be
>> set by the caller, resulting in a dangling acb and wrong status register value).
> 
> This fixes the reported bug, thanks.
> 
> I know this is a different subject, but I'm still unable to use the host cdrom
> if the -snapshot flag is passed, I think the idea of ignoring the flag for a
> read-only device would fix this, no?

Yes, it would. But as we discussed it would have other implications that
I wouldn't feel comfortable about (surprising semantics of 'commit'
would be one).

Passing things like bdrv_eject() or bdrv_is_inserted() to the backing
file still sounds like a cleaner approach, but probably isn't as easy.

Kevin

[Qemu-devel] [PATCH v2 3/3] vdi: Avoid direct AIO callback

[Kevin Wolf]

bdrv_aio_* must not call the callback before returning to its caller. In vdi,
this could happen in some error cases. This starts the real requests processing
in a BH to avoid this situation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

v2:
- Remove direct vdi_aio_read_cb() call

 block/vdi.c |   41 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 4c9e201..261cf9b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -152,6 +152,7 @@ typedef struct {
     /* Buffer for new allocated block. */
     void *block_buffer;
     void *orig_buf;
+    bool is_write;
     int header_modified;
     BlockDriverAIOCB *hd_aiocb;
     struct iovec hd_iov;
@@ -504,6 +505,8 @@ static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
         acb->hd_aiocb = NULL;
         acb->sector_num = sector_num;
         acb->qiov = qiov;
+        acb->is_write = is_write;
+
         if (qiov->niov > 1) {
             acb->buf = qemu_blockalign(bs, qiov->size);
             acb->orig_buf = acb->buf;
@@ -542,14 +545,20 @@ static int vdi_schedule_bh(QEMUBHFunc *cb, VdiAIOCB *acb)
 }
 
 static void vdi_aio_read_cb(void *opaque, int ret);
+static void vdi_aio_write_cb(void *opaque, int ret);
 
-static void vdi_aio_read_bh(void *opaque)
+static void vdi_aio_rw_bh(void *opaque)
 {
     VdiAIOCB *acb = opaque;
     logout("\n");
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
-    vdi_aio_read_cb(opaque, 0);
+
+    if (acb->is_write) {
+        vdi_aio_write_cb(opaque, 0);
+    } else {
+        vdi_aio_read_cb(opaque, 0);
+    }
 }
 
 static void vdi_aio_read_cb(void *opaque, int ret)
@@ -597,7 +606,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
     if (bmap_entry == VDI_UNALLOCATED) {
         /* Block not allocated, return zeros, no need to wait. */
         memset(acb->buf, 0, n_sectors * SECTOR_SIZE);
-        ret = vdi_schedule_bh(vdi_aio_read_bh, acb);
+        ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
         if (ret < 0) {
             goto done;
         }
@@ -630,12 +639,23 @@ static BlockDriverAIOCB *vdi_aio_readv(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     VdiAIOCB *acb;
+    int ret;
+
     logout("\n");
     acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
     if (!acb) {
         return NULL;
     }
-    vdi_aio_read_cb(acb, 0);
+
+    ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
+    if (ret < 0) {
+        if (acb->qiov->niov > 1) {
+            qemu_vfree(acb->orig_buf);
+        }
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
@@ -789,12 +809,23 @@ static BlockDriverAIOCB *vdi_aio_writev(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
     VdiAIOCB *acb;
+    int ret;
+
     logout("\n");
     acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
     if (!acb) {
         return NULL;
     }
-    vdi_aio_write_cb(acb, 0);
+
+    ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
+    if (ret < 0) {
+        if (acb->qiov->niov > 1) {
+            qemu_vfree(acb->orig_buf);
+        }
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
     return &acb->common;
 }
 
-- 
1.7.5.2