[PATCH v2 0/6] coroutine rwlock downgrade fix, minor VDI changes

[PATCH v2 0/6] coroutine rwlock downgrade fix, minor VDI changes

[David Edmondson]

Stressing the VDI code with qemu-img:

  qemu-img convert -p -W -m 16 -O vdi input.qcow2 output.vdi

leads to a hang relatively quickly on a machine with sufficient
CPUs. A similar test targetting either raw or qcow2 formats, or
avoiding out-of-order writes, completes fine.

At the point of the hang all of the coroutines are sitting in
qemu_co_queue_wait_impl(), called from either qemu_co_rwlock_rdlock()
or qemu_co_rwlock_upgrade(), all referencing the same CoRwlock
(BDRVVdiState.bmap_lock).

The comment in the last patch explains what I believe is happening -
downgrading an rwlock from write to read can later result in a failure
to schedule an appropriate coroutine when the read lock is released.

A less invasive change might be to simply have the read side of the
unlock code mark *all* queued coroutines as runnable. This seems
somewhat wasteful, as any read hopefuls that run before a write
hopeful will immediately put themselves back on the queue.

No code other than block/vdi.c appears to use
qemu_co_rwlock_downgrade().

The block/vdi.c changes are small things noticed by inspection when
looking for the cause of the hang.

v2:
- Add some r-by (Philippe, Paolo).
- Add a test for the rwlock downgrade behaviour (Paolo).
- Improve unlock to avoid thundering herd (Paolo).

David Edmondson (6):
  block/vdi: When writing new bmap entry fails, don't leak the buffer
  block/vdi: Don't assume that blocks are larger than VdiHeader
  coroutine/mutex: Store the coroutine in the CoWaitRecord only once
  test-coroutine: Add rwlock downgrade test
  coroutine/rwlock: Wake writers in preference to readers
  coroutine/rwlock: Avoid thundering herd when unlocking

 block/vdi.c                |  11 ++--
 include/qemu/coroutine.h   |   8 ++-
 tests/test-coroutine.c     | 112 +++++++++++++++++++++++++++++++++++++
 util/qemu-coroutine-lock.c |  27 ++++-----
 4 files changed, 138 insertions(+), 20 deletions(-)

-- 
2.30.1

[PATCH v2 1/6] block/vdi: When writing new bmap entry fails, don't leak the buffer

[David Edmondson]

If a new bitmap entry is allocated, requiring the entire block to be
written, avoiding leaking the buffer allocated for the block should
the write fail.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index 5627e7d764..2a6dc26124 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -690,6 +690,7 @@ nonallocating_write:
 
     logout("finished data write\n");
     if (ret < 0) {
+        g_free(block);
         return ret;
     }
 
-- 
2.30.1

[PATCH v2 2/6] block/vdi: Don't assume that blocks are larger than VdiHeader

[David Edmondson]

Given that the block size is read from the header of the VDI file, a
wide variety of sizes might be seen. Rather than re-using a block
sized memory region when writing the VDI header, allocate an
appropriately sized buffer.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 block/vdi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 2a6dc26124..548f8a057b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -696,18 +696,20 @@ nonallocating_write:
 
     if (block) {
         /* One or more new blocks were allocated. */
-        VdiHeader *header = (VdiHeader *) block;
+        VdiHeader *header;
         uint8_t *base;
         uint64_t offset;
         uint32_t n_sectors;
 
+        g_free(block);
+        header = g_malloc(sizeof(*header));
+
         logout("now writing modified header\n");
         assert(VDI_IS_ALLOCATED(bmap_first));
         *header = s->header;
         vdi_header_to_le(header);
-        ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
-        g_free(block);
-        block = NULL;
+        ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
+        g_free(header);
 
         if (ret < 0) {
             return ret;
-- 
2.30.1

[PATCH v2 3/6] coroutine/mutex: Store the coroutine in the CoWaitRecord only once

[David Edmondson]

When taking the slow path for mutex acquisition, set the coroutine
value in the CoWaitRecord in push_waiter(), rather than both there and
in the caller.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 util/qemu-coroutine-lock.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 5816bf8900..eb73cf11dc 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -204,7 +204,6 @@ static void coroutine_fn qemu_co_mutex_lock_slowpath(AioContext *ctx,
     unsigned old_handoff;
 
     trace_qemu_co_mutex_lock_entry(mutex, self);
-    w.co = self;
     push_waiter(mutex, &w);
 
     /* This is the "Responsibility Hand-Off" protocol; a lock() picks from
-- 
2.30.1

[PATCH v2 4/6] test-coroutine: Add rwlock downgrade test

[David Edmondson]

Test that downgrading an rwlock does not result in a failure to
schedule coroutines queued on the rwlock.

The diagram associated with test_co_rwlock_downgrade() describes the
intended behaviour, but what is observed currently corresponds to:

| c1     | c2         | c3         | c4       |
|--------+------------+------------+----------|
| rdlock |            |            |          |
| yield  |            |            |          |
|        | wrlock     |            |          |
|        | <queued>   |            |          |
|        |            | rdlock     |          |
|        |            | <queued>   |          |
|        |            |            | wrlock   |
|        |            |            | <queued> |
| unlock |            |            |          |
| yield  |            |            |          |
|        | <dequeued> |            |          |
|        | downgrade  |            |          |
|        | ...        |            |          |
|        | unlock     |            |          |
|        |            | <dequeued> |          |
|        |            | <queued>   |          |

In the test, this results in a failure...

ERROR:../tests/test-coroutine.c:369:test_co_rwlock_downgrade: assertion failed: (c3_done)
Bail out! ERROR:../tests/test-coroutine.c:369:test_co_rwlock_downgrade: assertion failed: (c3_done)

...as a result of the c3 coroutine failing to run to completion.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 tests/test-coroutine.c | 112 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
index e946d93a65..62b0092721 100644
--- a/tests/test-coroutine.c
+++ b/tests/test-coroutine.c
@@ -264,6 +264,117 @@ static void test_co_mutex_lockable(void)
     g_assert(QEMU_MAKE_LOCKABLE(null_pointer) == NULL);
 }
 
+static bool c1_done;
+static bool c2_done;
+static bool c3_done;
+static bool c4_done;
+
+static void coroutine_fn rwlock_c1(void *opaque)
+{
+    CoRwlock *l = opaque;
+
+    qemu_co_rwlock_rdlock(l);
+    qemu_coroutine_yield();
+
+    qemu_co_rwlock_unlock(l);
+    qemu_coroutine_yield();
+
+    c1_done = true;
+}
+
+static void coroutine_fn rwlock_c2(void *opaque)
+{
+    CoRwlock *l = opaque;
+
+    qemu_co_rwlock_wrlock(l);
+
+    qemu_co_rwlock_downgrade(l);
+    qemu_co_rwlock_unlock(l);
+    c2_done = true;
+}
+
+static void coroutine_fn rwlock_c3(void *opaque)
+{
+    CoRwlock *l = opaque;
+
+    qemu_co_rwlock_rdlock(l);
+
+    qemu_co_rwlock_unlock(l);
+    c3_done = true;
+}
+
+static void coroutine_fn rwlock_c4(void *opaque)
+{
+    CoRwlock *l = opaque;
+
+    qemu_co_rwlock_wrlock(l);
+
+    qemu_co_rwlock_unlock(l);
+    c4_done = true;
+}
+
+/*
+ * Check that downgrading a reader-writer lock does not cause a hang.
+ *
+ * Four coroutines are used to produce a situation where there are
+ * both reader and writer hopefuls waiting to acquire an rwlock that
+ * is held by a reader.
+ *
+ * The correct sequence of operations we aim to provoke can be
+ * represented as:
+ *
+ * | c1     | c2         | c3         | c4         |
+ * |--------+------------+------------+------------|
+ * | rdlock |            |            |            |
+ * | yield  |            |            |            |
+ * |        | wrlock     |            |            |
+ * |        | <queued>   |            |            |
+ * |        |            | rdlock     |            |
+ * |        |            | <queued>   |            |
+ * |        |            |            | wrlock     |
+ * |        |            |            | <queued>   |
+ * | unlock |            |            |            |
+ * | yield  |            |            |            |
+ * |        | <dequeued> |            |            |
+ * |        | downgrade  |            |            |
+ * |        | ...        |            |            |
+ * |        | unlock     |            |            |
+ * |        |            |            | <dequeued> |
+ * |        |            |            | unlock     |
+ * |        |            | <dequeued> |            |
+ * |        |            | unlock     |            |
+ *
+ * Significantly, when c2 unlocks the downgraded lock, it should be c4
+ * that is run rather than c3.
+ */
+static void test_co_rwlock_downgrade(void)
+{
+    CoRwlock l;
+    Coroutine *c1, *c2, *c3, *c4;
+
+    qemu_co_rwlock_init(&l);
+
+    c1 = qemu_coroutine_create(rwlock_c1, &l);
+    c2 = qemu_coroutine_create(rwlock_c2, &l);
+    c3 = qemu_coroutine_create(rwlock_c3, &l);
+    c4 = qemu_coroutine_create(rwlock_c4, &l);
+
+    qemu_coroutine_enter(c1);
+    qemu_coroutine_enter(c2);
+    qemu_coroutine_enter(c3);
+    qemu_coroutine_enter(c4);
+
+    qemu_coroutine_enter(c1);
+
+    g_assert(c2_done);
+    g_assert(c3_done);
+    g_assert(c4_done);
+
+    qemu_coroutine_enter(c1);
+
+    g_assert(c1_done);
+}
+
 /*
  * Check that creation, enter, and return work
  */
@@ -501,6 +612,7 @@ int main(int argc, char **argv)
     g_test_add_func("/basic/order", test_order);
     g_test_add_func("/locking/co-mutex", test_co_mutex);
     g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable);
+    g_test_add_func("/locking/co-rwlock/downgrade", test_co_rwlock_downgrade);
     if (g_test_perf()) {
         g_test_add_func("/perf/lifecycle", perf_lifecycle);
         g_test_add_func("/perf/nesting", perf_nesting);
-- 
2.30.1

[PATCH v2 5/6] coroutine/rwlock: Wake writers in preference to readers

[David Edmondson]

A feature of the current rwlock is that if multiple coroutines hold a
reader lock, all must be runnable. The unlock implementation relies on
this, choosing to wake a single coroutine when the final read lock
holder exits the critical section, assuming that it will wake a
coroutine attempting to acquire a write lock.

The downgrade implementation violates this assumption by creating a
read lock owning coroutine that is exclusively runnable - any other
coroutines that are waiting to acquire a read lock are *not* made
runnable when the write lock holder converts its ownership to read
only.

As a result of this, a coroutine that downgrades a write lock can
later cause unlock to wake a coroutine that is attempting to acquire a
read lock rather than one aiming for a write lock, should the
coroutines be so ordered in the wait queue.

If the wait queue contains both read hopefuls and write hopefuls, any
read hopeful coroutine that is woken will immediately go back onto the
wait queue when it attempts to acquire the rwlock, due to the pending
write acquisition. At this point there are no coroutines holding
either read or write locks and no way for the coroutines in the queue
to be made runnable. A hang ensues.

Address this by using separate queues for coroutines attempting to
acquire read and write ownership of the rwlock. When unlocking, prefer
to make runnable a coroutine that is waiting for a write lock, but if
none is available, make all coroutines waiting to take a read lock
runnable.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 include/qemu/coroutine.h   |  8 +++++---
 util/qemu-coroutine-lock.c | 24 +++++++++++++++---------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 84eab6e3bf..3dfbf57faf 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -241,7 +241,8 @@ typedef struct CoRwlock {
     int pending_writer;
     int reader;
     CoMutex mutex;
-    CoQueue queue;
+    CoQueue rqueue;
+    CoQueue wqueue;
 } CoRwlock;
 
 /**
@@ -283,8 +284,9 @@ void qemu_co_rwlock_downgrade(CoRwlock *lock);
 void qemu_co_rwlock_wrlock(CoRwlock *lock);
 
 /**
- * Unlocks the read/write lock and schedules the next coroutine that was
- * waiting for this lock to be run.
+ * Unlocks the read/write lock and schedules the next coroutine that
+ * was waiting for this lock to be run, preferring to wake one
+ * attempting to take a write lock over those taking a read lock.
  */
 void qemu_co_rwlock_unlock(CoRwlock *lock);
 
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index eb73cf11dc..c05c143142 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -330,7 +330,8 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
 void qemu_co_rwlock_init(CoRwlock *lock)
 {
     memset(lock, 0, sizeof(*lock));
-    qemu_co_queue_init(&lock->queue);
+    qemu_co_queue_init(&lock->rqueue);
+    qemu_co_queue_init(&lock->wqueue);
     qemu_co_mutex_init(&lock->mutex);
 }
 
@@ -341,7 +342,7 @@ void qemu_co_rwlock_rdlock(CoRwlock *lock)
     qemu_co_mutex_lock(&lock->mutex);
     /* For fairness, wait if a writer is in line.  */
     while (lock->pending_writer) {
-        qemu_co_queue_wait(&lock->queue, &lock->mutex);
+        qemu_co_queue_wait(&lock->rqueue, &lock->mutex);
     }
     lock->reader++;
     qemu_co_mutex_unlock(&lock->mutex);
@@ -356,17 +357,22 @@ void qemu_co_rwlock_unlock(CoRwlock *lock)
 
     assert(qemu_in_coroutine());
     if (!lock->reader) {
-        /* The critical section started in qemu_co_rwlock_wrlock.  */
-        qemu_co_queue_restart_all(&lock->queue);
+        /* The critical section started in qemu_co_rwlock_wrlock or
+         * qemu_co_rwlock_upgrade.
+         */
+        qemu_co_queue_restart_all(&lock->wqueue);
+        qemu_co_queue_restart_all(&lock->rqueue);
     } else {
         self->locks_held--;
 
         qemu_co_mutex_lock(&lock->mutex);
         lock->reader--;
         assert(lock->reader >= 0);
-        /* Wakeup only one waiting writer */
-        if (!lock->reader) {
-            qemu_co_queue_next(&lock->queue);
+        /* If there are no remaining readers wake one waiting writer
+         * or all waiting readers.
+         */
+        if (!lock->reader && !qemu_co_queue_next(&lock->wqueue)) {
+            qemu_co_queue_restart_all(&lock->rqueue);
         }
     }
     qemu_co_mutex_unlock(&lock->mutex);
@@ -392,7 +398,7 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock)
     qemu_co_mutex_lock(&lock->mutex);
     lock->pending_writer++;
     while (lock->reader) {
-        qemu_co_queue_wait(&lock->queue, &lock->mutex);
+        qemu_co_queue_wait(&lock->wqueue, &lock->mutex);
     }
     lock->pending_writer--;
 
@@ -411,7 +417,7 @@ void qemu_co_rwlock_upgrade(CoRwlock *lock)
     lock->reader--;
     lock->pending_writer++;
     while (lock->reader) {
-        qemu_co_queue_wait(&lock->queue, &lock->mutex);
+        qemu_co_queue_wait(&lock->wqueue, &lock->mutex);
     }
     lock->pending_writer--;
 
-- 
2.30.1

[PATCH v2 6/6] coroutine/rwlock: Avoid thundering herd when unlocking

[David Edmondson]

Given that we know whether the queued coroutines are reader hopefuls
or writer hopefuls, avoid marking all of the queued coroutines as
runnable when unlocking, choosing instead to wake a single queued
writer or all queued readers.

Suggested-by: Paolo Bonzini <paolo.bonzini@redhat.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 util/qemu-coroutine-lock.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index c05c143142..6e399f28c1 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -356,24 +356,20 @@ void qemu_co_rwlock_unlock(CoRwlock *lock)
     Coroutine *self = qemu_coroutine_self();
 
     assert(qemu_in_coroutine());
-    if (!lock->reader) {
-        /* The critical section started in qemu_co_rwlock_wrlock or
-         * qemu_co_rwlock_upgrade.
-         */
-        qemu_co_queue_restart_all(&lock->wqueue);
-        qemu_co_queue_restart_all(&lock->rqueue);
-    } else {
+    if (lock->reader) {
         self->locks_held--;
 
+        /* Read-side critical sections do not hold lock->mutex. */
         qemu_co_mutex_lock(&lock->mutex);
         lock->reader--;
         assert(lock->reader >= 0);
-        /* If there are no remaining readers wake one waiting writer
-         * or all waiting readers.
-         */
-        if (!lock->reader && !qemu_co_queue_next(&lock->wqueue)) {
-            qemu_co_queue_restart_all(&lock->rqueue);
-        }
+    }
+
+    /* If there are no remaining readers wake one waiting writer or
+     * all waiting readers.
+     */
+    if (!lock->reader && !qemu_co_queue_next(&lock->wqueue)) {
+        qemu_co_queue_restart_all(&lock->rqueue);
     }
     qemu_co_mutex_unlock(&lock->mutex);
 }
-- 
2.30.1