Re: [Qemu-devel] [PATCH] qemu-coroutine-lock: fix co_queue multi-adding bug

[Bin Wu <wu.wubin@huawei.com>]

On 2015/2/9 16:12, Fam Zheng wrote:
> On Sat, 02/07 17:51, w00214312 wrote:
>> From: Bin Wu <wu.wubin@huawei.com>
>>
>> When a coroutine holds a lock, other coroutines who want to get
>> the lock must wait on a co_queue by adding themselves to the
>> CoQueue. However, if a waiting coroutine is woken up with the
>> lock still be holding by other coroutine, this waiting coroutine
> 
> Could you explain who wakes up the waiting coroutine? Maybe the bug is that
> it shouldn't be awaken in the first place.
> 

During the mirror phase with nbd devices, if we send a cancel command or
physical network breaks down, the source qemu process will receive a readable
event and the main loop will invoke nbd_reply_ready to deal with it. This
function finds the connection is down and then goes into
nbd_teardown_connection. nbd_teardown_connection wakes up all working coroutines
by nbd_recv_coroutines_enter_all. These coroutines include the one which holds
the sending lock, the ones which wait for the lock, and the ones which wait for
receiving messages.

I think the purpose of nbd_recv_coroutines_enter_all is to terminate all waiting
coroutines by waking all of them up. If the coroutine waiting for the lock is
allowed for waking up, this implementation is ok. If not, we need to distinguish
the coroutines waiting for receiving messages from the ones waiting for the lock.

In my option, if the coroutines waiting for a lock is allowd for waking up, it
should be more robust :>

>> will add itself to the co_queue again. Latter, when the lock
>> is released, a coroutine re-enter will occur.
>>
>> We need to determine whether a coroutine is alread in the co_queue
> 
> s/alread/already/
> 
> Fam
> 

Thanks, my mistake.

>> before adding it to the waiting queue.
>>
>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>> ---
>>  include/block/coroutine_int.h | 1 +
>>  qemu-coroutine-lock.c         | 6 +++++-
>>  qemu-coroutine.c              | 1 +
>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
>> index f133d65..c524990 100644
>> --- a/include/block/coroutine_int.h
>> +++ b/include/block/coroutine_int.h
>> @@ -42,6 +42,7 @@ struct Coroutine {
>>      /* Coroutines that should be woken up when we yield or terminate */
>>      QTAILQ_HEAD(, Coroutine) co_queue_wakeup;
>>      QTAILQ_ENTRY(Coroutine) co_queue_next;
>> +    bool in_co_queue;
>>  };
>>  
>>  Coroutine *qemu_coroutine_new(void);
>> diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
>> index e4860ae..d256f53 100644
>> --- a/qemu-coroutine-lock.c
>> +++ b/qemu-coroutine-lock.c
>> @@ -36,7 +36,10 @@ void qemu_co_queue_init(CoQueue *queue)
>>  void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
>>  {
>>      Coroutine *self = qemu_coroutine_self();
>> -    QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
>> +    if (!self->in_co_queue) {
>> +        QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
>> +        self->in_co_queue = true;
>> +    }
>>      qemu_coroutine_yield();
>>      assert(qemu_in_coroutine());
>>  }
>> @@ -71,6 +74,7 @@ static bool qemu_co_queue_do_restart(CoQueue *queue, bool single)
>>  
>>      while ((next = QTAILQ_FIRST(&queue->entries)) != NULL) {
>>          QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
>> +        next->in_co_queue = false;
>>          QTAILQ_INSERT_TAIL(&self->co_queue_wakeup, next, co_queue_next);
>>          trace_qemu_co_queue_next(next);
>>          if (single) {
>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>> index 525247b..a103721 100644
>> --- a/qemu-coroutine.c
>> +++ b/qemu-coroutine.c
>> @@ -75,6 +75,7 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
>>      }
>>  
>>      co->entry = entry;
>> +    co->in_co_queue = false;
>>      QTAILQ_INIT(&co->co_queue_wakeup);
>>      return co;
>>  }
>> -- 
>> 1.7.12.4
>>
>>
> 
> .
> 

-- 
Bin Wu