[Qemu-devel] [PATCH 0/4] block: Keep track of parent quiescing

[Qemu-devel] [PATCH 0/4] block: Keep track of parent quiescing

[Max Reitz]

We currently do not keep track of how many times a child has quiesced
its parent.  We just guess based on the child’s quiesce_counter.  That
keeps biting us when we try to leave drained sections or detach children
(see e.g. commit 5cb2737e925042e).

I think we need an explicit counter to keep track of how many times a
parent has been quiesced (patch 1).  That just makes the code more
resilient.

Actually, no, we don’t need a counter, we need a boolean.  See patch 2
for the explanation.

Yes, it is a bit weird to introduce a counter first (patch 1) and then
immediately make it a boolean (patch 2).  But I believe this to be the
most logical change set.

(“Our current model relies on counting, so adding an explicit counter
makes sense.  It then turns out that counting is probably not the best
idea, so why not make it a boolean?”)


Max Reitz (4):
  block: Introduce BdrvChild.parent_quiesce_counter
  block: Make @parent_quiesced a bool
  iotests: Add @has_quit to vm.shutdown()
  iotests: Test commit with a filter on the chain

 include/block/block.h      |  9 +++---
 include/block/block_int.h  |  6 ++++
 block.c                    | 18 ++++-------
 block/io.c                 | 61 +++++++++++++++++++++++++++++---------
 tests/test-bdrv-drain.c    | 31 +++++++++++--------
 python/qemu/__init__.py    |  5 ++--
 tests/qemu-iotests/040     | 40 ++++++++++++++++++++++++-
 tests/qemu-iotests/040.out |  4 +--
 tests/qemu-iotests/255     |  2 +-
 9 files changed, 126 insertions(+), 50 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/4] block: Introduce BdrvChild.parent_quiesce_counter

[Max Reitz]

Commit 5cb2737e925042e6c7cd3fb0b01313950b03cddf laid out why
bdrv_do_drained_end() must decrement the quiesce_counter after
bdrv_drain_invoke().  It did not give a very good reason why it has to
happen after bdrv_parent_drained_end(), instead only claiming symmetry
to bdrv_do_drained_begin().

It turns out that delaying it for so long is wrong.

Situation: We have an active commit job (i.e. a mirror job) from top to
base for the following graph:

                  filter
                    |
                  [file]
                    |
                    v
top --[backing]--> base

Now the VM is closed, which results in the job being cancelled and a
bdrv_drain_all() happening pretty much simultaneously.

Beginning the drain means the job is paused once whenever one of its
nodes is quiesced.  This is reversed when the drain ends.

With how the code currently is, after base's drain ends (which means
that it will have unpaused the job once), its quiesce_counter remains at
1 while it goes to undrain its parents (bdrv_parent_drained_end()).  For
some reason or another, undraining filter causes the job to be kicked
and enter mirror_exit_common(), where it proceeds to invoke
block_job_remove_all_bdrv().

Now base will be detached from the job.  Because its quiesce_counter is
still 1, it will unpause the job once more.  So in total, undraining
base will unpause the job twice.  Eventually, this will lead to the
job's pause_count going negative -- well, it would, were there not an
assertion against this, which crashes qemu.

The general problem is that if in bdrv_parent_drained_end() we undrain
parent A, and then undrain parent B, which then leads to A detaching the
child, bdrv_replace_child_noperm() will undrain A as if we had not done
so yet; that is, one time too many.

It follows that we cannot decrement the quiesce_counter after invoking
bdrv_parent_drained_end().

Unfortunately, decrementing it before bdrv_parent_drained_end() would be
wrong, too.  Imagine the above situation in reverse: Undraining A leads
to B detaching the child.  If we had already decremented the
quiesce_counter by that point, bdrv_replace_child_noperm() would undrain
B one time too little; because it expects bdrv_parent_drained_end() to
issue this undrain.  But bdrv_parent_drained_end() won't do that,
because B is no longer a parent.

Therefore, we have to do something else.  This patch opts for
introducing a second quiesce_counter that counts how many times a
child's parent has been quiesced (though c->role->drained_*).  With
that, bdrv_replace_child_noperm() just has to undrain the parent exactly
that many times when removing a child, and it will always be right.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h     |  7 +++++++
 include/block/block_int.h |  9 +++++++++
 block.c                   | 15 +++++----------
 block/io.c                | 14 +++++++++++---
 4 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index f9415ed740..3c084e8222 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -616,6 +616,13 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
  */
 void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll);
 
+/**
+ * bdrv_parent_drained_end_single:
+ *
+ * End a quiesced section for the parent of @c.
+ */
+void bdrv_parent_drained_end_single(BdrvChild *c);
+
 /**
  * bdrv_parent_drained_end:
  *
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06df2bda1b..84c0369fb7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -729,6 +729,15 @@ struct BdrvChild {
      */
     bool frozen;
 
+    /*
+     * How many times the parent of this child has been drained
+     * (through role->drained_*).
+     * Usually, this is equal to bs->quiesce_counter (potentially
+     * reduced by bdrv_drain_all_count).  It may differ while the
+     * child is entering or leaving a drained section.
+     */
+    int parent_quiesce_counter;
+
     QLIST_ENTRY(BdrvChild) next;
     QLIST_ENTRY(BdrvChild) next_parent;
 };
diff --git a/block.c b/block.c
index e3e77feee0..b336a91415 100644
--- a/block.c
+++ b/block.c
@@ -2173,24 +2173,19 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
         if (child->role->detach) {
             child->role->detach(child);
         }
-        if (old_bs->quiesce_counter && child->role->drained_end) {
-            int num = old_bs->quiesce_counter;
-            if (child->role->parent_is_bds) {
-                num -= bdrv_drain_all_count;
-            }
-            assert(num >= 0);
-            for (i = 0; i < num; i++) {
-                child->role->drained_end(child);
-            }
+        while (child->parent_quiesce_counter) {
+            bdrv_parent_drained_end_single(child);
         }
         QLIST_REMOVE(child, next_parent);
+    } else {
+        assert(child->parent_quiesce_counter == 0);
     }
 
     child->bs = new_bs;
 
     if (new_bs) {
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
-        if (new_bs->quiesce_counter && child->role->drained_begin) {
+        if (new_bs->quiesce_counter) {
             int num = new_bs->quiesce_counter;
             if (child->role->parent_is_bds) {
                 num -= bdrv_drain_all_count;
diff --git a/block/io.c b/block/io.c
index 9ba1bada36..112eed385c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -55,6 +55,15 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
     }
 }
 
+void bdrv_parent_drained_end_single(BdrvChild *c)
+{
+    assert(c->parent_quiesce_counter > 0);
+    c->parent_quiesce_counter--;
+    if (c->role->drained_end) {
+        c->role->drained_end(c);
+    }
+}
+
 void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
                              bool ignore_bds_parents)
 {
@@ -64,9 +73,7 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
         if (c == ignore || (ignore_bds_parents && c->role->parent_is_bds)) {
             continue;
         }
-        if (c->role->drained_end) {
-            c->role->drained_end(c);
-        }
+        bdrv_parent_drained_end_single(c);
     }
 }
 
@@ -96,6 +103,7 @@ static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore,
 
 void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll)
 {
+    c->parent_quiesce_counter++;
     if (c->role->drained_begin) {
         c->role->drained_begin(c);
     }
-- 
2.21.0

[Qemu-devel] [PATCH 2/4] block: Make @parent_quiesced a bool

[Max Reitz]

Reliably ending the drain on a BDS's parents is quite difficult.  What
we have to achieve is to undrain exactly those parents that have been
added to the BDS while its quiesce_counter was elevated.  If we move
decrementing the quiesce_counter before the invocation of
bdrv_parent_drained_end(), that leaves us with the parents that existed
at the start of this function.  That seems simple enough, because new
parents are always added at the beginning of the list, so just iterating
through it from start to finish should give us exactly the parents we
are looking for.

Unfortunately, there is a catch.  Unquiescing one parent can lead to
another deciding to detach the child.  In the process, the BdrvChild
object may be destroyed.  That means using QLIST_FOREACH_SAFE() to
iterate the list is actually wrong: The @next pointer may be stale by
the time we get to the next iteration.  Using QLIST_FOREACH() would be
just as wrong, though, because the current BdrvChild object may be
destroyed just as well.

It follows that we cannot just iterate over the list from start to
finish.  Some ideas how to solve this problem:
- Store a list of the parent pointers at the beginning of the function,
  then try to unquiesce them all (as long as they are in bs->parents).
  Does not work because pointers may be reused by new parents.

- Add refcounts to BdrvChild objects.  Kind of silly because only the
  parent really has a valid reference.  Once that goes away, all fields
  must be cleared.  Therefore, all we could guarantee is that the list
  pointers (next/next_parent) stay valid.
  Also, parents can simply change the child a BdrvChild refers to.  If
  they do that, the BdrvChild object remains accessible, but (1) it
  refers to the wrong child, and (2) next_parent suddenly refers to a
  different list.
  So maybe this can be made to work, but it would always be kind of
  silly.

- Check the difference between the parent_quiesce_counters and the
  actual bs->quiesce_counter.  In theory, both should be equal after
  bdrv_parent_drained_end() (reduced by bdrv_drain_all_count for BDS
  parents).
  So we could reiterate the parent list (unquiescing one parent at a
  time) until all parents have the desired parent_quiesce_counter.
  In practice, this is difficult -- I have tried.  bdrv_drain_all_count
  may be one too high because somewhere down the stack there is a
  bdrv_drain_all_end() currently running.  Also, I suppose something can
  concurrently modify bs->quiesce_counter, and I am not sure how to
  handle such cases.
  In the best case, this would lead to rather complicated code that I
  could not trust but only pray that it works.  In the worst case, my
  prayers are not heard.

We can get around the whole issue by observing that it really does not
matter whether a parent is quiesced one time or ten.  We just have to
quiesce it once the moment the child tries to change from being
unquiesced to being quiesced.  We have to unquiesce it when the child is
unquiesced again.

Therefore, we can just make the parent_quiesce_counter a boolean and
call it parent_quiesced.  When bdrv_parent_drained_end() sees
bs->quiesce_counter going to 0, we unquiesce all parents.
bdrv_parent_drained_begin() in turn quiesces all unquiesced parents.

(This means we have to decrement bs->quiesce_counter before
bdrv_parent_drained_end().)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h     | 10 +------
 include/block/block_int.h |  9 +++----
 block.c                   |  7 +++--
 block/io.c                | 55 ++++++++++++++++++++++++++++-----------
 tests/test-bdrv-drain.c   | 31 +++++++++++++---------
 5 files changed, 65 insertions(+), 47 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 3c084e8222..687c03b275 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -620,18 +620,10 @@ void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll);
  * bdrv_parent_drained_end_single:
  *
  * End a quiesced section for the parent of @c.
+ * c->parent_quiesced must be true.
  */
 void bdrv_parent_drained_end_single(BdrvChild *c);
 
-/**
- * bdrv_parent_drained_end:
- *
- * End a quiesced section of all users of @bs. This is part of
- * bdrv_drained_end.
- */
-void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
-                             bool ignore_bds_parents);
-
 /**
  * bdrv_drain_poll:
  *
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 84c0369fb7..58fca37ba3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -730,13 +730,10 @@ struct BdrvChild {
     bool frozen;
 
     /*
-     * How many times the parent of this child has been drained
-     * (through role->drained_*).
-     * Usually, this is equal to bs->quiesce_counter (potentially
-     * reduced by bdrv_drain_all_count).  It may differ while the
-     * child is entering or leaving a drained section.
+     * Whether the parent of this child has been drained through
+     * role->drained_*.
      */
-    int parent_quiesce_counter;
+    bool parent_quiesced;
 
     QLIST_ENTRY(BdrvChild) next;
     QLIST_ENTRY(BdrvChild) next_parent;
diff --git a/block.c b/block.c
index b336a91415..6bc51e371f 100644
--- a/block.c
+++ b/block.c
@@ -2159,7 +2159,6 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
                                       BlockDriverState *new_bs)
 {
     BlockDriverState *old_bs = child->bs;
-    int i;
 
     assert(!child->frozen);
 
@@ -2173,12 +2172,12 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
         if (child->role->detach) {
             child->role->detach(child);
         }
-        while (child->parent_quiesce_counter) {
+        if (child->parent_quiesced) {
             bdrv_parent_drained_end_single(child);
         }
         QLIST_REMOVE(child, next_parent);
     } else {
-        assert(child->parent_quiesce_counter == 0);
+        assert(!child->parent_quiesced);
     }
 
     child->bs = new_bs;
@@ -2191,7 +2190,7 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
                 num -= bdrv_drain_all_count;
             }
             assert(num >= 0);
-            for (i = 0; i < num; i++) {
+            if (num) {
                 bdrv_parent_drained_begin_single(child, true);
             }
         }
diff --git a/block/io.c b/block/io.c
index 112eed385c..2408abffd9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -57,24 +57,47 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
 
 void bdrv_parent_drained_end_single(BdrvChild *c)
 {
-    assert(c->parent_quiesce_counter > 0);
-    c->parent_quiesce_counter--;
+    assert(c->parent_quiesced);
+    c->parent_quiesced = false;
     if (c->role->drained_end) {
         c->role->drained_end(c);
     }
 }
 
-void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
-                             bool ignore_bds_parents)
+static void bdrv_parent_drained_end(BlockDriverState *bs)
 {
-    BdrvChild *c, *next;
+    BdrvChild *c;
 
-    QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
-        if (c == ignore || (ignore_bds_parents && c->role->parent_is_bds)) {
-            continue;
-        }
-        bdrv_parent_drained_end_single(c);
+    if (bs->quiesce_counter) {
+        return;
     }
+
+    /*
+     * The list of parents can change, so repeat until all are
+     * unquiesced (or until bs->quiesce_counter is no longer zero).
+     */
+    do {
+        QLIST_FOREACH(c, &bs->parents, next_parent) {
+            if (!c->parent_quiesced) {
+                continue;
+            }
+
+            if (bs->quiesce_counter) {
+                /*
+                 * We can just leave here.  The first thing
+                 * bdrv_parent_drained_end_single() does is to set
+                 * c->parent_quiesced to false.  If something decides
+                 * to drain @bs while we are unquiescing some parent,
+                 * it will thus redrain that parent (and everything
+                 * else that we have already unquiesced).
+                 */
+                return;
+            }
+
+            bdrv_parent_drained_end_single(c);
+            break;
+        }
+    } while (c);
 }
 
 static bool bdrv_parent_drained_poll_single(BdrvChild *c)
@@ -103,9 +126,11 @@ static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore,
 
 void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll)
 {
-    c->parent_quiesce_counter++;
-    if (c->role->drained_begin) {
-        c->role->drained_begin(c);
+    if (!c->parent_quiesced) {
+        c->parent_quiesced = true;
+        if (c->role->drained_begin) {
+            c->role->drained_begin(c);
+        }
     }
     if (poll) {
         BDRV_POLL_WHILE(c->bs, bdrv_parent_drained_poll_single(c));
@@ -433,9 +458,9 @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
 
     /* Re-enable things in child-to-parent order */
     bdrv_drain_invoke(bs, false);
-    bdrv_parent_drained_end(bs, parent, ignore_bds_parents);
-
     old_quiesce_counter = atomic_fetch_dec(&bs->quiesce_counter);
+    bdrv_parent_drained_end(bs);
+
     if (old_quiesce_counter == 1) {
         aio_enable_external(bdrv_get_aio_context(bs));
     }
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 12e2ecf517..75609bf6d0 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -446,10 +446,14 @@ static void test_multiparent(void)
 
     do_drain_end(BDRV_SUBTREE_DRAIN, bs_b);
 
-    g_assert_cmpint(bs_a->quiesce_counter, ==, 1);
+    /*
+     * @backing is still drained by @bs_a, so it has not unquiesced
+     * its parents yet (and @bs_a retains its qc of 2).
+     */
+    g_assert_cmpint(bs_a->quiesce_counter, ==, 2);
     g_assert_cmpint(bs_b->quiesce_counter, ==, 1);
     g_assert_cmpint(backing->quiesce_counter, ==, 1);
-    g_assert_cmpint(a_s->drain_count, ==, 1);
+    g_assert_cmpint(a_s->drain_count, ==, 2);
     g_assert_cmpint(b_s->drain_count, ==, 1);
     g_assert_cmpint(backing_s->drain_count, ==, 1);
 
@@ -505,27 +509,28 @@ static void test_graph_change_drain_subtree(void)
     do_drain_begin(BDRV_SUBTREE_DRAIN, bs_b);
 
     bdrv_set_backing_hd(bs_b, backing, &error_abort);
-    g_assert_cmpint(bs_a->quiesce_counter, ==, 5);
-    g_assert_cmpint(bs_b->quiesce_counter, ==, 5);
-    g_assert_cmpint(backing->quiesce_counter, ==, 5);
-    g_assert_cmpint(a_s->drain_count, ==, 5);
-    g_assert_cmpint(b_s->drain_count, ==, 5);
+    g_assert_cmpint(bs_a->quiesce_counter, ==, 4); /* 3 + !!2 */
+    g_assert_cmpint(bs_b->quiesce_counter, ==, 3); /* !!3 + 2 */
+    g_assert_cmpint(backing->quiesce_counter, ==, 5); /* 3 + 2 */
+    g_assert_cmpint(a_s->drain_count, ==, 4);
+    g_assert_cmpint(b_s->drain_count, ==, 3);
     g_assert_cmpint(backing_s->drain_count, ==, 5);
 
     bdrv_set_backing_hd(bs_b, NULL, &error_abort);
-    g_assert_cmpint(bs_a->quiesce_counter, ==, 3);
+    /* @backing remains quiesced, so it does not unquiesce @bs_a */
+    g_assert_cmpint(bs_a->quiesce_counter, ==, 4);
     g_assert_cmpint(bs_b->quiesce_counter, ==, 2);
     g_assert_cmpint(backing->quiesce_counter, ==, 3);
-    g_assert_cmpint(a_s->drain_count, ==, 3);
+    g_assert_cmpint(a_s->drain_count, ==, 4);
     g_assert_cmpint(b_s->drain_count, ==, 2);
     g_assert_cmpint(backing_s->drain_count, ==, 3);
 
     bdrv_set_backing_hd(bs_b, backing, &error_abort);
-    g_assert_cmpint(bs_a->quiesce_counter, ==, 5);
-    g_assert_cmpint(bs_b->quiesce_counter, ==, 5);
+    g_assert_cmpint(bs_a->quiesce_counter, ==, 4);
+    g_assert_cmpint(bs_b->quiesce_counter, ==, 3);
     g_assert_cmpint(backing->quiesce_counter, ==, 5);
-    g_assert_cmpint(a_s->drain_count, ==, 5);
-    g_assert_cmpint(b_s->drain_count, ==, 5);
+    g_assert_cmpint(a_s->drain_count, ==, 4);
+    g_assert_cmpint(b_s->drain_count, ==, 3);
     g_assert_cmpint(backing_s->drain_count, ==, 5);
 
     do_drain_end(BDRV_SUBTREE_DRAIN, bs_b);
-- 
2.21.0

[Qemu-devel] [PATCH 3/4] iotests: Add @has_quit to vm.shutdown()

[Max Reitz]

If a test has issued a quit command already (which may be useful to do
explicitly because the test wants to show its effects),
QEMUMachine.shutdown() should not do so again.  Otherwise, the VM may
well return an ECONNRESET which will lead QEMUMachine.shutdown() to
killing it, which then turns into a "qemu received signal 9" line.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 python/qemu/__init__.py | 5 +++--
 tests/qemu-iotests/255  | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index dbaf8a5311..25207a2970 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -332,13 +332,14 @@ class QEMUMachine(object):
         self._load_io_log()
         self._post_shutdown()
 
-    def shutdown(self):
+    def shutdown(self, has_quit=False):
         """
         Terminate the VM and clean up
         """
         if self.is_running():
             try:
-                self._qmp.cmd('quit')
+                if not has_quit:
+                    self._qmp.cmd('quit')
                 self._qmp.close()
             except:
                 self._popen.kill()
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 49433ec122..3632d507d0 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -132,4 +132,4 @@ with iotests.FilePath('src.qcow2') as src_path, \
     vm.qmp_log('block-job-cancel', device='job0')
     vm.qmp_log('quit')
 
-    vm.shutdown()
+    vm.shutdown(has_quit=True)
-- 
2.21.0

[Qemu-devel] [PATCH 4/4] iotests: Test commit with a filter on the chain

[Max Reitz]

Before the previous patches, the first case resulted in a failed
assertion (which is noted as qemu receiving a SIGABRT in the test
output), and the second usually triggered a segmentation fault.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/040     | 40 +++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index b81133a474..aa0b1847e3 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -92,9 +92,10 @@ class TestSingleDrive(ImageCommitTestCase):
 
         self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
         self.vm.launch()
+        self.has_quit = False
 
     def tearDown(self):
-        self.vm.shutdown()
+        self.vm.shutdown(has_quit=self.has_quit)
         os.remove(test_img)
         os.remove(mid_img)
         os.remove(backing_img)
@@ -109,6 +110,43 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
         self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
+    def test_commit_with_filter_and_quit(self):
+        result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
+        self.assert_qmp(result, 'return', {})
+
+        # Add a filter outside of the backing chain
+        result = self.vm.qmp('blockdev-add', driver='throttle', node_name='filter', throttle_group='tg', file='mid')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-commit', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        # Quit immediately, thus forcing a simultaneous cancel of the
+        # block job and a bdrv_drain_all()
+        result = self.vm.qmp('quit')
+        self.assert_qmp(result, 'return', {})
+
+        self.has_quit = True
+
+    # Same as above, but this time we add the filter after starting the job
+    def test_commit_plus_filter_and_quit(self):
+        result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-commit', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        # Add a filter outside of the backing chain
+        result = self.vm.qmp('blockdev-add', driver='throttle', node_name='filter', throttle_group='tg', file='mid')
+        self.assert_qmp(result, 'return', {})
+
+        # Quit immediately, thus forcing a simultaneous cancel of the
+        # block job and a bdrv_drain_all()
+        result = self.vm.qmp('quit')
+        self.assert_qmp(result, 'return', {})
+
+        self.has_quit = True
+
     def test_device_not_found(self):
         result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % mid_img)
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 802ffaa0c0..220a5fa82c 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-...........................................
+...............................................
 ----------------------------------------------------------------------
-Ran 43 tests
+Ran 47 tests
 
 OK
-- 
2.21.0

Re: [Qemu-devel] [PATCH 0/4] block: Keep track of parent quiescing

[Kevin Wolf]

Am 05.06.2019 um 18:11 hat Max Reitz geschrieben:
> We currently do not keep track of how many times a child has quiesced
> its parent.  We just guess based on the child’s quiesce_counter.  That
> keeps biting us when we try to leave drained sections or detach children
> (see e.g. commit 5cb2737e925042e).
> 
> I think we need an explicit counter to keep track of how many times a
> parent has been quiesced (patch 1).  That just makes the code more
> resilient.
> 
> Actually, no, we don’t need a counter, we need a boolean.  See patch 2
> for the explanation.
> 
> Yes, it is a bit weird to introduce a counter first (patch 1) and then
> immediately make it a boolean (patch 2).  But I believe this to be the
> most logical change set.
> 
> (“Our current model relies on counting, so adding an explicit counter
> makes sense.  It then turns out that counting is probably not the best
> idea, so why not make it a boolean?”)

Trying to summarise an IRC discussion I just had with Max:

The real root problem isn't that the recursion in bdrv_do_drained_end()
doesn't correctly deal with graph changes, but that those graph changes
happen in the first place. The one basic guiding principle in my drain
rewrite was that during the recursion (both to children and parents), no
graph changes are allowed, which means that no aio_poll() calls are
allowed either.

Of course, while I think the principle is right and greatly simplifies
the code (or actually is the only thing that gives us any hope to get
things right), I messed up the implementation because
bdrv_drain_invoke() does use BDRV_POLL_WHILE() for ending a drained
section. This is wrong, and it could still cause crashes after this
series because a recursive call could remove a node that is currently
processed somewhere up the call chain.

The fix for the observed bugs should be to make drained_end completely
symmetric to drained_begin: Just start the bdrv_drain_invoke_entry()
coroutine, do the recursion and call all the callbacks (none of which
may modify the graph) and only after all of this is done, poll once at
the top level drain. (The poll condition could be simplified to just
wait for bdrv_drain_invoke() to be completed, we don't care about other
running requests in drained_end. But this is only an optimisation.)

Despite this being a full fix, we also agreed that patch 1 is a nice
cleanup and we want to keep it even if it doesn't strictly fix a bug any
more.

Kevin