Series comparison

-[Qemu-devel] [PULL 0/1] Block patches
+[Qemu-devel] [PULL for-2.11-rc1 v2 0/2] Block patches
-The following changes since commit 64175afc695c0672876fbbfc31b299c86d562cb4:
+The following changes since commit b0fbe46ad82982b289a44ee2495b59b0bad8a842:
-  arm_gicv3: Fix ICC_BPR1 reset value when EL3 not implemented (2017-06-07 17:21:44 +0100)
+  Update version for v2.11.0-rc0 release (2017-11-07 16:05:28 +0000)
 are available in the git repository at:
-  git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
+  git://github.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to 56faeb9bb6872b3f926b3b3e0452a70beea10af2:
+for you to fetch changes up to ef6dada8b44e1e7c4bec5c1115903af9af415b50:
-  block/gluster.c: Handle qdict_array_entries() failure (2017-06-09 08:41:29 -0400)
+  util/async: use atomic_mb_set in qemu_bh_cancel (2017-11-08 19:09:15 +0000)
 ----------------------------------------------------------------
-Gluster patch
+Pull request
 v2:
  * v1 emails 2/3 and 3/3 weren't sent due to an email failure
  * Included Sergio's updated wording in the commit description
 ----------------------------------------------------------------
-Peter Maydell (1):
+Sergio Lopez (1):
-  block/gluster.c: Handle qdict_array_entries() failure
+  util/async: use atomic_mb_set in qemu_bh_cancel
- block/gluster.c | 3 +--
+Stefan Hajnoczi (1):
-file changed, 1 insertion(+), 2 deletions(-)
+  tests-aio-multithread: fix /aio/multi/schedule race condition
  tests/test-aio-multithread.c | 5 ++---
  util/async.c                 | 2 +-
 files changed, 3 insertions(+), 4 deletions(-)
 --
-.9.3
+.13.6

-New patch
+[Qemu-devel] [PULL for-2.11-rc1 v2 1/2] tests-aio-multithread: fix /aio/multi/schedule race condition
+test_multi_co_schedule_entry() set to_schedule[id] in the final loop
+iteration before terminating the coroutine.  There is a race condition
+where the main thread attempts to enter the terminating or terminated
+coroutine when signalling coroutines to stop:
+  atomic_mb_set(&now_stopping, true);
+  for (i = 0; i < NUM_CONTEXTS; i++) {
+      ctx_run(i, finish_cb, NULL);  <--- enters dead coroutine!
+      to_schedule[i] = NULL;
+  }
+Make sure only to set to_schedule[id] if this coroutine really needs to
+be scheduled!
+Reported-by: "R.Nageswara Sastry" <nasastry@in.ibm.com>
+Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
+Message-id: 20171106190233.1175-1-stefanha@redhat.com
+Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+---
+ tests/test-aio-multithread.c | 5 ++---
+file changed, 2 insertions(+), 3 deletions(-)
+diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
+index XXXXXXX..XXXXXXX 100644
+--- a/tests/test-aio-multithread.c
++++ b/tests/test-aio-multithread.c
+@@ -XXX,XX +XXX,XX @@ static void finish_cb(void *opaque)
+ static coroutine_fn void test_multi_co_schedule_entry(void *opaque)
+ {
+     g_assert(to_schedule[id] == NULL);
+-    atomic_mb_set(&to_schedule[id], qemu_coroutine_self());
+     while (!atomic_mb_read(&now_stopping)) {
+         int n;
+         n = g_test_rand_int_range(0, NUM_CONTEXTS);
+         schedule_next(n);
++
++        atomic_mb_set(&to_schedule[id], qemu_coroutine_self());
+         qemu_coroutine_yield();
+-
+         g_assert(to_schedule[id] == NULL);
+-        atomic_mb_set(&to_schedule[id], qemu_coroutine_self());
+     }
+ }
+--
+.13.6

-[Qemu-devel] [PULL 1/1] block/gluster.c: Handle qdict_array_entries() failure
+[Qemu-devel] [PULL for-2.11-rc1 v2 2/2] util/async: use atomic_mb_set in qemu_bh_cancel
-From: Peter Maydell <peter.maydell@linaro.org>
+From: Sergio Lopez <slp@redhat.com>
-In qemu_gluster_parse_json(), the call to qdict_array_entries()
+Commit b7a745d added a qemu_bh_cancel call to the completion function
-could return a negative error code, which we were ignoring
+as an optimization to prevent it from unnecessarily rescheduling itself.
 because we assigned the result to an unsigned variable.
 Fix this by using the 'int' type instead, which matches the
 return type of qdict_array_entries() and also the type
 we use for the loop enumeration variable 'i'.
-(Spotted by Coverity, CID 1360960.)
+This completion function is scheduled from worker_thread, after setting
 the state of a ThreadPoolElement to THREAD_DONE.
-Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
+This was considered to be safe, as the completion function restarts the
-Reviewed-by: Eric Blake <eblake@redhat.com>
+loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW
-Reviewed-by: Jeff Cody <jcody@redhat.com>
+memory barrier, the read of req->state may actually happen _before_ the
-Message-id: 1496682098-1540-1-git-send-email-peter.maydell@linaro.org
+call, seeing it still as THREAD_QUEUED, and ending the completion
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+function without having processed a pending TPE linked at pool->head:
          worker thread             |            I/O thread
 ------------------------------------------------------------------------
                                    | speculatively read req->state
 req->state = THREAD_DONE;          |
 qemu_bh_schedule(p->completion_bh) |
   bh->scheduled = 1;               |
                                    | qemu_bh_cancel(p->completion_bh)
                                    |   bh->scheduled = 0;
                                    | if (req->state == THREAD_DONE)
                                    |   // sees THREAD_QUEUED
 The source of the misunderstanding was that qemu_bh_cancel is now being
 used by the _consumer_ rather than the producer, and therefore now needs
 to have acquire semantics just like e.g. aio_bh_poll.
 In some situations, if there are no other independent requests in the
 same aio context that could eventually trigger the scheduling of the
 completion function, the omitted TPE and all operations pending on it
 will get stuck forever.
 [Added Sergio's updated wording about the HW memory barrier.
 --Stefan]
 Signed-off-by: Sergio Lopez <slp@redhat.com>
 Message-id: 20171108063447.2842-1-slp@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- block/gluster.c | 3 +--
+ util/async.c | 2 +-
-file changed, 1 insertion(+), 2 deletions(-)
+file changed, 1 insertion(+), 1 deletion(-)
-diff --git a/block/gluster.c b/block/gluster.c
+diff --git a/util/async.c b/util/async.c
 index XXXXXXX..XXXXXXX 100644
---- a/block/gluster.c
+--- a/util/async.c
-+++ b/block/gluster.c
++++ b/util/async.c
-@@ -XXX,XX +XXX,XX @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
+@@ -XXX,XX +XXX,XX @@ void qemu_bh_schedule(QEMUBH *bh)
-     Error *local_err = NULL;
+  */
-     char *str = NULL;
+ void qemu_bh_cancel(QEMUBH *bh)
-     const char *ptr;
+ {
--    size_t num_servers;
+-    bh->scheduled = 0;
--    int i, type;
++    atomic_mb_set(&bh->scheduled, 0);
-+    int i, type, num_servers;
+ }
-     /* create opts info from runtime_json_opts list */
+ /* This func is async.The bottom half will do the delete action at the finial
      opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
 --
-.9.3
+.13.6

From: Peter Maydell <peter.maydell@linaro.org>

In qemu_gluster_parse_json(), the call to qdict_array_entries()
could return a negative error code, which we were ignoring
because we assigned the result to an unsigned variable.
Fix this by using the 'int' type instead, which matches the
return type of qdict_array_entries() and also the type
we use for the loop enumeration variable 'i'.

(Spotted by Coverity, CID 1360960.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 1496682098-1540-1-git-send-email-peter.maydell@linaro.org
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/gluster.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index XXXXXXX..XXXXXXX 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -XXX,XX +XXX,XX @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     Error *local_err = NULL;
     char *str = NULL;
     const char *ptr;
-    size_t num_servers;
-    int i, type;
+    int i, type, num_servers;
 
     /* create opts info from runtime_json_opts list */
     opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
-- 
2.9.3

test_multi_co_schedule_entry() set to_schedule[id] in the final loop
iteration before terminating the coroutine.  There is a race condition
where the main thread attempts to enter the terminating or terminated
coroutine when signalling coroutines to stop:

atomic_mb_set(&now_stopping, true);
  for (i = 0; i < NUM_CONTEXTS; i++) {
      ctx_run(i, finish_cb, NULL);  <--- enters dead coroutine!
      to_schedule[i] = NULL;
  }

Make sure only to set to_schedule[id] if this coroutine really needs to
be scheduled!

Reported-by: "R.Nageswara Sastry" <nasastry@in.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20171106190233.1175-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/test-aio-multithread.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-aio-multithread.c
+++ b/tests/test-aio-multithread.c
@@ -XXX,XX +XXX,XX @@ static void finish_cb(void *opaque)
 static coroutine_fn void test_multi_co_schedule_entry(void *opaque)
 {
     g_assert(to_schedule[id] == NULL);
-    atomic_mb_set(&to_schedule[id], qemu_coroutine_self());
 
     while (!atomic_mb_read(&now_stopping)) {
         int n;
 
         n = g_test_rand_int_range(0, NUM_CONTEXTS);
         schedule_next(n);
+
+        atomic_mb_set(&to_schedule[id], qemu_coroutine_self());
         qemu_coroutine_yield();
-
         g_assert(to_schedule[id] == NULL);
-        atomic_mb_set(&to_schedule[id], qemu_coroutine_self());
     }
 }
 
-- 
2.13.6

From: Sergio Lopez <slp@redhat.com>

Commit b7a745d added a qemu_bh_cancel call to the completion function
as an optimization to prevent it from unnecessarily rescheduling itself.

This completion function is scheduled from worker_thread, after setting
the state of a ThreadPoolElement to THREAD_DONE.

This was considered to be safe, as the completion function restarts the
loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW
memory barrier, the read of req->state may actually happen _before_ the
call, seeing it still as THREAD_QUEUED, and ending the completion
function without having processed a pending TPE linked at pool->head:

The source of the misunderstanding was that qemu_bh_cancel is now being
used by the _consumer_ rather than the producer, and therefore now needs
to have acquire semantics just like e.g. aio_bh_poll.

In some situations, if there are no other independent requests in the
same aio context that could eventually trigger the scheduling of the
completion function, the omitted TPE and all operations pending on it
will get stuck forever.

[Added Sergio's updated wording about the HW memory barrier.
--Stefan]

Signed-off-by: Sergio Lopez <slp@redhat.com>
Message-id: 20171108063447.2842-1-slp@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/async.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/async.c b/util/async.c
index XXXXXXX..XXXXXXX 100644
--- a/util/async.c
+++ b/util/async.c
@@ -XXX,XX +XXX,XX @@ void qemu_bh_schedule(QEMUBH *bh)
  */
 void qemu_bh_cancel(QEMUBH *bh)
 {
-    bh->scheduled = 0;
+    atomic_mb_set(&bh->scheduled, 0);
 }
 
 /* This func is async.The bottom half will do the delete action at the finial
-- 
2.13.6