[Qemu-devel] [RFC PATCH 0/5] move bdrv_invalidate_cache, bdrv_check to coroutines

[Qemu-devel] [RFC PATCH 0/5] move bdrv_invalidate_cache, bdrv_check to coroutines

[Paolo Bonzini]

bdrv_invalidate_cache and bdrv_check are reading/processing metadata,
which is usually protected by a lock (CoMutex).  However, they are
not called from coroutine context, so they are not using the same
protection.  This series moves these functions into coroutine context,
as suggested by Kevin for bdrv_check, so that they can take the CoMutex
just like everyone else.

In addition, bdrv_invalidate_cache is already being called from coroutine
context when doing incoming migration; making this explicit is cleaner.

This is particularly important for QED which, unlike QCOW2, drops the
lock while reading metadata; this series lets it make the
qemu_co_mutex_release/acquire unconditional, instead of wrapping it
with qemu_in_coroutine.  It will help Marc-André's effort to validate
coroutine_fn with clang's -Wthread-safety.

This series of course doesn't change the fact that concurrent I/O is
not a very good idea for either bdrv_invalidate_cache or bdrv_check:
in the case of bdrv_invalidate_cache, you should not do I/O prior to
invalidation, while the case of bdrv_check corrupted metadata may cause
all sorts of misbehavior.  However, it would now be possible to do
multiple bdrv_invalidate_cache in parallel if we were so inclined.

Note: I'm fairly sure of the qed changes, but a bit less of the qcow2
parts.  Patch 4 should make sure that there is no recursive locking,
and qemu-iotests is happy.

Paolo

Paolo Bonzini (5):
  qcow2: make qcow2_do_open a coroutine_fn
  qed: make bdrv_qed_do_open a coroutine_fn
  block: convert bdrv_invalidate_cache callback to coroutine_fn
  qcow2: introduce qcow2_write_caches and qcow2_flush_caches
  block: convert bdrv_check callback to coroutine_fn

 block.c                   | 84 +++++++++++++++++++++++++++++++++++++----
 block/iscsi.c             |  6 +--
 block/nfs.c               |  6 +--
 block/parallels.c         | 17 ++++++---
 block/qcow2-refcount.c    | 28 ++++++++++++++
 block/qcow2.c             | 95 +++++++++++++++++++++++++++++++++--------------
 block/qcow2.h             |  2 +
 block/qed-check.c         |  1 +
 block/qed-table.c         | 26 +++++--------
 block/qed.c               | 66 ++++++++++++++++++++++++--------
 block/rbd.c               |  6 +--
 block/vdi.c               |  6 +--
 block/vhdx.c              |  7 ++--
 block/vmdk.c              |  7 ++--
 include/block/block_int.h |  8 ++--
 15 files changed, 272 insertions(+), 93 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH 1/5] qcow2: make qcow2_do_open a coroutine_fn

[Paolo Bonzini]

It is called from qcow2_invalidate_cache in coroutine context, so always
load metadata from a coroutine.
---
 block/qcow2.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index cb081ea47f..b5de67d113 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -814,8 +814,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     return ret;
 }
 
-static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
-                         Error **errp)
+static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
+                                      int flags, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     unsigned int len, i;
@@ -1161,8 +1161,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    /* Initialise locks */
-    qemu_co_mutex_init(&s->lock);
     bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
 
     /* Repair image if dirty */
@@ -1205,16 +1203,53 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
+typedef struct QCow2OpenCo {
+    BlockDriverState *bs;
+    QDict *options;
+    int flags;
+    Error **errp;
+    int ret;
+} QCow2OpenCo;
+
+static void coroutine_fn qcow2_open_entry(void *opaque)
+{
+    QCow2OpenCo *qoc = opaque;
+    BDRVQcow2State *s = qoc->bs->opaque;
+
+    qemu_co_mutex_lock(&s->lock);
+    qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp);
+    qemu_co_mutex_unlock(&s->lock);
+}
+
 static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
+    BDRVQcow2State *s = bs->opaque;
+    QCow2OpenCo qoc = {
+        .bs = bs,
+        .options = options,
+        .flags = flags,
+        .errp = errp,
+        .ret = -EINPROGRESS
+    };
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    return qcow2_do_open(bs, options, flags, errp);
+    /* Initialise locks */
+    qemu_co_mutex_init(&s->lock);
+
+    if (qemu_in_coroutine()) {
+        /* From bdrv_co_create.  */
+        qcow2_open_entry(&qoc);
+    } else {
+        qemu_coroutine_enter(qemu_coroutine_create(qcow2_open_entry, &qoc));
+        BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
+    }
+    return qoc.ret;
 }
 
 static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
-- 
2.13.0

[Qemu-devel] [PATCH 2/5] qed: make bdrv_qed_do_open a coroutine_fn

[Paolo Bonzini]

It is called from bdrv_invalidate_cache in coroutine context, so always
load metadata from a coroutine.
---
 block/qed.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index d43cb55923..3c2867c946 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -381,8 +381,9 @@ static void bdrv_qed_init_state(BlockDriverState *bs)
     qemu_co_queue_init(&s->allocating_write_reqs);
 }
 
-static int bdrv_qed_do_open(BlockDriverState *bs, QDict *options, int flags,
-                            Error **errp)
+/* Called with table_lock held.  */
+static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
+                                         int flags, Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
     QEDHeader le_header;
@@ -513,9 +514,35 @@ out:
     return ret;
 }
 
+typedef struct QEDOpenCo {
+    BlockDriverState *bs;
+    QDict *options;
+    int flags;
+    Error **errp;
+    int ret;
+} QEDOpenCo;
+
+static void coroutine_fn bdrv_qed_open_entry(void *opaque)
+{
+    QEDOpenCo *qoc = opaque;
+    BDRVQEDState *s = qoc->bs->opaque;
+
+    qemu_co_mutex_lock(&s->table_lock);
+    qoc->ret = bdrv_qed_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp);
+    qemu_co_mutex_unlock(&s->table_lock);
+}
+
 static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
+    QEDOpenCo qoc = {
+        .bs = bs,
+        .options = options,
+        .flags = flags,
+        .errp = errp,
+        .ret = -EINPROGRESS
+    };
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
@@ -523,7 +550,14 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     bdrv_qed_init_state(bs);
-    return bdrv_qed_do_open(bs, options, flags, errp);
+    if (qemu_in_coroutine()) {
+        bdrv_qed_open_entry(&qoc);
+    } else {
+        qemu_coroutine_enter(qemu_coroutine_create(bdrv_qed_open_entry, &qoc));
+        BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
+    }
+    BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
+    return qoc.ret;
 }
 
 static void bdrv_qed_refresh_limits(BlockDriverState *bs, Error **errp)
-- 
2.13.0

[Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Paolo Bonzini]

QED's bdrv_invalidate_cache implementation would like to reuse functions
that acquire/release the metadata locks.  Call it from coroutine context
to simplify the logic.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c                   | 41 +++++++++++++++++++++++++++++++++++++----
 block/iscsi.c             |  6 +++---
 block/nfs.c               |  6 +++---
 block/qcow2.c             |  7 +++++--
 block/qed.c               | 13 +++++--------
 block/rbd.c               |  6 +++---
 include/block/block_int.h |  3 ++-
 7 files changed, 58 insertions(+), 24 deletions(-)

diff --git a/block.c b/block.c
index 223ca3f85f..c8856cc594 100644
--- a/block.c
+++ b/block.c
@@ -4014,7 +4014,8 @@ void bdrv_init_with_whitelist(void)
     bdrv_init();
 }
 
-void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
+static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
+                                                  Error **errp)
 {
     BdrvChild *child, *parent;
     uint64_t perm, shared_perm;
@@ -4030,7 +4031,7 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
     }
 
     QLIST_FOREACH(child, &bs->children, next) {
-        bdrv_invalidate_cache(child->bs, &local_err);
+        bdrv_co_invalidate_cache(child->bs, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
@@ -4038,8 +4039,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
     }
 
     bs->open_flags &= ~BDRV_O_INACTIVE;
-    if (bs->drv->bdrv_invalidate_cache) {
-        bs->drv->bdrv_invalidate_cache(bs, &local_err);
+    if (bs->drv->bdrv_co_invalidate_cache) {
+        bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
         if (local_err) {
             bs->open_flags |= BDRV_O_INACTIVE;
             error_propagate(errp, local_err);
@@ -4075,6 +4076,38 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
     }
 }
 
+typedef struct InvalidateCacheCo {
+    BlockDriverState *bs;
+    Error **errp;
+    bool done;
+} InvalidateCacheCo;
+
+static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
+{
+    InvalidateCacheCo *ico = opaque;
+    bdrv_co_invalidate_cache(ico->bs, ico->errp);
+    ico->done = true;
+}
+
+void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
+{
+    Coroutine *co;
+    InvalidateCacheCo ico = {
+        .bs = bs,
+        .done = false,
+        .errp = errp
+    };
+
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_invalidate_cache_co_entry(&ico);
+    } else {
+        co = qemu_coroutine_create(bdrv_invalidate_cache_co_entry, &ico);
+        qemu_coroutine_enter(co);
+        BDRV_POLL_WHILE(bs, !ico.done);
+    }
+}
+
 void bdrv_invalidate_cache_all(Error **errp)
 {
     BlockDriverState *bs;
diff --git a/block/iscsi.c b/block/iscsi.c
index 7aab379c48..01a4063d93 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2168,8 +2168,8 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
-static void iscsi_invalidate_cache(BlockDriverState *bs,
-                                   Error **errp)
+static void coroutine_fn iscsi_co_invalidate_cache(BlockDriverState *bs,
+                                                   Error **errp)
 {
     IscsiLun *iscsilun = bs->opaque;
     iscsi_allocmap_invalidate(iscsilun);
@@ -2200,7 +2200,7 @@ static BlockDriver bdrv_iscsi = {
     .create_opts            = &iscsi_create_opts,
     .bdrv_reopen_prepare    = iscsi_reopen_prepare,
     .bdrv_reopen_commit     = iscsi_reopen_commit,
-    .bdrv_invalidate_cache  = iscsi_invalidate_cache,
+    .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
 
     .bdrv_getlength  = iscsi_getlength,
     .bdrv_get_info   = iscsi_get_info,
diff --git a/block/nfs.c b/block/nfs.c
index 06e0c6f5f8..455240677f 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -865,8 +865,8 @@ static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
 }
 
 #ifdef LIBNFS_FEATURE_PAGECACHE
-static void nfs_invalidate_cache(BlockDriverState *bs,
-                                 Error **errp)
+static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs,
+                                                 Error **errp)
 {
     NFSClient *client = bs->opaque;
     nfs_pagecache_invalidate(client->context, client->fh);
@@ -899,7 +899,7 @@ static BlockDriver bdrv_nfs = {
     .bdrv_refresh_filename          = nfs_refresh_filename,
 
 #ifdef LIBNFS_FEATURE_PAGECACHE
-    .bdrv_invalidate_cache          = nfs_invalidate_cache,
+    .bdrv_co_invalidate_cache       = nfs_co_invalidate_cache,
 #endif
 };
 
diff --git a/block/qcow2.c b/block/qcow2.c
index b5de67d113..4ff7e89009 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1854,7 +1854,8 @@ static void qcow2_close(BlockDriverState *bs)
     qcow2_free_snapshots(bs);
 }
 
-static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
+static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
+                                                   Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     int flags = s->flags;
@@ -1877,7 +1878,9 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
     options = qdict_clone_shallow(bs->options);
 
     flags &= ~BDRV_O_INACTIVE;
+    qemu_co_mutex_lock(&s->lock);
     ret = qcow2_do_open(bs, options, flags, &local_err);
+    qemu_co_mutex_unlock(&s->lock);
     QDECREF(options);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -3540,7 +3543,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_change_backing_file   = qcow2_change_backing_file,
 
     .bdrv_refresh_limits        = qcow2_refresh_limits,
-    .bdrv_invalidate_cache      = qcow2_invalidate_cache,
+    .bdrv_co_invalidate_cache   = qcow2_co_invalidate_cache,
     .bdrv_inactivate            = qcow2_inactivate,
 
     .create_opts         = &qcow2_create_opts,
diff --git a/block/qed.c b/block/qed.c
index 3c2867c946..56254faee8 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1543,7 +1543,8 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
-static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp)
+static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
+                                                      Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
     Error *local_err = NULL;
@@ -1552,13 +1553,9 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp)
     bdrv_qed_close(bs);
 
     bdrv_qed_init_state(bs);
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_lock(&s->table_lock);
-    }
+    qemu_co_mutex_lock(&s->table_lock);
     ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_unlock(&s->table_lock);
-    }
+    qemu_co_mutex_unlock(&s->table_lock);
     if (local_err) {
         error_propagate(errp, local_err);
         error_prepend(errp, "Could not reopen qed layer: ");
@@ -1633,7 +1630,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_get_info            = bdrv_qed_get_info,
     .bdrv_refresh_limits      = bdrv_qed_refresh_limits,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
-    .bdrv_invalidate_cache    = bdrv_qed_invalidate_cache,
+    .bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache,
     .bdrv_check               = bdrv_qed_check,
     .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
     .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
diff --git a/block/rbd.c b/block/rbd.c
index 68c04a0d9d..2ccb9e41ca 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1077,8 +1077,8 @@ static BlockAIOCB *qemu_rbd_aio_pdiscard(BlockDriverState *bs,
 #endif
 
 #ifdef LIBRBD_SUPPORTS_INVALIDATE
-static void qemu_rbd_invalidate_cache(BlockDriverState *bs,
-                                      Error **errp)
+static void coroutine_fn qemu_rbd_co_invalidate_cache(BlockDriverState *bs,
+                                                      Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r = rbd_invalidate_cache(s->image);
@@ -1144,7 +1144,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_snapshot_list     = qemu_rbd_snap_list,
     .bdrv_snapshot_goto     = qemu_rbd_snap_rollback,
 #ifdef LIBRBD_SUPPORTS_INVALIDATE
-    .bdrv_invalidate_cache  = qemu_rbd_invalidate_cache,
+    .bdrv_co_invalidate_cache = qemu_rbd_co_invalidate_cache,
 #endif
 };
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 50ba349db7..8558d7b78d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -181,7 +181,8 @@ struct BlockDriver {
     /*
      * Invalidate any cached meta-data.
      */
-    void (*bdrv_invalidate_cache)(BlockDriverState *bs, Error **errp);
+    void coroutine_fn (*bdrv_co_invalidate_cache)(BlockDriverState *bs,
+                                                  Error **errp);
     int (*bdrv_inactivate)(BlockDriverState *bs);
 
     /*
-- 
2.13.0

[Qemu-devel] [PATCH 4/5] qcow2: introduce qcow2_write_caches and qcow2_flush_caches

[Paolo Bonzini]

Avoid recursively taking s->lock during bdrv_check.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2-refcount.c | 28 ++++++++++++++++++++++++++++
 block/qcow2.c          | 20 ++++----------------
 block/qcow2.h          |  2 ++
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..57bf91422b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1046,7 +1046,35 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
     }
 }
 
+int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
 
+    ret = qcow2_cache_write(bs, s->l2_table_cache);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (qcow2_need_accurate_refcounts(s)) {
+        ret = qcow2_cache_write(bs, s->refcount_block_cache);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
+int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
+{
+    int ret = qcow2_write_caches(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return bdrv_flush(bs->file->bs);
+}
 
 /*********************************************************/
 /* snapshots and image creation */
diff --git a/block/qcow2.c b/block/qcow2.c
index 4ff7e89009..9bab0d3012 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -278,7 +278,7 @@ static int qcow2_mark_clean(BlockDriverState *bs)
 
         s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY;
 
-        ret = bdrv_flush(bs);
+        ret = qcow2_flush_caches(bs);
         if (ret < 0) {
             return ret;
         }
@@ -308,7 +308,7 @@ int qcow2_mark_consistent(BlockDriverState *bs)
     BDRVQcow2State *s = bs->opaque;
 
     if (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT) {
-        int ret = bdrv_flush(bs);
+        int ret = qcow2_flush_caches(bs);
         if (ret < 0) {
             return ret;
         }
@@ -2968,22 +2968,10 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
     int ret;
 
     qemu_co_mutex_lock(&s->lock);
-    ret = qcow2_cache_write(bs, s->l2_table_cache);
-    if (ret < 0) {
-        qemu_co_mutex_unlock(&s->lock);
-        return ret;
-    }
-
-    if (qcow2_need_accurate_refcounts(s)) {
-        ret = qcow2_cache_write(bs, s->refcount_block_cache);
-        if (ret < 0) {
-            qemu_co_mutex_unlock(&s->lock);
-            return ret;
-        }
-    }
+    ret = qcow2_write_caches(bs);
     qemu_co_mutex_unlock(&s->lock);
 
-    return 0;
+    return ret;
 }
 
 static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
diff --git a/block/qcow2.h b/block/qcow2.h
index 87b15eb4aa..1d3fffd8ac 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -525,6 +525,8 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
 int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     int64_t l1_table_offset, int l1_size, int addend);
 
+int coroutine_fn qcow2_flush_caches(BlockDriverState *bs);
+int coroutine_fn qcow2_write_caches(BlockDriverState *bs);
 int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                           BdrvCheckMode fix);
 
-- 
2.13.0

[Qemu-devel] [PATCH 5/5] block: convert bdrv_check callback to coroutine_fn

[Paolo Bonzini]

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c                   | 43 ++++++++++++++++++++++++++++++++++++++++---
 block/parallels.c         | 17 +++++++++++------
 block/qcow2.c             | 23 +++++++++++++++++++----
 block/qed-check.c         |  1 +
 block/qed-table.c         | 26 +++++++++-----------------
 block/qed.c               | 13 +++++++++----
 block/vdi.c               |  6 +++---
 block/vhdx.c              |  7 ++++---
 block/vmdk.c              |  7 ++++---
 include/block/block_int.h |  5 +++--
 10 files changed, 103 insertions(+), 45 deletions(-)

diff --git a/block.c b/block.c
index c8856cc594..21742842ec 100644
--- a/block.c
+++ b/block.c
@@ -3245,17 +3245,54 @@ static void bdrv_delete(BlockDriverState *bs)
  * free of errors) or -errno when an internal error occurred. The results of the
  * check are stored in res.
  */
-int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
+static int coroutine_fn bdrv_co_check(BlockDriverState *bs,
+                                      BdrvCheckResult *res, BdrvCheckMode fix)
 {
     if (bs->drv == NULL) {
         return -ENOMEDIUM;
     }
-    if (bs->drv->bdrv_check == NULL) {
+    if (bs->drv->bdrv_co_check == NULL) {
         return -ENOTSUP;
     }
 
     memset(res, 0, sizeof(*res));
-    return bs->drv->bdrv_check(bs, res, fix);
+    return bs->drv->bdrv_co_check(bs, res, fix);
+}
+
+typedef struct CheckCo {
+    BlockDriverState *bs;
+    BdrvCheckResult *res;
+    BdrvCheckMode fix;
+    int ret;
+} CheckCo;
+
+static void bdrv_check_co_entry(void *opaque)
+{
+    CheckCo *cco = opaque;
+    cco->ret = bdrv_co_check(cco->bs, cco->res, cco->fix);
+}
+
+int bdrv_check(BlockDriverState *bs,
+               BdrvCheckResult *res, BdrvCheckMode fix)
+{
+    Coroutine *co;
+    CheckCo cco = {
+        .bs = bs,
+        .res = res,
+        .ret = -EINPROGRESS,
+        .fix = fix,
+    };
+
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_check_co_entry(&cco);
+    } else {
+        co = qemu_coroutine_create(bdrv_check_co_entry, &cco);
+        qemu_coroutine_enter(co);
+        BDRV_POLL_WHILE(bs, cco.ret == -EINPROGRESS);
+    }
+
+    return cco.ret;
 }
 
 /*
diff --git a/block/parallels.c b/block/parallels.c
index 82c6dd9aed..a198354f4e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -376,8 +376,9 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
 }
 
 
-static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
-                           BdrvCheckMode fix)
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+                                           BdrvCheckResult *res,
+                                           BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t size, prev_off, high_off;
@@ -392,6 +393,7 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
         return size;
     }
 
+    qemu_co_mutex_lock(&s->lock);
     if (s->header_unclean) {
         fprintf(stderr, "%s image was not closed correctly\n",
                 fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
@@ -440,11 +442,12 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
         prev_off = off;
     }
 
+    ret = 0;
     if (flush_bat) {
         ret = bdrv_pwrite_sync(bs->file, 0, s->header, s->header_size);
         if (ret < 0) {
             res->check_errors++;
-            return ret;
+            goto out;
         }
     }
 
@@ -462,13 +465,15 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
             if (ret < 0) {
                 error_report_err(local_err);
                 res->check_errors++;
-                return ret;
+                goto out;
             }
             res->leaks_fixed += count;
         }
     }
 
-    return 0;
+out:
+    qemu_co_mutex_unlock(&s->lock);
+    return ret;
 }
 
 
@@ -784,7 +789,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_writev = parallels_co_writev,
 
     .bdrv_co_create = parallels_co_create,
-    .bdrv_check     = parallels_check,
+    .bdrv_co_check  = parallels_co_check,
     .create_opts    = &parallels_create_opts,
 };
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 9bab0d3012..22a8650c5f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -319,8 +319,9 @@ int qcow2_mark_consistent(BlockDriverState *bs)
     return 0;
 }
 
-static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
-                       BdrvCheckMode fix)
+static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
+                                              BdrvCheckResult *result,
+                                              BdrvCheckMode fix)
 {
     int ret = qcow2_check_refcounts(bs, result, fix);
     if (ret < 0) {
@@ -337,6 +338,19 @@ static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
     return ret;
 }
 
+static int coroutine_fn qcow2_co_check(BlockDriverState *bs,
+                                       BdrvCheckResult *result,
+                                       BdrvCheckMode fix)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
+
+    qemu_co_mutex_lock(&s->lock);
+    ret = qcow2_co_check_locked(bs, result, fix);
+    qemu_co_mutex_unlock(&s->lock);
+    return ret;
+}
+
 static int validate_table_offset(BlockDriverState *bs, uint64_t offset,
                                  uint64_t entries, size_t entry_len)
 {
@@ -1168,7 +1182,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
         BdrvCheckResult result = {0};
 
-        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
+        ret = qcow2_co_check_locked(bs, &result,
+                                    BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not repair dirty image");
             goto fail;
@@ -3535,7 +3550,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_inactivate            = qcow2_inactivate,
 
     .create_opts         = &qcow2_create_opts,
-    .bdrv_check          = qcow2_check,
+    .bdrv_co_check       = qcow2_co_check,
     .bdrv_amend_options  = qcow2_amend_options,
 
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
diff --git a/block/qed-check.c b/block/qed-check.c
index dcd4f036b8..0edac03159 100644
--- a/block/qed-check.c
+++ b/block/qed-check.c
@@ -217,6 +217,7 @@ static void qed_check_mark_clean(BDRVQEDState *s, BdrvCheckResult *result)
     qed_write_header_sync(s);
 }
 
+/* Called with table_lock held.  */
 int qed_check(BDRVQEDState *s, BdrvCheckResult *result, bool fix)
 {
     QEDCheck check = {
diff --git a/block/qed-table.c b/block/qed-table.c
index eead8b0fc7..7df5680adb 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -18,7 +18,7 @@
 #include "qed.h"
 #include "qemu/bswap.h"
 
-/* Called either from qed_check or with table_lock held.  */
+/* Called with table_lock held.  */
 static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table)
 {
     QEMUIOVector qiov;
@@ -33,13 +33,9 @@ static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table)
 
     trace_qed_read_table(s, offset, table);
 
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_unlock(&s->table_lock);
-    }
+    qemu_co_mutex_unlock(&s->table_lock);
     ret = bdrv_preadv(s->bs->file, offset, &qiov);
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_lock(&s->table_lock);
-    }
+    qemu_co_mutex_lock(&s->table_lock);
     if (ret < 0) {
         goto out;
     }
@@ -67,7 +63,7 @@ out:
  * @n:          Number of elements
  * @flush:      Whether or not to sync to disk
  *
- * Called either from qed_check or with table_lock held.
+ * Called with table_lock held.
  */
 static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
                            unsigned int index, unsigned int n, bool flush)
@@ -104,13 +100,9 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
     /* Adjust for offset into table */
     offset += start * sizeof(uint64_t);
 
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_unlock(&s->table_lock);
-    }
+    qemu_co_mutex_unlock(&s->table_lock);
     ret = bdrv_pwritev(s->bs->file, offset, &qiov);
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_lock(&s->table_lock);
-    }
+    qemu_co_mutex_lock(&s->table_lock);
     trace_qed_write_table_cb(s, table, flush, ret);
     if (ret < 0) {
         goto out;
@@ -134,7 +126,7 @@ int qed_read_l1_table_sync(BDRVQEDState *s)
     return qed_read_table(s, s->header.l1_table_offset, s->l1_table);
 }
 
-/* Called either from qed_check or with table_lock held.  */
+/* Called with table_lock held.  */
 int qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n)
 {
     BLKDBG_EVENT(s->bs->file, BLKDBG_L1_UPDATE);
@@ -148,7 +140,7 @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
     return qed_write_l1_table(s, index, n);
 }
 
-/* Called either from qed_check or with table_lock held.  */
+/* Called with table_lock held.  */
 int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset)
 {
     int ret;
@@ -191,7 +183,7 @@ int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset
     return qed_read_l2_table(s, request, offset);
 }
 
-/* Called either from qed_check or with table_lock held.  */
+/* Called with table_lock held.  */
 int qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
                        unsigned int index, unsigned int n, bool flush)
 {
diff --git a/block/qed.c b/block/qed.c
index 56254faee8..576692bcf5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1566,12 +1566,17 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
     }
 }
 
-static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result,
-                          BdrvCheckMode fix)
+static int bdrv_qed_co_check(BlockDriverState *bs, BdrvCheckResult *result,
+                             BdrvCheckMode fix)
 {
     BDRVQEDState *s = bs->opaque;
+    int ret;
 
-    return qed_check(s, result, !!fix);
+    qemu_co_mutex_lock(&s->table_lock);
+    ret = qed_check(s, result, !!fix);
+    qemu_co_mutex_unlock(&s->table_lock);
+
+    return ret;
 }
 
 static QemuOptsList qed_create_opts = {
@@ -1631,7 +1636,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_refresh_limits      = bdrv_qed_refresh_limits,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache,
-    .bdrv_check               = bdrv_qed_check,
+    .bdrv_co_check            = bdrv_qed_co_check,
     .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
     .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
     .bdrv_co_drain            = bdrv_qed_co_drain,
diff --git a/block/vdi.c b/block/vdi.c
index 1d6e779551..aa3e58dd73 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -258,8 +258,8 @@ static void vdi_header_print(VdiHeader *header)
 }
 #endif
 
-static int vdi_check(BlockDriverState *bs, BdrvCheckResult *res,
-                     BdrvCheckMode fix)
+static int coroutine_fn vdi_co_check(BlockDriverState *bs, BdrvCheckResult *res,
+                                     BdrvCheckMode fix)
 {
     /* TODO: additional checks possible. */
     BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
@@ -907,7 +907,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_get_info = vdi_get_info,
 
     .create_opts = &vdi_create_opts,
-    .bdrv_check = vdi_check,
+    .bdrv_co_check = vdi_co_check,
 };
 
 static void bdrv_vdi_init(void)
diff --git a/block/vhdx.c b/block/vhdx.c
index e42a417ffd..d0f7db9bd7 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1939,8 +1939,9 @@ exit:
  * r/w and any log has already been replayed, so there is nothing (currently)
  * for us to do here
  */
-static int vhdx_check(BlockDriverState *bs, BdrvCheckResult *result,
-                       BdrvCheckMode fix)
+static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
+                                      BdrvCheckResult *result,
+                                      BdrvCheckMode fix)
 {
     BDRVVHDXState *s = bs->opaque;
 
@@ -2001,7 +2002,7 @@ static BlockDriver bdrv_vhdx = {
     .bdrv_co_writev         = vhdx_co_writev,
     .bdrv_co_create         = vhdx_co_create,
     .bdrv_get_info          = vhdx_get_info,
-    .bdrv_check             = vhdx_check,
+    .bdrv_co_check          = vhdx_co_check,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
 
     .create_opts            = &vhdx_create_opts,
diff --git a/block/vmdk.c b/block/vmdk.c
index 2579cfbec5..4d8f94f27c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2193,8 +2193,9 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
     return info;
 }
 
-static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result,
-                      BdrvCheckMode fix)
+static int coroutine_fn vmdk_co_check(BlockDriverState *bs,
+                                      BdrvCheckResult *result,
+                                      BdrvCheckMode fix)
 {
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent = NULL;
@@ -2353,7 +2354,7 @@ static BlockDriver bdrv_vmdk = {
     .instance_size                = sizeof(BDRVVmdkState),
     .bdrv_probe                   = vmdk_probe,
     .bdrv_open                    = vmdk_open,
-    .bdrv_check                   = vmdk_check,
+    .bdrv_co_check                = vmdk_co_check,
     .bdrv_reopen_prepare          = vmdk_reopen_prepare,
     .bdrv_child_perm              = bdrv_format_default_perms,
     .bdrv_co_preadv               = vmdk_co_preadv,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8558d7b78d..746025763b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -262,8 +262,9 @@ struct BlockDriver {
      * Returns 0 for completed check, -errno for internal errors.
      * The check results are stored in result.
      */
-    int (*bdrv_check)(BlockDriverState *bs, BdrvCheckResult *result,
-        BdrvCheckMode fix);
+    int coroutine_fn (*bdrv_co_check)(BlockDriverState *bs,
+                                      BdrvCheckResult *result,
+                                      BdrvCheckMode fix);
 
     int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
                               BlockDriverAmendStatusCB *status_cb,
-- 
2.13.0

Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Kevin Wolf]

Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
> QED's bdrv_invalidate_cache implementation would like to reuse functions
> that acquire/release the metadata locks.  Call it from coroutine context
> to simplify the logic.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/block/qcow2.c b/block/qcow2.c
> index b5de67d113..4ff7e89009 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1854,7 +1854,8 @@ static void qcow2_close(BlockDriverState *bs)
>      qcow2_free_snapshots(bs);
>  }
>  
> -static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
> +static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> +                                                   Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      int flags = s->flags;
> @@ -1877,7 +1878,9 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>      options = qdict_clone_shallow(bs->options);
>  
>      flags &= ~BDRV_O_INACTIVE;
> +    qemu_co_mutex_lock(&s->lock);
>      ret = qcow2_do_open(bs, options, flags, &local_err);
> +    qemu_co_mutex_unlock(&s->lock);
>      QDECREF(options);
>      if (local_err) {
>          error_propagate(errp, local_err);

How useful is this lock really? If we expect any requests while this
function is running (we don't, it would break horribly), it should
probably cover qcow2_close(), too.

Or maybe the cleaner way would be bdrv_drained_begin/end() around
everything directly in bdrv_co_invalidate_cache().

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: make qcow2_do_open a coroutine_fn

[Kevin Wolf]

Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
> It is called from qcow2_invalidate_cache in coroutine context, so always
> load metadata from a coroutine.
> ---
>  block/qcow2.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)

Missing S-o-b on patches 1 and 2?

These patches suggest that .bdrv_co_open() might be the next thing to
get rid of the coroutine wrappers again in driver code. But we'll always
have some wrappers in the drivers in an intermediate state, so doing
.bdrv_invalidate_cache first is fine.

Kevin

Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Paolo Bonzini]

On 11/07/2017 10:24, Kevin Wolf wrote:
> Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
>> QED's bdrv_invalidate_cache implementation would like to reuse functions
>> that acquire/release the metadata locks.  Call it from coroutine context
>> to simplify the logic.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index b5de67d113..4ff7e89009 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1854,7 +1854,8 @@ static void qcow2_close(BlockDriverState *bs)
>>      qcow2_free_snapshots(bs);
>>  }
>>  
>> -static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>> +static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>> +                                                   Error **errp)
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>>      int flags = s->flags;
>> @@ -1877,7 +1878,9 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>>      options = qdict_clone_shallow(bs->options);
>>  
>>      flags &= ~BDRV_O_INACTIVE;
>> +    qemu_co_mutex_lock(&s->lock);
>>      ret = qcow2_do_open(bs, options, flags, &local_err);
>> +    qemu_co_mutex_unlock(&s->lock);
>>      QDECREF(options);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
> 
> How useful is this lock really? If we expect any requests while this
> function is running (we don't, it would break horribly), it should
> probably cover qcow2_close(), too.

In the case of QED, this was needed because qed_read_table drops
s->table_lock.  Here I just copied the idiom.

However, I think it's cleaner to define which functions are called with
which lock held.  Sometimes it leads to taking a lock unnecessarily, but
it's clearer and it can also help once we annotate the source and let
the compiler do static analysis (see clang's -Wthread-safety).

Paolo

> Or maybe the cleaner way would be bdrv_drained_begin/end() around
> everything directly in bdrv_co_invalidate_cache().
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH 1/5] qcow2: make qcow2_do_open a coroutine_fn

[Paolo Bonzini]

On 11/07/2017 10:28, Kevin Wolf wrote:
> Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
>> It is called from qcow2_invalidate_cache in coroutine context, so always
>> load metadata from a coroutine.
>> ---
>>  block/qcow2.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 40 insertions(+), 5 deletions(-)
> 
> Missing S-o-b on patches 1 and 2?

Oops, yes.  Though I don't think these patches are for 2.10.  They are
on top of Stefan's bdrv_co_create series, by the way.

Paolo

> These patches suggest that .bdrv_co_open() might be the next thing to
> get rid of the coroutine wrappers again in driver code. But we'll always
> have some wrappers in the drivers in an intermediate state, so doing
> .bdrv_invalidate_cache first is fine.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Kevin Wolf]

Am 11.07.2017 um 10:33 hat Paolo Bonzini geschrieben:
> On 11/07/2017 10:24, Kevin Wolf wrote:
> > Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
> >> QED's bdrv_invalidate_cache implementation would like to reuse functions
> >> that acquire/release the metadata locks.  Call it from coroutine context
> >> to simplify the logic.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index b5de67d113..4ff7e89009 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -1854,7 +1854,8 @@ static void qcow2_close(BlockDriverState *bs)
> >>      qcow2_free_snapshots(bs);
> >>  }
> >>  
> >> -static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
> >> +static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> >> +                                                   Error **errp)
> >>  {
> >>      BDRVQcow2State *s = bs->opaque;
> >>      int flags = s->flags;
> >> @@ -1877,7 +1878,9 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
> >>      options = qdict_clone_shallow(bs->options);
> >>  
> >>      flags &= ~BDRV_O_INACTIVE;
> >> +    qemu_co_mutex_lock(&s->lock);
> >>      ret = qcow2_do_open(bs, options, flags, &local_err);
> >> +    qemu_co_mutex_unlock(&s->lock);
> >>      QDECREF(options);
> >>      if (local_err) {
> >>          error_propagate(errp, local_err);
> > 
> > How useful is this lock really? If we expect any requests while this
> > function is running (we don't, it would break horribly), it should
> > probably cover qcow2_close(), too.
> 
> In the case of QED, this was needed because qed_read_table drops
> s->table_lock.  Here I just copied the idiom.

Ok, makes sense.

> However, I think it's cleaner to define which functions are called
> with which lock held.  Sometimes it leads to taking a lock
> unnecessarily, but it's clearer and it can also help once we annotate
> the source and let the compiler do static analysis (see clang's
> -Wthread-safety).

Can you just add a comment to this effect to qcow2/qed_do_open() then?

> > Or maybe the cleaner way would be bdrv_drained_begin/end() around
> > everything directly in bdrv_co_invalidate_cache().

Should we do this anyway? Or assert that no requests are in flight?

Kevin

Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Paolo Bonzini]

On 11/07/2017 10:39, Kevin Wolf wrote:
> Can you just add a comment to this effect to qcow2/qed_do_open() then?

Good point.

>>> Or maybe the cleaner way would be bdrv_drained_begin/end() around
>>> everything directly in bdrv_co_invalidate_cache().
> 
> Should we do this anyway? Or assert that no requests are in flight?

I can certainly add assertions for clarity, but wouldn't requests have
failed the permission checks and caused assertion failures already?

Paolo

Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Kevin Wolf]

Am 11.07.2017 um 10:41 hat Paolo Bonzini geschrieben:
> On 11/07/2017 10:39, Kevin Wolf wrote:
> > Can you just add a comment to this effect to qcow2/qed_do_open() then?
> 
> Good point.
> 
> >>> Or maybe the cleaner way would be bdrv_drained_begin/end() around
> >>> everything directly in bdrv_co_invalidate_cache().
> > 
> > Should we do this anyway? Or assert that no requests are in flight?
> 
> I can certainly add assertions for clarity, but wouldn't requests have
> failed the permission checks and caused assertion failures already?

I'm pretty sure that read requests are allowed on inactive images.

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: make qcow2_do_open a coroutine_fn

[Kevin Wolf]

Am 11.07.2017 um 10:34 hat Paolo Bonzini geschrieben:
> On 11/07/2017 10:28, Kevin Wolf wrote:
> > Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
> >> It is called from qcow2_invalidate_cache in coroutine context, so always
> >> load metadata from a coroutine.
> >> ---
> >>  block/qcow2.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 40 insertions(+), 5 deletions(-)
> > 
> > Missing S-o-b on patches 1 and 2?
> 
> Oops, yes.  Though I don't think these patches are for 2.10.  They are
> on top of Stefan's bdrv_co_create series, by the way.

Why not for 2.10? They look simple and self-contained. I haven't looked
at bdrv_co_create yet, but it shouldn't be much more complicated either.

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: make qcow2_do_open a coroutine_fn

[Paolo Bonzini]

On 11/07/2017 11:07, Kevin Wolf wrote:
> Am 11.07.2017 um 10:34 hat Paolo Bonzini geschrieben:
>> On 11/07/2017 10:28, Kevin Wolf wrote:
>>> Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
>>>> It is called from qcow2_invalidate_cache in coroutine context, so always
>>>> load metadata from a coroutine.
>>>> ---
>>>>  block/qcow2.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 40 insertions(+), 5 deletions(-)
>>>
>>> Missing S-o-b on patches 1 and 2?
>>
>> Oops, yes.  Though I don't think these patches are for 2.10.  They are
>> on top of Stefan's bdrv_co_create series, by the way.
> 
> Why not for 2.10?

Because soft freeze is next Monday? :)  But I can certainly send a v2
today or tomorrow.

> They look simple and self-contained. I haven't looked
> at bdrv_co_create yet, but it shouldn't be much more complicated either.

It's simpler, as it's only renaming the callback.

Paolo