* [Qemu-devel] [PATCH 1/3] qcow2: Avoid direct AIO callback
2011-06-07 14:18 [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback Kevin Wolf
@ 2011-06-07 14:18 ` Kevin Wolf
2011-06-07 14:18 ` [Qemu-devel] [PATCH 2/3] qcow: " Kevin Wolf
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2011-06-07 14:18 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, lcapitulino
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH 2/3] qcow: Avoid direct AIO callback
2011-06-07 14:18 [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback Kevin Wolf
2011-06-07 14:18 ` [Qemu-devel] [PATCH 1/3] qcow2: " Kevin Wolf
@ 2011-06-07 14:18 ` Kevin Wolf
2011-06-07 14:18 ` [Qemu-devel] [PATCH 3/3] vdi: " Kevin Wolf
2011-06-10 15:32 ` [Qemu-devel] [PATCH 0/3] block: " Luiz Capitulino
3 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2011-06-07 14:18 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, lcapitulino
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH 3/3] vdi: Avoid direct AIO callback
2011-06-07 14:18 [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback Kevin Wolf
2011-06-07 14:18 ` [Qemu-devel] [PATCH 1/3] qcow2: " Kevin Wolf
2011-06-07 14:18 ` [Qemu-devel] [PATCH 2/3] qcow: " Kevin Wolf
@ 2011-06-07 14:18 ` Kevin Wolf
2011-06-15 12:39 ` [Qemu-devel] [PATCH v2 " Kevin Wolf
2011-06-10 15:32 ` [Qemu-devel] [PATCH 0/3] block: " Luiz Capitulino
3 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2011-06-07 14:18 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, lcapitulino
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH v2 3/3] vdi: Avoid direct AIO callback
2011-06-07 14:18 ` [Qemu-devel] [PATCH 3/3] vdi: " Kevin Wolf
@ 2011-06-15 12:39 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2011-06-15 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback
2011-06-07 14:18 [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback Kevin Wolf
` (2 preceding siblings ...)
2011-06-07 14:18 ` [Qemu-devel] [PATCH 3/3] vdi: " Kevin Wolf
@ 2011-06-10 15:32 ` Luiz Capitulino
2011-06-10 15:57 ` Kevin Wolf
3 siblings, 1 reply; 7+ messages in thread
From: Luiz Capitulino @ 2011-06-10 15:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel
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(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 0/3] block: Avoid direct AIO callback
2011-06-10 15:32 ` [Qemu-devel] [PATCH 0/3] block: " Luiz Capitulino
@ 2011-06-10 15:57 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2011-06-10 15:57 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread