[Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Paolo Bonzini]

Cleaning up the code and removing duplication makes it simpler to
later adapt it for the multiqueue work.

Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
to older snapshots, but the latter only worked with "dog vdi rollback".
Neither loadvm nor qemu-img worked for me.

Paolo

        v1->v2: placate patchew
        v2->v3: rebase

Paolo Bonzini (5):
  sheepdog: remove unused cancellation support
  sheepdog: reorganize coroutine flow
  sheepdog: do not use BlockAIOCB
  sheepdog: simplify inflight_aio_head management
  sheepdog: reorganize check for overlapping requests

 block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
 1 file changed, 83 insertions(+), 206 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/5] sheepdog: remove unused cancellation support

[Paolo Bonzini]

SheepdogAIOCB is internal to sheepdog.c, hence it is never canceled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 52 ----------------------------------------------------
 1 file changed, 52 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 4c9af89..0b30524 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -347,7 +347,6 @@ struct SheepdogAIOCB {
     Coroutine *coroutine;
     void (*aio_done_func)(SheepdogAIOCB *);
 
-    bool cancelable;
     int nr_pending;
 
     uint32_t min_affect_data_idx;
@@ -486,7 +485,6 @@ static inline void free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
 {
     SheepdogAIOCB *acb = aio_req->aiocb;
 
-    acb->cancelable = false;
     QLIST_REMOVE(aio_req, aio_siblings);
     g_free(aio_req);
 
@@ -499,57 +497,8 @@ static void coroutine_fn sd_finish_aiocb(SheepdogAIOCB *acb)
     qemu_aio_unref(acb);
 }
 
-/*
- * Check whether the specified acb can be canceled
- *
- * We can cancel aio when any request belonging to the acb is:
- *  - Not processed by the sheepdog server.
- *  - Not linked to the inflight queue.
- */
-static bool sd_acb_cancelable(const SheepdogAIOCB *acb)
-{
-    BDRVSheepdogState *s = acb->common.bs->opaque;
-    AIOReq *aioreq;
-
-    if (!acb->cancelable) {
-        return false;
-    }
-
-    QLIST_FOREACH(aioreq, &s->inflight_aio_head, aio_siblings) {
-        if (aioreq->aiocb == acb) {
-            return false;
-        }
-    }
-
-    return true;
-}
-
-static void sd_aio_cancel(BlockAIOCB *blockacb)
-{
-    SheepdogAIOCB *acb = (SheepdogAIOCB *)blockacb;
-    BDRVSheepdogState *s = acb->common.bs->opaque;
-    AIOReq *aioreq, *next;
-
-    if (sd_acb_cancelable(acb)) {
-        /* Remove outstanding requests from failed queue.  */
-        QLIST_FOREACH_SAFE(aioreq, &s->failed_aio_head, aio_siblings,
-                           next) {
-            if (aioreq->aiocb == acb) {
-                free_aio_req(s, aioreq);
-            }
-        }
-
-        assert(acb->nr_pending == 0);
-        if (acb->common.cb) {
-            acb->common.cb(acb->common.opaque, -ECANCELED);
-        }
-        sd_finish_aiocb(acb);
-    }
-}
-
 static const AIOCBInfo sd_aiocb_info = {
     .aiocb_size     = sizeof(SheepdogAIOCB),
-    .cancel_async   = sd_aio_cancel,
 };
 
 static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
@@ -569,7 +518,6 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
     acb->nb_sectors = nb_sectors;
 
     acb->aio_done_func = NULL;
-    acb->cancelable = true;
     acb->coroutine = qemu_coroutine_self();
     acb->ret = 0;
     acb->nr_pending = 0;
-- 
2.9.3

[Qemu-devel] [PATCH 2/5] sheepdog: reorganize coroutine flow

[Paolo Bonzini]

Delimit co_recv's lifetime clearly in aio_read_response.

Do a simple qemu_coroutine_enter in aio_read_response, letting
sd_co_writev call sd_write_done.

Handle nr_pending in the same way in sd_co_rw_vector,
sd_write_done and sd_co_flush_to_disk.

Remove sd_co_rw_vector's return value; just leave with no
pending requests.
---
 block/sheepdog.c | 115 ++++++++++++++++++++-----------------------------------
 1 file changed, 41 insertions(+), 74 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0b30524..84b6645 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -345,8 +345,6 @@ struct SheepdogAIOCB {
     enum AIOCBState aiocb_type;
 
     Coroutine *coroutine;
-    void (*aio_done_func)(SheepdogAIOCB *);
-
     int nr_pending;
 
     uint32_t min_affect_data_idx;
@@ -449,14 +447,13 @@ static const char * sd_strerror(int err)
  *
  * 1. In sd_co_rw_vector, we send the I/O requests to the server and
  *    link the requests to the inflight_list in the
- *    BDRVSheepdogState.  The function exits without waiting for
+ *    BDRVSheepdogState.  The function yields while waiting for
  *    receiving the response.
  *
  * 2. We receive the response in aio_read_response, the fd handler to
- *    the sheepdog connection.  If metadata update is needed, we send
- *    the write request to the vdi object in sd_write_done, the write
- *    completion function.  We switch back to sd_co_readv/writev after
- *    all the requests belonging to the AIOCB are finished.
+ *    the sheepdog connection.  We switch back to sd_co_readv/sd_writev
+ *    after all the requests belonging to the AIOCB are finished.  If
+ *    needed, sd_co_writev will send another requests for the vdi object.
  */
 
 static inline AIOReq *alloc_aio_req(BDRVSheepdogState *s, SheepdogAIOCB *acb,
@@ -491,12 +488,6 @@ static inline void free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
     acb->nr_pending--;
 }
 
-static void coroutine_fn sd_finish_aiocb(SheepdogAIOCB *acb)
-{
-    qemu_coroutine_enter(acb->coroutine);
-    qemu_aio_unref(acb);
-}
-
 static const AIOCBInfo sd_aiocb_info = {
     .aiocb_size     = sizeof(SheepdogAIOCB),
 };
@@ -517,7 +508,6 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
     acb->sector_num = sector_num;
     acb->nb_sectors = nb_sectors;
 
-    acb->aio_done_func = NULL;
     acb->coroutine = qemu_coroutine_self();
     acb->ret = 0;
     acb->nr_pending = 0;
@@ -788,9 +778,6 @@ static void coroutine_fn aio_read_response(void *opaque)
 
     switch (acb->aiocb_type) {
     case AIOCB_WRITE_UDATA:
-        /* this coroutine context is no longer suitable for co_recv
-         * because we may send data to update vdi objects */
-        s->co_recv = NULL;
         if (!is_data_obj(aio_req->oid)) {
             break;
         }
@@ -838,6 +825,11 @@ static void coroutine_fn aio_read_response(void *opaque)
         }
     }
 
+    /* No more data for this aio_req (reload_inode below uses its own file
+     * descriptor handler which doesn't use co_recv).
+    */
+    s->co_recv = NULL;
+
     switch (rsp.result) {
     case SD_RES_SUCCESS:
         break;
@@ -855,7 +847,7 @@ static void coroutine_fn aio_read_response(void *opaque)
             aio_req->oid = vid_to_vdi_oid(s->inode.vdi_id);
         }
         resend_aioreq(s, aio_req);
-        goto out;
+        return;
     default:
         acb->ret = -EIO;
         error_report("%s", sd_strerror(rsp.result));
@@ -868,13 +860,10 @@ static void coroutine_fn aio_read_response(void *opaque)
          * We've finished all requests which belong to the AIOCB, so
          * we can switch back to sd_co_readv/writev now.
          */
-        acb->aio_done_func(acb);
+        qemu_coroutine_enter(acb->coroutine);
     }
-out:
-    s->co_recv = NULL;
-    return;
+
 err:
-    s->co_recv = NULL;
     reconnect_to_sdog(opaque);
 }
 
@@ -1973,7 +1962,6 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset)
 /*
  * This function is called after writing data objects.  If we need to
  * update metadata, this sends a write request to the vdi object.
- * Otherwise, this switches back to sd_co_readv/writev.
  */
 static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
 {
@@ -1986,6 +1974,7 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
     mx = acb->max_dirty_data_idx;
     if (mn <= mx) {
         /* we need to update the vdi object. */
+        ++acb->nr_pending;
         offset = sizeof(s->inode) - sizeof(s->inode.data_vdi_id) +
             mn * sizeof(s->inode.data_vdi_id[0]);
         data_len = (mx - mn + 1) * sizeof(s->inode.data_vdi_id[0]);
@@ -1999,13 +1988,10 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
                                 data_len, offset, 0, false, 0, offset);
         QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
         add_aio_request(s, aio_req, &iov, 1, AIOCB_WRITE_UDATA);
-
-        acb->aio_done_func = sd_finish_aiocb;
-        acb->aiocb_type = AIOCB_WRITE_UDATA;
-        return;
+        if (--acb->nr_pending) {
+            qemu_coroutine_yield();
+        }
     }
-
-    sd_finish_aiocb(acb);
 }
 
 /* Delete current working VDI on the snapshot chain */
@@ -2117,7 +2103,7 @@ out:
  * Returns 1 when we need to wait a response, 0 when there is no sent
  * request and -errno in error cases.
  */
-static int coroutine_fn sd_co_rw_vector(void *p)
+static void coroutine_fn sd_co_rw_vector(void *p)
 {
     SheepdogAIOCB *acb = p;
     int ret = 0;
@@ -2138,7 +2124,7 @@ static int coroutine_fn sd_co_rw_vector(void *p)
         ret = sd_create_branch(s);
         if (ret) {
             acb->ret = -EIO;
-            goto out;
+            return;
         }
     }
 
@@ -2212,11 +2198,9 @@ static int coroutine_fn sd_co_rw_vector(void *p)
         idx++;
         done += len;
     }
-out:
-    if (!--acb->nr_pending) {
-        return acb->ret;
+    if (--acb->nr_pending) {
+        qemu_coroutine_yield();
     }
-    return 1;
 }
 
 static bool check_overlapping_aiocb(BDRVSheepdogState *s, SheepdogAIOCB *aiocb)
@@ -2249,7 +2233,6 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
     }
 
     acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors);
-    acb->aio_done_func = sd_write_done;
     acb->aiocb_type = AIOCB_WRITE_UDATA;
 
 retry:
@@ -2258,20 +2241,14 @@ retry:
         goto retry;
     }
 
-    ret = sd_co_rw_vector(acb);
-    if (ret <= 0) {
-        QLIST_REMOVE(acb, aiocb_siblings);
-        qemu_co_queue_restart_all(&s->overlapping_queue);
-        qemu_aio_unref(acb);
-        return ret;
-    }
-
-    qemu_coroutine_yield();
+    sd_co_rw_vector(acb);
+    sd_write_done(acb);
 
     QLIST_REMOVE(acb, aiocb_siblings);
     qemu_co_queue_restart_all(&s->overlapping_queue);
-
-    return acb->ret;
+    ret = acb->ret;
+    qemu_aio_unref(acb);
+    return ret;
 }
 
 static coroutine_fn int sd_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -2283,7 +2260,6 @@ static coroutine_fn int sd_co_readv(BlockDriverState *bs, int64_t sector_num,
 
     acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors);
     acb->aiocb_type = AIOCB_READ_UDATA;
-    acb->aio_done_func = sd_finish_aiocb;
 
 retry:
     if (check_overlapping_aiocb(s, acb)) {
@@ -2291,25 +2267,20 @@ retry:
         goto retry;
     }
 
-    ret = sd_co_rw_vector(acb);
-    if (ret <= 0) {
-        QLIST_REMOVE(acb, aiocb_siblings);
-        qemu_co_queue_restart_all(&s->overlapping_queue);
-        qemu_aio_unref(acb);
-        return ret;
-    }
-
-    qemu_coroutine_yield();
+    sd_co_rw_vector(acb);
 
     QLIST_REMOVE(acb, aiocb_siblings);
     qemu_co_queue_restart_all(&s->overlapping_queue);
-    return acb->ret;
+    ret = acb->ret;
+    qemu_aio_unref(acb);
+    return ret;
 }
 
 static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
 {
     BDRVSheepdogState *s = bs->opaque;
     SheepdogAIOCB *acb;
+    int ret;
     AIOReq *aio_req;
 
     if (s->cache_flags != SD_FLAG_CMD_CACHE) {
@@ -2318,15 +2289,19 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
 
     acb = sd_aio_setup(bs, NULL, 0, 0);
     acb->aiocb_type = AIOCB_FLUSH_CACHE;
-    acb->aio_done_func = sd_finish_aiocb;
 
+    acb->nr_pending++;
     aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
                             0, 0, 0, false, 0, 0);
     QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
     add_aio_request(s, aio_req, NULL, 0, acb->aiocb_type);
 
-    qemu_coroutine_yield();
-    return acb->ret;
+    if (--acb->nr_pending) {
+        qemu_coroutine_yield();
+    }
+    ret = acb->ret;
+    qemu_aio_unref(acb);
+    return ret;
 }
 
 static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
@@ -2783,7 +2758,6 @@ static coroutine_fn int sd_co_pdiscard(BlockDriverState *bs, int64_t offset,
     acb = sd_aio_setup(bs, &discard_iov, offset >> BDRV_SECTOR_BITS,
                        count >> BDRV_SECTOR_BITS);
     acb->aiocb_type = AIOCB_DISCARD_OBJ;
-    acb->aio_done_func = sd_finish_aiocb;
 
 retry:
     if (check_overlapping_aiocb(s, acb)) {
@@ -2791,20 +2765,13 @@ retry:
         goto retry;
     }
 
-    ret = sd_co_rw_vector(acb);
-    if (ret <= 0) {
-        QLIST_REMOVE(acb, aiocb_siblings);
-        qemu_co_queue_restart_all(&s->overlapping_queue);
-        qemu_aio_unref(acb);
-        return ret;
-    }
-
-    qemu_coroutine_yield();
+    sd_co_rw_vector(acb);
 
     QLIST_REMOVE(acb, aiocb_siblings);
     qemu_co_queue_restart_all(&s->overlapping_queue);
-
-    return acb->ret;
+    ret = acb->ret;
+    qemu_aio_unref(acb);
+    return ret;
 }
 
 static coroutine_fn int64_t
-- 
2.9.3

[Qemu-devel] [PATCH 3/5] sheepdog: do not use BlockAIOCB

[Paolo Bonzini]

Sheepdog's AIOCB are completely internal entities for a group of
requests and do not need dynamic allocation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 99 ++++++++++++++++++++++----------------------------------
 1 file changed, 39 insertions(+), 60 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 84b6645..b349f8f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -306,6 +306,7 @@ static inline size_t count_data_objs(const struct SheepdogInode *inode)
     } while (0)
 
 typedef struct SheepdogAIOCB SheepdogAIOCB;
+typedef struct BDRVSheepdogState BDRVSheepdogState;
 
 typedef struct AIOReq {
     SheepdogAIOCB *aiocb;
@@ -334,7 +335,7 @@ enum AIOCBState {
        || y->max_affect_data_idx < x->min_affect_data_idx))
 
 struct SheepdogAIOCB {
-    BlockAIOCB common;
+    BDRVSheepdogState *s;
 
     QEMUIOVector *qiov;
 
@@ -362,7 +363,7 @@ struct SheepdogAIOCB {
     QLIST_ENTRY(SheepdogAIOCB) aiocb_siblings;
 };
 
-typedef struct BDRVSheepdogState {
+struct BDRVSheepdogState {
     BlockDriverState *bs;
     AioContext *aio_context;
 
@@ -389,7 +390,7 @@ typedef struct BDRVSheepdogState {
 
     CoQueue overlapping_queue;
     QLIST_HEAD(inflight_aiocb_head, SheepdogAIOCB) inflight_aiocb_head;
-} BDRVSheepdogState;
+};
 
 typedef struct BDRVSheepdogReopenState {
     int fd;
@@ -488,20 +489,15 @@ static inline void free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
     acb->nr_pending--;
 }
 
-static const AIOCBInfo sd_aiocb_info = {
-    .aiocb_size     = sizeof(SheepdogAIOCB),
-};
-
-static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
-                                   int64_t sector_num, int nb_sectors)
+static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
+                         QEMUIOVector *qiov, int64_t sector_num, int nb_sectors,
+                         int type)
 {
-    SheepdogAIOCB *acb;
     uint32_t object_size;
-    BDRVSheepdogState *s = bs->opaque;
 
     object_size = (UINT32_C(1) << s->inode.block_size_shift);
 
-    acb = qemu_aio_get(&sd_aiocb_info, bs, NULL, NULL);
+    acb->s = s;
 
     acb->qiov = qiov;
 
@@ -518,8 +514,7 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
 
     acb->min_dirty_data_idx = UINT32_MAX;
     acb->max_dirty_data_idx = 0;
-
-    return acb;
+    acb->aiocb_type = type;
 }
 
 /* Return -EIO in case of error, file descriptor on success */
@@ -1965,7 +1960,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset)
  */
 static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
 {
-    BDRVSheepdogState *s = acb->common.bs->opaque;
+    BDRVSheepdogState *s = acb->s;
     struct iovec iov;
     AIOReq *aio_req;
     uint32_t offset, data_len, mn, mx;
@@ -2103,16 +2098,15 @@ out:
  * Returns 1 when we need to wait a response, 0 when there is no sent
  * request and -errno in error cases.
  */
-static void coroutine_fn sd_co_rw_vector(void *p)
+static void coroutine_fn sd_co_rw_vector(SheepdogAIOCB *acb)
 {
-    SheepdogAIOCB *acb = p;
     int ret = 0;
     unsigned long len, done = 0, total = acb->nb_sectors * BDRV_SECTOR_SIZE;
     unsigned long idx;
     uint32_t object_size;
     uint64_t oid;
     uint64_t offset;
-    BDRVSheepdogState *s = acb->common.bs->opaque;
+    BDRVSheepdogState *s = acb->s;
     SheepdogInode *inode = &s->inode;
     AIOReq *aio_req;
 
@@ -2220,7 +2214,7 @@ static bool check_overlapping_aiocb(BDRVSheepdogState *s, SheepdogAIOCB *aiocb)
 static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
                         int nb_sectors, QEMUIOVector *qiov)
 {
-    SheepdogAIOCB *acb;
+    SheepdogAIOCB acb;
     int ret;
     int64_t offset = (sector_num + nb_sectors) * BDRV_SECTOR_SIZE;
     BDRVSheepdogState *s = bs->opaque;
@@ -2232,76 +2226,65 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
         }
     }
 
-    acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors);
-    acb->aiocb_type = AIOCB_WRITE_UDATA;
+    sd_aio_setup(&acb, s, qiov, sector_num, nb_sectors, AIOCB_WRITE_UDATA);
 
 retry:
-    if (check_overlapping_aiocb(s, acb)) {
+    if (check_overlapping_aiocb(s, &acb)) {
         qemu_co_queue_wait(&s->overlapping_queue);
         goto retry;
     }
 
-    sd_co_rw_vector(acb);
-    sd_write_done(acb);
+    sd_co_rw_vector(&acb);
+    sd_write_done(&acb);
 
-    QLIST_REMOVE(acb, aiocb_siblings);
+    QLIST_REMOVE(&acb, aiocb_siblings);
     qemu_co_queue_restart_all(&s->overlapping_queue);
-    ret = acb->ret;
-    qemu_aio_unref(acb);
-    return ret;
+    return acb.ret;
 }
 
 static coroutine_fn int sd_co_readv(BlockDriverState *bs, int64_t sector_num,
                        int nb_sectors, QEMUIOVector *qiov)
 {
-    SheepdogAIOCB *acb;
-    int ret;
+    SheepdogAIOCB acb;
     BDRVSheepdogState *s = bs->opaque;
 
-    acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors);
-    acb->aiocb_type = AIOCB_READ_UDATA;
+    sd_aio_setup(&acb, s, qiov, sector_num, nb_sectors, AIOCB_READ_UDATA);
 
 retry:
-    if (check_overlapping_aiocb(s, acb)) {
+    if (check_overlapping_aiocb(s, &acb)) {
         qemu_co_queue_wait(&s->overlapping_queue);
         goto retry;
     }
 
-    sd_co_rw_vector(acb);
+    sd_co_rw_vector(&acb);
 
-    QLIST_REMOVE(acb, aiocb_siblings);
+    QLIST_REMOVE(&acb, aiocb_siblings);
     qemu_co_queue_restart_all(&s->overlapping_queue);
-    ret = acb->ret;
-    qemu_aio_unref(acb);
-    return ret;
+    return acb.ret;
 }
 
 static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
 {
     BDRVSheepdogState *s = bs->opaque;
-    SheepdogAIOCB *acb;
-    int ret;
+    SheepdogAIOCB acb;
     AIOReq *aio_req;
 
     if (s->cache_flags != SD_FLAG_CMD_CACHE) {
         return 0;
     }
 
-    acb = sd_aio_setup(bs, NULL, 0, 0);
-    acb->aiocb_type = AIOCB_FLUSH_CACHE;
+    sd_aio_setup(&acb, s, NULL, 0, 0, AIOCB_FLUSH_CACHE);
 
-    acb->nr_pending++;
-    aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
+    acb.nr_pending++;
+    aio_req = alloc_aio_req(s, &acb, vid_to_vdi_oid(s->inode.vdi_id),
                             0, 0, 0, false, 0, 0);
     QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
-    add_aio_request(s, aio_req, NULL, 0, acb->aiocb_type);
+    add_aio_request(s, aio_req, NULL, 0, acb.aiocb_type);
 
-    if (--acb->nr_pending) {
+    if (--acb.nr_pending) {
         qemu_coroutine_yield();
     }
-    ret = acb->ret;
-    qemu_aio_unref(acb);
-    return ret;
+    return acb.ret;
 }
 
 static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
@@ -2735,9 +2718,8 @@ static int sd_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
 static coroutine_fn int sd_co_pdiscard(BlockDriverState *bs, int64_t offset,
                                       int count)
 {
-    SheepdogAIOCB *acb;
+    SheepdogAIOCB acb;
     BDRVSheepdogState *s = bs->opaque;
-    int ret;
     QEMUIOVector discard_iov;
     struct iovec iov;
     uint32_t zero = 0;
@@ -2755,23 +2737,20 @@ static coroutine_fn int sd_co_pdiscard(BlockDriverState *bs, int64_t offset,
     if (!QEMU_IS_ALIGNED(offset | count, BDRV_SECTOR_SIZE)) {
         return -ENOTSUP;
     }
-    acb = sd_aio_setup(bs, &discard_iov, offset >> BDRV_SECTOR_BITS,
-                       count >> BDRV_SECTOR_BITS);
-    acb->aiocb_type = AIOCB_DISCARD_OBJ;
+    sd_aio_setup(&acb, s, &discard_iov, offset >> BDRV_SECTOR_BITS,
+                 count >> BDRV_SECTOR_BITS, AIOCB_DISCARD_OBJ);
 
 retry:
-    if (check_overlapping_aiocb(s, acb)) {
+    if (check_overlapping_aiocb(s, &acb)) {
         qemu_co_queue_wait(&s->overlapping_queue);
         goto retry;
     }
 
-    sd_co_rw_vector(acb);
+    sd_co_rw_vector(&acb);
 
-    QLIST_REMOVE(acb, aiocb_siblings);
+    QLIST_REMOVE(&acb, aiocb_siblings);
     qemu_co_queue_restart_all(&s->overlapping_queue);
-    ret = acb->ret;
-    qemu_aio_unref(acb);
-    return ret;
+    return acb.ret;
 }
 
 static coroutine_fn int64_t
-- 
2.9.3

[Qemu-devel] [PATCH 4/5] sheepdog: simplify inflight_aio_head management

[Paolo Bonzini]

Add to the list in add_aio_request and, indirectly, resend_aioreq.  Inline
free_aio_req in the caller, it does not simply undo alloc_aio_req's job.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index b349f8f..3cff5c3 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -479,16 +479,6 @@ static inline AIOReq *alloc_aio_req(BDRVSheepdogState *s, SheepdogAIOCB *acb,
     return aio_req;
 }
 
-static inline void free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
-{
-    SheepdogAIOCB *acb = aio_req->aiocb;
-
-    QLIST_REMOVE(aio_req, aio_siblings);
-    g_free(aio_req);
-
-    acb->nr_pending--;
-}
-
 static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
                          QEMUIOVector *qiov, int64_t sector_num, int nb_sectors,
                          int type)
@@ -730,7 +720,6 @@ static coroutine_fn void reconnect_to_sdog(void *opaque)
     while (!QLIST_EMPTY(&s->failed_aio_head)) {
         aio_req = QLIST_FIRST(&s->failed_aio_head);
         QLIST_REMOVE(aio_req, aio_siblings);
-        QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
         resend_aioreq(s, aio_req);
     }
 }
@@ -825,6 +814,7 @@ static void coroutine_fn aio_read_response(void *opaque)
     */
     s->co_recv = NULL;
 
+    QLIST_REMOVE(aio_req, aio_siblings);
     switch (rsp.result) {
     case SD_RES_SUCCESS:
         break;
@@ -849,8 +839,9 @@ static void coroutine_fn aio_read_response(void *opaque)
         break;
     }
 
-    free_aio_req(s, aio_req);
-    if (!acb->nr_pending) {
+    g_free(aio_req);
+
+    if (!--acb->nr_pending) {
         /*
          * We've finished all requests which belong to the AIOCB, so
          * we can switch back to sd_co_readv/writev now.
@@ -1108,6 +1099,8 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
     uint64_t old_oid = aio_req->base_oid;
     bool create = aio_req->create;
 
+    QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
+
     if (!nr_copies) {
         error_report("bug");
     }
@@ -1981,7 +1974,6 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
         iov.iov_len = sizeof(s->inode);
         aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
                                 data_len, offset, 0, false, 0, offset);
-        QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
         add_aio_request(s, aio_req, &iov, 1, AIOCB_WRITE_UDATA);
         if (--acb->nr_pending) {
             qemu_coroutine_yield();
@@ -2183,8 +2175,6 @@ static void coroutine_fn sd_co_rw_vector(SheepdogAIOCB *acb)
                                 old_oid,
                                 acb->aiocb_type == AIOCB_DISCARD_OBJ ?
                                 0 : done);
-        QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
-
         add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov,
                         acb->aiocb_type);
     done:
@@ -2278,7 +2268,6 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     acb.nr_pending++;
     aio_req = alloc_aio_req(s, &acb, vid_to_vdi_oid(s->inode.vdi_id),
                             0, 0, 0, false, 0, 0);
-    QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
     add_aio_request(s, aio_req, NULL, 0, acb.aiocb_type);
 
     if (--acb.nr_pending) {
-- 
2.9.3

[Qemu-devel] [PATCH 5/5] sheepdog: reorganize check for overlapping requests

[Paolo Bonzini]

Wrap the code that was copied repeatedly in the two functions,
sd_aio_setup and sd_aio_complete.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 66 ++++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 3cff5c3..189f499 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -479,6 +479,19 @@ static inline AIOReq *alloc_aio_req(BDRVSheepdogState *s, SheepdogAIOCB *acb,
     return aio_req;
 }
 
+static void wait_for_overlapping_aiocb(BDRVSheepdogState *s, SheepdogAIOCB *acb)
+{
+    SheepdogAIOCB *cb;
+
+retry:
+    QLIST_FOREACH(cb, &s->inflight_aiocb_head, aiocb_siblings) {
+        if (AIOCBOverlapping(acb, cb)) {
+            qemu_co_queue_wait(&s->overlapping_queue);
+            goto retry;
+        }
+    }
+}
+
 static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
                          QEMUIOVector *qiov, int64_t sector_num, int nb_sectors,
                          int type)
@@ -505,6 +518,13 @@ static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
     acb->min_dirty_data_idx = UINT32_MAX;
     acb->max_dirty_data_idx = 0;
     acb->aiocb_type = type;
+
+    if (type == AIOCB_FLUSH_CACHE) {
+        return;
+    }
+
+    wait_for_overlapping_aiocb(s, acb);
+    QLIST_INSERT_HEAD(&s->inflight_aiocb_head, acb, aiocb_siblings);
 }
 
 /* Return -EIO in case of error, file descriptor on success */
@@ -2187,18 +2207,14 @@ static void coroutine_fn sd_co_rw_vector(SheepdogAIOCB *acb)
     }
 }
 
-static bool check_overlapping_aiocb(BDRVSheepdogState *s, SheepdogAIOCB *aiocb)
+static void sd_aio_complete(SheepdogAIOCB *acb)
 {
-    SheepdogAIOCB *cb;
-
-    QLIST_FOREACH(cb, &s->inflight_aiocb_head, aiocb_siblings) {
-        if (AIOCBOverlapping(aiocb, cb)) {
-            return true;
-        }
+    if (acb->aiocb_type == AIOCB_FLUSH_CACHE) {
+        return;
     }
 
-    QLIST_INSERT_HEAD(&s->inflight_aiocb_head, aiocb, aiocb_siblings);
-    return false;
+    QLIST_REMOVE(acb, aiocb_siblings);
+    qemu_co_queue_restart_all(&acb->s->overlapping_queue);
 }
 
 static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
@@ -2217,18 +2233,10 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
     }
 
     sd_aio_setup(&acb, s, qiov, sector_num, nb_sectors, AIOCB_WRITE_UDATA);
-
-retry:
-    if (check_overlapping_aiocb(s, &acb)) {
-        qemu_co_queue_wait(&s->overlapping_queue);
-        goto retry;
-    }
-
     sd_co_rw_vector(&acb);
     sd_write_done(&acb);
+    sd_aio_complete(&acb);
 
-    QLIST_REMOVE(&acb, aiocb_siblings);
-    qemu_co_queue_restart_all(&s->overlapping_queue);
     return acb.ret;
 }
 
@@ -2239,17 +2247,9 @@ static coroutine_fn int sd_co_readv(BlockDriverState *bs, int64_t sector_num,
     BDRVSheepdogState *s = bs->opaque;
 
     sd_aio_setup(&acb, s, qiov, sector_num, nb_sectors, AIOCB_READ_UDATA);
-
-retry:
-    if (check_overlapping_aiocb(s, &acb)) {
-        qemu_co_queue_wait(&s->overlapping_queue);
-        goto retry;
-    }
-
     sd_co_rw_vector(&acb);
+    sd_aio_complete(&acb);
 
-    QLIST_REMOVE(&acb, aiocb_siblings);
-    qemu_co_queue_restart_all(&s->overlapping_queue);
     return acb.ret;
 }
 
@@ -2273,6 +2273,8 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     if (--acb.nr_pending) {
         qemu_coroutine_yield();
     }
+
+    sd_aio_complete(&acb);
     return acb.ret;
 }
 
@@ -2728,17 +2730,9 @@ static coroutine_fn int sd_co_pdiscard(BlockDriverState *bs, int64_t offset,
     }
     sd_aio_setup(&acb, s, &discard_iov, offset >> BDRV_SECTOR_BITS,
                  count >> BDRV_SECTOR_BITS, AIOCB_DISCARD_OBJ);
-
-retry:
-    if (check_overlapping_aiocb(s, &acb)) {
-        qemu_co_queue_wait(&s->overlapping_queue);
-        goto retry;
-    }
-
     sd_co_rw_vector(&acb);
+    sd_aio_complete(&acb);
 
-    QLIST_REMOVE(&acb, aiocb_siblings);
-    qemu_co_queue_restart_all(&s->overlapping_queue);
     return acb.ret;
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Paolo Bonzini]

On 29/11/2016 12:32, Paolo Bonzini wrote:
> Cleaning up the code and removing duplication makes it simpler to
> later adapt it for the multiqueue work.
> 
> Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
> to older snapshots, but the latter only worked with "dog vdi rollback".
> Neither loadvm nor qemu-img worked for me.
> 
> Paolo
> 
>         v1->v2: placate patchew
>         v2->v3: rebase
> 
> Paolo Bonzini (5):
>   sheepdog: remove unused cancellation support
>   sheepdog: reorganize coroutine flow
>   sheepdog: do not use BlockAIOCB
>   sheepdog: simplify inflight_aio_head management
>   sheepdog: reorganize check for overlapping requests
> 
>  block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
>  1 file changed, 83 insertions(+), 206 deletions(-)
> 

2.8 is now out, so ping.

Paolo

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Jeff Cody]

On Wed, Dec 21, 2016 at 03:07:07PM +0100, Paolo Bonzini wrote:
> 
> 
> On 29/11/2016 12:32, Paolo Bonzini wrote:
> > Cleaning up the code and removing duplication makes it simpler to
> > later adapt it for the multiqueue work.
> > 
> > Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
> > to older snapshots, but the latter only worked with "dog vdi rollback".
> > Neither loadvm nor qemu-img worked for me.
> > 
> > Paolo
> > 
> >         v1->v2: placate patchew
> >         v2->v3: rebase
> > 
> > Paolo Bonzini (5):
> >   sheepdog: remove unused cancellation support
> >   sheepdog: reorganize coroutine flow
> >   sheepdog: do not use BlockAIOCB
> >   sheepdog: simplify inflight_aio_head management
> >   sheepdog: reorganize check for overlapping requests
> > 
> >  block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
> >  1 file changed, 83 insertions(+), 206 deletions(-)
> > 
> 
> 2.8 is now out, so ping.
>

I don't have a functional sheepdog setup at the moment; have you tested
these patches, or should I set up a test rig for them? (I am guessing I
should do the latter; either way, I'll pull the patches in once I or someone
else has tested them).

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Paolo Bonzini]

On 04/01/2017 05:07, Jeff Cody wrote:
> On Wed, Dec 21, 2016 at 03:07:07PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 29/11/2016 12:32, Paolo Bonzini wrote:
>>> Cleaning up the code and removing duplication makes it simpler to
>>> later adapt it for the multiqueue work.
>>>
>>> Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
>>> to older snapshots, but the latter only worked with "dog vdi rollback".
>>> Neither loadvm nor qemu-img worked for me.
>>>
>>> Paolo
>>>
>>>         v1->v2: placate patchew
>>>         v2->v3: rebase
>>>
>>> Paolo Bonzini (5):
>>>   sheepdog: remove unused cancellation support
>>>   sheepdog: reorganize coroutine flow
>>>   sheepdog: do not use BlockAIOCB
>>>   sheepdog: simplify inflight_aio_head management
>>>   sheepdog: reorganize check for overlapping requests
>>>
>>>  block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
>>>  1 file changed, 83 insertions(+), 206 deletions(-)
>>>
>>
>> 2.8 is now out, so ping.
>>
> 
> I don't have a functional sheepdog setup at the moment; have you tested
> these patches, or should I set up a test rig for them? (I am guessing I
> should do the latter; either way, I'll pull the patches in once I or someone
> else has tested them).

Yes, I have tested them.  Fedora's sheepdog package is fubar right now.
I've built an updated one and asked for ownership but haven't had any
answer yet.

Paolo

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Jeff Cody]

On Wed, Jan 04, 2017 at 12:42:53PM +0100, Paolo Bonzini wrote:
> 
> 
> On 04/01/2017 05:07, Jeff Cody wrote:
> > On Wed, Dec 21, 2016 at 03:07:07PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 29/11/2016 12:32, Paolo Bonzini wrote:
> >>> Cleaning up the code and removing duplication makes it simpler to
> >>> later adapt it for the multiqueue work.
> >>>
> >>> Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
> >>> to older snapshots, but the latter only worked with "dog vdi rollback".
> >>> Neither loadvm nor qemu-img worked for me.
> >>>
> >>> Paolo
> >>>
> >>>         v1->v2: placate patchew
> >>>         v2->v3: rebase
> >>>
> >>> Paolo Bonzini (5):
> >>>   sheepdog: remove unused cancellation support
> >>>   sheepdog: reorganize coroutine flow
> >>>   sheepdog: do not use BlockAIOCB
> >>>   sheepdog: simplify inflight_aio_head management
> >>>   sheepdog: reorganize check for overlapping requests
> >>>
> >>>  block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
> >>>  1 file changed, 83 insertions(+), 206 deletions(-)
> >>>
> >>
> >> 2.8 is now out, so ping.
> >>
> > 
> > I don't have a functional sheepdog setup at the moment; have you tested
> > these patches, or should I set up a test rig for them? (I am guessing I
> > should do the latter; either way, I'll pull the patches in once I or someone
> > else has tested them).
> 
> Yes, I have tested them.  Fedora's sheepdog package is fubar right now.
> I've built an updated one and asked for ownership but haven't had any
> answer yet.
> 
> Paolo

Great, thanks.

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Paolo Bonzini]

On 04/01/2017 11:47, Jeff Cody wrote:
> On Wed, Jan 04, 2017 at 12:42:53PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 04/01/2017 05:07, Jeff Cody wrote:
>>> On Wed, Dec 21, 2016 at 03:07:07PM +0100, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 29/11/2016 12:32, Paolo Bonzini wrote:
>>>>> Cleaning up the code and removing duplication makes it simpler to
>>>>> later adapt it for the multiqueue work.
>>>>>
>>>>> Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
>>>>> to older snapshots, but the latter only worked with "dog vdi rollback".
>>>>> Neither loadvm nor qemu-img worked for me.
>>>>>
>>>>> Paolo
>>>>>
>>>>>         v1->v2: placate patchew
>>>>>         v2->v3: rebase
>>>>>
>>>>> Paolo Bonzini (5):
>>>>>   sheepdog: remove unused cancellation support
>>>>>   sheepdog: reorganize coroutine flow
>>>>>   sheepdog: do not use BlockAIOCB
>>>>>   sheepdog: simplify inflight_aio_head management
>>>>>   sheepdog: reorganize check for overlapping requests
>>>>>
>>>>>  block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
>>>>>  1 file changed, 83 insertions(+), 206 deletions(-)
>>>>>
>>>>
>>>> 2.8 is now out, so ping.
>>>>
>>>
>>> I don't have a functional sheepdog setup at the moment; have you tested
>>> these patches, or should I set up a test rig for them? (I am guessing I
>>> should do the latter; either way, I'll pull the patches in once I or someone
>>> else has tested them).
>>
>> Yes, I have tested them.  Fedora's sheepdog package is fubar right now.
>> I've built an updated one and asked for ownership but haven't had any
>> answer yet.
>>
>> Paolo
> 
> Great, thanks.
> 
> Applied to my block branch:
> 
> git://github.com/codyprime/qemu-kvm-jtc.git block
> 
> -Jeff
> 
> 

Ping?

Paolo

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Jeff Cody]

On Mon, Jan 30, 2017 at 07:48:56PM -0600, Paolo Bonzini wrote:
> 
> 
> On 04/01/2017 11:47, Jeff Cody wrote:
> > On Wed, Jan 04, 2017 at 12:42:53PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 04/01/2017 05:07, Jeff Cody wrote:
> >>> On Wed, Dec 21, 2016 at 03:07:07PM +0100, Paolo Bonzini wrote:
> >>>>
> >>>>
> >>>> On 29/11/2016 12:32, Paolo Bonzini wrote:
> >>>>> Cleaning up the code and removing duplication makes it simpler to
> >>>>> later adapt it for the multiqueue work.
> >>>>>
> >>>>> Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
> >>>>> to older snapshots, but the latter only worked with "dog vdi rollback".
> >>>>> Neither loadvm nor qemu-img worked for me.
> >>>>>
> >>>>> Paolo
> >>>>>
> >>>>>         v1->v2: placate patchew
> >>>>>         v2->v3: rebase
> >>>>>
> >>>>> Paolo Bonzini (5):
> >>>>>   sheepdog: remove unused cancellation support
> >>>>>   sheepdog: reorganize coroutine flow
> >>>>>   sheepdog: do not use BlockAIOCB
> >>>>>   sheepdog: simplify inflight_aio_head management
> >>>>>   sheepdog: reorganize check for overlapping requests
> >>>>>
> >>>>>  block/sheepdog.c | 289 ++++++++++++++++---------------------------------------
> >>>>>  1 file changed, 83 insertions(+), 206 deletions(-)
> >>>>>
> >>>>
> >>>> 2.8 is now out, so ping.
> >>>>
> >>>
> >>> I don't have a functional sheepdog setup at the moment; have you tested
> >>> these patches, or should I set up a test rig for them? (I am guessing I
> >>> should do the latter; either way, I'll pull the patches in once I or someone
> >>> else has tested them).
> >>
> >> Yes, I have tested them.  Fedora's sheepdog package is fubar right now.
> >> I've built an updated one and asked for ownership but haven't had any
> >> answer yet.
> >>
> >> Paolo
> > 
> > Great, thanks.
> > 
> > Applied to my block branch:
> > 
> > git://github.com/codyprime/qemu-kvm-jtc.git block
> > 
> > -Jeff
> > 
> > 
> 
> Ping?
> 

Sorry, never flushed my pull request queue.  Flushed now.

Thanks,
Jeff