* [Qemu-devel] block/curl: Remedy a crashing bug completing AIOCBs from cache
@ 2011-09-21 10:55 ` nick
0 siblings, 0 replies; 8+ messages in thread
From: nick @ 2011-09-21 10:55 UTC (permalink / raw)
To: nick, qemu-devel; +Cc: qemu-trivial
In QEMU master, attempting to read a cached block from a HTTP (or otherwise)
mounted ISO causes an assert to be triggered, killing the entire QEMU process.
It looks like this:
hw/ide/pci.c:314: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb ==
((void *)0)' failed.
The following two patches add flush capability to the curl backend, and avoid
triggering the assert by finishing the AIOCB in a QEMUBH callback, rather than
directly in curl_aio_readv().
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-trivial] [PATCH 1/2] block/curl: Implement a flush function on the fd handlers
2011-09-21 10:55 ` [Qemu-devel] " nick
@ 2011-09-21 10:55 ` nick
-1 siblings, 0 replies; 8+ messages in thread
From: nick @ 2011-09-21 10:55 UTC (permalink / raw)
To: nick, qemu-devel; +Cc: qemu-trivial
From: Nick Thomas <nick@bytemark.co.uk>
Signed-off-by: Nick Thomas <nick@bytemark.co.uk>
---
block/curl.c | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index f3f61cc..21fed93 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -76,6 +76,7 @@ typedef struct BDRVCURLState {
static void curl_clean_state(CURLState *s);
static void curl_multi_do(void *arg);
+static int curl_aio_flush(void *opaque);
static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
void *s, void *sp)
@@ -83,14 +84,16 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, fd);
switch (action) {
case CURL_POLL_IN:
- qemu_aio_set_fd_handler(fd, curl_multi_do, NULL, NULL, NULL, s);
+ qemu_aio_set_fd_handler(fd, curl_multi_do, NULL, curl_aio_flush,
+ NULL, s);
break;
case CURL_POLL_OUT:
- qemu_aio_set_fd_handler(fd, NULL, curl_multi_do, NULL, NULL, s);
+ qemu_aio_set_fd_handler(fd, NULL, curl_multi_do, curl_aio_flush,
+ NULL, s);
break;
case CURL_POLL_INOUT:
- qemu_aio_set_fd_handler(fd, curl_multi_do,
- curl_multi_do, NULL, NULL, s);
+ qemu_aio_set_fd_handler(fd, curl_multi_do, curl_multi_do,
+ curl_aio_flush, NULL, s);
break;
case CURL_POLL_REMOVE:
qemu_aio_set_fd_handler(fd, NULL, NULL, NULL, NULL, NULL);
@@ -412,6 +415,21 @@ out_noclean:
return -EINVAL;
}
+static int curl_aio_flush(void *opaque)
+{
+ BDRVCURLState *s = opaque;
+ int i, j;
+
+ for (i=0; i < CURL_NUM_STATES; i++) {
+ for(j=0; j < CURL_NUM_ACB; j++) {
+ if (s->states[i].acb[j]) {
+ return 1;
+ }
+ }
+ }
+ return 0;
+}
+
static void curl_aio_cancel(BlockDriverAIOCB *blockacb)
{
// Do we have to implement canceling? Seems to work without...
--
1.7.0.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 1/2] block/curl: Implement a flush function on the fd handlers
@ 2011-09-21 10:55 ` nick
0 siblings, 0 replies; 8+ messages in thread
From: nick @ 2011-09-21 10:55 UTC (permalink / raw)
To: nick, qemu-devel; +Cc: qemu-trivial
From: Nick Thomas <nick@bytemark.co.uk>
Signed-off-by: Nick Thomas <nick@bytemark.co.uk>
---
block/curl.c | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index f3f61cc..21fed93 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -76,6 +76,7 @@ typedef struct BDRVCURLState {
static void curl_clean_state(CURLState *s);
static void curl_multi_do(void *arg);
+static int curl_aio_flush(void *opaque);
static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
void *s, void *sp)
@@ -83,14 +84,16 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, fd);
switch (action) {
case CURL_POLL_IN:
- qemu_aio_set_fd_handler(fd, curl_multi_do, NULL, NULL, NULL, s);
+ qemu_aio_set_fd_handler(fd, curl_multi_do, NULL, curl_aio_flush,
+ NULL, s);
break;
case CURL_POLL_OUT:
- qemu_aio_set_fd_handler(fd, NULL, curl_multi_do, NULL, NULL, s);
+ qemu_aio_set_fd_handler(fd, NULL, curl_multi_do, curl_aio_flush,
+ NULL, s);
break;
case CURL_POLL_INOUT:
- qemu_aio_set_fd_handler(fd, curl_multi_do,
- curl_multi_do, NULL, NULL, s);
+ qemu_aio_set_fd_handler(fd, curl_multi_do, curl_multi_do,
+ curl_aio_flush, NULL, s);
break;
case CURL_POLL_REMOVE:
qemu_aio_set_fd_handler(fd, NULL, NULL, NULL, NULL, NULL);
@@ -412,6 +415,21 @@ out_noclean:
return -EINVAL;
}
+static int curl_aio_flush(void *opaque)
+{
+ BDRVCURLState *s = opaque;
+ int i, j;
+
+ for (i=0; i < CURL_NUM_STATES; i++) {
+ for(j=0; j < CURL_NUM_ACB; j++) {
+ if (s->states[i].acb[j]) {
+ return 1;
+ }
+ }
+ }
+ return 0;
+}
+
static void curl_aio_cancel(BlockDriverAIOCB *blockacb)
{
// Do we have to implement canceling? Seems to work without...
--
1.7.0.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-trivial] [PATCH 2/2] block/curl: Don't finish AIOCBs too early
2011-09-21 10:55 ` [Qemu-devel] " nick
@ 2011-09-21 10:55 ` nick
-1 siblings, 0 replies; 8+ messages in thread
From: nick @ 2011-09-21 10:55 UTC (permalink / raw)
To: nick, qemu-devel; +Cc: qemu-trivial
From: Nick Thomas <nick@bytemark.co.uk>
The previous behaviour was to finish AIOCBs inside curl_aio_readv()
if the data was cached. This caused the following failed assertion
at hw/ide/pci.c:314: bmdma_cmd_writeb
"Assertion `bm->bus->dma->aiocb == ((void *)0)' failed."
By scheduling a QEMUBH and performing the completion inside the
callback, we avoid this problem.
Signed-off-by: Nick Thomas <nick@bytemark.co.uk>
---
block/curl.c | 68 +++++++++++++++++++++++++++++++++++++++++----------------
1 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 21fed93..4209ac8 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -47,7 +47,12 @@ struct BDRVCURLState;
typedef struct CURLAIOCB {
BlockDriverAIOCB common;
+ QEMUBH *bh;
QEMUIOVector *qiov;
+
+ int64_t sector_num;
+ int nb_sectors;
+
size_t start;
size_t end;
} CURLAIOCB;
@@ -440,43 +445,42 @@ static AIOPool curl_aio_pool = {
.cancel = curl_aio_cancel,
};
-static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
+
+static void curl_readv_bh_cb(void *p)
{
- BDRVCURLState *s = bs->opaque;
- CURLAIOCB *acb;
- size_t start = sector_num * SECTOR_SIZE;
- size_t end;
CURLState *state;
- acb = qemu_aio_get(&curl_aio_pool, bs, cb, opaque);
- if (!acb)
- return NULL;
+ CURLAIOCB *acb = p;
+ BDRVCURLState *s = acb->common.bs->opaque;
- acb->qiov = qiov;
+ qemu_bh_delete(acb->bh);
+ acb->bh = NULL;
+
+ size_t start = acb->sector_num * SECTOR_SIZE;
+ size_t end;
// In case we have the requested data already (e.g. read-ahead),
// we can just call the callback and be done.
-
- switch (curl_find_buf(s, start, nb_sectors * SECTOR_SIZE, acb)) {
+ switch (curl_find_buf(s, start, acb->nb_sectors * SECTOR_SIZE, acb)) {
case FIND_RET_OK:
qemu_aio_release(acb);
// fall through
case FIND_RET_WAIT:
- return &acb->common;
+ return;
default:
break;
}
// No cache found, so let's start a new request
-
state = curl_init_state(s);
- if (!state)
- return NULL;
+ if (!state) {
+ acb->common.cb(acb->common.opaque, -EIO);
+ qemu_aio_release(acb);
+ return;
+ }
acb->start = 0;
- acb->end = (nb_sectors * SECTOR_SIZE);
+ acb->end = (acb->nb_sectors * SECTOR_SIZE);
state->buf_off = 0;
if (state->orig_buf)
@@ -489,12 +493,38 @@ static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
snprintf(state->range, 127, "%zd-%zd", start, end);
DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
- (nb_sectors * SECTOR_SIZE), start, state->range);
+ (acb->nb_sectors * SECTOR_SIZE), start, state->range);
curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
curl_multi_add_handle(s->multi, state->curl);
curl_multi_do(s);
+}
+
+static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
+ int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+ BlockDriverCompletionFunc *cb, void *opaque)
+{
+ CURLAIOCB *acb;
+
+ acb = qemu_aio_get(&curl_aio_pool, bs, cb, opaque);
+
+ if (!acb) {
+ return NULL;
+ }
+
+ acb->qiov = qiov;
+ acb->sector_num = sector_num;
+ acb->nb_sectors = nb_sectors;
+
+ acb->bh = qemu_bh_new(curl_readv_bh_cb, acb);
+
+ if (!acb->bh) {
+ DPRINTF("CURL: qemu_bh_new failed\n");
+ return NULL;
+ }
+
+ qemu_bh_schedule(acb->bh);
return &acb->common;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 2/2] block/curl: Don't finish AIOCBs too early
@ 2011-09-21 10:55 ` nick
0 siblings, 0 replies; 8+ messages in thread
From: nick @ 2011-09-21 10:55 UTC (permalink / raw)
To: nick, qemu-devel; +Cc: qemu-trivial
From: Nick Thomas <nick@bytemark.co.uk>
The previous behaviour was to finish AIOCBs inside curl_aio_readv()
if the data was cached. This caused the following failed assertion
at hw/ide/pci.c:314: bmdma_cmd_writeb
"Assertion `bm->bus->dma->aiocb == ((void *)0)' failed."
By scheduling a QEMUBH and performing the completion inside the
callback, we avoid this problem.
Signed-off-by: Nick Thomas <nick@bytemark.co.uk>
---
block/curl.c | 68 +++++++++++++++++++++++++++++++++++++++++----------------
1 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 21fed93..4209ac8 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -47,7 +47,12 @@ struct BDRVCURLState;
typedef struct CURLAIOCB {
BlockDriverAIOCB common;
+ QEMUBH *bh;
QEMUIOVector *qiov;
+
+ int64_t sector_num;
+ int nb_sectors;
+
size_t start;
size_t end;
} CURLAIOCB;
@@ -440,43 +445,42 @@ static AIOPool curl_aio_pool = {
.cancel = curl_aio_cancel,
};
-static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
+
+static void curl_readv_bh_cb(void *p)
{
- BDRVCURLState *s = bs->opaque;
- CURLAIOCB *acb;
- size_t start = sector_num * SECTOR_SIZE;
- size_t end;
CURLState *state;
- acb = qemu_aio_get(&curl_aio_pool, bs, cb, opaque);
- if (!acb)
- return NULL;
+ CURLAIOCB *acb = p;
+ BDRVCURLState *s = acb->common.bs->opaque;
- acb->qiov = qiov;
+ qemu_bh_delete(acb->bh);
+ acb->bh = NULL;
+
+ size_t start = acb->sector_num * SECTOR_SIZE;
+ size_t end;
// In case we have the requested data already (e.g. read-ahead),
// we can just call the callback and be done.
-
- switch (curl_find_buf(s, start, nb_sectors * SECTOR_SIZE, acb)) {
+ switch (curl_find_buf(s, start, acb->nb_sectors * SECTOR_SIZE, acb)) {
case FIND_RET_OK:
qemu_aio_release(acb);
// fall through
case FIND_RET_WAIT:
- return &acb->common;
+ return;
default:
break;
}
// No cache found, so let's start a new request
-
state = curl_init_state(s);
- if (!state)
- return NULL;
+ if (!state) {
+ acb->common.cb(acb->common.opaque, -EIO);
+ qemu_aio_release(acb);
+ return;
+ }
acb->start = 0;
- acb->end = (nb_sectors * SECTOR_SIZE);
+ acb->end = (acb->nb_sectors * SECTOR_SIZE);
state->buf_off = 0;
if (state->orig_buf)
@@ -489,12 +493,38 @@ static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
snprintf(state->range, 127, "%zd-%zd", start, end);
DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
- (nb_sectors * SECTOR_SIZE), start, state->range);
+ (acb->nb_sectors * SECTOR_SIZE), start, state->range);
curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
curl_multi_add_handle(s->multi, state->curl);
curl_multi_do(s);
+}
+
+static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
+ int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+ BlockDriverCompletionFunc *cb, void *opaque)
+{
+ CURLAIOCB *acb;
+
+ acb = qemu_aio_get(&curl_aio_pool, bs, cb, opaque);
+
+ if (!acb) {
+ return NULL;
+ }
+
+ acb->qiov = qiov;
+ acb->sector_num = sector_num;
+ acb->nb_sectors = nb_sectors;
+
+ acb->bh = qemu_bh_new(curl_readv_bh_cb, acb);
+
+ if (!acb->bh) {
+ DPRINTF("CURL: qemu_bh_new failed\n");
+ return NULL;
+ }
+
+ qemu_bh_schedule(acb->bh);
return &acb->common;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] block/curl: Remedy a crashing bug completing AIOCBs from cache
2011-09-21 10:55 ` [Qemu-devel] " nick
@ 2011-09-21 13:32 ` Kevin Wolf
-1 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2011-09-21 13:32 UTC (permalink / raw)
To: nick; +Cc: qemu-trivial, qemu-devel
Am 21.09.2011 12:55, schrieb nick@bytemark.co.uk:
> In QEMU master, attempting to read a cached block from a HTTP (or otherwise)
> mounted ISO causes an assert to be triggered, killing the entire QEMU process.
> It looks like this:
>
> hw/ide/pci.c:314: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb ==
> ((void *)0)' failed.
>
> The following two patches add flush capability to the curl backend, and avoid
> triggering the assert by finishing the AIOCB in a QEMUBH callback, rather than
> directly in curl_aio_readv().
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] block/curl: Remedy a crashing bug completing AIOCBs from cache
@ 2011-09-21 13:32 ` Kevin Wolf
0 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2011-09-21 13:32 UTC (permalink / raw)
To: nick; +Cc: qemu-trivial, qemu-devel
Am 21.09.2011 12:55, schrieb nick@bytemark.co.uk:
> In QEMU master, attempting to read a cached block from a HTTP (or otherwise)
> mounted ISO causes an assert to be triggered, killing the entire QEMU process.
> It looks like this:
>
> hw/ide/pci.c:314: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb ==
> ((void *)0)' failed.
>
> The following two patches add flush capability to the curl backend, and avoid
> triggering the assert by finishing the AIOCB in a QEMUBH callback, rather than
> directly in curl_aio_readv().
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread