diff mbox

[v5,06/11] curl: introduce CURLDataCache

Message ID 1369280289-20928-7-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng May 23, 2013, 3:38 a.m. UTC
Data buffer was contained by CURLState, they are allocated and freed
together. This patch try to isolate them, by introducing a dedicated
cache list to BDRVCURLState. The benifit is we can now release the
CURLState (and associated sockets) while keep the fetched data for later
use, and simplies the prefetch and buffer logic for some degree.

Note: There's already page cache in guest kernel, why cache data here?
Since we don't want to submit http/ftp/* request for every 2KB in
sequencial read, but there are crude guest that sends small IO reqs,
which will result in horrible performance.  GRUB/isolinux loading kernel
is a typical case and we workaround this by prefetch cache.  This is
what curl.c has been doing along. This patch just refectors the buffer.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 136 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 67 insertions(+), 69 deletions(-)

Comments

Stefan Hajnoczi May 23, 2013, 2:09 p.m. UTC | #1
On Thu, May 23, 2013 at 11:38:04AM +0800, Fam Zheng wrote:
> +typedef struct CURLDataCache {
> +    char *data;
> +    size_t base_pos;

Must be int64_t.  QEMU compiled on 32-bit hosts would only allow 4 GB
images with size_t!

> +    size_t data_len;
> +    size_t write_pos;
> +    /* Ref count for CURLState */
> +    int use_count;

It's better to introduce this field when you add code to use it.  When
possible, don't add unused code in a patch, it makes it harder to
review.

> +static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
> +                             CURLDataCache *cache)
> +{
> +    size_t aio_base = acb->sector_num * SECTOR_SIZE;

int64_t

> +    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
> +    size_t off = aio_base - cache->base_pos;
> +
> +    qemu_iovec_from_buf(acb->qiov, 0, cache->data + off, aio_bytes);
> +    acb->common.cb(acb->common.opaque, 0);
> +    DPRINTF("AIO Request OK: %10zd %10zd\n", aio_base, aio_bytes);

PRId64 for 64-bit aio_base

> @@ -589,26 +577,24 @@ static const AIOCBInfo curl_aiocb_info = {
>  static void curl_readv_bh_cb(void *p)
>  {
>      CURLState *state;
> -
> +    CURLDataCache *cache = NULL;
>      CURLAIOCB *acb = p;
>      BDRVCURLState *s = acb->common.bs->opaque;
> +    size_t aio_base, aio_bytes;

int64_t aio_base;
Fam Zheng May 24, 2013, 3 a.m. UTC | #2
On Thu, 05/23 16:09, Stefan Hajnoczi wrote:
> On Thu, May 23, 2013 at 11:38:04AM +0800, Fam Zheng wrote:
> > +typedef struct CURLDataCache {
> > +    char *data;
> > +    size_t base_pos;
> 
> Must be int64_t.  QEMU compiled on 32-bit hosts would only allow 4 GB
> images with size_t!

OK.

> 
> > +    size_t data_len;
> > +    size_t write_pos;
> > +    /* Ref count for CURLState */
> > +    int use_count;
> 
> It's better to introduce this field when you add code to use it.  When
> possible, don't add unused code in a patch, it makes it harder to
> review.

Moving to later patch.

> 
> > +static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
> > +                             CURLDataCache *cache)
> > +{
> > +    size_t aio_base = acb->sector_num * SECTOR_SIZE;
> 
> int64_t
> 
> > +    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
> > +    size_t off = aio_base - cache->base_pos;
> > +
> > +    qemu_iovec_from_buf(acb->qiov, 0, cache->data + off, aio_bytes);
> > +    acb->common.cb(acb->common.opaque, 0);
> > +    DPRINTF("AIO Request OK: %10zd %10zd\n", aio_base, aio_bytes);
> 
> PRId64 for 64-bit aio_base

OK, thanks.

> 
> > @@ -589,26 +577,24 @@ static const AIOCBInfo curl_aiocb_info = {
> >  static void curl_readv_bh_cb(void *p)
> >  {
> >      CURLState *state;
> > -
> > +    CURLDataCache *cache = NULL;
> >      CURLAIOCB *acb = p;
> >      BDRVCURLState *s = acb->common.bs->opaque;
> > +    size_t aio_base, aio_bytes;
> 
> int64_t aio_base;

Yes, will change.
diff mbox

Patch

diff --git a/block/curl.c b/block/curl.c
index 4fd5bb9..e387ae1 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -43,10 +43,6 @@ 
 #define SECTOR_SIZE     512
 #define READ_AHEAD_SIZE (256 * 1024)
 
-#define FIND_RET_NONE   0
-#define FIND_RET_OK     1
-#define FIND_RET_WAIT   2
-
 struct BDRVCURLState;
 
 typedef struct CURLAIOCB {
@@ -61,6 +57,16 @@  typedef struct CURLAIOCB {
     size_t end;
 } CURLAIOCB;
 
+typedef struct CURLDataCache {
+    char *data;
+    size_t base_pos;
+    size_t data_len;
+    size_t write_pos;
+    /* Ref count for CURLState */
+    int use_count;
+    QLIST_ENTRY(CURLDataCache) next;
+} CURLDataCache;
+
 typedef struct CURLState
 {
     struct BDRVCURLState *s;
@@ -90,6 +96,8 @@  typedef struct BDRVCURLState {
     char *url;
     size_t readahead_size;
     QEMUTimer *timer;
+    /* List of data cache ordered by access, freed from tail */
+    QLIST_HEAD(, CURLDataCache) cache;
     /* Whether http server accept range in header */
     bool accept_range;
 } BDRVCURLState;
@@ -98,6 +106,19 @@  static void curl_clean_state(CURLState *s);
 static void curl_fd_handler(void *arg);
 static int curl_aio_flush(void *opaque);
 
+static CURLDataCache *curl_find_cache(BDRVCURLState *bs,
+                                      size_t start, size_t len)
+{
+    CURLDataCache *c;
+    QLIST_FOREACH(c, &bs->cache, next) {
+        if (start >= c->base_pos &&
+            start + len <= c->base_pos + c->write_pos) {
+            return c;
+        }
+    }
+    return NULL;
+}
+
 static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
                         void *s, void *sp)
 {
@@ -181,6 +202,23 @@  static int curl_multi_timer_cb(CURLM *multi, long timeout_ms, void *s)
     return 0;
 }
 
+static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
+                             CURLDataCache *cache)
+{
+    size_t aio_base = acb->sector_num * SECTOR_SIZE;
+    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
+    size_t off = aio_base - cache->base_pos;
+
+    qemu_iovec_from_buf(acb->qiov, 0, cache->data + off, aio_bytes);
+    acb->common.cb(acb->common.opaque, 0);
+    DPRINTF("AIO Request OK: %10zd %10zd\n", aio_base, aio_bytes);
+    qemu_aio_release(acb);
+    acb = NULL;
+    /* Move cache next in the list */
+    QLIST_REMOVE(cache, next);
+    QLIST_INSERT_HEAD(&bs->cache, cache, next);
+}
+
 static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     CURLState *s = ((CURLState*)opaque);
@@ -214,59 +252,6 @@  read_end:
     return realsize;
 }
 
-static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
-                         CURLAIOCB *acb)
-{
-    int i;
-    size_t end = start + len;
-
-    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);
-
-        if (!state->orig_buf)
-            continue;
-        if (!state->buf_off)
-            continue;
-
-        // Does the existing buffer cover our section?
-        if ((start >= state->buf_start) &&
-            (start <= buf_end) &&
-            (end >= state->buf_start) &&
-            (end <= buf_end))
-        {
-            char *buf = state->orig_buf + (start - state->buf_start);
-
-            qemu_iovec_from_buf(acb->qiov, 0, buf, len);
-            acb->common.cb(acb->common.opaque, 0);
-
-            return FIND_RET_OK;
-        }
-
-        // Wait for unfinished chunks
-        if ((start >= state->buf_start) &&
-            (start <= buf_fend) &&
-            (end >= state->buf_start) &&
-            (end <= buf_fend))
-        {
-            int j;
-
-            acb->start = start - state->buf_start;
-            acb->end = acb->start + len;
-
-            for (j=0; j<CURL_NUM_ACB; j++) {
-                if (!state->acb[j]) {
-                    state->acb[j] = acb;
-                    return FIND_RET_WAIT;
-                }
-            }
-        }
-    }
-
-    return FIND_RET_NONE;
-}
-
 static void curl_fd_handler(void *arg)
 {
     CURLSockInfo *sock = (CURLSockInfo *)arg;
@@ -299,7 +284,9 @@  static void curl_fd_handler(void *arg)
             case CURLMSG_DONE:
             {
                 CURLState *state = NULL;
-                curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE, (char**)&state);
+                curl_easy_getinfo(msg->easy_handle,
+                                  CURLINFO_PRIVATE,
+                                  (char **)&state);
 
                 /* ACBs for successful messages get completed in curl_read_cb */
                 if (msg->data.result != CURLE_OK) {
@@ -495,6 +482,7 @@  static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     }
 
     QLIST_INIT(&s->socks);
+    QLIST_INIT(&s->cache);
 
     DPRINTF("CURL: Opening %s\n", file);
     s->url = g_strdup(file);
@@ -589,26 +577,24 @@  static const AIOCBInfo curl_aiocb_info = {
 static void curl_readv_bh_cb(void *p)
 {
     CURLState *state;
-
+    CURLDataCache *cache = NULL;
     CURLAIOCB *acb = p;
     BDRVCURLState *s = acb->common.bs->opaque;
+    size_t aio_base, aio_bytes;
 
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
 
+    aio_base = acb->sector_num * SECTOR_SIZE;
+    aio_bytes = acb->nb_sectors * SECTOR_SIZE;
+
     size_t start = acb->sector_num * SECTOR_SIZE;
     size_t end;
 
-    // In case we have the requested data already (e.g. read-ahead),
-    // we can just call the callback and be done.
-    switch (curl_find_buf(s, start, acb->nb_sectors * SECTOR_SIZE, acb)) {
-        case FIND_RET_OK:
-            qemu_aio_release(acb);
-            // fall through
-        case FIND_RET_WAIT:
-            return;
-        default:
-            break;
+    cache = curl_find_cache(s, aio_base, aio_bytes);
+    if (cache) {
+        curl_complete_io(s, acb, cache);
+        return;
     }
 
     // No cache found, so let's start a new request
@@ -691,6 +677,18 @@  static void curl_close(BlockDriverState *bs)
     if (s->multi)
         curl_multi_cleanup(s->multi);
 
+    while (!QLIST_EMPTY(&s->cache)) {
+        CURLDataCache *cache = QLIST_FIRST(&s->cache);
+        assert(cache->use_count == 0);
+        if (cache->data) {
+            g_free(cache->data);
+            cache->data = NULL;
+        }
+        QLIST_REMOVE(cache, next);
+        g_free(cache);
+        cache = NULL;
+    }
+
     while (!QLIST_EMPTY(&s->socks)) {
         CURLSockInfo *sock = QLIST_FIRST(&s->socks);
         QLIST_REMOVE(sock, next);