[PATCH V2 0/7] block/rbd: migrate to coroutines and add write zeroes support

[PATCH V2 0/7] block/rbd: migrate to coroutines and add write zeroes support

[Peter Lieven]

this series migrates the qemu rbd driver from the old aio emulation
to native coroutines and adds write zeroes support which is important
for block operations.

To achive this we first bump the librbd requirement to the already
outdated luminous release of ceph to get rid of some wrappers and
ifdef'ry in the code.

V1->V2:
 - this patch is now rebased on top of current master with Paolos
   upcoming fixes for the meson.build script included:
    - meson: accept either shared or static libraries if --disable-static
    - meson: honor --enable-rbd if cc.links test fails
 - Patch 1: adjusted to meson.build script
 - Patch 2: unchanged
 - Patch 3: new patch
 - Patch 4: do not implement empty detach_aio_context callback [Jason]
 - Patch 5: - fix aio completion cleanup in error case [Jason]
            - return error codes from librbd
 - Patch 6: - add support for thick provisioning [Jason]
            - do not set write zeroes alignment
 - Patch 7: new patch

Peter Lieven (7):
  block/rbd: bump librbd requirement to luminous release
  block/rbd: store object_size in BDRVRBDState
  block/rbd: update s->image_size in qemu_rbd_getlength
  block/rbd: add bdrv_attach_aio_context
  block/rbd: migrate from aio to coroutines
  block/rbd: add write zeroes support
  block/rbd: drop qemu_rbd_refresh_limits

 block/rbd.c | 418 +++++++++++++++++-----------------------------------
 meson.build |  13 +-
 2 files changed, 142 insertions(+), 289 deletions(-)

-- 
2.17.1

[PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Peter Lieven]

even luminous (version 12.2) is unmaintained for over 3 years now.
Bump the requirement to get rid of the ifdef'ry in the code.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 120 ++++------------------------------------------------
 meson.build |  13 ++++--
 2 files changed, 17 insertions(+), 116 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..a191c74619 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -55,24 +55,10 @@
  * leading "\".
  */
 
-/* rbd_aio_discard added in 0.1.2 */
-#if LIBRBD_VERSION_CODE >= LIBRBD_VERSION(0, 1, 2)
-#define LIBRBD_SUPPORTS_DISCARD
-#else
-#undef LIBRBD_SUPPORTS_DISCARD
-#endif
-
 #define OBJ_MAX_SIZE (1UL << OBJ_DEFAULT_OBJ_ORDER)
 
 #define RBD_MAX_SNAPS 100
 
-/* The LIBRBD_SUPPORTS_IOVEC is defined in librbd.h */
-#ifdef LIBRBD_SUPPORTS_IOVEC
-#define LIBRBD_USE_IOVEC 1
-#else
-#define LIBRBD_USE_IOVEC 0
-#endif
-
 typedef enum {
     RBD_AIO_READ,
     RBD_AIO_WRITE,
@@ -84,7 +70,6 @@ typedef struct RBDAIOCB {
     BlockAIOCB common;
     int64_t ret;
     QEMUIOVector *qiov;
-    char *bounce;
     RBDAIOCmd cmd;
     int error;
     struct BDRVRBDState *s;
@@ -94,7 +79,6 @@ typedef struct RADOSCB {
     RBDAIOCB *acb;
     struct BDRVRBDState *s;
     int64_t size;
-    char *buf;
     int64_t ret;
 } RADOSCB;
 
@@ -332,13 +316,9 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
 
 static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
 {
-    if (LIBRBD_USE_IOVEC) {
-        RBDAIOCB *acb = rcb->acb;
-        iov_memset(acb->qiov->iov, acb->qiov->niov, offs, 0,
-                   acb->qiov->size - offs);
-    } else {
-        memset(rcb->buf + offs, 0, rcb->size - offs);
-    }
+    RBDAIOCB *acb = rcb->acb;
+    iov_memset(acb->qiov->iov, acb->qiov->niov, offs, 0,
+               acb->qiov->size - offs);
 }
 
 /* FIXME Deprecate and remove keypairs or make it available in QMP. */
@@ -493,13 +473,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
 
     g_free(rcb);
 
-    if (!LIBRBD_USE_IOVEC) {
-        if (acb->cmd == RBD_AIO_READ) {
-            qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size);
-        }
-        qemu_vfree(acb->bounce);
-    }
-
     acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
 
     qemu_aio_unref(acb);
@@ -866,28 +839,6 @@ static void rbd_finish_aiocb(rbd_completion_t c, RADOSCB *rcb)
                                      rbd_finish_bh, rcb);
 }
 
-static int rbd_aio_discard_wrapper(rbd_image_t image,
-                                   uint64_t off,
-                                   uint64_t len,
-                                   rbd_completion_t comp)
-{
-#ifdef LIBRBD_SUPPORTS_DISCARD
-    return rbd_aio_discard(image, off, len, comp);
-#else
-    return -ENOTSUP;
-#endif
-}
-
-static int rbd_aio_flush_wrapper(rbd_image_t image,
-                                 rbd_completion_t comp)
-{
-#ifdef LIBRBD_SUPPORTS_AIO_FLUSH
-    return rbd_aio_flush(image, comp);
-#else
-    return -ENOTSUP;
-#endif
-}
-
 static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
                                  int64_t off,
                                  QEMUIOVector *qiov,
@@ -910,21 +861,6 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
 
     rcb = g_new(RADOSCB, 1);
 
-    if (!LIBRBD_USE_IOVEC) {
-        if (cmd == RBD_AIO_DISCARD || cmd == RBD_AIO_FLUSH) {
-            acb->bounce = NULL;
-        } else {
-            acb->bounce = qemu_try_blockalign(bs, qiov->size);
-            if (acb->bounce == NULL) {
-                goto failed;
-            }
-        }
-        if (cmd == RBD_AIO_WRITE) {
-            qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
-        }
-        rcb->buf = acb->bounce;
-    }
-
     acb->ret = 0;
     acb->error = 0;
     acb->s = s;
@@ -938,7 +874,7 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
     }
 
     switch (cmd) {
-    case RBD_AIO_WRITE: {
+    case RBD_AIO_WRITE:
         /*
          * RBD APIs don't allow us to write more than actual size, so in order
          * to support growing images, we resize the image before write
@@ -950,25 +886,16 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
                 goto failed_completion;
             }
         }
-#ifdef LIBRBD_SUPPORTS_IOVEC
-            r = rbd_aio_writev(s->image, qiov->iov, qiov->niov, off, c);
-#else
-            r = rbd_aio_write(s->image, off, size, rcb->buf, c);
-#endif
+        r = rbd_aio_writev(s->image, qiov->iov, qiov->niov, off, c);
         break;
-    }
     case RBD_AIO_READ:
-#ifdef LIBRBD_SUPPORTS_IOVEC
-            r = rbd_aio_readv(s->image, qiov->iov, qiov->niov, off, c);
-#else
-            r = rbd_aio_read(s->image, off, size, rcb->buf, c);
-#endif
+        r = rbd_aio_readv(s->image, qiov->iov, qiov->niov, off, c);
         break;
     case RBD_AIO_DISCARD:
-        r = rbd_aio_discard_wrapper(s->image, off, size, c);
+        r = rbd_aio_discard(s->image, off, size, c);
         break;
     case RBD_AIO_FLUSH:
-        r = rbd_aio_flush_wrapper(s->image, c);
+        r = rbd_aio_flush(s->image, c);
         break;
     default:
         r = -EINVAL;
@@ -983,9 +910,6 @@ failed_completion:
     rbd_aio_release(c);
 failed:
     g_free(rcb);
-    if (!LIBRBD_USE_IOVEC) {
-        qemu_vfree(acb->bounce);
-    }
 
     qemu_aio_unref(acb);
     return NULL;
@@ -1011,7 +935,6 @@ static BlockAIOCB *qemu_rbd_aio_pwritev(BlockDriverState *bs,
                          RBD_AIO_WRITE);
 }
 
-#ifdef LIBRBD_SUPPORTS_AIO_FLUSH
 static BlockAIOCB *qemu_rbd_aio_flush(BlockDriverState *bs,
                                       BlockCompletionFunc *cb,
                                       void *opaque)
@@ -1019,20 +942,6 @@ static BlockAIOCB *qemu_rbd_aio_flush(BlockDriverState *bs,
     return rbd_start_aio(bs, 0, NULL, 0, cb, opaque, RBD_AIO_FLUSH);
 }
 
-#else
-
-static int qemu_rbd_co_flush(BlockDriverState *bs)
-{
-#if LIBRBD_VERSION_CODE >= LIBRBD_VERSION(0, 1, 1)
-    /* rbd_flush added in 0.1.1 */
-    BDRVRBDState *s = bs->opaque;
-    return rbd_flush(s->image);
-#else
-    return 0;
-#endif
-}
-#endif
-
 static int qemu_rbd_getinfo(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVRBDState *s = bs->opaque;
@@ -1198,7 +1107,6 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
     return snap_count;
 }
 
-#ifdef LIBRBD_SUPPORTS_DISCARD
 static BlockAIOCB *qemu_rbd_aio_pdiscard(BlockDriverState *bs,
                                          int64_t offset,
                                          int bytes,
@@ -1208,9 +1116,7 @@ static BlockAIOCB *qemu_rbd_aio_pdiscard(BlockDriverState *bs,
     return rbd_start_aio(bs, offset, NULL, bytes, cb, opaque,
                          RBD_AIO_DISCARD);
 }
-#endif
 
-#ifdef LIBRBD_SUPPORTS_INVALIDATE
 static void coroutine_fn qemu_rbd_co_invalidate_cache(BlockDriverState *bs,
                                                       Error **errp)
 {
@@ -1220,7 +1126,6 @@ static void coroutine_fn qemu_rbd_co_invalidate_cache(BlockDriverState *bs,
         error_setg_errno(errp, -r, "Failed to invalidate the cache");
     }
 }
-#endif
 
 static QemuOptsList qemu_rbd_create_opts = {
     .name = "rbd-create-opts",
@@ -1278,23 +1183,14 @@ static BlockDriver bdrv_rbd = {
     .bdrv_aio_preadv        = qemu_rbd_aio_preadv,
     .bdrv_aio_pwritev       = qemu_rbd_aio_pwritev,
 
-#ifdef LIBRBD_SUPPORTS_AIO_FLUSH
     .bdrv_aio_flush         = qemu_rbd_aio_flush,
-#else
-    .bdrv_co_flush_to_disk  = qemu_rbd_co_flush,
-#endif
-
-#ifdef LIBRBD_SUPPORTS_DISCARD
     .bdrv_aio_pdiscard      = qemu_rbd_aio_pdiscard,
-#endif
 
     .bdrv_snapshot_create   = qemu_rbd_snap_create,
     .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
     .bdrv_snapshot_list     = qemu_rbd_snap_list,
     .bdrv_snapshot_goto     = qemu_rbd_snap_rollback,
-#ifdef LIBRBD_SUPPORTS_INVALIDATE
     .bdrv_co_invalidate_cache = qemu_rbd_co_invalidate_cache,
-#endif
 
     .strong_runtime_opts    = qemu_rbd_strong_runtime_opts,
 };
diff --git a/meson.build b/meson.build
index 5943aa8a51..02d263ad33 100644
--- a/meson.build
+++ b/meson.build
@@ -691,19 +691,24 @@ if not get_option('rbd').auto() or have_block
                            required: get_option('rbd'),
                            kwargs: static_kwargs)
   if librados.found() and librbd.found()
-    if cc.links('''
+    result = cc.run('''
       #include <stdio.h>
       #include <rbd/librbd.h>
       int main(void) {
         rados_t cluster;
         rados_create(&cluster, NULL);
+        rados_shutdown(cluster);
+        #if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 12, 0)
+        return 1;
+        #endif
         return 0;
-      }''', dependencies: [librbd, librados])
+    }''', dependencies: [librbd, librados], name: 'librbd version check')
+    if result.compiled() and result.returncode() == 0
       rbd = declare_dependency(dependencies: [librbd, librados])
     elif get_option('rbd').enabled()
-      error('could not link librados')
+      error('librados/librbd >= 12.0.0 required')
     else
-      warning('could not link librados, disabling')
+      warning('librados/librbd >= 12.0.0 not found, disabling rbd support')
     endif
   endif
 endif
-- 
2.17.1

[PATCH V2 2/7] block/rbd: store object_size in BDRVRBDState

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index a191c74619..1028596c68 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -90,6 +90,7 @@ typedef struct BDRVRBDState {
     char *snap;
     char *namespace;
     uint64_t image_size;
+    uint64_t object_size;
 } BDRVRBDState;
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
@@ -663,6 +664,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     const QDictEntry *e;
     Error *local_err = NULL;
     char *keypairs, *secretid;
+    rbd_image_info_t info;
     int r;
 
     keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
@@ -727,13 +729,15 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    r = rbd_get_size(s->image, &s->image_size);
+    r = rbd_stat(s->image, &info, sizeof(info));
     if (r < 0) {
-        error_setg_errno(errp, -r, "error getting image size from %s",
+        error_setg_errno(errp, -r, "error getting image info from %s",
                          s->image_name);
         rbd_close(s->image);
         goto failed_open;
     }
+    s->image_size = info.size;
+    s->object_size = info.obj_size;
 
     /* If we are using an rbd snapshot, we must be r/o, otherwise
      * leave as-is */
@@ -945,15 +949,7 @@ static BlockAIOCB *qemu_rbd_aio_flush(BlockDriverState *bs,
 static int qemu_rbd_getinfo(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVRBDState *s = bs->opaque;
-    rbd_image_info_t info;
-    int r;
-
-    r = rbd_stat(s->image, &info, sizeof(info));
-    if (r < 0) {
-        return r;
-    }
-
-    bdi->cluster_size = info.obj_size;
+    bdi->cluster_size = s->object_size;
     return 0;
 }
 
-- 
2.17.1

[PATCH V2 3/7] block/rbd: update s->image_size in qemu_rbd_getlength

[Peter Lieven]

in case the image size changed we should adjust our internally stored size as well.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/rbd.c b/block/rbd.c
index 1028596c68..f68ebcf240 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -964,6 +964,7 @@ static int64_t qemu_rbd_getlength(BlockDriverState *bs)
         return r;
     }
 
+    s->image_size = info.size;
     return info.size;
 }
 
-- 
2.17.1

[PATCH V2 4/7] block/rbd: add bdrv_attach_aio_context

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index f68ebcf240..7abd0252c9 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -91,6 +91,7 @@ typedef struct BDRVRBDState {
     char *namespace;
     uint64_t image_size;
     uint64_t object_size;
+    AioContext *aio_context;
 } BDRVRBDState;
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
@@ -749,6 +750,8 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
+    s->aio_context = bdrv_get_aio_context(bs);
+
     /* When extending regular files, we get zeros from the OS */
     bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
 
@@ -839,8 +842,7 @@ static void rbd_finish_aiocb(rbd_completion_t c, RADOSCB *rcb)
     rcb->ret = rbd_aio_get_return_value(c);
     rbd_aio_release(c);
 
-    replay_bh_schedule_oneshot_event(bdrv_get_aio_context(acb->common.bs),
-                                     rbd_finish_bh, rcb);
+    replay_bh_schedule_oneshot_event(acb->s->aio_context, rbd_finish_bh, rcb);
 }
 
 static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
@@ -1160,6 +1162,13 @@ static const char *const qemu_rbd_strong_runtime_opts[] = {
     NULL
 };
 
+static void qemu_rbd_attach_aio_context(BlockDriverState *bs,
+                                       AioContext *new_context)
+{
+    BDRVRBDState *s = bs->opaque;
+    s->aio_context = new_context;
+}
+
 static BlockDriver bdrv_rbd = {
     .format_name            = "rbd",
     .instance_size          = sizeof(BDRVRBDState),
@@ -1189,6 +1198,8 @@ static BlockDriver bdrv_rbd = {
     .bdrv_snapshot_goto     = qemu_rbd_snap_rollback,
     .bdrv_co_invalidate_cache = qemu_rbd_co_invalidate_cache,
 
+    .bdrv_attach_aio_context  = qemu_rbd_attach_aio_context,
+
     .strong_runtime_opts    = qemu_rbd_strong_runtime_opts,
 };
 
-- 
2.17.1

[PATCH V2 5/7] block/rbd: migrate from aio to coroutines

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 253 ++++++++++++++++++----------------------------------
 1 file changed, 86 insertions(+), 167 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 7abd0252c9..d11a3c6dd1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -66,22 +66,6 @@ typedef enum {
     RBD_AIO_FLUSH
 } RBDAIOCmd;
 
-typedef struct RBDAIOCB {
-    BlockAIOCB common;
-    int64_t ret;
-    QEMUIOVector *qiov;
-    RBDAIOCmd cmd;
-    int error;
-    struct BDRVRBDState *s;
-} RBDAIOCB;
-
-typedef struct RADOSCB {
-    RBDAIOCB *acb;
-    struct BDRVRBDState *s;
-    int64_t size;
-    int64_t ret;
-} RADOSCB;
-
 typedef struct BDRVRBDState {
     rados_t cluster;
     rados_ioctx_t io_ctx;
@@ -94,6 +78,13 @@ typedef struct BDRVRBDState {
     AioContext *aio_context;
 } BDRVRBDState;
 
+typedef struct RBDTask {
+    BDRVRBDState *s;
+    Coroutine *co;
+    bool complete;
+    int64_t ret;
+} RBDTask;
+
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
                             BlockdevOptionsRbd *opts, bool cache,
                             const char *keypairs, const char *secretid,
@@ -316,13 +307,6 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
     return ret;
 }
 
-static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
-{
-    RBDAIOCB *acb = rcb->acb;
-    iov_memset(acb->qiov->iov, acb->qiov->niov, offs, 0,
-               acb->qiov->size - offs);
-}
-
 /* FIXME Deprecate and remove keypairs or make it available in QMP. */
 static int qemu_rbd_do_create(BlockdevCreateOptions *options,
                               const char *keypairs, const char *password_secret,
@@ -440,46 +424,6 @@ exit:
     return ret;
 }
 
-/*
- * This aio completion is being called from rbd_finish_bh() and runs in qemu
- * BH context.
- */
-static void qemu_rbd_complete_aio(RADOSCB *rcb)
-{
-    RBDAIOCB *acb = rcb->acb;
-    int64_t r;
-
-    r = rcb->ret;
-
-    if (acb->cmd != RBD_AIO_READ) {
-        if (r < 0) {
-            acb->ret = r;
-            acb->error = 1;
-        } else if (!acb->error) {
-            acb->ret = rcb->size;
-        }
-    } else {
-        if (r < 0) {
-            qemu_rbd_memset(rcb, 0);
-            acb->ret = r;
-            acb->error = 1;
-        } else if (r < rcb->size) {
-            qemu_rbd_memset(rcb, r);
-            if (!acb->error) {
-                acb->ret = rcb->size;
-            }
-        } else if (!acb->error) {
-            acb->ret = r;
-        }
-    }
-
-    g_free(rcb);
-
-    acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
-
-    qemu_aio_unref(acb);
-}
-
 static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp)
 {
     const char **vals;
@@ -817,88 +761,49 @@ static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
     return 0;
 }
 
-static const AIOCBInfo rbd_aiocb_info = {
-    .aiocb_size = sizeof(RBDAIOCB),
-};
-
-static void rbd_finish_bh(void *opaque)
+static void qemu_rbd_finish_bh(void *opaque)
 {
-    RADOSCB *rcb = opaque;
-    qemu_rbd_complete_aio(rcb);
+    RBDTask *task = opaque;
+    task->complete = 1;
+    aio_co_wake(task->co);
 }
 
-/*
- * This is the callback function for rbd_aio_read and _write
- *
- * Note: this function is being called from a non qemu thread so
- * we need to be careful about what we do here. Generally we only
- * schedule a BH, and do the rest of the io completion handling
- * from rbd_finish_bh() which runs in a qemu context.
- */
-static void rbd_finish_aiocb(rbd_completion_t c, RADOSCB *rcb)
+static void qemu_rbd_completion_cb(rbd_completion_t c, RBDTask *task)
 {
-    RBDAIOCB *acb = rcb->acb;
-
-    rcb->ret = rbd_aio_get_return_value(c);
+    task->ret = rbd_aio_get_return_value(c);
     rbd_aio_release(c);
-
-    replay_bh_schedule_oneshot_event(acb->s->aio_context, rbd_finish_bh, rcb);
+    aio_bh_schedule_oneshot(task->s->aio_context, qemu_rbd_finish_bh, task);
 }
 
-static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
-                                 int64_t off,
-                                 QEMUIOVector *qiov,
-                                 int64_t size,
-                                 BlockCompletionFunc *cb,
-                                 void *opaque,
-                                 RBDAIOCmd cmd)
+static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs,
+                                          uint64_t offset,
+                                          uint64_t bytes,
+                                          QEMUIOVector *qiov,
+                                          int flags,
+                                          RBDAIOCmd cmd)
 {
-    RBDAIOCB *acb;
-    RADOSCB *rcb = NULL;
+    BDRVRBDState *s = bs->opaque;
+    RBDTask task = { .s = s, .co = qemu_coroutine_self() };
     rbd_completion_t c;
     int r;
 
-    BDRVRBDState *s = bs->opaque;
-
-    acb = qemu_aio_get(&rbd_aiocb_info, bs, cb, opaque);
-    acb->cmd = cmd;
-    acb->qiov = qiov;
-    assert(!qiov || qiov->size == size);
-
-    rcb = g_new(RADOSCB, 1);
+    assert(!qiov || qiov->size == bytes);
 
-    acb->ret = 0;
-    acb->error = 0;
-    acb->s = s;
-
-    rcb->acb = acb;
-    rcb->s = acb->s;
-    rcb->size = size;
-    r = rbd_aio_create_completion(rcb, (rbd_callback_t) rbd_finish_aiocb, &c);
+    r = rbd_aio_create_completion(&task,
+                                  (rbd_callback_t) qemu_rbd_completion_cb, &c);
     if (r < 0) {
-        goto failed;
+        return r;
     }
 
     switch (cmd) {
-    case RBD_AIO_WRITE:
-        /*
-         * RBD APIs don't allow us to write more than actual size, so in order
-         * to support growing images, we resize the image before write
-         * operations that exceed the current size.
-         */
-        if (off + size > s->image_size) {
-            r = qemu_rbd_resize(bs, off + size);
-            if (r < 0) {
-                goto failed_completion;
-            }
-        }
-        r = rbd_aio_writev(s->image, qiov->iov, qiov->niov, off, c);
-        break;
     case RBD_AIO_READ:
-        r = rbd_aio_readv(s->image, qiov->iov, qiov->niov, off, c);
+        r = rbd_aio_readv(s->image, qiov->iov, qiov->niov, offset, c);
+        break;
+    case RBD_AIO_WRITE:
+        r = rbd_aio_writev(s->image, qiov->iov, qiov->niov, offset, c);
         break;
     case RBD_AIO_DISCARD:
-        r = rbd_aio_discard(s->image, off, size, c);
+        r = rbd_aio_discard(s->image, offset, bytes, c);
         break;
     case RBD_AIO_FLUSH:
         r = rbd_aio_flush(s->image, c);
@@ -908,44 +813,69 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
     }
 
     if (r < 0) {
-        goto failed_completion;
+        error_report("rbd request failed early: cmd %d offset %" PRIu64
+                     " bytes %" PRIu64 " flags %d r %d (%s)", cmd, offset,
+                     bytes, flags, r, strerror(-r));
+        rbd_aio_release(c);
+        return r;
     }
-    return &acb->common;
 
-failed_completion:
-    rbd_aio_release(c);
-failed:
-    g_free(rcb);
+    while (!task.complete) {
+        qemu_coroutine_yield();
+    }
+
+    if (task.ret < 0) {
+        error_report("rbd request failed: cmd %d offset %" PRIu64 " bytes %"
+                     PRIu64 " flags %d task.ret %" PRIi64 " (%s)", cmd, offset,
+                     bytes, flags, task.ret, strerror(-task.ret));
+        return task.ret;
+    }
 
-    qemu_aio_unref(acb);
-    return NULL;
+    /* zero pad short reads */
+    if (cmd == RBD_AIO_READ && task.ret < qiov->size) {
+        qemu_iovec_memset(qiov, task.ret, 0, qiov->size - task.ret);
+    }
+
+    return 0;
+}
+
+static int
+coroutine_fn qemu_rbd_co_preadv(BlockDriverState *bs, uint64_t offset,
+                               uint64_t bytes, QEMUIOVector *qiov,
+                               int flags)
+{
+    return qemu_rbd_start_co(bs, offset, bytes, qiov, flags, RBD_AIO_READ);
 }
 
-static BlockAIOCB *qemu_rbd_aio_preadv(BlockDriverState *bs,
-                                       uint64_t offset, uint64_t bytes,
-                                       QEMUIOVector *qiov, int flags,
-                                       BlockCompletionFunc *cb,
-                                       void *opaque)
+static int
+coroutine_fn qemu_rbd_co_pwritev(BlockDriverState *bs, uint64_t offset,
+                                 uint64_t bytes, QEMUIOVector *qiov,
+                                 int flags)
 {
-    return rbd_start_aio(bs, offset, qiov, bytes, cb, opaque,
-                         RBD_AIO_READ);
+    BDRVRBDState *s = bs->opaque;
+    /*
+     * RBD APIs don't allow us to write more than actual size, so in order
+     * to support growing images, we resize the image before write
+     * operations that exceed the current size.
+     */
+    if (offset + bytes > s->image_size) {
+        int r = qemu_rbd_resize(bs, offset + bytes);
+        if (r < 0) {
+            return r;
+        }
+    }
+    return qemu_rbd_start_co(bs, offset, bytes, qiov, flags, RBD_AIO_WRITE);
 }
 
-static BlockAIOCB *qemu_rbd_aio_pwritev(BlockDriverState *bs,
-                                        uint64_t offset, uint64_t bytes,
-                                        QEMUIOVector *qiov, int flags,
-                                        BlockCompletionFunc *cb,
-                                        void *opaque)
+static int coroutine_fn qemu_rbd_co_flush(BlockDriverState *bs)
 {
-    return rbd_start_aio(bs, offset, qiov, bytes, cb, opaque,
-                         RBD_AIO_WRITE);
+    return qemu_rbd_start_co(bs, 0, 0, NULL, 0, RBD_AIO_FLUSH);
 }
 
-static BlockAIOCB *qemu_rbd_aio_flush(BlockDriverState *bs,
-                                      BlockCompletionFunc *cb,
-                                      void *opaque)
+static int coroutine_fn qemu_rbd_co_pdiscard(BlockDriverState *bs,
+                                             int64_t offset, int count)
 {
-    return rbd_start_aio(bs, 0, NULL, 0, cb, opaque, RBD_AIO_FLUSH);
+    return qemu_rbd_start_co(bs, offset, count, NULL, 0, RBD_AIO_DISCARD);
 }
 
 static int qemu_rbd_getinfo(BlockDriverState *bs, BlockDriverInfo *bdi)
@@ -1106,16 +1036,6 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
     return snap_count;
 }
 
-static BlockAIOCB *qemu_rbd_aio_pdiscard(BlockDriverState *bs,
-                                         int64_t offset,
-                                         int bytes,
-                                         BlockCompletionFunc *cb,
-                                         void *opaque)
-{
-    return rbd_start_aio(bs, offset, NULL, bytes, cb, opaque,
-                         RBD_AIO_DISCARD);
-}
-
 static void coroutine_fn qemu_rbd_co_invalidate_cache(BlockDriverState *bs,
                                                       Error **errp)
 {
@@ -1186,11 +1106,10 @@ static BlockDriver bdrv_rbd = {
     .bdrv_co_truncate       = qemu_rbd_co_truncate,
     .protocol_name          = "rbd",
 
-    .bdrv_aio_preadv        = qemu_rbd_aio_preadv,
-    .bdrv_aio_pwritev       = qemu_rbd_aio_pwritev,
-
-    .bdrv_aio_flush         = qemu_rbd_aio_flush,
-    .bdrv_aio_pdiscard      = qemu_rbd_aio_pdiscard,
+    .bdrv_co_preadv         = qemu_rbd_co_preadv,
+    .bdrv_co_pwritev        = qemu_rbd_co_pwritev,
+    .bdrv_co_flush_to_disk  = qemu_rbd_co_flush,
+    .bdrv_co_pdiscard       = qemu_rbd_co_pdiscard,
 
     .bdrv_snapshot_create   = qemu_rbd_snap_create,
     .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
-- 
2.17.1

[PATCH V2 6/7] block/rbd: add write zeroes support

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/block/rbd.c b/block/rbd.c
index d11a3c6dd1..35dc1dc90e 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -63,7 +63,8 @@ typedef enum {
     RBD_AIO_READ,
     RBD_AIO_WRITE,
     RBD_AIO_DISCARD,
-    RBD_AIO_FLUSH
+    RBD_AIO_FLUSH,
+    RBD_AIO_WRITE_ZEROES
 } RBDAIOCmd;
 
 typedef struct BDRVRBDState {
@@ -695,6 +696,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->aio_context = bdrv_get_aio_context(bs);
+#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
+    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
+#endif
 
     /* When extending regular files, we get zeros from the OS */
     bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
@@ -808,6 +812,18 @@ static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs,
     case RBD_AIO_FLUSH:
         r = rbd_aio_flush(s->image, c);
         break;
+#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
+    case RBD_AIO_WRITE_ZEROES: {
+        int zero_flags = 0;
+#ifdef RBD_WRITE_ZEROES_FLAG_THICK_PROVISION
+        if (!(flags & BDRV_REQ_MAY_UNMAP)) {
+            zero_flags = RBD_WRITE_ZEROES_FLAG_THICK_PROVISION;
+        }
+#endif
+        r = rbd_aio_write_zeroes(s->image, offset, bytes, c, zero_flags, 0);
+        break;
+    }
+#endif
     default:
         r = -EINVAL;
     }
@@ -878,6 +894,21 @@ static int coroutine_fn qemu_rbd_co_pdiscard(BlockDriverState *bs,
     return qemu_rbd_start_co(bs, offset, count, NULL, 0, RBD_AIO_DISCARD);
 }
 
+#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
+static int
+coroutine_fn qemu_rbd_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
+                                      int count, BdrvRequestFlags flags)
+{
+#ifndef RBD_WRITE_ZEROES_FLAG_THICK_PROVISION
+    if (!(flags & BDRV_REQ_MAY_UNMAP)) {
+        return -ENOTSUP;
+    }
+#endif
+    return qemu_rbd_start_co(bs, offset, count, NULL, flags,
+                             RBD_AIO_WRITE_ZEROES);
+}
+#endif
+
 static int qemu_rbd_getinfo(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVRBDState *s = bs->opaque;
@@ -1110,6 +1141,9 @@ static BlockDriver bdrv_rbd = {
     .bdrv_co_pwritev        = qemu_rbd_co_pwritev,
     .bdrv_co_flush_to_disk  = qemu_rbd_co_flush,
     .bdrv_co_pdiscard       = qemu_rbd_co_pdiscard,
+#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
+    .bdrv_co_pwrite_zeroes  = qemu_rbd_co_pwrite_zeroes,
+#endif
 
     .bdrv_snapshot_create   = qemu_rbd_snap_create,
     .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
-- 
2.17.1

[PATCH V2 7/7] block/rbd: drop qemu_rbd_refresh_limits

[Peter Lieven]

librbd supports 1 byte alignment for all aio operations.

Currently, there is no API call to query limits from the ceph backend.
So drop the bdrv_refresh_limits completely until there is such an API call.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/rbd.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 35dc1dc90e..5f96fbf3d1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -219,14 +219,6 @@ done:
     return;
 }
 
-
-static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
-{
-    /* XXX Does RBD support AIO on less than 512-byte alignment? */
-    bs->bl.request_alignment = 512;
-}
-
-
 static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
                              Error **errp)
 {
@@ -1124,7 +1116,6 @@ static BlockDriver bdrv_rbd = {
     .format_name            = "rbd",
     .instance_size          = sizeof(BDRVRBDState),
     .bdrv_parse_filename    = qemu_rbd_parse_filename,
-    .bdrv_refresh_limits    = qemu_rbd_refresh_limits,
     .bdrv_file_open         = qemu_rbd_open,
     .bdrv_close             = qemu_rbd_close,
     .bdrv_reopen_prepare    = qemu_rbd_reopen_prepare,
-- 
2.17.1

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Kevin Wolf]

Am 26.01.2021 um 12:25 hat Peter Lieven geschrieben:
> even luminous (version 12.2) is unmaintained for over 3 years now.
> Bump the requirement to get rid of the ifdef'ry in the code.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>

> diff --git a/meson.build b/meson.build
> index 5943aa8a51..02d263ad33 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -691,19 +691,24 @@ if not get_option('rbd').auto() or have_block
>                             required: get_option('rbd'),
>                             kwargs: static_kwargs)
>    if librados.found() and librbd.found()
> -    if cc.links('''
> +    result = cc.run('''

Doesn't running compiled binaries break cross compilation?

>        #include <stdio.h>
>        #include <rbd/librbd.h>
>        int main(void) {
>          rados_t cluster;
>          rados_create(&cluster, NULL);
> +        rados_shutdown(cluster);
> +        #if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 12, 0)
> +        return 1;
> +        #endif
>          return 0;

Would #error achieve what you want without running the binary?

But most, if not all, other version checks use pkg-config instead of
trying to compile code, so that's probably what we should be doing here,
too.

> -      }''', dependencies: [librbd, librados])
> +    }''', dependencies: [librbd, librados], name: 'librbd version check')
> +    if result.compiled() and result.returncode() == 0
>        rbd = declare_dependency(dependencies: [librbd, librados])
>      elif get_option('rbd').enabled()
> -      error('could not link librados')
> +      error('librados/librbd >= 12.0.0 required')
>      else
> -      warning('could not link librados, disabling')
> +      warning('librados/librbd >= 12.0.0 not found, disabling rbd support')
>      endif
>    endif
>  endif

Kevin

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Daniel P. Berrangé]

On Mon, Feb 15, 2021 at 11:11:23AM +0100, Kevin Wolf wrote:
> Am 26.01.2021 um 12:25 hat Peter Lieven geschrieben:
> > even luminous (version 12.2) is unmaintained for over 3 years now.
> > Bump the requirement to get rid of the ifdef'ry in the code.
> > 
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> 
> > diff --git a/meson.build b/meson.build
> > index 5943aa8a51..02d263ad33 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -691,19 +691,24 @@ if not get_option('rbd').auto() or have_block
> >                             required: get_option('rbd'),
> >                             kwargs: static_kwargs)
> >    if librados.found() and librbd.found()
> > -    if cc.links('''
> > +    result = cc.run('''
> 
> Doesn't running compiled binaries break cross compilation?
> 
> >        #include <stdio.h>
> >        #include <rbd/librbd.h>
> >        int main(void) {
> >          rados_t cluster;
> >          rados_create(&cluster, NULL);
> > +        rados_shutdown(cluster);
> > +        #if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 12, 0)
> > +        return 1;
> > +        #endif
> >          return 0;
> 
> Would #error achieve what you want without running the binary?
> 
> But most, if not all, other version checks use pkg-config instead of
> trying to compile code, so that's probably what we should be doing here,
> too.

Yep, for something that is merely a version number check there's no
need to compile anything. pkg-config can just validate the version
straightup.

> 
> > -      }''', dependencies: [librbd, librados])
> > +    }''', dependencies: [librbd, librados], name: 'librbd version check')
> > +    if result.compiled() and result.returncode() == 0
> >        rbd = declare_dependency(dependencies: [librbd, librados])
> >      elif get_option('rbd').enabled()
> > -      error('could not link librados')
> > +      error('librados/librbd >= 12.0.0 required')
> >      else
> > -      warning('could not link librados, disabling')
> > +      warning('librados/librbd >= 12.0.0 not found, disabling rbd support')
> >      endif
> >    endif
> >  endif

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH V2 4/7] block/rbd: add bdrv_attach_aio_context

[Kevin Wolf]

Am 26.01.2021 um 12:25 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/rbd.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index f68ebcf240..7abd0252c9 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -91,6 +91,7 @@ typedef struct BDRVRBDState {
>      char *namespace;
>      uint64_t image_size;
>      uint64_t object_size;
> +    AioContext *aio_context;
>  } BDRVRBDState;

A commit message explaining the why would be helpful here.

This is already stored in BlockDriverState, which should be available
everywhere. Keeping redundant information needs a good justification,
which seems unlikely when BlockDriverState and BDRVRBDState are already
connected through the BlockDriverState.opaque pointer.

The rest of the series doesn't seem to make more use of it either.

Kevin

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Daniel P. Berrangé]

On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
> even luminous (version 12.2) is unmaintained for over 3 years now.
> Bump the requirement to get rid of the ifdef'ry in the code.

We have clear rules on when we bump minimum versions, determined by
the OS platforms we target:

  https://qemu.readthedocs.io/en/latest/system/build-platforms.html

At this time RHEL-7 is usually the oldest platform, and it
builds with RBD 10.2.5, so we can't bump the version to 12.2.

I'm afraid this patch has to be dropped.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Peter Lieven]

Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
> On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
>> even luminous (version 12.2) is unmaintained for over 3 years now.
>> Bump the requirement to get rid of the ifdef'ry in the code.
> We have clear rules on when we bump minimum versions, determined by
> the OS platforms we target:
>
>    https://qemu.readthedocs.io/en/latest/system/build-platforms.html
>
> At this time RHEL-7 is usually the oldest platform, and it
> builds with RBD 10.2.5, so we can't bump the version to 12.2.
>
> I'm afraid this patch has to be dropped.


I have asked exactly this question before I started work on this series and got reply

from Jason that he sees no problem in bumping to a release which is already unmaintained

for 3 years.


If qemu 6.0 is required to build on RHEL-7 than I am afraid we can abandon the whole series.


Peter

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Peter Lieven]

Am 15.02.21 um 11:19 schrieb Daniel P. Berrangé:
> On Mon, Feb 15, 2021 at 11:11:23AM +0100, Kevin Wolf wrote:
>> Am 26.01.2021 um 12:25 hat Peter Lieven geschrieben:
>>> even luminous (version 12.2) is unmaintained for over 3 years now.
>>> Bump the requirement to get rid of the ifdef'ry in the code.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> diff --git a/meson.build b/meson.build
>>> index 5943aa8a51..02d263ad33 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -691,19 +691,24 @@ if not get_option('rbd').auto() or have_block
>>>                              required: get_option('rbd'),
>>>                              kwargs: static_kwargs)
>>>     if librados.found() and librbd.found()
>>> -    if cc.links('''
>>> +    result = cc.run('''
>> Doesn't running compiled binaries break cross compilation?
>>
>>>         #include <stdio.h>
>>>         #include <rbd/librbd.h>
>>>         int main(void) {
>>>           rados_t cluster;
>>>           rados_create(&cluster, NULL);
>>> +        rados_shutdown(cluster);
>>> +        #if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 12, 0)
>>> +        return 1;
>>> +        #endif
>>>           return 0;
>> Would #error achieve what you want without running the binary?
>>
>> But most, if not all, other version checks use pkg-config instead of
>> trying to compile code, so that's probably what we should be doing here,
>> too.
> Yep, for something that is merely a version number check there's no
> need to compile anything. pkg-config can just validate the version
> straightup.


I would have loved to, but at least the Ubuntu/Debian packages do not contain

pkg-config files. I can switch to #error, of course. My initial version of the patch

distinguished between can't compile and version is too old. With #error we just

can say doesn't compile, but I think this would be ok.


Peter

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Daniel P. Berrangé]

On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
> Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
> > On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
> > > even luminous (version 12.2) is unmaintained for over 3 years now.
> > > Bump the requirement to get rid of the ifdef'ry in the code.
> > We have clear rules on when we bump minimum versions, determined by
> > the OS platforms we target:
> > 
> >    https://qemu.readthedocs.io/en/latest/system/build-platforms.html
> > 
> > At this time RHEL-7 is usually the oldest platform, and it
> > builds with RBD 10.2.5, so we can't bump the version to 12.2.
> > 
> > I'm afraid this patch has to be dropped.
> 
> 
> I have asked exactly this question before I started work on this series and got reply
> 
> from Jason that he sees no problem in bumping to a release which is already unmaintained
> 
> for 3 years.

I'm afraid Jason is wrong here.  It doesn't matter what the upstream
consider the support status to be. QEMU targets what the OS vendors
ship, and they still consider this to be a supported version.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Peter Lieven]

Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
> On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
>> Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
>>> On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
>>>> even luminous (version 12.2) is unmaintained for over 3 years now.
>>>> Bump the requirement to get rid of the ifdef'ry in the code.
>>> We have clear rules on when we bump minimum versions, determined by
>>> the OS platforms we target:
>>>
>>>     https://qemu.readthedocs.io/en/latest/system/build-platforms.html
>>>
>>> At this time RHEL-7 is usually the oldest platform, and it
>>> builds with RBD 10.2.5, so we can't bump the version to 12.2.
>>>
>>> I'm afraid this patch has to be dropped.
>>
>> I have asked exactly this question before I started work on this series and got reply
>>
>> from Jason that he sees no problem in bumping to a release which is already unmaintained
>>
>> for 3 years.
> I'm afraid Jason is wrong here.  It doesn't matter what the upstream
> consider the support status to be. QEMU targets what the OS vendors
> ship, and they still consider this to be a supported version.


Okay, but the whole coroutine stuff would get a total mess with all the ifdef'ry.

Would it be an option to make a big ifdef in the rbd driver? One with old code for < 12.0.0 and one

with new code for >= 12.0.0?


Peter

Re: [PATCH V2 4/7] block/rbd: add bdrv_attach_aio_context

[Peter Lieven]

Am 15.02.21 um 11:20 schrieb Kevin Wolf:
> Am 26.01.2021 um 12:25 hat Peter Lieven geschrieben:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/rbd.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/rbd.c b/block/rbd.c
>> index f68ebcf240..7abd0252c9 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -91,6 +91,7 @@ typedef struct BDRVRBDState {
>>       char *namespace;
>>       uint64_t image_size;
>>       uint64_t object_size;
>> +    AioContext *aio_context;
>>   } BDRVRBDState;
> A commit message explaining the why would be helpful here.
>
> This is already stored in BlockDriverState, which should be available
> everywhere. Keeping redundant information needs a good justification,
> which seems unlikely when BlockDriverState and BDRVRBDState are already
> connected through the BlockDriverState.opaque pointer.
>
> The rest of the series doesn't seem to make more use of it either.


You are right. I was not aware that the aio_context is already there.

We keep a local copy of aio_context in iscsi and nfs driver as well. That

is where I got it from. I will change it if we don't drop the series completely.


Peter

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Daniel P. Berrangé]

On Mon, Feb 15, 2021 at 12:45:01PM +0100, Peter Lieven wrote:
> Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
> > On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
> > > Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
> > > > On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
> > > > > even luminous (version 12.2) is unmaintained for over 3 years now.
> > > > > Bump the requirement to get rid of the ifdef'ry in the code.
> > > > We have clear rules on when we bump minimum versions, determined by
> > > > the OS platforms we target:
> > > > 
> > > >     https://qemu.readthedocs.io/en/latest/system/build-platforms.html
> > > > 
> > > > At this time RHEL-7 is usually the oldest platform, and it
> > > > builds with RBD 10.2.5, so we can't bump the version to 12.2.
> > > > 
> > > > I'm afraid this patch has to be dropped.
> > > 
> > > I have asked exactly this question before I started work on this series and got reply
> > > 
> > > from Jason that he sees no problem in bumping to a release which is already unmaintained
> > > 
> > > for 3 years.
> > I'm afraid Jason is wrong here.  It doesn't matter what the upstream
> > consider the support status to be. QEMU targets what the OS vendors
> > ship, and they still consider this to be a supported version.
> 
> 
> Okay, but the whole coroutine stuff would get a total mess with all the ifdef'ry.

Doesn't seem like the write zeros code is adding much more comapred to
the ifdefs that already exist... 


> Would it be an option to make a big ifdef in the rbd driver? One with old code for < 12.0.0 and one
> 
> with new code for >= 12.0.0?

..but I don't have a strong opinion on that, since I'm not maintaining this
driver.


BTW, we will be free to drop RHEL-7 in the next development cycle of
QEMU, starting after the forthcoming 6.0.0 release is out, as it will
fall out of our OS support matrix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Peter Lieven]

Am 15.02.21 um 12:51 schrieb Daniel P. Berrangé:
> On Mon, Feb 15, 2021 at 12:45:01PM +0100, Peter Lieven wrote:
>> Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
>>> On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
>>>> Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
>>>>> On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
>>>>>> even luminous (version 12.2) is unmaintained for over 3 years now.
>>>>>> Bump the requirement to get rid of the ifdef'ry in the code.
>>>>> We have clear rules on when we bump minimum versions, determined by
>>>>> the OS platforms we target:
>>>>>
>>>>>      https://qemu.readthedocs.io/en/latest/system/build-platforms.html
>>>>>
>>>>> At this time RHEL-7 is usually the oldest platform, and it
>>>>> builds with RBD 10.2.5, so we can't bump the version to 12.2.
>>>>>
>>>>> I'm afraid this patch has to be dropped.
>>>> I have asked exactly this question before I started work on this series and got reply
>>>>
>>>> from Jason that he sees no problem in bumping to a release which is already unmaintained
>>>>
>>>> for 3 years.
>>> I'm afraid Jason is wrong here.  It doesn't matter what the upstream
>>> consider the support status to be. QEMU targets what the OS vendors
>>> ship, and they still consider this to be a supported version.
>>
>> Okay, but the whole coroutine stuff would get a total mess with all the ifdef'ry.
> Doesn't seem like the write zeros code is adding much more comapred to
> the ifdefs that already exist...


Yes, I don't like it as well, but write zeroes support was only added in Nautilus (14.x) and the thick provisioning

that Jason asked me to add came only with Octopus (15.x).


>
>
>> Would it be an option to make a big ifdef in the rbd driver? One with old code for < 12.0.0 and one
>>
>> with new code for >= 12.0.0?
> ..but I don't have a strong opinion on that, since I'm not maintaining this
> driver.
>
>
> BTW, we will be free to drop RHEL-7 in the next development cycle of
> QEMU, starting after the forthcoming 6.0.0 release is out, as it will
> fall out of our OS support matrix.


Thanks for that hint. I would say lets hold this series back until Qemu 6.1.

Where can I find the OS support matrix for 6.1 - maybe we can bump the requirement to nautilus to

reduce the ifdef'ry further.


Peter

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Daniel P. Berrangé]

On Mon, Feb 15, 2021 at 12:55:24PM +0100, Peter Lieven wrote:
> Am 15.02.21 um 12:51 schrieb Daniel P. Berrangé:
> > On Mon, Feb 15, 2021 at 12:45:01PM +0100, Peter Lieven wrote:
> > > Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
> > > > On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
> > > > > Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
> > > > > > On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
> > > > > > > even luminous (version 12.2) is unmaintained for over 3 years now.
> > > > > > > Bump the requirement to get rid of the ifdef'ry in the code.
> > > > > > We have clear rules on when we bump minimum versions, determined by
> > > > > > the OS platforms we target:
> > > > > > 
> > > > > >      https://qemu.readthedocs.io/en/latest/system/build-platforms.html
> > > > > > 
> > > > > > At this time RHEL-7 is usually the oldest platform, and it
> > > > > > builds with RBD 10.2.5, so we can't bump the version to 12.2.
> > > > > > 
> > > > > > I'm afraid this patch has to be dropped.
> > > > > I have asked exactly this question before I started work on this series and got reply
> > > > > 
> > > > > from Jason that he sees no problem in bumping to a release which is already unmaintained
> > > > > 
> > > > > for 3 years.
> > > > I'm afraid Jason is wrong here.  It doesn't matter what the upstream
> > > > consider the support status to be. QEMU targets what the OS vendors
> > > > ship, and they still consider this to be a supported version.
> > > 
> > > Okay, but the whole coroutine stuff would get a total mess with all the ifdef'ry.
> > Doesn't seem like the write zeros code is adding much more comapred to
> > the ifdefs that already exist...
> 
> 
> Yes, I don't like it as well, but write zeroes support was only added in Nautilus (14.x) and the thick provisioning
> 
> that Jason asked me to add came only with Octopus (15.x).
> 
> 
> > 
> > 
> > > Would it be an option to make a big ifdef in the rbd driver? One with old code for < 12.0.0 and one
> > > 
> > > with new code for >= 12.0.0?
> > ..but I don't have a strong opinion on that, since I'm not maintaining this
> > driver.
> > 
> > 
> > BTW, we will be free to drop RHEL-7 in the next development cycle of
> > QEMU, starting after the forthcoming 6.0.0 release is out, as it will
> > fall out of our OS support matrix.
> 
> 
> Thanks for that hint. I would say lets hold this series back until Qemu 6.1.
> 
> Where can I find the OS support matrix for 6.1 - maybe we can bump the requirement to nautilus to
> 
> reduce the ifdef'ry further.

The rules are documented here:

  https://qemu.readthedocs.io/en/latest/system/build-platforms.html

RHEL-7 falls out because in May it will be 2 years since RHEL-8 came
out and thus

  "The project aims to support the most recent major version at
   all times. Support for the previous major version will be 
   dropped 2 years after the new major version is released or 
   when the vendor itself drops support, whichever comes first."

someone has todo the investigation to find out versions across the
distros.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Kevin Wolf]

Am 15.02.2021 um 12:45 hat Peter Lieven geschrieben:
> Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
> > On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
> > > Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
> > > > On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
> > > > > even luminous (version 12.2) is unmaintained for over 3 years now.
> > > > > Bump the requirement to get rid of the ifdef'ry in the code.
> > > > We have clear rules on when we bump minimum versions, determined by
> > > > the OS platforms we target:
> > > > 
> > > >     https://qemu.readthedocs.io/en/latest/system/build-platforms.html
> > > > 
> > > > At this time RHEL-7 is usually the oldest platform, and it
> > > > builds with RBD 10.2.5, so we can't bump the version to 12.2.
> > > > 
> > > > I'm afraid this patch has to be dropped.
> > > 
> > > I have asked exactly this question before I started work on this series and got reply
> > > 
> > > from Jason that he sees no problem in bumping to a release which is already unmaintained
> > > 
> > > for 3 years.
> > I'm afraid Jason is wrong here.  It doesn't matter what the upstream
> > consider the support status to be. QEMU targets what the OS vendors
> > ship, and they still consider this to be a supported version.
> 
> 
> Okay, but the whole coroutine stuff would get a total mess with all
> the ifdef'ry.

Hm, but how are these ifdefs even related to the coroutine conversation?
It's a bit more code that you're moving around, but shouldn't it be
unchanged from the old code, just moving from an AIO callback to a
coroutine? Or am I missing some complications?

> Would it be an option to make a big ifdef in the rbd driver? One with
> old code for < 12.0.0 and one
> 
> with new code for >= 12.0.0?

I don't think this is a good idea, this would be a huge mess to
maintain.

The conversion is probably a good idea in general, simply because it's
more in line with the rest of the block layer, but I don't think it adds
anything per se, so it's hard to justify such duplication with the
benefits it brings.

Kevin

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Kevin Wolf]

Am 15.02.2021 um 12:34 hat Peter Lieven geschrieben:
> Am 15.02.21 um 11:19 schrieb Daniel P. Berrangé:
> > On Mon, Feb 15, 2021 at 11:11:23AM +0100, Kevin Wolf wrote:
> > > Am 26.01.2021 um 12:25 hat Peter Lieven geschrieben:
> > > > even luminous (version 12.2) is unmaintained for over 3 years now.
> > > > Bump the requirement to get rid of the ifdef'ry in the code.
> > > > 
> > > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > diff --git a/meson.build b/meson.build
> > > > index 5943aa8a51..02d263ad33 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -691,19 +691,24 @@ if not get_option('rbd').auto() or have_block
> > > >                              required: get_option('rbd'),
> > > >                              kwargs: static_kwargs)
> > > >     if librados.found() and librbd.found()
> > > > -    if cc.links('''
> > > > +    result = cc.run('''
> > > Doesn't running compiled binaries break cross compilation?
> > > 
> > > >         #include <stdio.h>
> > > >         #include <rbd/librbd.h>
> > > >         int main(void) {
> > > >           rados_t cluster;
> > > >           rados_create(&cluster, NULL);
> > > > +        rados_shutdown(cluster);
> > > > +        #if LIBRBD_VERSION_CODE < LIBRBD_VERSION(1, 12, 0)
> > > > +        return 1;
> > > > +        #endif
> > > >           return 0;
> > > Would #error achieve what you want without running the binary?
> > > 
> > > But most, if not all, other version checks use pkg-config instead of
> > > trying to compile code, so that's probably what we should be doing here,
> > > too.
> > Yep, for something that is merely a version number check there's no
> > need to compile anything. pkg-config can just validate the version
> > straightup.
> 
> 
> I would have loved to, but at least the Ubuntu/Debian packages do not
> contain pkg-config files.

Oh. That's a shame.

> I can switch to #error, of course. My initial version of the patch
> distinguished between can't compile and version is too old. With
> #error we just can say doesn't compile, but I think this would be ok.

Yes, I think #error and a less specific message is better than breaking
cross compilation.

Maybe also add a comment as to why we can't use pkg-config so that
others won't take it as an example where pkg-config would work.

Kevin

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Peter Lieven]

Am 15.02.21 um 13:13 schrieb Kevin Wolf:
> Am 15.02.2021 um 12:45 hat Peter Lieven geschrieben:
>> Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
>>> On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
>>>> Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
>>>>> On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
>>>>>> even luminous (version 12.2) is unmaintained for over 3 years now.
>>>>>> Bump the requirement to get rid of the ifdef'ry in the code.
>>>>> We have clear rules on when we bump minimum versions, determined by
>>>>> the OS platforms we target:
>>>>>
>>>>>      https://qemu.readthedocs.io/en/latest/system/build-platforms.html
>>>>>
>>>>> At this time RHEL-7 is usually the oldest platform, and it
>>>>> builds with RBD 10.2.5, so we can't bump the version to 12.2.
>>>>>
>>>>> I'm afraid this patch has to be dropped.
>>>> I have asked exactly this question before I started work on this series and got reply
>>>>
>>>> from Jason that he sees no problem in bumping to a release which is already unmaintained
>>>>
>>>> for 3 years.
>>> I'm afraid Jason is wrong here.  It doesn't matter what the upstream
>>> consider the support status to be. QEMU targets what the OS vendors
>>> ship, and they still consider this to be a supported version.
>>
>> Okay, but the whole coroutine stuff would get a total mess with all
>> the ifdef'ry.
> Hm, but how are these ifdefs even related to the coroutine conversation?
> It's a bit more code that you're moving around, but shouldn't it be
> unchanged from the old code, just moving from an AIO callback to a
> coroutine? Or am I missing some complications?


No, the ifdef's only come back in for the write zeroes part.


>
>> Would it be an option to make a big ifdef in the rbd driver? One with
>> old code for < 12.0.0 and one
>>
>> with new code for >= 12.0.0?
> I don't think this is a good idea, this would be a huge mess to
> maintain.
>
> The conversion is probably a good idea in general, simply because it's
> more in line with the rest of the block layer, but I don't think it adds
> anything per se, so it's hard to justify such duplication with the
> benefits it brings.


I would wait for Jasons comment on the rbd part of the series and then spin a V3

with a for-6.1 tag.


Peter

Re: [PATCH V2 1/7] block/rbd: bump librbd requirement to luminous release

[Jason Dillaman]

On Mon, Feb 15, 2021 at 8:29 AM Peter Lieven <pl@kamp.de> wrote:
>
> Am 15.02.21 um 13:13 schrieb Kevin Wolf:
> > Am 15.02.2021 um 12:45 hat Peter Lieven geschrieben:
> >> Am 15.02.21 um 12:41 schrieb Daniel P. Berrangé:
> >>> On Mon, Feb 15, 2021 at 12:32:24PM +0100, Peter Lieven wrote:
> >>>> Am 15.02.21 um 11:24 schrieb Daniel P. Berrangé:
> >>>>> On Tue, Jan 26, 2021 at 12:25:34PM +0100, Peter Lieven wrote:
> >>>>>> even luminous (version 12.2) is unmaintained for over 3 years now.
> >>>>>> Bump the requirement to get rid of the ifdef'ry in the code.
> >>>>> We have clear rules on when we bump minimum versions, determined by
> >>>>> the OS platforms we target:
> >>>>>
> >>>>>      https://qemu.readthedocs.io/en/latest/system/build-platforms.html
> >>>>>
> >>>>> At this time RHEL-7 is usually the oldest platform, and it
> >>>>> builds with RBD 10.2.5, so we can't bump the version to 12.2.
> >>>>>
> >>>>> I'm afraid this patch has to be dropped.
> >>>> I have asked exactly this question before I started work on this series and got reply
> >>>>
> >>>> from Jason that he sees no problem in bumping to a release which is already unmaintained
> >>>>
> >>>> for 3 years.
> >>> I'm afraid Jason is wrong here.  It doesn't matter what the upstream
> >>> consider the support status to be. QEMU targets what the OS vendors
> >>> ship, and they still consider this to be a supported version.
> >>
> >> Okay, but the whole coroutine stuff would get a total mess with all
> >> the ifdef'ry.
> > Hm, but how are these ifdefs even related to the coroutine conversation?
> > It's a bit more code that you're moving around, but shouldn't it be
> > unchanged from the old code, just moving from an AIO callback to a
> > coroutine? Or am I missing some complications?
>
>
> No, the ifdef's only come back in for the write zeroes part.
>
>
> >
> >> Would it be an option to make a big ifdef in the rbd driver? One with
> >> old code for < 12.0.0 and one
> >>
> >> with new code for >= 12.0.0?
> > I don't think this is a good idea, this would be a huge mess to
> > maintain.
> >
> > The conversion is probably a good idea in general, simply because it's
> > more in line with the rest of the block layer, but I don't think it adds
> > anything per se, so it's hard to justify such duplication with the
> > benefits it brings.
>
>
> I would wait for Jasons comment on the rbd part of the series and then spin a V3
>
> with a for-6.1 tag.

Sorry for the long delay -- I was delayed from being out-of-town. I've
reviewed and play-tested the patches and it looks good for me. I'll
wait for V3 before adding my official review.

>
>
> Peter
>


-- 
Jason