Re: [Qemu-devel] [PULL 1/4] curl: do not use aio_context_acquire/release

[Paolo Bonzini]

On 03/05/2017 16:54, Richard W.M. Jones wrote:
> On Mon, Feb 27, 2017 at 04:34:44PM +0000, Stefan Hajnoczi wrote:
>> From: Paolo Bonzini <address@hidden>
>>
>> Now that all bottom halves and callbacks take care of taking the
>> AioContext lock, we can migrate some users away from it and to a
>> specific QemuMutex or CoMutex.
>>
>> Protect BDRVCURLState access with a QemuMutex.
>>
>> Reviewed-by: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> Message-id: address@hidden
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1447590
> 
> I've been tracking down a bug in the curl driver which affects
> virt-v2v, and this commit is implicated.
> 
> It manifests itself as a hang while downloading a certain file within
> a remotely located disk image accessed over https.
> 
> Unfortunately the bug environment is extremely difficult to reproduce
> (not the bug itself -- that is very easy to reproduce once you've set
> up the environment).  Anyway I don't have a simple reproducer which
> anyone could try.  I'll try to work on that next.
> 
> However I bisected it and it is caused by this commit.  The hang
> affects qemu from master.  Reverting this commit on top of qemu from
> master fixes the hang.
> 
> Is there anything obviously wrong with the commit?

Maybe there is, can you grab an all-threads backtrace via gdb?

Paolo

> Rich.
> 
>>  block/curl.c | 24 +++++++++++++++---------
>>  1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 2939cc7..e83dcd8 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -135,6 +135,7 @@ typedef struct BDRVCURLState {
>>      char *cookie;
>>      bool accept_range;
>>      AioContext *aio_context;
>> +    QemuMutex mutex;
>>      char *username;
>>      char *password;
>>      char *proxyusername;
>> @@ -333,6 +334,7 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, 
>> size_t len,
>>      return FIND_RET_NONE;
>>  }
>>  
>> +/* Called with s->mutex held.  */
>>  static void curl_multi_check_completion(BDRVCURLState *s)
>>  {
>>      int msgs_in_queue;
>> @@ -374,7 +376,9 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>>                          continue;
>>                      }
>>  
>> +                    qemu_mutex_unlock(&s->mutex);
>>                      acb->common.cb(acb->common.opaque, -EPROTO);
>> +                    qemu_mutex_lock(&s->mutex);
>>                      qemu_aio_unref(acb);
>>                      state->acb[i] = NULL;
>>                  }
>> @@ -386,6 +390,7 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>>      }
>>  }
>>  
>> +/* Called with s->mutex held.  */
>>  static void curl_multi_do_locked(CURLState *s)
>>  {
>>      CURLSocket *socket, *next_socket;
>> @@ -409,19 +414,19 @@ static void curl_multi_do(void *arg)
>>  {
>>      CURLState *s = (CURLState *)arg;
>>  
>> -    aio_context_acquire(s->s->aio_context);
>> +    qemu_mutex_lock(&s->s->mutex);
>>      curl_multi_do_locked(s);
>> -    aio_context_release(s->s->aio_context);
>> +    qemu_mutex_unlock(&s->s->mutex);
>>  }
>>  
>>  static void curl_multi_read(void *arg)
>>  {
>>      CURLState *s = (CURLState *)arg;
>>  
>> -    aio_context_acquire(s->s->aio_context);
>> +    qemu_mutex_lock(&s->s->mutex);
>>      curl_multi_do_locked(s);
>>      curl_multi_check_completion(s->s);
>> -    aio_context_release(s->s->aio_context);
>> +    qemu_mutex_unlock(&s->s->mutex);
>>  }
>>  
>>  static void curl_multi_timeout_do(void *arg)
>> @@ -434,11 +439,11 @@ static void curl_multi_timeout_do(void *arg)
>>          return;
>>      }
>>  
>> -    aio_context_acquire(s->aio_context);
>> +    qemu_mutex_lock(&s->mutex);
>>      curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
>>  
>>      curl_multi_check_completion(s);
>> -    aio_context_release(s->aio_context);
>> +    qemu_mutex_unlock(&s->mutex);
>>  #else
>>      abort();
>>  #endif
>> @@ -771,6 +776,7 @@ static int curl_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>      curl_easy_cleanup(state->curl);
>>      state->curl = NULL;
>>  
>> +    qemu_mutex_init(&s->mutex);
>>      curl_attach_aio_context(bs, bdrv_get_aio_context(bs));
>>  
>>      qemu_opts_del(opts);
>> @@ -801,12 +807,11 @@ static void curl_readv_bh_cb(void *p)
>>      CURLAIOCB *acb = p;
>>      BlockDriverState *bs = acb->common.bs;
>>      BDRVCURLState *s = bs->opaque;
>> -    AioContext *ctx = bdrv_get_aio_context(bs);
>>  
>>      size_t start = acb->sector_num * BDRV_SECTOR_SIZE;
>>      size_t end;
>>  
>> -    aio_context_acquire(ctx);
>> +    qemu_mutex_lock(&s->mutex);
>>  
>>      // In case we have the requested data already (e.g. read-ahead),
>>      // we can just call the callback and be done.
>> @@ -854,7 +859,7 @@ static void curl_readv_bh_cb(void *p)
>>      curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
>>  
>>  out:
>> -    aio_context_release(ctx);
>> +    qemu_mutex_unlock(&s->mutex);
>>      if (ret != -EINPROGRESS) {
>>          acb->common.cb(acb->common.opaque, ret);
>>          qemu_aio_unref(acb);
>> @@ -883,6 +888,7 @@ static void curl_close(BlockDriverState *bs)
>>  
>>      DPRINTF("CURL: Close\n");
>>      curl_detach_aio_context(bs);
>> +    qemu_mutex_destroy(&s->mutex);
>>  
>>      g_free(s->cookie);
>>      g_free(s->url);
>> -- 
>> 2.9.3
>>
>