[Qemu-devel] [PATCH v2 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Qemu-devel] [PATCH v2 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Stefan Hajnoczi]

v2:
 * Leave BH scheduled so that the code can be simplified [Paolo]

These patches convert thread-pool.c from EventNotifier to QEMUBH.  They then
solve the deadlock when nested aio_poll() calls are made.

Please speak out whether you want this in QEMU 2.1 or not.  I'm not aware of
the nested aio_poll() deadlock ever having been reported, so maybe we can defer
to QEMU 2.2.

Stefan Hajnoczi (2):
  thread-pool: avoid per-thread-pool EventNotifier
  thread-pool: avoid deadlock in nested aio_poll() calls

 thread-pool.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/2] thread-pool: avoid per-thread-pool EventNotifier

[Stefan Hajnoczi]

EventNotifier is implemented using an eventfd or pipe.  It therefore
consumes file descriptors, which can be limited by rlimits and should
therefore be used sparingly.

Switch from EventNotifier to QEMUBH in thread-pool.c.  Originally
EventNotifier was used because qemu_bh_schedule() was not thread-safe
yet.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 thread-pool.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/thread-pool.c b/thread-pool.c
index dfb699d..4cfd078 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -21,7 +21,6 @@
 #include "block/coroutine.h"
 #include "trace.h"
 #include "block/block_int.h"
-#include "qemu/event_notifier.h"
 #include "block/thread-pool.h"
 #include "qemu/main-loop.h"
 
@@ -57,8 +56,8 @@ struct ThreadPoolElement {
 };
 
 struct ThreadPool {
-    EventNotifier notifier;
     AioContext *ctx;
+    QEMUBH *completion_bh;
     QemuMutex lock;
     QemuCond check_cancel;
     QemuCond worker_stopped;
@@ -119,7 +118,7 @@ static void *worker_thread(void *opaque)
             qemu_cond_broadcast(&pool->check_cancel);
         }
 
-        event_notifier_set(&pool->notifier);
+        qemu_bh_schedule(pool->completion_bh);
     }
 
     pool->cur_threads--;
@@ -168,12 +167,11 @@ static void spawn_thread(ThreadPool *pool)
     }
 }
 
-static void event_notifier_ready(EventNotifier *notifier)
+static void thread_pool_completion_bh(void *opaque)
 {
-    ThreadPool *pool = container_of(notifier, ThreadPool, notifier);
+    ThreadPool *pool = opaque;
     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) {
@@ -215,7 +213,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
         qemu_sem_timedwait(&pool->sem, 0) == 0) {
         QTAILQ_REMOVE(&pool->request_list, elem, reqs);
         elem->state = THREAD_CANCELED;
-        event_notifier_set(&pool->notifier);
+        qemu_bh_schedule(pool->completion_bh);
     } else {
         pool->pending_cancellations++;
         while (elem->state != THREAD_CANCELED && elem->state != THREAD_DONE) {
@@ -224,7 +222,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
         pool->pending_cancellations--;
     }
     qemu_mutex_unlock(&pool->lock);
-    event_notifier_ready(&pool->notifier);
+    thread_pool_completion_bh(pool);
 }
 
 static const AIOCBInfo thread_pool_aiocb_info = {
@@ -293,8 +291,8 @@ static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
     }
 
     memset(pool, 0, sizeof(*pool));
-    event_notifier_init(&pool->notifier, false);
     pool->ctx = ctx;
+    pool->completion_bh = aio_bh_new(ctx, thread_pool_completion_bh, pool);
     qemu_mutex_init(&pool->lock);
     qemu_cond_init(&pool->check_cancel);
     qemu_cond_init(&pool->worker_stopped);
@@ -304,8 +302,6 @@ static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
 
     QLIST_INIT(&pool->head);
     QTAILQ_INIT(&pool->request_list);
-
-    aio_set_event_notifier(ctx, &pool->notifier, event_notifier_ready);
 }
 
 ThreadPool *thread_pool_new(AioContext *ctx)
@@ -339,11 +335,10 @@ void thread_pool_free(ThreadPool *pool)
 
     qemu_mutex_unlock(&pool->lock);
 
-    aio_set_event_notifier(pool->ctx, &pool->notifier, NULL);
+    qemu_bh_delete(pool->completion_bh);
     qemu_sem_destroy(&pool->sem);
     qemu_cond_destroy(&pool->check_cancel);
     qemu_cond_destroy(&pool->worker_stopped);
     qemu_mutex_destroy(&pool->lock);
-    event_notifier_cleanup(&pool->notifier);
     g_free(pool);
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

The thread pool has a race condition if two elements complete before
thread_pool_completion_bh() runs:

  If element A's callback waits for element B using aio_poll() it will
  deadlock since pool->completion_bh is not marked scheduled when the
  nested aio_poll() runs.

Fix this by marking the BH scheduled while thread_pool_completion_bh()
is executing.  This way any nested aio_poll() loops will enter
thread_pool_completion_bh() and complete the remaining elements.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 thread-pool.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/thread-pool.c b/thread-pool.c
index 4cfd078..23888dc 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -185,6 +185,12 @@ restart:
             QLIST_REMOVE(elem, all);
             /* Read state before ret.  */
             smp_rmb();
+
+            /* Schedule ourselves in case elem->common.cb() calls aio_poll() to
+             * wait for another request that completed at the same time.
+             */
+            qemu_bh_schedule(pool->completion_bh);
+
             elem->common.cb(elem->common.opaque, elem->ret);
             qemu_aio_release(elem);
             goto restart;
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Paolo Bonzini]

Il 15/07/2014 16:44, Stefan Hajnoczi ha scritto:
> v2:
>  * Leave BH scheduled so that the code can be simplified [Paolo]
>
> These patches convert thread-pool.c from EventNotifier to QEMUBH.  They then
> solve the deadlock when nested aio_poll() calls are made.
>
> Please speak out whether you want this in QEMU 2.1 or not.  I'm not aware of
> the nested aio_poll() deadlock ever having been reported, so maybe we can defer
> to QEMU 2.2.

It was reported as a hang in block_commit.  Marcin, can you please test 
these patches?

Paolo

Re: [Qemu-devel] [PATCH v2 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Paolo Bonzini]

Il 15/07/2014 16:44, Stefan Hajnoczi ha scritto:
> The thread pool has a race condition if two elements complete before
> thread_pool_completion_bh() runs:
>
>   If element A's callback waits for element B using aio_poll() it will
>   deadlock since pool->completion_bh is not marked scheduled when the
>   nested aio_poll() runs.
>
> Fix this by marking the BH scheduled while thread_pool_completion_bh()
> is executing.  This way any nested aio_poll() loops will enter
> thread_pool_completion_bh() and complete the remaining elements.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  thread-pool.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/thread-pool.c b/thread-pool.c
> index 4cfd078..23888dc 100644
> --- a/thread-pool.c
> +++ b/thread-pool.c
> @@ -185,6 +185,12 @@ restart:
>              QLIST_REMOVE(elem, all);
>              /* Read state before ret.  */
>              smp_rmb();
> +
> +            /* Schedule ourselves in case elem->common.cb() calls aio_poll() to
> +             * wait for another request that completed at the same time.
> +             */
> +            qemu_bh_schedule(pool->completion_bh);
> +
>              elem->common.cb(elem->common.opaque, elem->ret);
>              qemu_aio_release(elem);
>              goto restart;

Please make this "return" instead, so that it is clearer that the next 
invocation of the BH is a continuation of this one.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Marcin Gibuła]

W dniu 2014-07-15 17:17, Paolo Bonzini pisze:
> Il 15/07/2014 16:44, Stefan Hajnoczi ha scritto:
>> v2:
>>  * Leave BH scheduled so that the code can be simplified [Paolo]
>>
>> These patches convert thread-pool.c from EventNotifier to QEMUBH.
>> They then
>> solve the deadlock when nested aio_poll() calls are made.
>>
>> Please speak out whether you want this in QEMU 2.1 or not.  I'm not
>> aware of
>> the nested aio_poll() deadlock ever having been reported, so maybe we
>> can defer
>> to QEMU 2.2.
>
> It was reported as a hang in block_commit.  Marcin, can you please test
> these patches?

I try to test it tomorrow. The same hang was also in linux-aio however 
(I was able to reproduce it).

-- 
mg

Re: [Qemu-devel] [PATCH v2 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Tue, Jul 15, 2014 at 04:44:24PM +0200, Stefan Hajnoczi wrote:
> v2:
>  * Leave BH scheduled so that the code can be simplified [Paolo]
> 
> These patches convert thread-pool.c from EventNotifier to QEMUBH.  They then
> solve the deadlock when nested aio_poll() calls are made.
> 
> Please speak out whether you want this in QEMU 2.1 or not.  I'm not aware of
> the nested aio_poll() deadlock ever having been reported, so maybe we can defer
> to QEMU 2.2.
> 
> Stefan Hajnoczi (2):
>   thread-pool: avoid per-thread-pool EventNotifier
>   thread-pool: avoid deadlock in nested aio_poll() calls
> 
>  thread-pool.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Marcin Gibuła]

On 15.07.2014 17:17, Paolo Bonzini wrote:
> Il 15/07/2014 16:44, Stefan Hajnoczi ha scritto:
>> v2:
>>  * Leave BH scheduled so that the code can be simplified [Paolo]
>>
>> These patches convert thread-pool.c from EventNotifier to QEMUBH.
>> They then
>> solve the deadlock when nested aio_poll() calls are made.
>>
>> Please speak out whether you want this in QEMU 2.1 or not.  I'm not
>> aware of
>> the nested aio_poll() deadlock ever having been reported, so maybe we
>> can defer
>> to QEMU 2.2.
>
> It was reported as a hang in block_commit.  Marcin, can you please test
> these patches?

Sorry for late answer - yes, it seems to fix block_commit hang when 
using thread-pool.

-- 
mg