* [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
@ 2017-05-15 10:00 Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
` (10 more replies)
0 siblings, 11 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block
Compared to v2, this silences checkpatch and correctly destroy the mutex on
exiting from curl_open with an error.
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 | 217 +++++++++++++++++++++++++++++++++--------------------------
1 file changed, 121 insertions(+), 96 deletions(-)
--
2.12.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v3 1/7] curl: strengthen assertion in curl_clean_state
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block, qemu-stable
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
Reviewed-by: Jeff Cody <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 aa6e8cc0e5..010b0604c5 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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 2/7] curl: never invoke callbacks with s->mutex held
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block, qemu-stable
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
Reviewed-by: Jeff Cody <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 010b0604c5..924c2a2088 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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 3/7] curl: avoid recursive locking of BDRVCURLState mutex
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block, qemu-stable
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: Kun Wei <kuwei@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Cc: qemu-stable@nongnu.org
Cc: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v2->v3: add missing qemu_mutex_destroy on failure case
block/curl.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/curl.c b/block/curl.c
index 924c2a2088..5e6ddf3a34 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);
}
@@ -677,6 +684,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
return -EROFS;
}
+ qemu_mutex_init(&s->mutex);
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
if (local_err) {
@@ -747,7 +755,9 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
DPRINTF("CURL: Opening %s\n", file);
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);
@@ -806,6 +817,7 @@ out:
curl_easy_cleanup(state->curl);
state->curl = NULL;
out_noclean:
+ qemu_mutex_destroy(&s->mutex);
g_free(s->cookie);
g_free(s->url);
qemu_opts_del(opts);
--
2.12.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v3 4/7] curl: split curl_find_state/curl_init_state
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (2 preceding siblings ...)
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block
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.
Reviewed-by: Jeff Cody <jcody@redhat.com>
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 5e6ddf3a34..29132ab835 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,
@@ -857,8 +855,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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 5/7] curl: convert CURLAIOCB to byte values
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (3 preceding siblings ...)
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 6/7] curl: convert readv to coroutines Paolo Bonzini
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block
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.
Reviewed-by: Jeff Cody <jcody@redhat.com>
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 29132ab835..401a1b1015 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);
@@ -837,14 +837,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;
@@ -872,7 +872,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);
@@ -887,9 +887,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);
@@ -914,8 +914,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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 6/7] curl: convert readv to coroutines
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (4 preceding siblings ...)
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block
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.
Reviewed-by: Jeff Cody <jcody@redhat.com>
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 401a1b1015..86a4a73834 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;
}
}
@@ -822,19 +820,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;
@@ -844,14 +834,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
@@ -867,7 +851,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;
}
@@ -882,7 +866,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;
@@ -899,26 +883,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)
@@ -949,7 +931,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,
@@ -965,7 +947,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,
@@ -981,7 +963,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,
@@ -997,7 +979,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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 7/7] curl: do not do aio_poll when waiting for a free CURLState
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (5 preceding siblings ...)
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 6/7] curl: convert readv to coroutines Paolo Bonzini
@ 2017-05-15 10:00 ` Paolo Bonzini
2017-05-15 11:05 ` [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-05-15 10:00 UTC (permalink / raw)
To: qemu-devel; +Cc: jcody, rjones, qemu-block
Instead, put the CURLAIOCB on a wait list and yield; 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 easily
use QSIMPLEQ and open-code a CoQueue, protected by the BDRVCURLState
QemuMutex instead of a CoMutex.
Reviewed-by: Jeff Cody <jcody@redhat.com>
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 86a4a73834..630c6e5187 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);
+ QSIMPLEQ_INIT(&s->free_state_waitq);
s->aio_context = bdrv_get_aio_context(bs);
s->url = g_strdup(file);
qemu_mutex_lock(&s->mutex);
@@ -844,8 +857,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] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (6 preceding siblings ...)
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
@ 2017-05-15 11:05 ` no-reply
2017-05-15 19:28 ` [Qemu-devel] [Qemu-block] " Max Reitz
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: no-reply @ 2017-05-15 11:05 UTC (permalink / raw)
To: pbonzini; +Cc: famz, qemu-devel, jcody, rjones, qemu-block
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
Message-id: 20170515100059.15795-1-pbonzini@redhat.com
Type: series
=== 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
From https://github.com/patchew-project/qemu
* [new tag] patchew/20170515104543.32044-1-kraxel@redhat.com -> patchew/20170515104543.32044-1-kraxel@redhat.com
Switched to a new branch 'test'
e926adb curl: do not do aio_poll when waiting for a free CURLState
4eebc68 curl: convert readv to coroutines
f4b49bc curl: convert CURLAIOCB to byte values
610980d curl: split curl_find_state/curl_init_state
c5ca4dc curl: avoid recursive locking of BDRVCURLState mutex
3ace9ce curl: never invoke callbacks with s->mutex held
b31fcbf 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++) {
^
total: 1 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)
#40: FILE: block/curl.c:463:
+ for (i=0; i < CURL_NUM_STATES; i++) {
^
total: 1 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] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (7 preceding siblings ...)
2017-05-15 11:05 ` [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
@ 2017-05-15 19:28 ` Max Reitz
2017-05-16 14:35 ` [Qemu-devel] " Jeff Cody
2017-05-18 13:23 ` Richard W.M. Jones
10 siblings, 0 replies; 12+ messages in thread
From: Max Reitz @ 2017-05-15 19:28 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: rjones, qemu-block
[-- Attachment #1: Type: text/plain, Size: 773 bytes --]
On 2017-05-15 12:00, Paolo Bonzini wrote:
> Compared to v2, this silences checkpatch and correctly destroy the mutex on
> exiting from curl_open with an error.
>
> 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 | 217 +++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 121 insertions(+), 96 deletions(-)
Modulo checkpatch:
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (8 preceding siblings ...)
2017-05-15 19:28 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2017-05-16 14:35 ` Jeff Cody
2017-05-18 13:23 ` Richard W.M. Jones
10 siblings, 0 replies; 12+ messages in thread
From: Jeff Cody @ 2017-05-16 14:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, rjones, qemu-block
On Mon, May 15, 2017 at 12:00:52PM +0200, Paolo Bonzini wrote:
> Compared to v2, this silences checkpatch and correctly destroy the mutex on
> exiting from curl_open with an error.
>
> 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 | 217 +++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 121 insertions(+), 96 deletions(-)
>
> --
> 2.12.2
>
Thanks,
I fixed up the coding styling issues in patch 1 and 4.
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc block
-Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
` (9 preceding siblings ...)
2017-05-16 14:35 ` [Qemu-devel] " Jeff Cody
@ 2017-05-18 13:23 ` Richard W.M. Jones
10 siblings, 0 replies; 12+ messages in thread
From: Richard W.M. Jones @ 2017-05-18 13:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, jcody, qemu-block
On Mon, May 15, 2017 at 12:00:52PM +0200, Paolo Bonzini wrote:
> Compared to v2, this silences checkpatch and correctly destroy the mutex on
> exiting from curl_open with an error.
FWIW I hit the curl bug again and tested the current patch
series, and it also fixes the bug (as expected).
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-05-18 13:23 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-15 10:00 [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 6/7] curl: convert readv to coroutines Paolo Bonzini
2017-05-15 10:00 ` [Qemu-devel] [PATCH v3 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
2017-05-15 11:05 ` [Qemu-devel] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
2017-05-15 19:28 ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-16 14:35 ` [Qemu-devel] " Jeff Cody
2017-05-18 13:23 ` 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.