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 rearming thread pool notifier if there are 
more than one completed requests on list.

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.

Signed-off-by: Marcin Gibula <m.gibula@beyond.pl>
---

Il 31/05/2014 20:29, Marcin Gibuła ha scritto:
>
> @@ -185,6 +191,14 @@ restart:
>          }
>          if (elem->state == THREAD_DONE && elem->common.cb) {
>              QLIST_REMOVE(elem, all);
> +            /* If more completed requests are waiting, notifier needs
> +             * to be rearmed so callback can progress with aio_pool().
> +             */
> +            pool->pending_completions--;
> +            if (pool->pending_completions) {
> +                event_notifier_set(notifier);
> +            }
> +
>              /* Read state before ret.  */
>              smp_rmb();
>              elem->common.cb(elem->common.opaque, elem->ret);

Good catch!  The main problem with the patch is that you need to use 
atomic_inc/atomic_dec to increment and decrement pool->pending_completions.

Secondarily, event_notifier_set is pretty heavy-weight, does it work if 
you wrap the loop like this?

restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
         ...
     }
     if (pool->pending_completions) {
         goto restart;
     }
     event_notifier_test_and_clear(notifier);
     if (pool->pending_completions) {
         event_notifier_set(notifier);
         goto restart;
     }

Finally, the same bug is also in block/linux-aio.c and block/win32-aio.c.

Paolo

> Good catch!  The main problem with the patch is that you need to use
> atomic_inc/atomic_dec to increment and decrement pool->pending_completions.

Ok.

> Secondarily, event_notifier_set is pretty heavy-weight, does it work if
> you wrap the loop like this?
>
> restart:
>      QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
>          ...
>      }
>      if (pool->pending_completions) {
>          goto restart;
>      }
>      event_notifier_test_and_clear(notifier);
>      if (pool->pending_completions) {
>          event_notifier_set(notifier);
>          goto restart;
>      }

I'll test it tomorrow. I assume you want to avoid calling 
event_notifier_set() until function is reentered via aio_pool?

 > Finally, the same bug is also in block/linux-aio.c and
 > block/win32-aio.c.

I can try with linux-aio, but my knowledge of windows api is zero...

Il 01/06/2014 21:02, Marcin Gibuła ha scritto:
>> Good catch!  The main problem with the patch is that you need to use
>> atomic_inc/atomic_dec to increment and decrement
>> pool->pending_completions.
>
> Ok.
>
>> Secondarily, event_notifier_set is pretty heavy-weight, does it work if
>> you wrap the loop like this?
>>
>> restart:
>>      QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
>>          ...
>>      }
>>      if (pool->pending_completions) {
>>          goto restart;
>>      }
>>      event_notifier_test_and_clear(notifier);
>>      if (pool->pending_completions) {
>>          event_notifier_set(notifier);
>>          goto restart;
>>      }
>
> I'll test it tomorrow. I assume you want to avoid calling
> event_notifier_set() until function is reentered via aio_pool?

Yes.  But actually, I need to check if it's possible to fix 
bdrv_drain_all.  If you're in coroutine context, you can defer the 
draining to a safe point using a bottom half.  If you're not in 
coroutine context, perhaps bdrv_drain_all has to be made illegal.  Which 
means a bunch of code auditing...

Paolo

>> Finally, the same bug is also in block/linux-aio.c and
>> block/win32-aio.c.
>
> I can try with linux-aio, but my knowledge of windows api is zero...
>

>> I'll test it tomorrow. I assume you want to avoid calling
>> event_notifier_set() until function is reentered via aio_pool?
>
> Yes.  But actually, I need to check if it's possible to fix
> bdrv_drain_all.  If you're in coroutine context, you can defer the
> draining to a safe point using a bottom half.  If you're not in
> coroutine context, perhaps bdrv_drain_all has to be made illegal.  Which
> means a bunch of code auditing...

For what it's worth, your solution also works fine, I couldn't recreate 
hang with it. Updated patch proposal posted earlier today.