Re: [PATCH 2/3] qcow2: Assert that qcow2_cache_get() callers hold s->lock

[Kevin Wolf]

Am 23.10.2019 um 17:26 hat Kevin Wolf geschrieben:
> qcow2_cache_do_get() requires that s->lock is locked because it can
> yield between picking a cache entry and actually taking ownership of it
> by setting offset and increasing the reference count.
> 
> Add an assertion to make sure the caller really holds the lock. The
> function can be called outside of coroutine context, where bdrv_pread
> and flushes become synchronous operations. The lock cannot and need not
> be taken in this case.
> 
> Signed-off-by: Kevin Wolf <address@hidden>

Oops, this one was a bit too optimistic. :-)

I'm still running tests to see if any other code paths trigger the
assertion, but image creation calls this without the lock held (which is
harmless because nobody else knows about the image so there won't be
concurrent requests). The following patch is needed additionally to make
image creation work with the new assertion.

Kevin


diff --git a/block/qcow2.c b/block/qcow2.c
index 0bc69e6996..7761cf3e07 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3213,6 +3213,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
     BlockDriverState *data_bs = NULL;
+    BDRVQcow2State *s;
     QCowHeader *header;
     size_t cluster_size;
     int version;
@@ -3424,7 +3425,12 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
         goto out;
     }

+    s = blk_bs(blk)->opaque;
+
+    qemu_co_mutex_lock(&s->lock);
     ret = qcow2_alloc_clusters(blk_bs(blk), 3 * cluster_size);
+    qemu_co_mutex_unlock(&s->lock);
+
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not allocate clusters for qcow2 "
                          "header and refcount table");
@@ -3437,7 +3443,6 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)

     /* Set the external data file if necessary */
     if (data_bs) {
-        BDRVQcow2State *s = blk_bs(blk)->opaque;
         s->image_data_file = g_strdup(data_bs->filename);
     }