[Qemu-devel] [PULL 0/4] Block patches

[Qemu-devel] [PULL 0/4] Block patches

[Stefan Hajnoczi]

The following changes since commit d992f2f1368ceb92e6bfd8efece174110f4236ff:

  Merge remote-tracking branch 'remotes/artyom/tags/pull-sun4v-20170226' into staging (2017-02-26 22:40:23 +0000)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 1ab17f9f5c63c2798d707aeb22588e4fcc17b2cd:

  tests-aio-multithread: use atomic_read properly (2017-02-27 14:00:53 +0000)

----------------------------------------------------------------

----------------------------------------------------------------

Paolo Bonzini (4):
  curl: do not use aio_context_acquire/release
  nfs: do not use aio_context_acquire/release
  iscsi: do not use aio_context_acquire/release
  tests-aio-multithread: use atomic_read properly

 block/curl.c                 | 24 ++++++++-----
 block/iscsi.c                | 83 ++++++++++++++++++++++++++++++++++----------
 block/nfs.c                  | 23 +++++++++---
 tests/test-aio-multithread.c |  4 +--
 4 files changed, 100 insertions(+), 34 deletions(-)

-- 
2.9.3

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

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

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 <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20170222180725.28611-2-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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

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

[Richard W.M. Jones]

On Mon, Feb 27, 2017 at 04:34:44PM +0000, Stefan Hajnoczi wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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 <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-id: 20170222180725.28611-2-pbonzini@redhat.com
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

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?

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
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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 <pbonzini@redhat.com>
>>
>> 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 <stefanha@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Message-id: 20170222180725.28611-2-pbonzini@redhat.com
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> 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
>>
> 

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

[Richard W.M. Jones]

On Wed, May 03, 2017 at 04:59:22PM +0200, Paolo Bonzini wrote:
> Maybe there is, can you grab an all-threads backtrace via gdb?

Program received signal SIGINT, Interrupt.
0x00007f1d57f861bd in __lll_lock_wait () from /lib64/libpthread.so.0
(gdb) t a a bt

Thread 9 (Thread 0x7f1d49967700 (LWP 29949)):
#0  0x00007f1d53a39bf9 in syscall () at /lib64/libc.so.6
#1  0x000055ea5c358856 in qemu_event_wait (val=<optimized out>, f=<optimized out>) at /home/rjones/d/qemu/include/qemu/futex.h:26
#2  0x000055ea5c358856 in qemu_event_wait (ev=ev@entry=0x55ea5cde4ac4 <rcu_call_ready_event>) at util/qemu-thread-posix.c:399
#3  0x000055ea5c3680fe in call_rcu_thread (opaque=<optimized out>)
    at util/rcu.c:249
#4  0x00007f1d57f7fdc5 in start_thread () at /lib64/libpthread.so.0
#5  0x00007f1d53a3f73d in clone () at /lib64/libc.so.6

Thread 7 (Thread 0x7f1d46987700 (LWP 29955)):
#0  0x00007f1d57f861bd in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x00007f1d57f81d02 in _L_lock_791 () at /lib64/libpthread.so.0
#2  0x00007f1d57f81c08 in pthread_mutex_lock () at /lib64/libpthread.so.0
#3  0x000055ea5c3583e9 in qemu_mutex_lock (mutex=mutex@entry=0x55ea5c9a2400 <qemu_global_mutex>) at util/qemu-thread-posix.c:60
#4  0x000055ea5bff512c in qemu_mutex_lock_iothread ()
    at /home/rjones/d/qemu/cpus.c:1565
#5  0x000055ea5c0081c7 in kvm_cpu_exec (cpu=cpu@entry=0x55ea5e7df020)
    at /home/rjones/d/qemu/kvm-all.c:2096
#6  0x000055ea5bff5332 in qemu_kvm_cpu_thread_fn (arg=0x55ea5e7df020)
    at /home/rjones/d/qemu/cpus.c:1118
#7  0x00007f1d57f7fdc5 in start_thread () at /lib64/libpthread.so.0
#8  0x00007f1d53a3f73d in clone () at /lib64/libc.so.6

Thread 1 (Thread 0x7f1d5a4e2c00 (LWP 29947)):
#0  0x00007f1d57f861bd in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x00007f1d57f81d02 in _L_lock_791 () at /lib64/libpthread.so.0
#2  0x00007f1d57f81c08 in pthread_mutex_lock () at /lib64/libpthread.so.0
#3  0x000055ea5c3583e9 in qemu_mutex_lock (mutex=mutex@entry=0x55ea5e5ac178)
    at util/qemu-thread-posix.c:60
#4  0x000055ea5c2fb2d1 in curl_readv_bh_cb (p=0x55ea6013c620)
    at block/curl.c:824
#5  0x000055ea5c352fe1 in aio_bh_poll (bh=0x55ea60eb99d0) at util/async.c:90
#6  0x000055ea5c352fe1 in aio_bh_poll (ctx=ctx@entry=0x55ea5e555970)
    at util/async.c:118
#7  0x000055ea5c3563c4 in aio_poll (ctx=0x55ea5e555970, blocking=blocking@entry=true) at util/aio-posix.c:682
#8  0x000055ea5c2fb09e in curl_init_state (bs=0x55ea5e5a56b0, s=s@entry=0x55ea5e5ab100) at block/curl.c:470
#9  0x000055ea5c2fb39b in curl_readv_bh_cb (p=0x55ea5f650c30)
    at block/curl.c:839
#10 0x000055ea5c352fe1 in aio_bh_poll (bh=0x55ea60eb1910) at util/async.c:90
#11 0x000055ea5c352fe1 in aio_bh_poll (ctx=ctx@entry=0x55ea5e555970)
    at util/async.c:118
#12 0x000055ea5c355f80 in aio_dispatch (ctx=0x55ea5e555970)
    at util/aio-posix.c:429
#13 0x000055ea5c352ebe in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at util/async.c:261
#14 0x00007f1d55489d7a in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#15 0x000055ea5c35523c in main_loop_wait () at util/main-loop.c:213
#16 0x000055ea5c35523c in main_loop_wait (timeout=<optimized out>)
    at util/main-loop.c:261
---Type <return> to continue, or q <return> to quit---
#17 0x000055ea5c35523c in main_loop_wait (nonblocking=nonblocking@entry=0)
    at util/main-loop.c:517
#18 0x000055ea5bfb44a7 in main () at vl.c:1899
#19 0x000055ea5bfb44a7 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4717


I'll get you one with more symbols in a minute, just installing
debuginfo ...

BTW this bug is reproducible on both Fedora 26 and RHEL 7.4
(in both cases with either qemu 2.9 or qemu from git).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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

[Richard W.M. Jones]

Same backtrace, but with some more symbols:

__lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
135		2: movl	%edx, %eax

(gdb) t a a bt

Thread 3 (Thread 0x7f1d49967700 (LWP 29949)):
#0  0x00007f1d53a39bf9 in syscall ()
    at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
#1  0x000055ea5c358856 in qemu_event_wait (val=<optimized out>, f=<optimized out>) at /home/rjones/d/qemu/include/qemu/futex.h:26
#2  0x000055ea5c358856 in qemu_event_wait (ev=ev@entry=0x55ea5cde4ac4 <rcu_call_ready_event>) at util/qemu-thread-posix.c:399
#3  0x000055ea5c3680fe in call_rcu_thread (opaque=<optimized out>)
    at util/rcu.c:249
#4  0x00007f1d57f7fdc5 in start_thread (arg=0x7f1d49967700)
    at pthread_create.c:308
#5  0x00007f1d53a3f73d in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113

Thread 2 (Thread 0x7f1d46987700 (LWP 29955)):
#0  0x00007f1d57f861bd in __lll_lock_wait ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007f1d57f81d02 in _L_lock_791 () at /lib64/libpthread.so.0
#2  0x00007f1d57f81c08 in __GI___pthread_mutex_lock (mutex=mutex@entry=0x55ea5c9a2400 <qemu_global_mutex>) at pthread_mutex_lock.c:64
#3  0x000055ea5c3583e9 in qemu_mutex_lock (mutex=mutex@entry=0x55ea5c9a2400 <qemu_global_mutex>) at util/qemu-thread-posix.c:60
#4  0x000055ea5bff512c in qemu_mutex_lock_iothread ()
    at /home/rjones/d/qemu/cpus.c:1565
#5  0x000055ea5c0081c7 in kvm_cpu_exec (cpu=cpu@entry=0x55ea5e7df020)
    at /home/rjones/d/qemu/kvm-all.c:2096
#6  0x000055ea5bff5332 in qemu_kvm_cpu_thread_fn (arg=0x55ea5e7df020)
    at /home/rjones/d/qemu/cpus.c:1118
#7  0x00007f1d57f7fdc5 in start_thread (arg=0x7f1d46987700)
    at pthread_create.c:308
#8  0x00007f1d53a3f73d in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113

Thread 1 (Thread 0x7f1d5a4e2c00 (LWP 29947)):
#0  0x00007f1d57f861bd in __lll_lock_wait ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007f1d57f81d02 in _L_lock_791 () at /lib64/libpthread.so.0
#2  0x00007f1d57f81c08 in __GI___pthread_mutex_lock (mutex=mutex@entry=0x55ea5e5ac178) at pthread_mutex_lock.c:64
#3  0x000055ea5c3583e9 in qemu_mutex_lock (mutex=mutex@entry=0x55ea5e5ac178)
    at util/qemu-thread-posix.c:60
#4  0x000055ea5c2fb2d1 in curl_readv_bh_cb (p=0x55ea6013c620)
    at block/curl.c:824
#5  0x000055ea5c352fe1 in aio_bh_poll (bh=0x55ea60eb99d0) at util/async.c:90
#6  0x000055ea5c352fe1 in aio_bh_poll (ctx=ctx@entry=0x55ea5e555970)
    at util/async.c:118
#7  0x000055ea5c3563c4 in aio_poll (ctx=0x55ea5e555970, blocking=blocking@entry=true) at util/aio-posix.c:682
#8  0x000055ea5c2fb09e in curl_init_state (bs=0x55ea5e5a56b0, s=s@entry=0x55ea5e5ab100) at block/curl.c:470
#9  0x000055ea5c2fb39b in curl_readv_bh_cb (p=0x55ea5f650c30)
    at block/curl.c:839
#10 0x000055ea5c352fe1 in aio_bh_poll (bh=0x55ea60eb1910) at util/async.c:90
#11 0x000055ea5c352fe1 in aio_bh_poll (ctx=ctx@entry=0x55ea5e555970)
    at util/async.c:118
#12 0x000055ea5c355f80 in aio_dispatch (ctx=0x55ea5e555970)
    at util/aio-posix.c:429
#13 0x000055ea5c352ebe in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at util/async.c:261
#14 0x00007f1d55489d7a in g_main_context_dispatch (context=0x55ea5e555d40)
    at gmain.c:3152
#15 0x00007f1d55489d7a in g_main_context_dispatch (context=context@entry=0x55ea5e555d40) at gmain.c:3767
#16 0x000055ea5c35523c in main_loop_wait () at util/main-loop.c:213
#17 0x000055ea5c35523c in main_loop_wait (timeout=<optimized out>)
    at util/main-loop.c:261
#18 0x000055ea5c35523c in main_loop_wait (nonblocking=nonblocking@entry=0)
    at util/main-loop.c:517
#19 0x000055ea5bfb44a7 in main () at vl.c:1899
#20 0x000055ea5bfb44a7 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4717


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

[Paolo Bonzini]

On 03/05/2017 17:34, Richard W.M. Jones wrote:
> Thread 1 (Thread 0x7f1d5a4e2c00 (LWP 29947)):
> #0  0x00007f1d57f861bd in __lll_lock_wait ()
>     at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> #1  0x00007f1d57f81d02 in _L_lock_791 () at /lib64/libpthread.so.0
> #2  0x00007f1d57f81c08 in __GI___pthread_mutex_lock (mutex=mutex@entry=0x55ea5e5ac178) at pthread_mutex_lock.c:64
> #3  0x000055ea5c3583e9 in qemu_mutex_lock (mutex=mutex@entry=0x55ea5e5ac178)
>     at util/qemu-thread-posix.c:60
> #4  0x000055ea5c2fb2d1 in curl_readv_bh_cb (p=0x55ea6013c620)
>     at block/curl.c:824
> #5  0x000055ea5c352fe1 in aio_bh_poll (bh=0x55ea60eb99d0) at util/async.c:90
> #6  0x000055ea5c352fe1 in aio_bh_poll (ctx=ctx@entry=0x55ea5e555970)
>     at util/async.c:118
> #7  0x000055ea5c3563c4 in aio_poll (ctx=0x55ea5e555970, blocking=blocking@entry=true) at util/aio-posix.c:682
> #8  0x000055ea5c2fb09e in curl_init_state (bs=0x55ea5e5a56b0, s=s@entry=0x55ea5e5ab100) at block/curl.c:470
> #9  0x000055ea5c2fb39b in curl_readv_bh_cb (p=0x55ea5f650c30)
>     at block/curl.c:839
> #10 0x000055ea5c352fe1 in aio_bh_poll (bh=0x55ea60eb1910) at util/async.c:90
> #11 0x000055ea5c352fe1 in aio_bh_poll (ctx=ctx@entry=0x55ea5e555970)
>     at util/async.c:118
> #12 0x000055ea5c355f80 in aio_dispatch (ctx=0x55ea5e555970)
>     at util/aio-posix.c:429
> #13 0x000055ea5c352ebe in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at util/async.c:261
> #14 0x00007f1d55489d7a in g_main_context_dispatch (context=0x55ea5e555d40)
>     at gmain.c:3152
> #15 0x00007f1d55489d7a in g_main_context_dispatch (context=context@entry=0x55ea5e555d40) at gmain.c:3767
> #16 0x000055ea5c35523c in main_loop_wait () at util/main-loop.c:213
> #17 0x000055ea5c35523c in main_loop_wait (timeout=<optimized out>)
>     at util/main-loop.c:261
> #18 0x000055ea5c35523c in main_loop_wait (nonblocking=nonblocking@entry=0)
>     at util/main-loop.c:517
> #19 0x000055ea5bfb44a7 in main () at vl.c:1899
> #20 0x000055ea5bfb44a7 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4717
> 
> 

Please test this:

diff --git a/block/curl.c b/block/curl.c
index 2708d57c2f..cfe02bfd79 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -467,7 +467,9 @@ static CURLState *curl_init_state(BlockDriverState
*bs, BDRVCURLState *s)
             break;
         }
         if (!state) {
+            qemu_mutex_unlock(&s->mutex);
             aio_poll(bdrv_get_aio_context(bs), true);
+            qemu_mutex_lock(&s->mutex);
         }
     } while(!state);


Thanks,

Paolo

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

[Richard W.M. Jones]

On Wed, May 03, 2017 at 05:46:49PM +0200, Paolo Bonzini wrote:
> Please test this:
> 
> diff --git a/block/curl.c b/block/curl.c
> index 2708d57c2f..cfe02bfd79 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -467,7 +467,9 @@ static CURLState *curl_init_state(BlockDriverState
> *bs, BDRVCURLState *s)
>              break;
>          }
>          if (!state) {
> +            qemu_mutex_unlock(&s->mutex);
>              aio_poll(bdrv_get_aio_context(bs), true);
> +            qemu_mutex_lock(&s->mutex);
>          }
>      } while(!state);
> 
> 

Yes, this fixes it, thanks Paolo.

Reported-by: Kun Wei <kuwei@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

[Paolo Bonzini]

On 03/05/2017 17:55, Richard W.M. Jones wrote:
> On Wed, May 03, 2017 at 05:46:49PM +0200, Paolo Bonzini wrote:
>> Please test this:
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 2708d57c2f..cfe02bfd79 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -467,7 +467,9 @@ static CURLState *curl_init_state(BlockDriverState
>> *bs, BDRVCURLState *s)
>>              break;
>>          }
>>          if (!state) {
>> +            qemu_mutex_unlock(&s->mutex);
>>              aio_poll(bdrv_get_aio_context(bs), true);
>> +            qemu_mutex_lock(&s->mutex);
>>          }
>>      } while(!state);
>>
>>
> 
> Yes, this fixes it, thanks Paolo.
> 
> Reported-by: Kun Wei <kuwei@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>

Ok, will submit tomorrow formally.

[Qemu-devel] [PULL 2/4] nfs: do not use aio_context_acquire/release

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

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 libnfs calls with a QemuMutex.  Callbacks are invoked
using bottom halves, so we don't even have to drop it around
callback invocations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20170222180725.28611-3-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/nfs.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index ffb54be..890d5d4 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -54,6 +54,7 @@ typedef struct NFSClient {
     int events;
     bool has_zero_init;
     AioContext *aio_context;
+    QemuMutex mutex;
     blkcnt_t st_blocks;
     bool cache_used;
     NFSServer *server;
@@ -191,6 +192,7 @@ static void nfs_parse_filename(const char *filename, QDict *options,
 static void nfs_process_read(void *arg);
 static void nfs_process_write(void *arg);
 
+/* Called with QemuMutex held.  */
 static void nfs_set_events(NFSClient *client)
 {
     int ev = nfs_which_events(client->context);
@@ -209,20 +211,20 @@ static void nfs_process_read(void *arg)
 {
     NFSClient *client = arg;
 
-    aio_context_acquire(client->aio_context);
+    qemu_mutex_lock(&client->mutex);
     nfs_service(client->context, POLLIN);
     nfs_set_events(client);
-    aio_context_release(client->aio_context);
+    qemu_mutex_unlock(&client->mutex);
 }
 
 static void nfs_process_write(void *arg)
 {
     NFSClient *client = arg;
 
-    aio_context_acquire(client->aio_context);
+    qemu_mutex_lock(&client->mutex);
     nfs_service(client->context, POLLOUT);
     nfs_set_events(client);
-    aio_context_release(client->aio_context);
+    qemu_mutex_unlock(&client->mutex);
 }
 
 static void nfs_co_init_task(BlockDriverState *bs, NFSRPC *task)
@@ -242,6 +244,7 @@ static void nfs_co_generic_bh_cb(void *opaque)
     aio_co_wake(task->co);
 }
 
+/* Called (via nfs_service) with QemuMutex held.  */
 static void
 nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
                   void *private_data)
@@ -273,12 +276,15 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, uint64_t offset,
     nfs_co_init_task(bs, &task);
     task.iov = iov;
 
+    qemu_mutex_lock(&client->mutex);
     if (nfs_pread_async(client->context, client->fh,
                         offset, bytes, nfs_co_generic_cb, &task) != 0) {
+        qemu_mutex_unlock(&client->mutex);
         return -ENOMEM;
     }
 
     nfs_set_events(client);
+    qemu_mutex_unlock(&client->mutex);
     while (!task.complete) {
         qemu_coroutine_yield();
     }
@@ -317,9 +323,11 @@ static int coroutine_fn nfs_co_pwritev(BlockDriverState *bs, uint64_t offset,
         buf = iov->iov[0].iov_base;
     }
 
+    qemu_mutex_lock(&client->mutex);
     if (nfs_pwrite_async(client->context, client->fh,
                          offset, bytes, buf,
                          nfs_co_generic_cb, &task) != 0) {
+        qemu_mutex_unlock(&client->mutex);
         if (my_buffer) {
             g_free(buf);
         }
@@ -327,6 +335,7 @@ static int coroutine_fn nfs_co_pwritev(BlockDriverState *bs, uint64_t offset,
     }
 
     nfs_set_events(client);
+    qemu_mutex_unlock(&client->mutex);
     while (!task.complete) {
         qemu_coroutine_yield();
     }
@@ -349,12 +358,15 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
 
     nfs_co_init_task(bs, &task);
 
+    qemu_mutex_lock(&client->mutex);
     if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
                         &task) != 0) {
+        qemu_mutex_unlock(&client->mutex);
         return -ENOMEM;
     }
 
     nfs_set_events(client);
+    qemu_mutex_unlock(&client->mutex);
     while (!task.complete) {
         qemu_coroutine_yield();
     }
@@ -440,6 +452,7 @@ static void nfs_file_close(BlockDriverState *bs)
 {
     NFSClient *client = bs->opaque;
     nfs_client_close(client);
+    qemu_mutex_destroy(&client->mutex);
 }
 
 static NFSServer *nfs_config(QDict *options, Error **errp)
@@ -647,6 +660,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
     if (ret < 0) {
         return ret;
     }
+    qemu_mutex_init(&client->mutex);
     bs->total_sectors = ret;
     ret = 0;
     return ret;
@@ -702,6 +716,7 @@ static int nfs_has_zero_init(BlockDriverState *bs)
     return client->has_zero_init;
 }
 
+/* Called (via nfs_service) with QemuMutex held.  */
 static void
 nfs_get_allocated_file_size_cb(int ret, struct nfs_context *nfs, void *data,
                                void *private_data)
-- 
2.9.3

[Qemu-devel] [PULL 3/4] iscsi: do not use aio_context_acquire/release

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

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 libiscsi calls with a QemuMutex.  Callbacks are invoked
using bottom halves, so we don't even have to drop it around
callback invocations.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20170222180725.28611-4-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 19 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index c4f813b..76319a1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -58,6 +58,7 @@ typedef struct IscsiLun {
     int events;
     QEMUTimer *nop_timer;
     QEMUTimer *event_timer;
+    QemuMutex mutex;
     struct scsi_inquiry_logical_block_provisioning lbp;
     struct scsi_inquiry_block_limits bl;
     unsigned char *zeroblock;
@@ -252,6 +253,7 @@ static int iscsi_translate_sense(struct scsi_sense *sense)
     return ret;
 }
 
+/* Called (via iscsi_service) with QemuMutex held.  */
 static void
 iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                         void *command_data, void *opaque)
@@ -352,6 +354,7 @@ static const AIOCBInfo iscsi_aiocb_info = {
 static void iscsi_process_read(void *arg);
 static void iscsi_process_write(void *arg);
 
+/* Called with QemuMutex held.  */
 static void
 iscsi_set_events(IscsiLun *iscsilun)
 {
@@ -395,10 +398,10 @@ iscsi_process_read(void *arg)
     IscsiLun *iscsilun = arg;
     struct iscsi_context *iscsi = iscsilun->iscsi;
 
-    aio_context_acquire(iscsilun->aio_context);
+    qemu_mutex_lock(&iscsilun->mutex);
     iscsi_service(iscsi, POLLIN);
     iscsi_set_events(iscsilun);
-    aio_context_release(iscsilun->aio_context);
+    qemu_mutex_unlock(&iscsilun->mutex);
 }
 
 static void
@@ -407,10 +410,10 @@ iscsi_process_write(void *arg)
     IscsiLun *iscsilun = arg;
     struct iscsi_context *iscsi = iscsilun->iscsi;
 
-    aio_context_acquire(iscsilun->aio_context);
+    qemu_mutex_lock(&iscsilun->mutex);
     iscsi_service(iscsi, POLLOUT);
     iscsi_set_events(iscsilun);
-    aio_context_release(iscsilun->aio_context);
+    qemu_mutex_unlock(&iscsilun->mutex);
 }
 
 static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)
@@ -589,6 +592,7 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     uint64_t lba;
     uint32_t num_sectors;
     bool fua = flags & BDRV_REQ_FUA;
+    int r = 0;
 
     if (fua) {
         assert(iscsilun->dpofua);
@@ -604,6 +608,7 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     lba = sector_qemu2lun(sector_num, iscsilun);
     num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
     iscsi_co_init_iscsitask(iscsilun, &iTask);
+    qemu_mutex_lock(&iscsilun->mutex);
 retry:
     if (iscsilun->use_16_for_rw) {
 #if LIBISCSI_API_VERSION >= (20160603)
@@ -640,7 +645,9 @@ retry:
 #endif
     while (!iTask.complete) {
         iscsi_set_events(iscsilun);
+        qemu_mutex_unlock(&iscsilun->mutex);
         qemu_coroutine_yield();
+        qemu_mutex_lock(&iscsilun->mutex);
     }
 
     if (iTask.task != NULL) {
@@ -655,12 +662,15 @@ retry:
 
     if (iTask.status != SCSI_STATUS_GOOD) {
         iscsi_allocmap_set_invalid(iscsilun, sector_num, nb_sectors);
-        return iTask.err_code;
+        r = iTask.err_code;
+        goto out_unlock;
     }
 
     iscsi_allocmap_set_allocated(iscsilun, sector_num, nb_sectors);
 
-    return 0;
+out_unlock:
+    qemu_mutex_unlock(&iscsilun->mutex);
+    return r;
 }
 
 
@@ -693,18 +703,21 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
         goto out;
     }
 
+    qemu_mutex_lock(&iscsilun->mutex);
 retry:
     if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
                                   sector_qemu2lun(sector_num, iscsilun),
                                   8 + 16, iscsi_co_generic_cb,
                                   &iTask) == NULL) {
         ret = -ENOMEM;
-        goto out;
+        goto out_unlock;
     }
 
     while (!iTask.complete) {
         iscsi_set_events(iscsilun);
+        qemu_mutex_unlock(&iscsilun->mutex);
         qemu_coroutine_yield();
+        qemu_mutex_lock(&iscsilun->mutex);
     }
 
     if (iTask.do_retry) {
@@ -721,20 +734,20 @@ retry:
          * because the device is busy or the cmd is not
          * supported) we pretend all blocks are allocated
          * for backwards compatibility */
-        goto out;
+        goto out_unlock;
     }
 
     lbas = scsi_datain_unmarshall(iTask.task);
     if (lbas == NULL) {
         ret = -EIO;
-        goto out;
+        goto out_unlock;
     }
 
     lbasd = &lbas->descriptors[0];
 
     if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
         ret = -EIO;
-        goto out;
+        goto out_unlock;
     }
 
     *pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun);
@@ -756,6 +769,8 @@ retry:
     if (*pnum > nb_sectors) {
         *pnum = nb_sectors;
     }
+out_unlock:
+    qemu_mutex_unlock(&iscsilun->mutex);
 out:
     if (iTask.task != NULL) {
         scsi_free_scsi_task(iTask.task);
@@ -818,6 +833,7 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
     num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
 
     iscsi_co_init_iscsitask(iscsilun, &iTask);
+    qemu_mutex_lock(&iscsilun->mutex);
 retry:
     if (iscsilun->use_16_for_rw) {
 #if LIBISCSI_API_VERSION >= (20160603)
@@ -855,7 +871,9 @@ retry:
 #endif
     while (!iTask.complete) {
         iscsi_set_events(iscsilun);
+        qemu_mutex_unlock(&iscsilun->mutex);
         qemu_coroutine_yield();
+        qemu_mutex_lock(&iscsilun->mutex);
     }
 
     if (iTask.task != NULL) {
@@ -867,6 +885,7 @@ retry:
         iTask.complete = 0;
         goto retry;
     }
+    qemu_mutex_unlock(&iscsilun->mutex);
 
     if (iTask.status != SCSI_STATUS_GOOD) {
         return iTask.err_code;
@@ -881,6 +900,7 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
     struct IscsiTask iTask;
 
     iscsi_co_init_iscsitask(iscsilun, &iTask);
+    qemu_mutex_lock(&iscsilun->mutex);
 retry:
     if (iscsi_synchronizecache10_task(iscsilun->iscsi, iscsilun->lun, 0, 0, 0,
                                       0, iscsi_co_generic_cb, &iTask) == NULL) {
@@ -889,7 +909,9 @@ retry:
 
     while (!iTask.complete) {
         iscsi_set_events(iscsilun);
+        qemu_mutex_unlock(&iscsilun->mutex);
         qemu_coroutine_yield();
+        qemu_mutex_lock(&iscsilun->mutex);
     }
 
     if (iTask.task != NULL) {
@@ -901,6 +923,7 @@ retry:
         iTask.complete = 0;
         goto retry;
     }
+    qemu_mutex_unlock(&iscsilun->mutex);
 
     if (iTask.status != SCSI_STATUS_GOOD) {
         return iTask.err_code;
@@ -910,6 +933,7 @@ retry:
 }
 
 #ifdef __linux__
+/* Called (via iscsi_service) with QemuMutex held.  */
 static void
 iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
                      void *command_data, void *opaque)
@@ -1034,6 +1058,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     acb->task->expxferlen = acb->ioh->dxfer_len;
 
     data.size = 0;
+    qemu_mutex_lock(&iscsilun->mutex);
     if (acb->task->xfer_dir == SCSI_XFER_WRITE) {
         if (acb->ioh->iovec_count == 0) {
             data.data = acb->ioh->dxferp;
@@ -1049,6 +1074,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
                                  iscsi_aio_ioctl_cb,
                                  (data.size > 0) ? &data : NULL,
                                  acb) != 0) {
+        qemu_mutex_unlock(&iscsilun->mutex);
         scsi_free_scsi_task(acb->task);
         qemu_aio_unref(acb);
         return NULL;
@@ -1068,6 +1094,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     }
 
     iscsi_set_events(iscsilun);
+    qemu_mutex_unlock(&iscsilun->mutex);
 
     return &acb->common;
 }
@@ -1092,6 +1119,7 @@ coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
     struct unmap_list list;
+    int r = 0;
 
     if (!is_byte_request_lun_aligned(offset, count, iscsilun)) {
         return -ENOTSUP;
@@ -1106,15 +1134,19 @@ coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
     list.num = count / iscsilun->block_size;
 
     iscsi_co_init_iscsitask(iscsilun, &iTask);
+    qemu_mutex_lock(&iscsilun->mutex);
 retry:
     if (iscsi_unmap_task(iscsilun->iscsi, iscsilun->lun, 0, 0, &list, 1,
                          iscsi_co_generic_cb, &iTask) == NULL) {
-        return -ENOMEM;
+        r = -ENOMEM;
+        goto out_unlock;
     }
 
     while (!iTask.complete) {
         iscsi_set_events(iscsilun);
+        qemu_mutex_unlock(&iscsilun->mutex);
         qemu_coroutine_yield();
+        qemu_mutex_lock(&iscsilun->mutex);
     }
 
     if (iTask.task != NULL) {
@@ -1131,17 +1163,20 @@ retry:
         /* the target might fail with a check condition if it
            is not happy with the alignment of the UNMAP request
            we silently fail in this case */
-        return 0;
+        goto out_unlock;
     }
 
     if (iTask.status != SCSI_STATUS_GOOD) {
-        return iTask.err_code;
+        r = iTask.err_code;
+        goto out_unlock;
     }
 
     iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
                                count >> BDRV_SECTOR_BITS);
 
-    return 0;
+out_unlock:
+    qemu_mutex_unlock(&iscsilun->mutex);
+    return r;
 }
 
 static int
@@ -1153,6 +1188,7 @@ coroutine_fn iscsi_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
     uint64_t lba;
     uint32_t nb_blocks;
     bool use_16_for_ws = iscsilun->use_16_for_rw;
+    int r = 0;
 
     if (!is_byte_request_lun_aligned(offset, count, iscsilun)) {
         return -ENOTSUP;
@@ -1186,6 +1222,7 @@ coroutine_fn iscsi_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
         }
     }
 
+    qemu_mutex_lock(&iscsilun->mutex);
     iscsi_co_init_iscsitask(iscsilun, &iTask);
 retry:
     if (use_16_for_ws) {
@@ -1205,7 +1242,9 @@ retry:
 
     while (!iTask.complete) {
         iscsi_set_events(iscsilun);
+        qemu_mutex_unlock(&iscsilun->mutex);
         qemu_coroutine_yield();
+        qemu_mutex_lock(&iscsilun->mutex);
     }
 
     if (iTask.status == SCSI_STATUS_CHECK_CONDITION &&
@@ -1215,7 +1254,8 @@ retry:
         /* WRITE SAME is not supported by the target */
         iscsilun->has_write_same = false;
         scsi_free_scsi_task(iTask.task);
-        return -ENOTSUP;
+        r = -ENOTSUP;
+        goto out_unlock;
     }
 
     if (iTask.task != NULL) {
@@ -1231,7 +1271,8 @@ retry:
     if (iTask.status != SCSI_STATUS_GOOD) {
         iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
                                    count >> BDRV_SECTOR_BITS);
-        return iTask.err_code;
+        r = iTask.err_code;
+        goto out_unlock;
     }
 
     if (flags & BDRV_REQ_MAY_UNMAP) {
@@ -1242,7 +1283,9 @@ retry:
                                      count >> BDRV_SECTOR_BITS);
     }
 
-    return 0;
+out_unlock:
+    qemu_mutex_unlock(&iscsilun->mutex);
+    return r;
 }
 
 static void apply_chap(struct iscsi_context *iscsi, QemuOpts *opts,
@@ -1331,7 +1374,7 @@ static void iscsi_nop_timed_event(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
 
-    aio_context_acquire(iscsilun->aio_context);
+    qemu_mutex_lock(&iscsilun->mutex);
     if (iscsi_get_nops_in_flight(iscsilun->iscsi) >= MAX_NOP_FAILURES) {
         error_report("iSCSI: NOP timeout. Reconnecting...");
         iscsilun->request_timed_out = true;
@@ -1344,7 +1387,7 @@ static void iscsi_nop_timed_event(void *opaque)
     iscsi_set_events(iscsilun);
 
 out:
-    aio_context_release(iscsilun->aio_context);
+    qemu_mutex_unlock(&iscsilun->mutex);
 }
 
 static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
@@ -1890,6 +1933,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     scsi_free_scsi_task(task);
     task = NULL;
 
+    qemu_mutex_init(&iscsilun->mutex);
     iscsi_attach_aio_context(bs, iscsilun->aio_context);
 
     /* Guess the internal cluster (page) size of the iscsi target by the means
@@ -1935,6 +1979,7 @@ static void iscsi_close(BlockDriverState *bs)
     iscsi_destroy_context(iscsi);
     g_free(iscsilun->zeroblock);
     iscsi_allocmap_free(iscsilun);
+    qemu_mutex_destroy(&iscsilun->mutex);
     memset(iscsilun, 0, sizeof(IscsiLun));
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 4/4] tests-aio-multithread: use atomic_read properly

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

nodes[id].next is written by other threads.  If atomic_read is not used
(matching atomic_set in mcs_mutex_lock!) the compiler can optimize the
whole "if" away!

Reported-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Greg Kurz <groug@kaod.org>
Message-id: 20170227111726.9237-1-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/test-aio-multithread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
index f11e990..8b0b40e 100644
--- a/tests/test-aio-multithread.c
+++ b/tests/test-aio-multithread.c
@@ -309,7 +309,7 @@ static void mcs_mutex_lock(void)
 static void mcs_mutex_unlock(void)
 {
     int next;
-    if (nodes[id].next == -1) {
+    if (atomic_read(&nodes[id].next) == -1) {
         if (atomic_read(&mutex_head) == id &&
             atomic_cmpxchg(&mutex_head, id, -1) == id) {
             /* Last item in the list, exit.  */
@@ -323,7 +323,7 @@ static void mcs_mutex_unlock(void)
     }
 
     /* Wake up the next in line.  */
-    next = nodes[id].next;
+    next = atomic_read(&nodes[id].next);
     nodes[next].locked = 0;
     qemu_futex_wake(&nodes[next].locked, 1);
 }
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/4] Block patches

[Peter Maydell]

On 27 February 2017 at 16:34, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit d992f2f1368ceb92e6bfd8efece174110f4236ff:
>
>   Merge remote-tracking branch 'remotes/artyom/tags/pull-sun4v-20170226' into staging (2017-02-26 22:40:23 +0000)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 1ab17f9f5c63c2798d707aeb22588e4fcc17b2cd:
>
>   tests-aio-multithread: use atomic_read properly (2017-02-27 14:00:53 +0000)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> Paolo Bonzini (4):
>   curl: do not use aio_context_acquire/release
>   nfs: do not use aio_context_acquire/release
>   iscsi: do not use aio_context_acquire/release
>   tests-aio-multithread: use atomic_read properly

Applied, thanks.

-- PMM