thread-pool: fix deadlock when callbacks depends on each other

When two coroutines submit I/O and first coroutine depends on second to complete (by calling bdrv_drain_all), deadlock may occur.

This is because both requests may have completed before thread pool notifier got called. Then, when notifier gets executed and first coroutine calls aio_pool() to make progress, it will hang forever, as notifier's descriptor has been already marked clear.

This patch fixes this, by deferring clearing notifier until no completions are pending.

Without this patch, I could reproduce this bug with snapshot-commit with about 1 per 10 tries. With this patch, I couldn't reproduce it any more.

---

@@ -76,6 +76,8 @@ struct ThreadPool {
     int new_threads;     /* backlog of threads we need to create */
     int pending_threads; /* threads created but not running yet */
     int pending_cancellations; /* whether we need a cond_broadcast */
+    int pending_completions; /* whether we need to rearm notifier when
+                                executing callback */
     bool stopping;
 };
 

@@ -110,6 +112,10 @@ static void *worker_thread(void *opaque)
         ret = req->func(req->arg);
 
         req->ret = ret;
+        if (req->common.cb) {
+            atomic_inc(&pool->pending_completions);
+        }
+
         /* Write ret before state.  */
         smp_wmb();
         req->state = THREAD_DONE;

@@ -173,7 +179,6 @@ static void event_notifier_ready(EventNo
     ThreadPool *pool = container_of(notifier, ThreadPool, notifier);
     ThreadPoolElement *elem, *next;
 
-    event_notifier_test_and_clear(notifier);
 restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
         if (elem->state != THREAD_CANCELED && elem->state != 

THREAD_DONE) {
@@ -185,6 +190,8 @@ restart:

         }
         if (elem->state == THREAD_DONE && elem->common.cb) {
             QLIST_REMOVE(elem, all);
+            atomic_dec(&pool->pending_completions);
+
             /* Read state before ret.  */
             smp_rmb();
             elem->common.cb(elem->common.opaque, elem->ret);
@@ -196,6 +203,19 @@ restart:
             qemu_aio_release(elem);
         }
     }
+
+    /* Double test of pending_completions is necessary to
+     * ensure that there is no race between testing it and
+     * clearing notifier.
+     */
+    if (atomic_read(&pool->pending_completions)) {
+        goto restart;
+    }
+    event_notifier_test_and_clear(notifier);
+    if (atomic_read(&pool->pending_completions)) {

+ event_notifier_set(notifier);
+ goto restart;
+ }
}

static void thread_pool_cancel(BlockDriverAIOCB *acb)

bdrv_drain_all() is a very heavy-weight operation. Coroutines should avoid it if possible. Please post the file/line/function where this call was made, perhaps there is a better way to wait for the other coroutine. This isn't a fix for this bug but it's a cleanup.

This is an interesting bug that definitely needs a test case to prevent regressions in the future.

Please take a look at tests/test-thread-pool.c and add a test to it. It can be reproduced deterministically - just call aio_poll() after the dummy worker functions have both completed. Then the next aio_poll() call in the thread pool callback will suffer the problem you described.

Stefan

Il 04/06/2014 12:01, Stefan Hajnoczi ha scritto:

The question if we want to consider this thread-pool.c behavior a real bug or just a misfeature (the real bug being elsewhere).

Even though this patch avoids the performance problems of v1, we would have to fix at least two other cases and it's not obvious (a) that those two are the only ones (b) tgat those two can be fixed without affecting performance.

If the bottom half code is immune from this event notifier problem, bdrv_drain/bdrv_drain_all calls in coroutine context can defer the actual draining to a bottom half and reenter the coroutine afterwards; we can then audit that all other calls should come from the main loop rather than aio_poll.

Paolo

As in original bug report:

#4  0x00007f699c095c0a in bdrv_drain_all () at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/block.c:1805
#5  0x00007f699c09c87e in bdrv_close (bs=bs@entry=0x7f699f0bc520) at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/block.c:1695
#6  0x00007f699c09c5fa in bdrv_delete (bs=0x7f699f0bc520) at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/block.c:1978
#7  bdrv_unref (bs=0x7f699f0bc520) at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/block.c:5198
#8  0x00007f699c09c812 in bdrv_drop_intermediate (active=active@entry=0x7f699ebfd330, top=top@entry=0x7f699f0bc520, base=base@entry=0x7f699eec43d0) at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/block.c:2567
#9  0x00007f699c0a1963 in commit_run (opaque=0x7f699f17dcc0) at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/block/commit.c:144

#10 0x00007f699c0e0dca in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at /var/tmp/portage/app-emulation/qemu-2.0.0_rc2/work/qemu-2.0.0-rc2/coroutine-ucontext.c:118

mirror_run probably has this as well. I didn't check others.

mg