* [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
@ 2017-05-10 14:31 Paolo Bonzini
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
` (8 more replies)
0 siblings, 9 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
Since the last patch in v1 didn't work, I bit the bullet and converted
the whole thing to coroutines (patches 4-6). This in turns allows a more
elegant solution to wait for CURLStates to get free (patch 7).
I tested this by lowering CURL_NUM_STATES to 2. With this change, the
buggy case triggers a couple times while booting a Fedora netinst image.
Paolo
Paolo Bonzini (7):
curl: strengthen assertion in curl_clean_state
curl: never invoke callbacks with s->mutex held
curl: avoid recursive locking of BDRVCURLState mutex
curl: split curl_find_state/curl_init_state
curl: convert CURLAIOCB to byte values
curl: convert readv to coroutines
curl: do not do aio_poll when waiting for a free CURLState
block/curl.c | 216 +++++++++++++++++++++++++---------------------
1 files changed, 120 insertions(+), 96 deletions(-)
--
2.12.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
@ 2017-05-10 14:31 ` Paolo Bonzini
2017-05-10 16:33 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:35 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
` (7 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
curl_clean_state should only be called after all AIOCBs have been
completed. This is not so obvious for the call from curl_detach_aio_context,
so assert that.
Cc: qemu-stable@nongnu.org
Cc: jcody@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/block/curl.c b/block/curl.c
index 2708d57c2f..25a301e7b4 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -532,6 +532,11 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
static void curl_clean_state(CURLState *s)
{
+ int j;
+ for (j=0; j<CURL_NUM_ACB; j++) {
+ assert(!s->acb[j]);
+ }
+
if (s->s->multi)
curl_multi_remove_handle(s->s->multi, s->curl);
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
@ 2017-05-10 14:32 ` Paolo Bonzini
2017-05-10 16:33 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:40 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
` (6 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
All curl callbacks go through curl_multi_do, and hence are called with
s->mutex held. Note that with comments, and make curl_read_cb drop the
lock before invoking the callback.
Likewise for curl_find_buf, where the callback can be invoked by the
caller.
Cc: qemu-stable@nongnu.org
Cc: jcody@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 25a301e7b4..9a00fdc28e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -147,6 +147,7 @@ static void curl_multi_do(void *arg);
static void curl_multi_read(void *arg);
#ifdef NEED_CURL_TIMER_CALLBACK
+/* Called from curl_multi_do_locked, with s->mutex held. */
static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
{
BDRVCURLState *s = opaque;
@@ -163,6 +164,7 @@ static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
}
#endif
+/* Called from curl_multi_do_locked, with s->mutex held. */
static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
void *userp, void *sp)
{
@@ -212,6 +214,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
return 0;
}
+/* Called from curl_multi_do_locked, with s->mutex held. */
static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
{
BDRVCURLState *s = opaque;
@@ -226,6 +229,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
return realsize;
}
+/* Called from curl_multi_do_locked, with s->mutex held. */
static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
{
CURLState *s = ((CURLState*)opaque);
@@ -264,7 +268,9 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
request_length - offset);
}
+ qemu_mutex_unlock(&s->s->mutex);
acb->common.cb(acb->common.opaque, 0);
+ qemu_mutex_lock(&s->s->mutex);
qemu_aio_unref(acb);
s->acb[i] = NULL;
}
@@ -305,8 +311,6 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
if (clamped_len < len) {
qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
}
- acb->common.cb(acb->common.opaque, 0);
-
return FIND_RET_OK;
}
@@ -832,8 +836,8 @@ static void curl_readv_bh_cb(void *p)
// we can just call the callback and be done.
switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
case FIND_RET_OK:
- qemu_aio_unref(acb);
- // fall through
+ ret = 0;
+ goto out;
case FIND_RET_WAIT:
goto out;
default:
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
2017-05-10 14:32 ` [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
@ 2017-05-10 14:32 ` Paolo Bonzini
2017-05-10 16:38 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:56 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
` (5 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
The curl driver has a ugly hack where, if it cannot find an empty CURLState,
it just uses aio_poll to wait for one to be empty. This is probably
buggy when used together with dataplane, and the simplest way to fix it
is to use coroutines instead.
A more immediate effect of the bug however is that it can cause a
recursive call to curl_readv_bh_cb and recursively taking the
BDRVCURLState mutex. This causes a deadlock.
The fix is to unlock the mutex around aio_poll, but for cleanliness we
should also take the mutex around all calls to curl_init_state, even if
reaching the unlock/lock pair is impossible. The same is true for
curl_clean_state.
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Cc: jcody@redhat.com
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/block/curl.c b/block/curl.c
index 9a00fdc28e..b18e79bf54 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -281,6 +281,7 @@ read_end:
return size * nmemb;
}
+/* Called with s->mutex held. */
static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
CURLAIOCB *acb)
{
@@ -453,6 +454,7 @@ static void curl_multi_timeout_do(void *arg)
#endif
}
+/* Called with s->mutex held. */
static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
{
CURLState *state = NULL;
@@ -471,7 +473,9 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
break;
}
if (!state) {
+ qemu_mutex_unlock(&s->mutex);
aio_poll(bdrv_get_aio_context(bs), true);
+ qemu_mutex_lock(&s->mutex);
}
} while(!state);
@@ -534,6 +538,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
return state;
}
+/* Called with s->mutex held. */
static void curl_clean_state(CURLState *s)
{
int j;
@@ -565,6 +570,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
BDRVCURLState *s = bs->opaque;
int i;
+ qemu_mutex_lock(&s->mutex);
for (i = 0; i < CURL_NUM_STATES; i++) {
if (s->states[i].in_use) {
curl_clean_state(&s->states[i]);
@@ -580,6 +586,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
curl_multi_cleanup(s->multi);
s->multi = NULL;
}
+ qemu_mutex_unlock(&s->mutex);
timer_del(&s->timer);
}
@@ -745,9 +752,12 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
DPRINTF("CURL: Opening %s\n", file);
+ qemu_mutex_init(&s->mutex);
s->aio_context = bdrv_get_aio_context(bs);
s->url = g_strdup(file);
+ qemu_mutex_lock(&s->mutex);
state = curl_init_state(bs, s);
+ qemu_mutex_unlock(&s->mutex);
if (!state)
goto out_noclean;
@@ -791,11 +801,12 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
DPRINTF("CURL: Size = %zd\n", s->len);
+ qemu_mutex_lock(&s->mutex);
curl_clean_state(state);
+ qemu_mutex_unlock(&s->mutex);
curl_easy_cleanup(state->curl);
state->curl = NULL;
- qemu_mutex_init(&s->mutex);
curl_attach_aio_context(bs, bdrv_get_aio_context(bs));
qemu_opts_del(opts);
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (2 preceding siblings ...)
2017-05-10 14:32 ` [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
@ 2017-05-10 14:32 ` Paolo Bonzini
2017-05-10 17:26 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-12 21:38 ` Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
` (4 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
If curl_easy_init fails, a CURLState is left with s->in_use = 1. Split
curl_init_state in two, so that we can distinguish the two failures and
call curl_clean_state if needed.
While at it, simplify curl_find_state, removing a dummy loop. The
aio_poll loop is moved to the sole caller that needs it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index b18e79bf54..4b4d5a2389 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -455,34 +455,27 @@ static void curl_multi_timeout_do(void *arg)
}
/* Called with s->mutex held. */
-static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
+static CURLState *curl_find_state(BDRVCURLState *s)
{
CURLState *state = NULL;
- int i, j;
-
- do {
- for (i=0; i<CURL_NUM_STATES; i++) {
- for (j=0; j<CURL_NUM_ACB; j++)
- if (s->states[i].acb[j])
- continue;
- if (s->states[i].in_use)
- continue;
+ int i;
+ for (i=0; i<CURL_NUM_STATES; i++) {
+ if (!s->states[i].in_use) {
state = &s->states[i];
state->in_use = 1;
break;
}
- if (!state) {
- qemu_mutex_unlock(&s->mutex);
- aio_poll(bdrv_get_aio_context(bs), true);
- qemu_mutex_lock(&s->mutex);
- }
- } while(!state);
+ }
+ return state;
+}
+static int curl_init_state(BDRVCURLState *s, CURLState *state)
+{
if (!state->curl) {
state->curl = curl_easy_init();
if (!state->curl) {
- return NULL;
+ return -EIO;
}
curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
@@ -535,7 +528,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
QLIST_INIT(&state->sockets);
state->s = s;
- return state;
+ return 0;
}
/* Called with s->mutex held. */
@@ -756,13 +749,18 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
s->aio_context = bdrv_get_aio_context(bs);
s->url = g_strdup(file);
qemu_mutex_lock(&s->mutex);
- state = curl_init_state(bs, s);
+ state = curl_find_state(s);
qemu_mutex_unlock(&s->mutex);
- if (!state)
+ if (!state) {
goto out_noclean;
+ }
// Get file size
+ if (curl_init_state(s, state) < 0) {
+ goto out;
+ }
+
s->accept_range = false;
curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
@@ -856,8 +854,18 @@ static void curl_readv_bh_cb(void *p)
}
// No cache found, so let's start a new request
- state = curl_init_state(acb->common.bs, s);
- if (!state) {
+ for (;;) {
+ state = curl_find_state(s);
+ if (state) {
+ break;
+ }
+ qemu_mutex_unlock(&s->mutex);
+ aio_poll(bdrv_get_aio_context(bs), true);
+ qemu_mutex_lock(&s->mutex);
+ }
+
+ if (curl_init_state(s, state) < 0) {
+ curl_clean_state(state);
ret = -EIO;
goto out;
}
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (3 preceding siblings ...)
2017-05-10 14:32 ` [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
@ 2017-05-10 14:32 ` Paolo Bonzini
2017-05-10 17:36 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-12 21:38 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines Paolo Bonzini
` (3 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
This is in preparation for the conversion from bdrv_aio_readv to
bdrv_co_preadv, and it also requires changing some of the size_t values
to uint64_t. This was broken before for disks > 2TB, but now it would
break at 4GB.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 44 ++++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 4b4d5a2389..3e288f2bc7 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -96,8 +96,8 @@ typedef struct CURLAIOCB {
BlockAIOCB common;
QEMUIOVector *qiov;
- int64_t sector_num;
- int nb_sectors;
+ uint64_t offset;
+ uint64_t bytes;
size_t start;
size_t end;
@@ -115,7 +115,7 @@ typedef struct CURLState
CURL *curl;
QLIST_HEAD(, CURLSocket) sockets;
char *orig_buf;
- size_t buf_start;
+ uint64_t buf_start;
size_t buf_off;
size_t buf_len;
char range[128];
@@ -126,7 +126,7 @@ typedef struct CURLState
typedef struct BDRVCURLState {
CURLM *multi;
QEMUTimer timer;
- size_t len;
+ uint64_t len;
CURLState states[CURL_NUM_STATES];
char *url;
size_t readahead_size;
@@ -257,7 +257,7 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
continue;
if ((s->buf_off >= acb->end)) {
- size_t request_length = acb->nb_sectors * BDRV_SECTOR_SIZE;
+ size_t request_length = acb->bytes;
qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
acb->end - acb->start);
@@ -282,18 +282,18 @@ read_end:
}
/* Called with s->mutex held. */
-static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
+static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
CURLAIOCB *acb)
{
int i;
- size_t end = start + len;
- size_t clamped_end = MIN(end, s->len);
- size_t clamped_len = clamped_end - start;
+ uint64_t end = start + len;
+ uint64_t clamped_end = MIN(end, s->len);
+ uint64_t clamped_len = clamped_end - start;
for (i=0; i<CURL_NUM_STATES; i++) {
CURLState *state = &s->states[i];
- size_t buf_end = (state->buf_start + state->buf_off);
- size_t buf_fend = (state->buf_start + state->buf_len);
+ uint64_t buf_end = (state->buf_start + state->buf_off);
+ uint64_t buf_fend = (state->buf_start + state->buf_len);
if (!state->orig_buf)
continue;
@@ -788,7 +788,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
#endif
- s->len = (size_t)d;
+ s->len = d;
if ((!strncasecmp(s->url, "http://", strlen("http://"))
|| !strncasecmp(s->url, "https://", strlen("https://")))
@@ -797,7 +797,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
"Server does not support 'range' (byte ranges).");
goto out;
}
- DPRINTF("CURL: Size = %zd\n", s->len);
+ DPRINTF("CURL: Size = %" PRIu64 "\n", s->len);
qemu_mutex_lock(&s->mutex);
curl_clean_state(state);
@@ -836,14 +836,14 @@ static void curl_readv_bh_cb(void *p)
BlockDriverState *bs = acb->common.bs;
BDRVCURLState *s = bs->opaque;
- size_t start = acb->sector_num * BDRV_SECTOR_SIZE;
- size_t end;
+ uint64_t start = acb->offset;
+ uint64_t end;
qemu_mutex_lock(&s->mutex);
// 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, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
+ switch (curl_find_buf(s, start, acb->bytes, acb)) {
case FIND_RET_OK:
ret = 0;
goto out;
@@ -871,7 +871,7 @@ static void curl_readv_bh_cb(void *p)
}
acb->start = 0;
- acb->end = MIN(acb->nb_sectors * BDRV_SECTOR_SIZE, s->len - start);
+ acb->end = MIN(acb->bytes, s->len - start);
state->buf_off = 0;
g_free(state->orig_buf);
@@ -886,9 +886,9 @@ static void curl_readv_bh_cb(void *p)
}
state->acb[0] = acb;
- snprintf(state->range, 127, "%zd-%zd", start, end);
- DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
- (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);
+ snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
+ DPRINTF("CURL (AIO): Reading %" PRIu64 " at %" PRIu64 " (%s)\n",
+ acb->bytes, start, state->range);
curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
curl_multi_add_handle(s->multi, state->curl);
@@ -913,8 +913,8 @@ static BlockAIOCB *curl_aio_readv(BlockDriverState *bs,
acb = qemu_aio_get(&curl_aiocb_info, bs, cb, opaque);
acb->qiov = qiov;
- acb->sector_num = sector_num;
- acb->nb_sectors = nb_sectors;
+ acb->offset = sector_num * BDRV_SECTOR_SIZE;
+ acb->bytes = nb_sectors * BDRV_SECTOR_SIZE;
aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), curl_readv_bh_cb, acb);
return &acb->common;
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (4 preceding siblings ...)
2017-05-10 14:32 ` [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
@ 2017-05-10 14:32 ` Paolo Bonzini
2017-05-12 21:40 ` Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
` (2 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
This is pretty simple. The bottom half goes away because, unlike
bdrv_aio_readv, coroutine-based read can return immediately without
yielding. However, for simplicity I kept the former bottom half
handler in a separate function.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 94 ++++++++++++++++++++++++------------------------------------
1 file changed, 38 insertions(+), 56 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 3e288f2bc7..80870bd60c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -76,10 +76,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
#define CURL_TIMEOUT_DEFAULT 5
#define CURL_TIMEOUT_MAX 10000
-#define FIND_RET_NONE 0
-#define FIND_RET_OK 1
-#define FIND_RET_WAIT 2
-
#define CURL_BLOCK_OPT_URL "url"
#define CURL_BLOCK_OPT_READAHEAD "readahead"
#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
@@ -93,11 +89,12 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
struct BDRVCURLState;
typedef struct CURLAIOCB {
- BlockAIOCB common;
+ Coroutine *co;
QEMUIOVector *qiov;
uint64_t offset;
uint64_t bytes;
+ int ret;
size_t start;
size_t end;
@@ -268,11 +265,11 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
request_length - offset);
}
+ acb->ret = 0;
+ s->acb[i] = NULL;
qemu_mutex_unlock(&s->s->mutex);
- acb->common.cb(acb->common.opaque, 0);
+ aio_co_wake(acb->co);
qemu_mutex_lock(&s->s->mutex);
- qemu_aio_unref(acb);
- s->acb[i] = NULL;
}
}
@@ -282,8 +279,8 @@ read_end:
}
/* Called with s->mutex held. */
-static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
- CURLAIOCB *acb)
+static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
+ CURLAIOCB *acb)
{
int i;
uint64_t end = start + len;
@@ -312,7 +309,8 @@ static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
if (clamped_len < len) {
qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
}
- return FIND_RET_OK;
+ acb->ret = 0;
+ return true;
}
// Wait for unfinished chunks
@@ -330,13 +328,13 @@ static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
for (j=0; j<CURL_NUM_ACB; j++) {
if (!state->acb[j]) {
state->acb[j] = acb;
- return FIND_RET_WAIT;
+ return true;
}
}
}
}
- return FIND_RET_NONE;
+ return false;
}
/* Called with s->mutex held. */
@@ -381,11 +379,11 @@ static void curl_multi_check_completion(BDRVCURLState *s)
continue;
}
+ acb->ret = -EIO;
+ state->acb[i] = NULL;
qemu_mutex_unlock(&s->mutex);
- acb->common.cb(acb->common.opaque, -EIO);
+ aio_co_wake(acb->co);
qemu_mutex_lock(&s->mutex);
- qemu_aio_unref(acb);
- state->acb[i] = NULL;
}
}
@@ -821,19 +819,11 @@ out_noclean:
return -EINVAL;
}
-static const AIOCBInfo curl_aiocb_info = {
- .aiocb_size = sizeof(CURLAIOCB),
-};
-
-
-static void curl_readv_bh_cb(void *p)
+static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
{
CURLState *state;
int running;
- int ret = -EINPROGRESS;
- CURLAIOCB *acb = p;
- BlockDriverState *bs = acb->common.bs;
BDRVCURLState *s = bs->opaque;
uint64_t start = acb->offset;
@@ -843,14 +833,8 @@ static void curl_readv_bh_cb(void *p)
// 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, acb->bytes, acb)) {
- case FIND_RET_OK:
- ret = 0;
- goto out;
- case FIND_RET_WAIT:
- goto out;
- default:
- break;
+ if (curl_find_buf(s, start, acb->bytes, acb)) {
+ goto out;
}
// No cache found, so let's start a new request
@@ -866,7 +850,7 @@ static void curl_readv_bh_cb(void *p)
if (curl_init_state(s, state) < 0) {
curl_clean_state(state);
- ret = -EIO;
+ acb->ret = -EIO;
goto out;
}
@@ -881,7 +865,7 @@ static void curl_readv_bh_cb(void *p)
state->orig_buf = g_try_malloc(state->buf_len);
if (state->buf_len && state->orig_buf == NULL) {
curl_clean_state(state);
- ret = -ENOMEM;
+ acb->ret = -ENOMEM;
goto out;
}
state->acb[0] = acb;
@@ -898,26 +882,24 @@ static void curl_readv_bh_cb(void *p)
out:
qemu_mutex_unlock(&s->mutex);
- if (ret != -EINPROGRESS) {
- acb->common.cb(acb->common.opaque, ret);
- qemu_aio_unref(acb);
- }
}
-static BlockAIOCB *curl_aio_readv(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockCompletionFunc *cb, void *opaque)
+static int coroutine_fn curl_co_preadv(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
{
- CURLAIOCB *acb;
-
- acb = qemu_aio_get(&curl_aiocb_info, bs, cb, opaque);
-
- acb->qiov = qiov;
- acb->offset = sector_num * BDRV_SECTOR_SIZE;
- acb->bytes = nb_sectors * BDRV_SECTOR_SIZE;
-
- aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), curl_readv_bh_cb, acb);
- return &acb->common;
+ CURLAIOCB acb = {
+ .co = qemu_coroutine_self(),
+ .ret = -EINPROGRESS,
+ .qiov = qiov,
+ .offset = offset,
+ .bytes = bytes
+ };
+
+ curl_setup_preadv(bs, &acb);
+ while (acb.ret == -EINPROGRESS) {
+ qemu_coroutine_yield();
+ }
+ return acb.ret;
}
static void curl_close(BlockDriverState *bs)
@@ -948,7 +930,7 @@ static BlockDriver bdrv_http = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_co_preadv = curl_co_preadv,
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
@@ -964,7 +946,7 @@ static BlockDriver bdrv_https = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_co_preadv = curl_co_preadv,
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
@@ -980,7 +962,7 @@ static BlockDriver bdrv_ftp = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_co_preadv = curl_co_preadv,
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
@@ -996,7 +978,7 @@ static BlockDriver bdrv_ftps = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_co_preadv = curl_co_preadv,
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (5 preceding siblings ...)
2017-05-10 14:32 ` [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines Paolo Bonzini
@ 2017-05-10 14:32 ` Paolo Bonzini
2017-05-10 17:54 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-12 21:41 ` [Qemu-devel] " Jeff Cody
2017-05-10 15:11 ` [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
2017-05-10 15:57 ` Richard W.M. Jones
8 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-10 14:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, qemu-stable, qemu-block, rjones
Instead, put the CURLAIOCB on a wait list; curl_clean_state will
wake the corresponding coroutine.
Because of CURL's callback-based structure, we cannot easily convert
everything to CoMutex/CoQueue; keeping the QemuMutex is simpler.
However, CoQueue is a simple wrapper around a linked list, so we can
use QSIMPLEQ easily to open-code a CoQueue that has a QemuMutex's
protection instead of a CoMutex's.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/block/curl.c b/block/curl.c
index 80870bd60c..4ccdf63510 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -98,6 +98,8 @@ typedef struct CURLAIOCB {
size_t start;
size_t end;
+
+ QSIMPLEQ_ENTRY(CURLAIOCB) next;
} CURLAIOCB;
typedef struct CURLSocket {
@@ -133,6 +135,7 @@ typedef struct BDRVCURLState {
bool accept_range;
AioContext *aio_context;
QemuMutex mutex;
+ QSIMPLEQ_HEAD(, CURLAIOCB) free_state_waitq;
char *username;
char *password;
char *proxyusername;
@@ -532,6 +535,7 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
/* Called with s->mutex held. */
static void curl_clean_state(CURLState *s)
{
+ CURLAIOCB *next;
int j;
for (j=0; j<CURL_NUM_ACB; j++) {
assert(!s->acb[j]);
@@ -548,6 +552,14 @@ static void curl_clean_state(CURLState *s)
}
s->in_use = 0;
+
+ next = QSIMPLEQ_FIRST(&s->s->free_state_waitq);
+ if (next) {
+ QSIMPLEQ_REMOVE_HEAD(&s->s->free_state_waitq, next);
+ qemu_mutex_unlock(&s->s->mutex);
+ aio_co_wake(next->co);
+ qemu_mutex_lock(&s->s->mutex);
+ }
}
static void curl_parse_filename(const char *filename, QDict *options,
@@ -744,6 +756,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
DPRINTF("CURL: Opening %s\n", file);
qemu_mutex_init(&s->mutex);
+ QSIMPLEQ_INIT(&s->free_state_waitq);
s->aio_context = bdrv_get_aio_context(bs);
s->url = g_strdup(file);
qemu_mutex_lock(&s->mutex);
@@ -843,8 +856,9 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
if (state) {
break;
}
+ QSIMPLEQ_INSERT_TAIL(&s->free_state_waitq, acb, next);
qemu_mutex_unlock(&s->mutex);
- aio_poll(bdrv_get_aio_context(bs), true);
+ qemu_coroutine_yield();
qemu_mutex_lock(&s->mutex);
}
--
2.12.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (6 preceding siblings ...)
2017-05-10 14:32 ` [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
@ 2017-05-10 15:11 ` no-reply
2017-05-10 15:57 ` Richard W.M. Jones
8 siblings, 0 replies; 28+ messages in thread
From: no-reply @ 2017-05-10 15:11 UTC (permalink / raw)
To: pbonzini; +Cc: famz, qemu-devel, jcody, qemu-stable, qemu-block, rjones
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
Type: series
Message-id: 20170510143205.32013-1-pbonzini@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
a91e9d4 curl: do not do aio_poll when waiting for a free CURLState
6c92680 curl: convert readv to coroutines
84c046c curl: convert CURLAIOCB to byte values
ab431ec curl: split curl_find_state/curl_init_state
ba309a5 curl: avoid recursive locking of BDRVCURLState mutex
a6b6d90 curl: never invoke callbacks with s->mutex held
6f11502 curl: strengthen assertion in curl_clean_state
=== OUTPUT BEGIN ===
Checking PATCH 1/7: curl: strengthen assertion in curl_clean_state...
ERROR: spaces required around that '=' (ctx:VxV)
#24: FILE: block/curl.c:536:
+ for (j=0; j<CURL_NUM_ACB; j++) {
^
ERROR: spaces required around that '<' (ctx:VxV)
#24: FILE: block/curl.c:536:
+ for (j=0; j<CURL_NUM_ACB; j++) {
^
total: 2 errors, 0 warnings, 11 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/7: curl: never invoke callbacks with s->mutex held...
Checking PATCH 3/7: curl: avoid recursive locking of BDRVCURLState mutex...
Checking PATCH 4/7: curl: split curl_find_state/curl_init_state...
ERROR: spaces required around that '=' (ctx:VxV)
#39: FILE: block/curl.c:463:
+ for (i=0; i<CURL_NUM_STATES; i++) {
^
ERROR: spaces required around that '<' (ctx:VxV)
#39: FILE: block/curl.c:463:
+ for (i=0; i<CURL_NUM_STATES; i++) {
^
total: 2 errors, 0 warnings, 92 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/7: curl: convert CURLAIOCB to byte values...
Checking PATCH 6/7: curl: convert readv to coroutines...
Checking PATCH 7/7: curl: do not do aio_poll when waiting for a free CURLState...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (7 preceding siblings ...)
2017-05-10 15:11 ` [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
@ 2017-05-10 15:57 ` Richard W.M. Jones
2017-05-15 19:12 ` [Qemu-devel] [Qemu-block] " Max Reitz
8 siblings, 1 reply; 28+ messages in thread
From: Richard W.M. Jones @ 2017-05-10 15:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, jcody, qemu-stable, qemu-block
On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote:
> Since the last patch in v1 didn't work, I bit the bullet and converted
> the whole thing to coroutines (patches 4-6). This in turns allows a more
> elegant solution to wait for CURLStates to get free (patch 7).
>
> I tested this by lowering CURL_NUM_STATES to 2. With this change, the
> buggy case triggers a couple times while booting a Fedora netinst image.
This series fixes the original bug, so:
Tested-by: Richard W.M. Jones <rjones@redhat.com>
I think the Reported-by in patch 3 should credit Kun Wei for finding
the bug, and we should probably mention the BZ too:
Reported-by: Kun Wei <kuwei@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1447590
A nit pick perhaps but in patch 5 you say "This was broken before for
disks > 2TB, but now it would break at 4GB.".
I understand after reading it a few times that you mean it would be
broken at 4GB, if you hadn't changed size_t -> uint64_t (on 32 bit
platforms). Perhaps better to clarify that sentence.
---
I also ran some performance and stability testing. I used virt-ls for
this. The following command will iterate over every file in a remote
guest image and print an md5sum:
LIBGUESTFS_BACKEND=direct \
LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
virt-ls -a http://somehost/rhel-guest-image-7.1-20150224.0.x86_64.qcow2 \
-lR --checksum /
I timed this with and without your patches, but there was no
significant difference (but note that virt-ls is a fundamentally
sequential program).
It didn't crash or hang at any time during my testing.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] curl: strengthen assertion in curl_clean_state
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
@ 2017-05-10 16:33 ` Max Reitz
2017-05-11 20:35 ` [Qemu-devel] " Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2017-05-10 16:33 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 977 bytes --]
On 10.05.2017 16:31, Paolo Bonzini wrote:
> curl_clean_state should only be called after all AIOCBs have been
> completed. This is not so obvious for the call from curl_detach_aio_context,
> so assert that.
>
> Cc: qemu-stable@nongnu.org
> Cc: jcody@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/block/curl.c b/block/curl.c
> index 2708d57c2f..25a301e7b4 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -532,6 +532,11 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>
> static void curl_clean_state(CURLState *s)
> {
> + int j;
> + for (j=0; j<CURL_NUM_ACB; j++) {
See checkpatch output, but apart from that:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> + assert(!s->acb[j]);
> + }
> +
> if (s->s->multi)
> curl_multi_remove_handle(s->s->multi, s->curl);
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] curl: never invoke callbacks with s->mutex held
2017-05-10 14:32 ` [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
@ 2017-05-10 16:33 ` Max Reitz
2017-05-11 20:40 ` [Qemu-devel] " Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2017-05-10 16:33 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
On 10.05.2017 16:32, Paolo Bonzini wrote:
> All curl callbacks go through curl_multi_do, and hence are called with
> s->mutex held. Note that with comments, and make curl_read_cb drop the
> lock before invoking the callback.
>
> Likewise for curl_find_buf, where the callback can be invoked by the
> caller.
>
> Cc: qemu-stable@nongnu.org
> Cc: jcody@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex
2017-05-10 14:32 ` [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
@ 2017-05-10 16:38 ` Max Reitz
2017-05-11 20:56 ` [Qemu-devel] " Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2017-05-10 16:38 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]
On 10.05.2017 16:32, Paolo Bonzini wrote:
> The curl driver has a ugly hack where, if it cannot find an empty CURLState,
> it just uses aio_poll to wait for one to be empty. This is probably
> buggy when used together with dataplane, and the simplest way to fix it
> is to use coroutines instead.
>
> A more immediate effect of the bug however is that it can cause a
> recursive call to curl_readv_bh_cb and recursively taking the
> BDRVCURLState mutex. This causes a deadlock.
>
> The fix is to unlock the mutex around aio_poll, but for cleanliness we
> should also take the mutex around all calls to curl_init_state, even if
> reaching the unlock/lock pair is impossible. The same is true for
> curl_clean_state.
>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: jcody@redhat.com
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] curl: split curl_find_state/curl_init_state
2017-05-10 14:32 ` [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
@ 2017-05-10 17:26 ` Max Reitz
2017-05-11 13:49 ` [Qemu-devel] " Paolo Bonzini
2017-05-12 21:38 ` Jeff Cody
1 sibling, 1 reply; 28+ messages in thread
From: Max Reitz @ 2017-05-10 17:26 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]
On 10.05.2017 16:32, Paolo Bonzini wrote:
> If curl_easy_init fails, a CURLState is left with s->in_use = 1. Split
> curl_init_state in two, so that we can distinguish the two failures and
> call curl_clean_state if needed.
>
> While at it, simplify curl_find_state, removing a dummy loop. The
> aio_poll loop is moved to the sole caller that needs it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index b18e79bf54..4b4d5a2389 100644
> --- a/block/curl.c
> +++ b/block/curl.c
[...]
> @@ -856,8 +854,18 @@ static void curl_readv_bh_cb(void *p)
> }
>
> // No cache found, so let's start a new request
> - state = curl_init_state(acb->common.bs, s);
> - if (!state) {
> + for (;;) {
> + state = curl_find_state(s);
> + if (state) {
> + break;
> + }
> + qemu_mutex_unlock(&s->mutex);
> + aio_poll(bdrv_get_aio_context(bs), true);
> + qemu_mutex_lock(&s->mutex);
> + }
> +
> + if (curl_init_state(s, state) < 0) {
> + curl_clean_state(state);
We could also initialize state to NULL and call curl_clean_state() under
out: if state != NULL. Then again, it would only save us two
curl_clean_state() instances...
So I'll leave it to you and unconditionally give a
Reviewed-by: Max Reitz <mreitz@redhat.com>
> ret = -EIO;
> goto out;
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] curl: convert CURLAIOCB to byte values
2017-05-10 14:32 ` [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
@ 2017-05-10 17:36 ` Max Reitz
2017-05-10 18:37 ` Eric Blake
2017-05-12 21:38 ` [Qemu-devel] " Jeff Cody
1 sibling, 1 reply; 28+ messages in thread
From: Max Reitz @ 2017-05-10 17:36 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]
On 10.05.2017 16:32, Paolo Bonzini wrote:
> This is in preparation for the conversion from bdrv_aio_readv to
> bdrv_co_preadv, and it also requires changing some of the size_t values
> to uint64_t. This was broken before for disks > 2TB, but now it would
> break at 4GB.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 44 ++++++++++++++++++++++----------------------
> 1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 4b4d5a2389..3e288f2bc7 100644
> --- a/block/curl.c
> +++ b/block/curl.c
[...]
> @@ -788,7 +788,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> }
> #endif
>
> - s->len = (size_t)d;
> + s->len = d;
On a very personal level, I'd rather have an explicit cast when
converting floating point values to integers. Anyway:
Reviewed-by: Max Reitz <mreitz@redhat.com>
On a very surprised level: Why does curl return the content length as a
double?! I can't think of a reason where that might be a good idea. If
your integer no longer fits into a uin64_t, the double will be inexact,
so it pretty much is useless, too...
>
> if ((!strncasecmp(s->url, "http://", strlen("http://"))
> || !strncasecmp(s->url, "https://", strlen("https://")))
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState
2017-05-10 14:32 ` [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
@ 2017-05-10 17:54 ` Max Reitz
2017-05-12 21:41 ` [Qemu-devel] " Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2017-05-10 17:54 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
On 10.05.2017 16:32, Paolo Bonzini wrote:
> Instead, put the CURLAIOCB on a wait list; curl_clean_state will
> wake the corresponding coroutine.
>
> Because of CURL's callback-based structure, we cannot easily convert
> everything to CoMutex/CoQueue; keeping the QemuMutex is simpler.
> However, CoQueue is a simple wrapper around a linked list, so we can
> use QSIMPLEQ easily to open-code a CoQueue that has a QemuMutex's
> protection instead of a CoMutex's.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] curl: convert CURLAIOCB to byte values
2017-05-10 17:36 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-10 18:37 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-05-10 18:37 UTC (permalink / raw)
To: Max Reitz, Paolo Bonzini, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
[-- Attachment #1: Type: text/plain, Size: 694 bytes --]
On 05/10/2017 12:36 PM, Max Reitz wrote:
> On a very surprised level: Why does curl return the content length as a
> double?! I can't think of a reason where that might be a good idea. If
> your integer no longer fits into a uin64_t, the double will be inexact,
> so it pretty much is useless, too...
Not to mention: NO ONE has off_t larger than 63 bits (there physically
is not that much storage around!), so a signed 64-bit integer should
always be sufficient, rather than artificially limiting exact answers to
53 bits of precision via a double.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state
2017-05-10 17:26 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-11 13:49 ` Paolo Bonzini
0 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-11 13:49 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: qemu-stable, qemu-block, rjones
On 10/05/2017 19:26, Max Reitz wrote:
>> + if (curl_init_state(s, state) < 0) {
>> + curl_clean_state(state);
>
> We could also initialize state to NULL and call curl_clean_state() under
> out: if state != NULL. Then again, it would only save us two
> curl_clean_state() instances...
>
> So I'll leave it to you and unconditionally give a
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Makes sense, but OTOH you could also move locking to curl_co_preadv and
get rid of "out" altogether. So for now I left curl_clean_state here.
Paolo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
2017-05-10 16:33 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-11 20:35 ` Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Cody @ 2017-05-11 20:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:31:59PM +0200, Paolo Bonzini wrote:
> curl_clean_state should only be called after all AIOCBs have been
> completed. This is not so obvious for the call from curl_detach_aio_context,
> so assert that.
>
> Cc: qemu-stable@nongnu.org
> Cc: jcody@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/block/curl.c b/block/curl.c
> index 2708d57c2f..25a301e7b4 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -532,6 +532,11 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>
> static void curl_clean_state(CURLState *s)
> {
> + int j;
> + for (j=0; j<CURL_NUM_ACB; j++) {
> + assert(!s->acb[j]);
> + }
> +
> if (s->s->multi)
> curl_multi_remove_handle(s->s->multi, s->curl);
>
> --
> 2.12.2
>
>
Minor formatting nit aside (if no other revisions needed, I can fix that on
apply):
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held
2017-05-10 14:32 ` [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
2017-05-10 16:33 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-11 20:40 ` Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Cody @ 2017-05-11 20:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:32:00PM +0200, Paolo Bonzini wrote:
> All curl callbacks go through curl_multi_do, and hence are called with
> s->mutex held. Note that with comments, and make curl_read_cb drop the
> lock before invoking the callback.
>
> Likewise for curl_find_buf, where the callback can be invoked by the
> caller.
>
> Cc: qemu-stable@nongnu.org
> Cc: jcody@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 25a301e7b4..9a00fdc28e 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -147,6 +147,7 @@ static void curl_multi_do(void *arg);
> static void curl_multi_read(void *arg);
>
> #ifdef NEED_CURL_TIMER_CALLBACK
> +/* Called from curl_multi_do_locked, with s->mutex held. */
> static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
> {
> BDRVCURLState *s = opaque;
> @@ -163,6 +164,7 @@ static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
> }
> #endif
>
> +/* Called from curl_multi_do_locked, with s->mutex held. */
> static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
> void *userp, void *sp)
> {
> @@ -212,6 +214,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
> return 0;
> }
>
> +/* Called from curl_multi_do_locked, with s->mutex held. */
> static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> {
> BDRVCURLState *s = opaque;
> @@ -226,6 +229,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> return realsize;
> }
>
> +/* Called from curl_multi_do_locked, with s->mutex held. */
> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> {
> CURLState *s = ((CURLState*)opaque);
> @@ -264,7 +268,9 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> request_length - offset);
> }
>
> + qemu_mutex_unlock(&s->s->mutex);
> acb->common.cb(acb->common.opaque, 0);
> + qemu_mutex_lock(&s->s->mutex);
> qemu_aio_unref(acb);
> s->acb[i] = NULL;
> }
> @@ -305,8 +311,6 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
> if (clamped_len < len) {
> qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
> }
> - acb->common.cb(acb->common.opaque, 0);
> -
> return FIND_RET_OK;
> }
>
> @@ -832,8 +836,8 @@ static void curl_readv_bh_cb(void *p)
> // we can just call the callback and be done.
> switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
> case FIND_RET_OK:
> - qemu_aio_unref(acb);
> - // fall through
> + ret = 0;
> + goto out;
> case FIND_RET_WAIT:
> goto out;
> default:
> --
> 2.12.2
>
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex
2017-05-10 14:32 ` [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
2017-05-10 16:38 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-11 20:56 ` Jeff Cody
2017-05-12 14:48 ` Paolo Bonzini
1 sibling, 1 reply; 28+ messages in thread
From: Jeff Cody @ 2017-05-11 20:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:32:01PM +0200, Paolo Bonzini wrote:
> The curl driver has a ugly hack where, if it cannot find an empty CURLState,
> it just uses aio_poll to wait for one to be empty. This is probably
> buggy when used together with dataplane, and the simplest way to fix it
> is to use coroutines instead.
>
> A more immediate effect of the bug however is that it can cause a
> recursive call to curl_readv_bh_cb and recursively taking the
> BDRVCURLState mutex. This causes a deadlock.
>
> The fix is to unlock the mutex around aio_poll, but for cleanliness we
> should also take the mutex around all calls to curl_init_state, even if
> reaching the unlock/lock pair is impossible. The same is true for
> curl_clean_state.
>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: jcody@redhat.com
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 9a00fdc28e..b18e79bf54 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -281,6 +281,7 @@ read_end:
> return size * nmemb;
> }
>
> +/* Called with s->mutex held. */
> static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
> CURLAIOCB *acb)
> {
> @@ -453,6 +454,7 @@ static void curl_multi_timeout_do(void *arg)
> #endif
> }
>
> +/* Called with s->mutex held. */
> static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> {
> CURLState *state = NULL;
> @@ -471,7 +473,9 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> break;
> }
> if (!state) {
> + qemu_mutex_unlock(&s->mutex);
> aio_poll(bdrv_get_aio_context(bs), true);
> + qemu_mutex_lock(&s->mutex);
> }
> } while(!state);
>
> @@ -534,6 +538,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> return state;
> }
>
> +/* Called with s->mutex held. */
> static void curl_clean_state(CURLState *s)
> {
> int j;
> @@ -565,6 +570,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
> BDRVCURLState *s = bs->opaque;
> int i;
>
> + qemu_mutex_lock(&s->mutex);
> for (i = 0; i < CURL_NUM_STATES; i++) {
> if (s->states[i].in_use) {
> curl_clean_state(&s->states[i]);
> @@ -580,6 +586,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
> curl_multi_cleanup(s->multi);
> s->multi = NULL;
> }
> + qemu_mutex_unlock(&s->mutex);
>
> timer_del(&s->timer);
> }
> @@ -745,9 +752,12 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> }
>
> DPRINTF("CURL: Opening %s\n", file);
> + qemu_mutex_init(&s->mutex);
This mutex init is now done above possible returns on error, so we should
call qemu_mutex_destroy() on errors after this point.
> s->aio_context = bdrv_get_aio_context(bs);
> s->url = g_strdup(file);
> + qemu_mutex_lock(&s->mutex);
> state = curl_init_state(bs, s);
> + qemu_mutex_unlock(&s->mutex);
> if (!state)
> goto out_noclean;
>
> @@ -791,11 +801,12 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> }
> DPRINTF("CURL: Size = %zd\n", s->len);
>
> + qemu_mutex_lock(&s->mutex);
> curl_clean_state(state);
> + qemu_mutex_unlock(&s->mutex);
> curl_easy_cleanup(state->curl);
> state->curl = NULL;
>
> - qemu_mutex_init(&s->mutex);
> curl_attach_aio_context(bs, bdrv_get_aio_context(bs));
>
> qemu_opts_del(opts);
> --
> 2.12.2
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex
2017-05-11 20:56 ` [Qemu-devel] " Jeff Cody
@ 2017-05-12 14:48 ` Paolo Bonzini
0 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-05-12 14:48 UTC (permalink / raw)
To: Jeff Cody; +Cc: rjones, qemu-devel, qemu-block, qemu-stable
On 11/05/2017 22:56, Jeff Cody wrote:
> On Wed, May 10, 2017 at 04:32:01PM +0200, Paolo Bonzini wrote:
>> The curl driver has a ugly hack where, if it cannot find an empty CURLState,
>> it just uses aio_poll to wait for one to be empty. This is probably
>> buggy when used together with dataplane, and the simplest way to fix it
>> is to use coroutines instead.
>>
>> A more immediate effect of the bug however is that it can cause a
>> recursive call to curl_readv_bh_cb and recursively taking the
>> BDRVCURLState mutex. This causes a deadlock.
>>
>> The fix is to unlock the mutex around aio_poll, but for cleanliness we
>> should also take the mutex around all calls to curl_init_state, even if
>> reaching the unlock/lock pair is impossible. The same is true for
>> curl_clean_state.
>>
>> Reported-by: Richard W.M. Jones <rjones@redhat.com>
>> Cc: jcody@redhat.com
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> block/curl.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 9a00fdc28e..b18e79bf54 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -281,6 +281,7 @@ read_end:
>> return size * nmemb;
>> }
>>
>> +/* Called with s->mutex held. */
>> static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
>> CURLAIOCB *acb)
>> {
>> @@ -453,6 +454,7 @@ static void curl_multi_timeout_do(void *arg)
>> #endif
>> }
>>
>> +/* Called with s->mutex held. */
>> static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>> {
>> CURLState *state = NULL;
>> @@ -471,7 +473,9 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>> break;
>> }
>> if (!state) {
>> + qemu_mutex_unlock(&s->mutex);
>> aio_poll(bdrv_get_aio_context(bs), true);
>> + qemu_mutex_lock(&s->mutex);
>> }
>> } while(!state);
>>
>> @@ -534,6 +538,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>> return state;
>> }
>>
>> +/* Called with s->mutex held. */
>> static void curl_clean_state(CURLState *s)
>> {
>> int j;
>> @@ -565,6 +570,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
>> BDRVCURLState *s = bs->opaque;
>> int i;
>>
>> + qemu_mutex_lock(&s->mutex);
>> for (i = 0; i < CURL_NUM_STATES; i++) {
>> if (s->states[i].in_use) {
>> curl_clean_state(&s->states[i]);
>> @@ -580,6 +586,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
>> curl_multi_cleanup(s->multi);
>> s->multi = NULL;
>> }
>> + qemu_mutex_unlock(&s->mutex);
>>
>> timer_del(&s->timer);
>> }
>> @@ -745,9 +752,12 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>> }
>>
>> DPRINTF("CURL: Opening %s\n", file);
>> + qemu_mutex_init(&s->mutex);
>
> This mutex init is now done above possible returns on error, so we should
> call qemu_mutex_destroy() on errors after this point.
Ok, I'll wait for you to complete the review and send v3.
Paolo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state
2017-05-10 14:32 ` [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
2017-05-10 17:26 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-12 21:38 ` Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Cody @ 2017-05-12 21:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:32:02PM +0200, Paolo Bonzini wrote:
> If curl_easy_init fails, a CURLState is left with s->in_use = 1. Split
> curl_init_state in two, so that we can distinguish the two failures and
> call curl_clean_state if needed.
>
> While at it, simplify curl_find_state, removing a dummy loop. The
> aio_poll loop is moved to the sole caller that needs it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index b18e79bf54..4b4d5a2389 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -455,34 +455,27 @@ static void curl_multi_timeout_do(void *arg)
> }
>
> /* Called with s->mutex held. */
> -static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> +static CURLState *curl_find_state(BDRVCURLState *s)
> {
> CURLState *state = NULL;
> - int i, j;
> -
> - do {
> - for (i=0; i<CURL_NUM_STATES; i++) {
> - for (j=0; j<CURL_NUM_ACB; j++)
> - if (s->states[i].acb[j])
> - continue;
> - if (s->states[i].in_use)
> - continue;
> + int i;
>
> + for (i=0; i<CURL_NUM_STATES; i++) {
> + if (!s->states[i].in_use) {
> state = &s->states[i];
> state->in_use = 1;
> break;
> }
> - if (!state) {
> - qemu_mutex_unlock(&s->mutex);
> - aio_poll(bdrv_get_aio_context(bs), true);
> - qemu_mutex_lock(&s->mutex);
> - }
> - } while(!state);
> + }
> + return state;
> +}
>
> +static int curl_init_state(BDRVCURLState *s, CURLState *state)
> +{
> if (!state->curl) {
> state->curl = curl_easy_init();
> if (!state->curl) {
> - return NULL;
> + return -EIO;
> }
> curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
> curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
> @@ -535,7 +528,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> QLIST_INIT(&state->sockets);
> state->s = s;
>
> - return state;
> + return 0;
> }
>
> /* Called with s->mutex held. */
> @@ -756,13 +749,18 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> s->aio_context = bdrv_get_aio_context(bs);
> s->url = g_strdup(file);
> qemu_mutex_lock(&s->mutex);
> - state = curl_init_state(bs, s);
> + state = curl_find_state(s);
> qemu_mutex_unlock(&s->mutex);
> - if (!state)
> + if (!state) {
> goto out_noclean;
> + }
>
> // Get file size
>
> + if (curl_init_state(s, state) < 0) {
> + goto out;
> + }
> +
> s->accept_range = false;
> curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
> curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
> @@ -856,8 +854,18 @@ static void curl_readv_bh_cb(void *p)
> }
>
> // No cache found, so let's start a new request
> - state = curl_init_state(acb->common.bs, s);
> - if (!state) {
> + for (;;) {
> + state = curl_find_state(s);
> + if (state) {
> + break;
> + }
> + qemu_mutex_unlock(&s->mutex);
> + aio_poll(bdrv_get_aio_context(bs), true);
> + qemu_mutex_lock(&s->mutex);
> + }
> +
> + if (curl_init_state(s, state) < 0) {
> + curl_clean_state(state);
For some reason, I initially thought this might cause problems with the
assert in curl_clean_state(), but that isn't the case.
Reviewed-by: Jeff Cody <jcody@redhat.com>
> ret = -EIO;
> goto out;
> }
> --
> 2.12.2
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values
2017-05-10 14:32 ` [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
2017-05-10 17:36 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-12 21:38 ` Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Cody @ 2017-05-12 21:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:32:03PM +0200, Paolo Bonzini wrote:
> This is in preparation for the conversion from bdrv_aio_readv to
> bdrv_co_preadv, and it also requires changing some of the size_t values
> to uint64_t. This was broken before for disks > 2TB, but now it would
> break at 4GB.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 44 ++++++++++++++++++++++----------------------
> 1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 4b4d5a2389..3e288f2bc7 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -96,8 +96,8 @@ typedef struct CURLAIOCB {
> BlockAIOCB common;
> QEMUIOVector *qiov;
>
> - int64_t sector_num;
> - int nb_sectors;
> + uint64_t offset;
> + uint64_t bytes;
>
> size_t start;
> size_t end;
> @@ -115,7 +115,7 @@ typedef struct CURLState
> CURL *curl;
> QLIST_HEAD(, CURLSocket) sockets;
> char *orig_buf;
> - size_t buf_start;
> + uint64_t buf_start;
> size_t buf_off;
> size_t buf_len;
> char range[128];
> @@ -126,7 +126,7 @@ typedef struct CURLState
> typedef struct BDRVCURLState {
> CURLM *multi;
> QEMUTimer timer;
> - size_t len;
> + uint64_t len;
> CURLState states[CURL_NUM_STATES];
> char *url;
> size_t readahead_size;
> @@ -257,7 +257,7 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> continue;
>
> if ((s->buf_off >= acb->end)) {
> - size_t request_length = acb->nb_sectors * BDRV_SECTOR_SIZE;
> + size_t request_length = acb->bytes;
>
> qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
> acb->end - acb->start);
> @@ -282,18 +282,18 @@ read_end:
> }
>
> /* Called with s->mutex held. */
> -static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
> +static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
> CURLAIOCB *acb)
> {
> int i;
> - size_t end = start + len;
> - size_t clamped_end = MIN(end, s->len);
> - size_t clamped_len = clamped_end - start;
> + uint64_t end = start + len;
> + uint64_t clamped_end = MIN(end, s->len);
> + uint64_t clamped_len = clamped_end - start;
>
> for (i=0; i<CURL_NUM_STATES; i++) {
> CURLState *state = &s->states[i];
> - size_t buf_end = (state->buf_start + state->buf_off);
> - size_t buf_fend = (state->buf_start + state->buf_len);
> + uint64_t buf_end = (state->buf_start + state->buf_off);
> + uint64_t buf_fend = (state->buf_start + state->buf_len);
>
> if (!state->orig_buf)
> continue;
> @@ -788,7 +788,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> }
> #endif
>
> - s->len = (size_t)d;
> + s->len = d;
>
> if ((!strncasecmp(s->url, "http://", strlen("http://"))
> || !strncasecmp(s->url, "https://", strlen("https://")))
> @@ -797,7 +797,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> "Server does not support 'range' (byte ranges).");
> goto out;
> }
> - DPRINTF("CURL: Size = %zd\n", s->len);
> + DPRINTF("CURL: Size = %" PRIu64 "\n", s->len);
>
> qemu_mutex_lock(&s->mutex);
> curl_clean_state(state);
> @@ -836,14 +836,14 @@ static void curl_readv_bh_cb(void *p)
> BlockDriverState *bs = acb->common.bs;
> BDRVCURLState *s = bs->opaque;
>
> - size_t start = acb->sector_num * BDRV_SECTOR_SIZE;
> - size_t end;
> + uint64_t start = acb->offset;
> + uint64_t end;
>
> qemu_mutex_lock(&s->mutex);
>
> // 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, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
> + switch (curl_find_buf(s, start, acb->bytes, acb)) {
> case FIND_RET_OK:
> ret = 0;
> goto out;
> @@ -871,7 +871,7 @@ static void curl_readv_bh_cb(void *p)
> }
>
> acb->start = 0;
> - acb->end = MIN(acb->nb_sectors * BDRV_SECTOR_SIZE, s->len - start);
> + acb->end = MIN(acb->bytes, s->len - start);
>
> state->buf_off = 0;
> g_free(state->orig_buf);
> @@ -886,9 +886,9 @@ static void curl_readv_bh_cb(void *p)
> }
> state->acb[0] = acb;
>
> - snprintf(state->range, 127, "%zd-%zd", start, end);
> - DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
> - (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);
> + snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
> + DPRINTF("CURL (AIO): Reading %" PRIu64 " at %" PRIu64 " (%s)\n",
> + acb->bytes, start, state->range);
> curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
>
> curl_multi_add_handle(s->multi, state->curl);
> @@ -913,8 +913,8 @@ static BlockAIOCB *curl_aio_readv(BlockDriverState *bs,
> acb = qemu_aio_get(&curl_aiocb_info, bs, cb, opaque);
>
> acb->qiov = qiov;
> - acb->sector_num = sector_num;
> - acb->nb_sectors = nb_sectors;
> + acb->offset = sector_num * BDRV_SECTOR_SIZE;
> + acb->bytes = nb_sectors * BDRV_SECTOR_SIZE;
>
> aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), curl_readv_bh_cb, acb);
> return &acb->common;
> --
> 2.12.2
>
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines
2017-05-10 14:32 ` [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines Paolo Bonzini
@ 2017-05-12 21:40 ` Jeff Cody
0 siblings, 0 replies; 28+ messages in thread
From: Jeff Cody @ 2017-05-12 21:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:32:04PM +0200, Paolo Bonzini wrote:
> This is pretty simple. The bottom half goes away because, unlike
> bdrv_aio_readv, coroutine-based read can return immediately without
> yielding. However, for simplicity I kept the former bottom half
> handler in a separate function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 94 ++++++++++++++++++++++++------------------------------------
> 1 file changed, 38 insertions(+), 56 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 3e288f2bc7..80870bd60c 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -76,10 +76,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
> #define CURL_TIMEOUT_DEFAULT 5
> #define CURL_TIMEOUT_MAX 10000
>
> -#define FIND_RET_NONE 0
> -#define FIND_RET_OK 1
> -#define FIND_RET_WAIT 2
> -
> #define CURL_BLOCK_OPT_URL "url"
> #define CURL_BLOCK_OPT_READAHEAD "readahead"
> #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
> @@ -93,11 +89,12 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
> struct BDRVCURLState;
>
> typedef struct CURLAIOCB {
> - BlockAIOCB common;
> + Coroutine *co;
> QEMUIOVector *qiov;
>
> uint64_t offset;
> uint64_t bytes;
> + int ret;
>
> size_t start;
> size_t end;
> @@ -268,11 +265,11 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> request_length - offset);
> }
>
> + acb->ret = 0;
> + s->acb[i] = NULL;
> qemu_mutex_unlock(&s->s->mutex);
> - acb->common.cb(acb->common.opaque, 0);
> + aio_co_wake(acb->co);
> qemu_mutex_lock(&s->s->mutex);
> - qemu_aio_unref(acb);
> - s->acb[i] = NULL;
> }
> }
>
> @@ -282,8 +279,8 @@ read_end:
> }
>
> /* Called with s->mutex held. */
> -static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
> - CURLAIOCB *acb)
> +static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
> + CURLAIOCB *acb)
> {
> int i;
> uint64_t end = start + len;
> @@ -312,7 +309,8 @@ static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
> if (clamped_len < len) {
> qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
> }
> - return FIND_RET_OK;
> + acb->ret = 0;
> + return true;
> }
>
> // Wait for unfinished chunks
> @@ -330,13 +328,13 @@ static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
> for (j=0; j<CURL_NUM_ACB; j++) {
> if (!state->acb[j]) {
> state->acb[j] = acb;
> - return FIND_RET_WAIT;
> + return true;
> }
> }
> }
> }
>
> - return FIND_RET_NONE;
> + return false;
> }
>
> /* Called with s->mutex held. */
> @@ -381,11 +379,11 @@ static void curl_multi_check_completion(BDRVCURLState *s)
> continue;
> }
>
> + acb->ret = -EIO;
> + state->acb[i] = NULL;
> qemu_mutex_unlock(&s->mutex);
> - acb->common.cb(acb->common.opaque, -EIO);
> + aio_co_wake(acb->co);
> qemu_mutex_lock(&s->mutex);
> - qemu_aio_unref(acb);
> - state->acb[i] = NULL;
> }
> }
>
> @@ -821,19 +819,11 @@ out_noclean:
> return -EINVAL;
> }
>
> -static const AIOCBInfo curl_aiocb_info = {
> - .aiocb_size = sizeof(CURLAIOCB),
> -};
> -
> -
> -static void curl_readv_bh_cb(void *p)
> +static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
> {
> CURLState *state;
> int running;
> - int ret = -EINPROGRESS;
>
> - CURLAIOCB *acb = p;
> - BlockDriverState *bs = acb->common.bs;
> BDRVCURLState *s = bs->opaque;
>
> uint64_t start = acb->offset;
> @@ -843,14 +833,8 @@ static void curl_readv_bh_cb(void *p)
>
> // 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, acb->bytes, acb)) {
> - case FIND_RET_OK:
> - ret = 0;
> - goto out;
> - case FIND_RET_WAIT:
> - goto out;
> - default:
> - break;
> + if (curl_find_buf(s, start, acb->bytes, acb)) {
> + goto out;
> }
>
> // No cache found, so let's start a new request
> @@ -866,7 +850,7 @@ static void curl_readv_bh_cb(void *p)
>
> if (curl_init_state(s, state) < 0) {
> curl_clean_state(state);
> - ret = -EIO;
> + acb->ret = -EIO;
> goto out;
> }
>
> @@ -881,7 +865,7 @@ static void curl_readv_bh_cb(void *p)
> state->orig_buf = g_try_malloc(state->buf_len);
> if (state->buf_len && state->orig_buf == NULL) {
> curl_clean_state(state);
> - ret = -ENOMEM;
> + acb->ret = -ENOMEM;
> goto out;
> }
> state->acb[0] = acb;
> @@ -898,26 +882,24 @@ static void curl_readv_bh_cb(void *p)
>
> out:
> qemu_mutex_unlock(&s->mutex);
> - if (ret != -EINPROGRESS) {
> - acb->common.cb(acb->common.opaque, ret);
> - qemu_aio_unref(acb);
> - }
> }
>
> -static BlockAIOCB *curl_aio_readv(BlockDriverState *bs,
> - int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> - BlockCompletionFunc *cb, void *opaque)
> +static int coroutine_fn curl_co_preadv(BlockDriverState *bs,
> + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> {
> - CURLAIOCB *acb;
> -
> - acb = qemu_aio_get(&curl_aiocb_info, bs, cb, opaque);
> -
> - acb->qiov = qiov;
> - acb->offset = sector_num * BDRV_SECTOR_SIZE;
> - acb->bytes = nb_sectors * BDRV_SECTOR_SIZE;
> -
> - aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), curl_readv_bh_cb, acb);
> - return &acb->common;
> + CURLAIOCB acb = {
> + .co = qemu_coroutine_self(),
> + .ret = -EINPROGRESS,
> + .qiov = qiov,
> + .offset = offset,
> + .bytes = bytes
> + };
> +
> + curl_setup_preadv(bs, &acb);
> + while (acb.ret == -EINPROGRESS) {
> + qemu_coroutine_yield();
> + }
> + return acb.ret;
> }
>
> static void curl_close(BlockDriverState *bs)
> @@ -948,7 +930,7 @@ static BlockDriver bdrv_http = {
> .bdrv_close = curl_close,
> .bdrv_getlength = curl_getlength,
>
> - .bdrv_aio_readv = curl_aio_readv,
> + .bdrv_co_preadv = curl_co_preadv,
>
> .bdrv_detach_aio_context = curl_detach_aio_context,
> .bdrv_attach_aio_context = curl_attach_aio_context,
> @@ -964,7 +946,7 @@ static BlockDriver bdrv_https = {
> .bdrv_close = curl_close,
> .bdrv_getlength = curl_getlength,
>
> - .bdrv_aio_readv = curl_aio_readv,
> + .bdrv_co_preadv = curl_co_preadv,
>
> .bdrv_detach_aio_context = curl_detach_aio_context,
> .bdrv_attach_aio_context = curl_attach_aio_context,
> @@ -980,7 +962,7 @@ static BlockDriver bdrv_ftp = {
> .bdrv_close = curl_close,
> .bdrv_getlength = curl_getlength,
>
> - .bdrv_aio_readv = curl_aio_readv,
> + .bdrv_co_preadv = curl_co_preadv,
>
> .bdrv_detach_aio_context = curl_detach_aio_context,
> .bdrv_attach_aio_context = curl_attach_aio_context,
> @@ -996,7 +978,7 @@ static BlockDriver bdrv_ftps = {
> .bdrv_close = curl_close,
> .bdrv_getlength = curl_getlength,
>
> - .bdrv_aio_readv = curl_aio_readv,
> + .bdrv_co_preadv = curl_co_preadv,
>
> .bdrv_detach_aio_context = curl_detach_aio_context,
> .bdrv_attach_aio_context = curl_attach_aio_context,
> --
> 2.12.2
>
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState
2017-05-10 14:32 ` [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
2017-05-10 17:54 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-12 21:41 ` Jeff Cody
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Cody @ 2017-05-12 21:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-stable, qemu-block, rjones
On Wed, May 10, 2017 at 04:32:05PM +0200, Paolo Bonzini wrote:
> Instead, put the CURLAIOCB on a wait list; curl_clean_state will
> wake the corresponding coroutine.
>
> Because of CURL's callback-based structure, we cannot easily convert
> everything to CoMutex/CoQueue; keeping the QemuMutex is simpler.
> However, CoQueue is a simple wrapper around a linked list, so we can
> use QSIMPLEQ easily to open-code a CoQueue that has a QemuMutex's
> protection instead of a CoMutex's.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 80870bd60c..4ccdf63510 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -98,6 +98,8 @@ typedef struct CURLAIOCB {
>
> size_t start;
> size_t end;
> +
> + QSIMPLEQ_ENTRY(CURLAIOCB) next;
> } CURLAIOCB;
>
> typedef struct CURLSocket {
> @@ -133,6 +135,7 @@ typedef struct BDRVCURLState {
> bool accept_range;
> AioContext *aio_context;
> QemuMutex mutex;
> + QSIMPLEQ_HEAD(, CURLAIOCB) free_state_waitq;
> char *username;
> char *password;
> char *proxyusername;
> @@ -532,6 +535,7 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
> /* Called with s->mutex held. */
> static void curl_clean_state(CURLState *s)
> {
> + CURLAIOCB *next;
> int j;
> for (j=0; j<CURL_NUM_ACB; j++) {
> assert(!s->acb[j]);
> @@ -548,6 +552,14 @@ static void curl_clean_state(CURLState *s)
> }
>
> s->in_use = 0;
> +
> + next = QSIMPLEQ_FIRST(&s->s->free_state_waitq);
> + if (next) {
> + QSIMPLEQ_REMOVE_HEAD(&s->s->free_state_waitq, next);
> + qemu_mutex_unlock(&s->s->mutex);
> + aio_co_wake(next->co);
> + qemu_mutex_lock(&s->s->mutex);
> + }
> }
>
> static void curl_parse_filename(const char *filename, QDict *options,
> @@ -744,6 +756,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>
> DPRINTF("CURL: Opening %s\n", file);
> qemu_mutex_init(&s->mutex);
> + QSIMPLEQ_INIT(&s->free_state_waitq);
> s->aio_context = bdrv_get_aio_context(bs);
> s->url = g_strdup(file);
> qemu_mutex_lock(&s->mutex);
> @@ -843,8 +856,9 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
> if (state) {
> break;
> }
> + QSIMPLEQ_INSERT_TAIL(&s->free_state_waitq, acb, next);
> qemu_mutex_unlock(&s->mutex);
> - aio_poll(bdrv_get_aio_context(bs), true);
> + qemu_coroutine_yield();
> qemu_mutex_lock(&s->mutex);
> }
>
> --
> 2.12.2
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-10 15:57 ` Richard W.M. Jones
@ 2017-05-15 19:12 ` Max Reitz
2017-05-15 20:30 ` Richard W.M. Jones
0 siblings, 1 reply; 28+ messages in thread
From: Max Reitz @ 2017-05-15 19:12 UTC (permalink / raw)
To: Richard W.M. Jones, Paolo Bonzini; +Cc: qemu-devel, qemu-block, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]
On 2017-05-10 17:57, Richard W.M. Jones wrote:
> On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote:
>> Since the last patch in v1 didn't work, I bit the bullet and converted
>> the whole thing to coroutines (patches 4-6). This in turns allows a more
>> elegant solution to wait for CURLStates to get free (patch 7).
>>
>> I tested this by lowering CURL_NUM_STATES to 2. With this change, the
>> buggy case triggers a couple times while booting a Fedora netinst image.
>
> This series fixes the original bug, so:
>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
>
> I think the Reported-by in patch 3 should credit Kun Wei for finding
> the bug, and we should probably mention the BZ too:
>
> Reported-by: Kun Wei <kuwei@redhat.com>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1447590
This one is older, though:
https://bugzilla.redhat.com/show_bug.cgi?id=1437393
:-)
Max
> A nit pick perhaps but in patch 5 you say "This was broken before for
> disks > 2TB, but now it would break at 4GB.".
> I understand after reading it a few times that you mean it would be
> broken at 4GB, if you hadn't changed size_t -> uint64_t (on 32 bit
> platforms). Perhaps better to clarify that sentence.
>
> ---
>
> I also ran some performance and stability testing. I used virt-ls for
> this. The following command will iterate over every file in a remote
> guest image and print an md5sum:
>
> LIBGUESTFS_BACKEND=direct \
> LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
> virt-ls -a http://somehost/rhel-guest-image-7.1-20150224.0.x86_64.qcow2 \
> -lR --checksum /
>
> I timed this with and without your patches, but there was no
> significant difference (but note that virt-ls is a fundamentally
> sequential program).
>
> It didn't crash or hang at any time during my testing.
>
> Rich.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-15 19:12 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-15 20:30 ` Richard W.M. Jones
0 siblings, 0 replies; 28+ messages in thread
From: Richard W.M. Jones @ 2017-05-15 20:30 UTC (permalink / raw)
To: Max Reitz; +Cc: Paolo Bonzini, qemu-devel, qemu-block, qemu-stable
On Mon, May 15, 2017 at 09:12:54PM +0200, Max Reitz wrote:
> On 2017-05-10 17:57, Richard W.M. Jones wrote:
> > On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote:
> >> Since the last patch in v1 didn't work, I bit the bullet and converted
> >> the whole thing to coroutines (patches 4-6). This in turns allows a more
> >> elegant solution to wait for CURLStates to get free (patch 7).
> >>
> >> I tested this by lowering CURL_NUM_STATES to 2. With this change, the
> >> buggy case triggers a couple times while booting a Fedora netinst image.
> >
> > This series fixes the original bug, so:
> >
> > Tested-by: Richard W.M. Jones <rjones@redhat.com>
> >
> > I think the Reported-by in patch 3 should credit Kun Wei for finding
> > the bug, and we should probably mention the BZ too:
> >
> > Reported-by: Kun Wei <kuwei@redhat.com>
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1447590
>
> This one is older, though:
> https://bugzilla.redhat.com/show_bug.cgi?id=1437393
Fair enough, it does seem to be the same issue.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-05-15 20:30 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
2017-05-10 16:33 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:35 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
2017-05-10 16:33 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:40 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
2017-05-10 16:38 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:56 ` [Qemu-devel] " Jeff Cody
2017-05-12 14:48 ` Paolo Bonzini
2017-05-10 14:32 ` [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
2017-05-10 17:26 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 13:49 ` [Qemu-devel] " Paolo Bonzini
2017-05-12 21:38 ` Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
2017-05-10 17:36 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-10 18:37 ` Eric Blake
2017-05-12 21:38 ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines Paolo Bonzini
2017-05-12 21:40 ` Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
2017-05-10 17:54 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-12 21:41 ` [Qemu-devel] " Jeff Cody
2017-05-10 15:11 ` [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
2017-05-10 15:57 ` Richard W.M. Jones
2017-05-15 19:12 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-15 20:30 ` Richard W.M. Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.