[Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[no-reply]

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

Re: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Richard W.M. Jones]

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

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] curl: strengthen assertion in curl_clean_state

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] curl: never invoke callbacks with s->mutex held

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] curl: split curl_find_state/curl_init_state

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] curl: convert CURLAIOCB to byte values

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] curl: convert CURLAIOCB to byte values

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state

[Jeff Cody]

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>

Re: [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held

[Jeff Cody]

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>

Re: [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex

[Jeff Cody]

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
> 
> 

Re: [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state

[Jeff Cody]

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
> 
> 

Re: [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values

[Jeff Cody]

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>

Re: [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines

[Jeff Cody]

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>

Re: [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState

[Jeff Cody]

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>

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Richard W.M. Jones]

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