[Qemu-devel] [PATCH v2 00/12] block: Fixes for concurrent block jobs

[Qemu-devel] [PATCH v2 00/12] block: Fixes for concurrent block jobs

[Max Reitz]

This is a v2 to “block: Add BDS.never_freeze”.

It depends on my “block: Delay poll when ending drained sections”
series:

Depends-on: <20190619152603.5937-1-mreitz@redhat.com>


It turned out that if you run 030 (or just the new test_overlapping_5
case) sufficiently often, it breaks; which is why I’m hesitant to just
merge the “add never_freeze” series as it is.

There are several reasons for why this test case breaks, I hope patches
3 to 6 fix them.  Patch 12 adds a test that is much more reliable than
test_overlapping_5 at detecting the problems fixed by at least patches 4
to 6.  (I think that 3 doesn’t really need a test.)

I’m sure there are other ways to see these problems, but well, coming
from 030, concurrent commit/stream jobs are how I reproduced them.
Hence the same of this series.

Patch 2 is for something I encountered on the way.  Patch 11 tests it.


v2:
- Added a bunch of more patches.


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/12:[----] [--] 'block: Add BDS.never_freeze'
002/12:[down] 'block/stream: Fix error path'
003/12:[down] 'block/stream: Swap backing file change order'
004/12:[down] 'block: Keep subtree drained in drop_intermediate'
005/12:[down] 'block: Reduce (un)drains when replacing a child'
006/12:[down] 'block: Deep-clear inherits_from'
007/12:[----] [--] 'iotests: Fix throttling in 030'
008/12:[----] [--] 'iotests: Compare error messages in 030'
009/12:[----] [--] 'iotests: Add @use_log to VM.run_job()'
010/12:[----] [--] 'iotests: Add new case to 030'
011/12:[down] 'iotests: Add read-only test case to 030'
012/12:[down] 'iotests: Add test for concurrent stream/commit'



Max Reitz (12):
  block: Add BDS.never_freeze
  block/stream: Fix error path
  block/stream: Swap backing file change order
  block: Keep subtree drained in drop_intermediate
  block: Reduce (un)drains when replacing a child
  block: Deep-clear inherits_from
  iotests: Fix throttling in 030
  iotests: Compare error messages in 030
  iotests: Add @use_log to VM.run_job()
  iotests: Add new case to 030
  iotests: Add read-only test case to 030
  iotests: Add test for concurrent stream/commit

 include/block/block_int.h     |   3 +
 block.c                       |  93 +++++++++++++------
 block/commit.c                |   4 +
 block/mirror.c                |   4 +
 block/stream.c                |   4 +-
 tests/qemu-iotests/030        | 150 +++++++++++++++++++++++++------
 tests/qemu-iotests/030.out    |   4 +-
 tests/qemu-iotests/258        | 163 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/258.out    |  33 +++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  18 ++--
 11 files changed, 413 insertions(+), 64 deletions(-)
 create mode 100755 tests/qemu-iotests/258
 create mode 100644 tests/qemu-iotests/258.out

-- 
2.21.0

[Qemu-devel] [PATCH v2 01/12] block: Add BDS.never_freeze

[Max Reitz]

The commit and the mirror block job must be able to drop their filter
node at any point.  However, this will not be possible if any of the
BdrvChild links to them is frozen.  Therefore, we need to prevent them
from ever becoming frozen.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 include/block/block_int.h | 3 +++
 block.c                   | 8 ++++++++
 block/commit.c            | 4 ++++
 block/mirror.c            | 4 ++++
 4 files changed, 19 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index fce9a9e7ee..bc378a49dc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -908,6 +908,9 @@ struct BlockDriverState {
 
     /* Only read/written by whoever has set active_flush_req to true.  */
     unsigned int flushed_gen;             /* Flushed write generation */
+
+    /* BdrvChild links to this node may never be frozen */
+    bool never_freeze;
 };
 
 struct BlockBackendRootState {
diff --git a/block.c b/block.c
index 1652f3d29b..a06f07347f 100644
--- a/block.c
+++ b/block.c
@@ -4420,6 +4420,14 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
         return -EPERM;
     }
 
+    for (i = bs; i != base; i = backing_bs(i)) {
+        if (i->backing && backing_bs(i)->never_freeze) {
+            error_setg(errp, "Cannot freeze '%s' link to '%s'",
+                       i->backing->name, backing_bs(i)->node_name);
+            return -EPERM;
+        }
+    }
+
     for (i = bs; i != base; i = backing_bs(i)) {
         if (i->backing) {
             i->backing->frozen = true;
diff --git a/block/commit.c b/block/commit.c
index ca7e408b26..2c5a6d4ebc 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -298,6 +298,10 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     if (!filter_node_name) {
         commit_top_bs->implicit = true;
     }
+
+    /* So that we can always drop this node */
+    commit_top_bs->never_freeze = true;
+
     commit_top_bs->total_sectors = top->total_sectors;
 
     bdrv_append(commit_top_bs, top, &local_err);
diff --git a/block/mirror.c b/block/mirror.c
index 2fcec70e35..8cb75fb409 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1551,6 +1551,10 @@ static BlockJob *mirror_start_job(
     if (!filter_node_name) {
         mirror_top_bs->implicit = true;
     }
+
+    /* So that we can always drop this node */
+    mirror_top_bs->never_freeze = true;
+
     mirror_top_bs->total_sectors = bs->total_sectors;
     mirror_top_bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED;
     mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
-- 
2.21.0

[Qemu-devel] [PATCH v2 02/12] block/stream: Fix error path

[Max Reitz]

As of commit c624b015bf14fe01f1e6452a36e63b3ea1ae4998, the stream job
only freezes the chain until the overlay of the base node.  The error
path must consider this.

Fixes: c624b015bf14fe01f1e6452a36e63b3ea1ae4998
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/stream.c b/block/stream.c
index cd5e2ba9b0..b27e61625d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -284,5 +284,5 @@ fail:
     if (bs_read_only) {
         bdrv_reopen_set_read_only(bs, true, NULL);
     }
-    bdrv_unfreeze_backing_chain(bs, base);
+    bdrv_unfreeze_backing_chain(bs, bottom);
 }
-- 
2.21.0

[Qemu-devel] [PATCH v2 03/12] block/stream: Swap backing file change order

[Max Reitz]

bdrv_change_backing_file() can result in yields.  Therefore, @base may
no longer be the the backing_bs() of s->bottom afterwards.

Just swap the order of the two calls to fix this.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/stream.c b/block/stream.c
index b27e61625d..6ac1e7bec4 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -78,8 +78,8 @@ static int stream_prepare(Job *job)
                 base_fmt = base->drv->format_name;
             }
         }
-        ret = bdrv_change_backing_file(bs, base_id, base_fmt);
         bdrv_set_backing_hd(bs, base, &local_err);
+        ret = bdrv_change_backing_file(bs, base_id, base_fmt);
         if (local_err) {
             error_report_err(local_err);
             return -EPERM;
-- 
2.21.0

[Qemu-devel] [PATCH v2 04/12] block: Keep subtree drained in drop_intermediate

[Max Reitz]

bdrv_drop_intermediate() calls BdrvChildRole.update_filename().  That
may poll, thus changing the graph, which potentially breaks the
QLIST_FOREACH_SAFE() loop.

Just keep the whole subtree drained.  This is probably the right thing
to do anyway (dropping nodes while the subtree is not drained seems
wrong).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index a06f07347f..96b3dc7d53 100644
--- a/block.c
+++ b/block.c
@@ -4493,6 +4493,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
     int ret = -EIO;
 
     bdrv_ref(top);
+    bdrv_subtree_drained_begin(top);
 
     if (!top->drv || !base->drv) {
         goto exit;
@@ -4564,6 +4565,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
 
     ret = 0;
 exit:
+    bdrv_subtree_drained_end(top);
     bdrv_unref(top);
     return ret;
 }
-- 
2.21.0

[Qemu-devel] [PATCH v2 05/12] block: Reduce (un)drains when replacing a child

[Max Reitz]

Currently, bdrv_replace_child_noperm() undrains the parent until it is
completely undrained, then re-drains it after attaching the new child
node.

This is a problem with bdrv_drop_intermediate(): We want to keep the
whole subtree drained, including parents, while the operation is
under way.  bdrv_replace_child_noperm() breaks this by allowing every
parent to become unquiesced briefly, and then redraining it.

In fact, there is no reason why the parent should become unquiesced and
be allowed to submit requests to the new child node if that new node is
supposed to be kept drained.  So if anything, we have to drain the
parent before detaching the old child node.  Conversely, we have to
undrain it only after attaching the new child node.

Thus, change the whole drain algorithm here: Calculate the number of
times we have to drain/undrain the parent before replacing the child
node then drain it (if necessary), replace the child node, and then
undrain it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 49 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 96b3dc7d53..1fc9e709ad 100644
--- a/block.c
+++ b/block.c
@@ -2246,13 +2246,27 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
                                       BlockDriverState *new_bs)
 {
     BlockDriverState *old_bs = child->bs;
-    int i;
+    int new_bs_quiesce_counter;
+    int drain_saldo;
 
     assert(!child->frozen);
 
     if (old_bs && new_bs) {
         assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
     }
+
+    new_bs_quiesce_counter = (new_bs ? new_bs->quiesce_counter : 0);
+    drain_saldo = new_bs_quiesce_counter - child->parent_quiesce_counter;
+
+    /*
+     * If the new child node is drained but the old one was not, flush
+     * all outstanding requests to the old child node.
+     */
+    while (drain_saldo > 0 && child->role->drained_begin) {
+        bdrv_parent_drained_begin_single(child, true);
+        drain_saldo--;
+    }
+
     if (old_bs) {
         /* Detach first so that the recursive drain sections coming from @child
          * are already gone and we only end the drain sections that came from
@@ -2260,28 +2274,22 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
         if (child->role->detach) {
             child->role->detach(child);
         }
-        while (child->parent_quiesce_counter) {
-            bdrv_parent_drained_end_single(child, true);
-        }
         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) {
-            int num = new_bs->quiesce_counter;
-            if (child->role->parent_is_bds) {
-                num -= bdrv_drain_all_count;
-            }
-            assert(num >= 0);
-            for (i = 0; i < num; i++) {
-                bdrv_parent_drained_begin_single(child, true);
-            }
-        }
+
+        /*
+         * Detaching the old node may have led to the new node's
+         * quiesce_counter having been decreased.  Not a problem, we
+         * just need to recognize this here and then invoke
+         * drained_end appropriately more often.
+         */
+        assert(new_bs->quiesce_counter <= new_bs_quiesce_counter);
+        drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter;
 
         /* Attach only after starting new drained sections, so that recursive
          * drain sections coming from @child don't get an extra .drained_begin
@@ -2290,6 +2298,15 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
             child->role->attach(child);
         }
     }
+
+    /*
+     * If the old child node was drained but the new one is not, allow
+     * requests to come in only after the new node has been attached.
+     */
+    while (drain_saldo < 0 && child->role->drained_end) {
+        bdrv_parent_drained_end_single(child, true);
+        drain_saldo++;
+    }
 }
 
 /*
-- 
2.21.0

[Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Max Reitz]

BDS.inherits_from does not always point to an immediate parent node.
When launching a block job with a filter node, for example, the node
directly below the filter will not point to the filter, but keep its old
pointee (above the filter).

If that pointee goes away while the job is still running, the node's
inherits_from will not be updated and thus point to garbage.  To fix
this, bdrv_unref_child() has to check not only the parent node's
immediate children for nodes whose inherits_from needs to be cleared,
but its whole subtree.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 1fc9e709ad..5f5a06eb7d 100644
--- a/block.c
+++ b/block.c
@@ -2493,18 +2493,20 @@ void bdrv_root_unref_child(BdrvChild *child)
     bdrv_unref(child_bs);
 }
 
-void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
+/**
+ * Clear all inherits_from pointers from children and grandchildren of
+ * @root that point to @root, where necessary.
+ */
+static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child)
 {
-    if (child == NULL) {
-        return;
-    }
-
-    if (child->bs->inherits_from == parent) {
-        BdrvChild *c;
+    BdrvChild *c;
 
-        /* Remove inherits_from only when the last reference between parent and
-         * child->bs goes away. */
-        QLIST_FOREACH(c, &parent->children, next) {
+    if (child->bs->inherits_from == root) {
+        /*
+         * Remove inherits_from only when the last reference between root and
+         * child->bs goes away.
+         */
+        QLIST_FOREACH(c, &root->children, next) {
             if (c != child && c->bs == child->bs) {
                 break;
             }
@@ -2514,6 +2516,18 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
         }
     }
 
+    QLIST_FOREACH(c, &child->bs->children, next) {
+        bdrv_unset_inherits_from(root, c);
+    }
+}
+
+void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
+{
+    if (child == NULL) {
+        return;
+    }
+
+    bdrv_unset_inherits_from(parent, child);
     bdrv_root_unref_child(child);
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 07/12] iotests: Fix throttling in 030

[Max Reitz]

Currently, TestParallelOps in 030 creates images that are too small for
job throttling to be effective.  This is reflected by the fact that it
never undoes the throttling.

Increase the image size and undo the throttling when the job should be
completed.  Also, add throttling in test_overlapping_4, or the jobs may
not be so overlapping after all.  In fact, the error usually emitted
here is that node2 simply does not exist, not that overlapping jobs are
not allowed -- the fact that this job ignores the exact error messages
and just checks the error class is something that should be fixed in a
follow-up patch.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030 | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index c6311d1825..2cf8d54dc5 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -154,7 +154,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 class TestParallelOps(iotests.QMPTestCase):
     num_ops = 4 # Number of parallel block-stream operations
     num_imgs = num_ops * 2 + 1
-    image_len = num_ops * 512 * 1024
+    image_len = num_ops * 4 * 1024 * 1024
     imgs = []
 
     def setUp(self):
@@ -176,11 +176,11 @@ class TestParallelOps(iotests.QMPTestCase):
         # Put data into the images we are copying data from
         odd_img_indexes = [x for x in reversed(range(self.num_imgs)) if x % 2 == 1]
         for i in range(len(odd_img_indexes)):
-            # Alternate between 256KB and 512KB.
+            # Alternate between 2MB and 4MB.
             # This way jobs will not finish in the same order they were created
-            num_kb = 256 + 256 * (i % 2)
+            num_mb = 2 + 2 * (i % 2)
             qemu_io('-f', iotests.imgfmt,
-                    '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
+                    '-c', 'write -P 0xFF %dM %dM' % (i * 4, num_mb),
                     self.imgs[odd_img_indexes[i]])
 
         # Attach the drive to the VM
@@ -213,6 +213,10 @@ class TestParallelOps(iotests.QMPTestCase):
             result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=512*1024)
             self.assert_qmp(result, 'return', {})
 
+        for job in pending_jobs:
+            result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
+            self.assert_qmp(result, 'return', {})
+
         # Wait for all jobs to be finished.
         while len(pending_jobs) > 0:
             for event in self.vm.get_qmp_events(wait=True):
@@ -260,6 +264,9 @@ class TestParallelOps(iotests.QMPTestCase):
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[0], top=self.imgs[1], job_id='commit-node0')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
+        result = self.vm.qmp('block-job-set-speed', device='stream-node4', speed=0)
+        self.assert_qmp(result, 'return', {})
+
         self.wait_until_completed(drive='stream-node4')
         self.assert_no_active_block_jobs()
 
@@ -289,6 +296,9 @@ class TestParallelOps(iotests.QMPTestCase):
         result = self.vm.qmp('block-stream', device='drive0', base=self.imgs[5], job_id='stream-drive0')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
+        result = self.vm.qmp('block-job-set-speed', device='commit-node3', speed=0)
+        self.assert_qmp(result, 'return', {})
+
         self.wait_until_completed(drive='commit-node3')
 
     # Similar to test_overlapping_2, but here block-commit doesn't use the 'top' parameter.
@@ -309,6 +319,9 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_qmp(event, 'data/type', 'commit')
         self.assert_qmp_absent(event, 'data/error')
 
+        result = self.vm.qmp('block-job-set-speed', device='commit-drive0', speed=0)
+        self.assert_qmp(result, 'return', {})
+
         result = self.vm.qmp('block-job-complete', device='commit-drive0')
         self.assert_qmp(result, 'return', {})
 
@@ -321,13 +334,18 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # Commit from node2 into node0
-        result = self.vm.qmp('block-commit', device='drive0', top=self.imgs[2], base=self.imgs[0])
+        result = self.vm.qmp('block-commit', device='drive0',
+                             top=self.imgs[2], base=self.imgs[0],
+                             speed=1024*1024)
         self.assert_qmp(result, 'return', {})
 
         # Stream from node2 into node4
         result = self.vm.qmp('block-stream', device='node4', base_node='node2', job_id='node4')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
+        self.assert_qmp(result, 'return', {})
+
         self.wait_until_completed()
         self.assert_no_active_block_jobs()
 
@@ -378,6 +396,10 @@ class TestParallelOps(iotests.QMPTestCase):
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[5], speed=1024*1024)
         self.assert_qmp(result, 'return', {})
 
+        for job in ['drive0', 'node4']:
+            result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
+            self.assert_qmp(result, 'return', {})
+
         # Wait for all jobs to be finished.
         pending_jobs = ['node4', 'drive0']
         while len(pending_jobs) > 0:
-- 
2.21.0

[Qemu-devel] [PATCH v2 08/12] iotests: Compare error messages in 030

[Max Reitz]

Currently, 030 just compares the error class, which does not say
anything.

Before HEAD^ added throttling to test_overlapping_4, that test actually
usually failed because node2 was already gone, not because it was the
commit and stream job were not allowed to overlap.

Prevent such problems in the future by comparing the error description
instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030 | 66 +++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 2cf8d54dc5..10fe1de89d 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -144,11 +144,12 @@ class TestSingleDrive(iotests.QMPTestCase):
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-stream', device='nonexistent')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            'Cannot find device=nonexistent nor node_name=nonexistent')
 
     def test_job_id_missing(self):
         result = self.vm.qmp('block-stream', device='mid')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', "Invalid job ID ''")
 
 
 class TestParallelOps(iotests.QMPTestCase):
@@ -245,24 +246,30 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2])
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node4' is busy: block device is in use by block job: stream")
 
         result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3', base=self.imgs[2])
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node3' is busy: block device is in use by block job: stream")
 
         result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4-v2')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node4' is busy: block device is in use by block job: stream")
 
         # block-commit should also fail if it touches nodes used by the stream job
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[4], job_id='commit-node4')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node4' is busy: block device is in use by block job: stream")
 
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[1], top=self.imgs[3], job_id='commit-node1')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node3' is busy: block device is in use by block job: stream")
 
         # This fails because it needs to modify the backing string in node2, which is blocked
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[0], top=self.imgs[1], job_id='commit-node0')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node2' is busy: block device is in use by block job: stream")
 
         result = self.vm.qmp('block-job-set-speed', device='stream-node4', speed=0)
         self.assert_qmp(result, 'return', {})
@@ -281,20 +288,25 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node3' is busy: block device is in use by block job: commit")
 
         result = self.vm.qmp('block-stream', device='node6', base=self.imgs[2], job_id='stream-node6')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node5' is busy: block device is in use by block job: commit")
 
         result = self.vm.qmp('block-stream', device='node4', base=self.imgs[2], job_id='stream-node4')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node4' is busy: block device is in use by block job: commit")
 
         result = self.vm.qmp('block-stream', device='node6', base=self.imgs[4], job_id='stream-node6-v2')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node5' is busy: block device is in use by block job: commit")
 
         # This fails because block-commit currently blocks the active layer even if it's not used
         result = self.vm.qmp('block-stream', device='drive0', base=self.imgs[5], job_id='stream-drive0')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'drive0' is busy: block device is in use by block job: commit")
 
         result = self.vm.qmp('block-job-set-speed', device='commit-node3', speed=0)
         self.assert_qmp(result, 'return', {})
@@ -312,7 +324,8 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-stream', device='node5', base=self.imgs[3], job_id='stream-node6')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node5' is busy: block device is in use by block job: commit")
 
         event = self.vm.event_wait(name='BLOCK_JOB_READY')
         self.assert_qmp(event, 'data/device', 'commit-drive0')
@@ -328,20 +341,21 @@ class TestParallelOps(iotests.QMPTestCase):
         self.wait_until_completed(drive='commit-drive0')
 
     # In this case the base node of the stream job is the same as the
-    # top node of commit job. Since block-commit removes the top node
-    # when it finishes, this is not allowed.
+    # top node of commit job. Since this results in the commit filter
+    # node being part of the stream chain, this is not allowed.
     def test_overlapping_4(self):
         self.assert_no_active_block_jobs()
 
         # Commit from node2 into node0
         result = self.vm.qmp('block-commit', device='drive0',
                              top=self.imgs[2], base=self.imgs[0],
-                             speed=1024*1024)
+                             filter_node_name='commit-filter', speed=1024*1024)
         self.assert_qmp(result, 'return', {})
 
         # Stream from node2 into node4
         result = self.vm.qmp('block-stream', device='node4', base_node='node2', job_id='node4')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Cannot freeze 'backing' link to 'commit-filter'")
 
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
         self.assert_qmp(result, 'return', {})
@@ -428,19 +442,23 @@ class TestParallelOps(iotests.QMPTestCase):
 
         # Error: the base node does not exist
         result = self.vm.qmp('block-stream', device='node4', base_node='none', job_id='stream')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            'Cannot find device= nor node_name=none')
 
         # Error: the base node is not a backing file of the top node
         result = self.vm.qmp('block-stream', device='node4', base_node='node6', job_id='stream')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node6' is not a backing image of 'node4'")
 
         # Error: the base node is the same as the top node
         result = self.vm.qmp('block-stream', device='node4', base_node='node4', job_id='stream')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "Node 'node4' is not a backing image of 'node4'")
 
         # Error: cannot specify 'base' and 'base-node' at the same time
         result = self.vm.qmp('block-stream', device='node4', base=self.imgs[2], base_node='node2', job_id='stream')
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc',
+            "'base' and 'base-node' cannot be specified at the same time")
 
         # Success: the base node is a backing file of the top node
         result = self.vm.qmp('block-stream', device='node4', base_node='node2', job_id='stream')
@@ -873,7 +891,7 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', speed=-1)
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', "Invalid parameter 'speed'")
 
         self.assert_no_active_block_jobs()
 
@@ -882,7 +900,7 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
-        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', "Invalid parameter 'speed'")
 
         self.cancel_and_wait(resume=True)
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 09/12] iotests: Add @use_log to VM.run_job()

[Max Reitz]

unittest-style tests generally do not use the log file, but VM.run_job()
can still be useful to them.  Add a parameter to it that hides its
output from the log file.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3ecef5bc90..ce74177ab1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -542,7 +542,7 @@ class VM(qtest.QEMUQtestMachine):
 
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-                pre_finalize=None, wait=60.0):
+                pre_finalize=None, use_log=True, wait=60.0):
         match_device = {'data': {'device': job}}
         match_id = {'data': {'id': job}}
         events = [
@@ -557,7 +557,8 @@ class VM(qtest.QEMUQtestMachine):
         while True:
             ev = filter_qmp_event(self.events_wait(events))
             if ev['event'] != 'JOB_STATUS_CHANGE':
-                log(ev)
+                if use_log:
+                    log(ev)
                 continue
             status = ev['data']['status']
             if status == 'aborting':
@@ -565,13 +566,20 @@ class VM(qtest.QEMUQtestMachine):
                 for j in result['return']:
                     if j['id'] == job:
                         error = j['error']
-                        log('Job failed: %s' % (j['error']))
+                        if use_log:
+                            log('Job failed: %s' % (j['error']))
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
-                self.qmp_log('job-finalize', id=job)
+                if use_log:
+                    self.qmp_log('job-finalize', id=job)
+                else:
+                    self.qmp('job-finalize', id=job)
             elif status == 'concluded' and not auto_dismiss:
-                self.qmp_log('job-dismiss', id=job)
+                if use_log:
+                    self.qmp_log('job-dismiss', id=job)
+                else:
+                    self.qmp('job-dismiss', id=job)
             elif status == 'null':
                 return error
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 10/12] iotests: Add new case to 030

[Max Reitz]

We recently removed the dependency of the stream job on its base node.
That makes it OK to use a commit filter node there.  Test that.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030     | 25 +++++++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 10fe1de89d..a0397072bc 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -363,6 +363,31 @@ class TestParallelOps(iotests.QMPTestCase):
         self.wait_until_completed()
         self.assert_no_active_block_jobs()
 
+    # In this case the base node of the stream job is the commit job's
+    # filter node.  stream does not have a real dependency on its base
+    # node, so even though commit removes it when it is done, there is
+    # no conflict.
+    def test_overlapping_5(self):
+        self.assert_no_active_block_jobs()
+
+        # Commit from node2 into node0
+        result = self.vm.qmp('block-commit', device='drive0',
+                             top_node='node2', base_node='node0',
+                             filter_node_name='commit-filter', speed=1024*1024)
+        self.assert_qmp(result, 'return', {})
+
+        # Stream from node2 into node4
+        result = self.vm.qmp('block-stream', device='node4',
+                             base_node='commit-filter', job_id='node4')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
+        self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+        self.assert_no_active_block_jobs()
+
     # Test a block-stream and a block-commit job in parallel
     # Here the stream job is supposed to finish quickly in order to reproduce
     # the scenario that triggers the bug fixed in 3d5d319e1221 and 1a63a907507
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 4fd1c2dcd2..5eb508de07 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-.........................
+..........................
 ----------------------------------------------------------------------
-Ran 25 tests
+Ran 26 tests
 
 OK
-- 
2.21.0

[Qemu-devel] [PATCH v2 11/12] iotests: Add read-only test case to 030

[Max Reitz]

This tests that the stream job exits cleanly (without abort) when the
top node is read-only and cannot be reopened read/write.

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

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index a0397072bc..1b69f318c6 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -36,7 +36,9 @@ class TestSingleDrive(iotests.QMPTestCase):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
         qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 524288 512', mid_img)
-        self.vm = iotests.VM().add_drive("blkdebug::" + test_img, "backing.node-name=mid")
+        self.vm = iotests.VM().add_drive("blkdebug::" + test_img,
+                                         "backing.node-name=mid," +
+                                         "backing.backing.node-name=base")
         self.vm.launch()
 
     def tearDown(self):
@@ -151,6 +153,31 @@ class TestSingleDrive(iotests.QMPTestCase):
         result = self.vm.qmp('block-stream', device='mid')
         self.assert_qmp(result, 'error/desc', "Invalid job ID ''")
 
+    def test_read_only(self):
+        # Create a new file that we can attach (we need a read-only top)
+        with iotests.FilePath('ro-top.img') as ro_top_path:
+            qemu_img('create', '-f', iotests.imgfmt, ro_top_path,
+                     str(self.image_len))
+
+            result = self.vm.qmp('blockdev-add',
+                                 node_name='ro-top',
+                                 driver=iotests.imgfmt,
+                                 read_only=True,
+                                 file={
+                                     'driver': 'file',
+                                     'filename': ro_top_path,
+                                     'read-only': True
+                                 },
+                                 backing='mid')
+            self.assert_qmp(result, 'return', {})
+
+            result = self.vm.qmp('block-stream', job_id='stream',
+                                 device='ro-top', base_node='base')
+            self.assert_qmp(result, 'error/desc', 'Block node is read-only')
+
+            result = self.vm.qmp('blockdev-del', node_name='ro-top')
+            self.assert_qmp(result, 'return', {})
+
 
 class TestParallelOps(iotests.QMPTestCase):
     num_ops = 4 # Number of parallel block-stream operations
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 5eb508de07..6d9bee1a4b 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-..........................
+...........................
 ----------------------------------------------------------------------
-Ran 26 tests
+Ran 27 tests
 
 OK
-- 
2.21.0

[Qemu-devel] [PATCH v2 12/12] iotests: Add test for concurrent stream/commit

[Max Reitz]

We already have 030 for that in general, but this tests very specific
cases of both jobs finishing concurrently.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/258     | 163 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/258.out |  33 ++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 197 insertions(+)
 create mode 100755 tests/qemu-iotests/258
 create mode 100644 tests/qemu-iotests/258.out

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
new file mode 100755
index 0000000000..b84cf02254
--- /dev/null
+++ b/tests/qemu-iotests/258
@@ -0,0 +1,163 @@
+#!/usr/bin/env python
+#
+# Very specific tests for adjacent commit/stream block jobs
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Max Reitz <mreitz@redhat.com>
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent, \
+        filter_qmp_testfiles, filter_qmp_imgfmt
+
+# Need backing file and change-backing-file support
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
+iotests.verify_platform(['linux'])
+
+
+# Returns a node for blockdev-add
+def node(node_name, path, backing=None, fmt=None, throttle=None):
+    if fmt is None:
+        fmt = iotests.imgfmt
+
+    res = {
+        'node-name': node_name,
+        'driver': fmt,
+        'file': {
+            'driver': 'file',
+            'filename': path
+        }
+    }
+
+    if backing is not None:
+        res['backing'] = backing
+
+    if throttle:
+        res['file'] = {
+            'driver': 'throttle',
+            'throttle-group': throttle,
+            'file': res['file']
+        }
+
+    return res
+
+# Finds a node in the debug block graph
+def find_graph_node(graph, node_id):
+    return next(node for node in graph['nodes'] if node['id'] == node_id)
+
+
+def test_concurrent_finish(write_to_stream_node):
+    log('')
+    log('=== Commit and stream finish concurrently (letting %s write) ===' % \
+        ('stream' if write_to_stream_node else 'commit'))
+    log('')
+
+    # All chosen in such a way that when the commit job wants to
+    # finish, it polls and thus makes stream finish concurrently --
+    # and the other way around, depending on whether the commit job
+    # is finalized before stream completes or not.
+
+    with iotests.FilePath('node4.img') as node4_path, \
+         iotests.FilePath('node3.img') as node3_path, \
+         iotests.FilePath('node2.img') as node2_path, \
+         iotests.FilePath('node1.img') as node1_path, \
+         iotests.FilePath('node0.img') as node0_path, \
+         iotests.VM() as vm:
+
+        # It is important to use raw for the base layer (so that
+        # permissions are just handed through to the protocol layer)
+        assert qemu_img('create', '-f', 'raw', node0_path, '64M') == 0
+
+        stream_throttle=None
+        commit_throttle=None
+
+        for path in [node1_path, node2_path, node3_path, node4_path]:
+            assert qemu_img('create', '-f', iotests.imgfmt, path, '64M') == 0
+
+        if write_to_stream_node:
+            # This is what (most of the time) makes commit finish
+            # earlier and then pull in stream
+            assert qemu_io_silent(node2_path,
+                                  '-c', 'write %iK 64K' % (65536 - 192),
+                                  '-c', 'write %iK 64K' % (65536 -  64)) == 0
+
+            stream_throttle='tg'
+        else:
+            # And this makes stream finish earlier
+            assert qemu_io_silent(node1_path,
+                                  '-c', 'write %iK 64K' % (65536 - 64)) == 0
+
+            commit_throttle='tg'
+
+        vm.launch()
+
+        vm.qmp_log('object-add',
+                   qom_type='throttle-group',
+                   id='tg',
+                   props={
+                       'x-iops-write': 1,
+                       'x-iops-write-max': 1
+                   })
+
+        vm.qmp_log('blockdev-add',
+                   filters=[filter_qmp_testfiles, filter_qmp_imgfmt],
+                   **node('node4', node4_path, throttle=stream_throttle,
+                     backing=node('node3', node3_path,
+                     backing=node('node2', node2_path,
+                     backing=node('node1', node1_path,
+                     backing=node('node0', node0_path, throttle=commit_throttle,
+                                  fmt='raw'))))))
+
+        vm.qmp_log('block-commit',
+                   job_id='commit',
+                   device='node4',
+                   filter_node_name='commit-filter',
+                   top_node='node1',
+                   base_node='node0',
+                   auto_finalize=False)
+
+        vm.qmp_log('block-stream',
+                   job_id='stream',
+                   device='node3',
+                   base_node='commit-filter')
+
+        if write_to_stream_node:
+            vm.run_job('commit', auto_finalize=False, auto_dismiss=True)
+            vm.run_job('stream', auto_finalize=True, auto_dismiss=True)
+        else:
+            # No, the jobs do not really finish concurrently here,
+            # the stream job does complete strictly before commit.
+            # But still, this is close enough for what we want to
+            # test.
+            vm.run_job('stream', auto_finalize=True, auto_dismiss=True)
+            vm.run_job('commit', auto_finalize=False, auto_dismiss=True)
+
+        # Assert that the backing node of node3 is node 0 now
+        graph = vm.qmp('x-debug-query-block-graph')['return']
+        for edge in graph['edges']:
+            if edge['name'] == 'backing' and \
+               find_graph_node(graph, edge['parent'])['name'] == 'node3':
+                assert find_graph_node(graph, edge['child'])['name'] == 'node0'
+                break
+
+
+def main():
+    log('Running tests:')
+    test_concurrent_finish(True)
+    test_concurrent_finish(False)
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/qemu-iotests/258.out b/tests/qemu-iotests/258.out
new file mode 100644
index 0000000000..ce6e9ba3e5
--- /dev/null
+++ b/tests/qemu-iotests/258.out
@@ -0,0 +1,33 @@
+Running tests:
+
+=== Commit and stream finish concurrently (letting stream write) ===
+
+{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "throttle-group": "tg"}, "node-name": "node4"}}
+{"return": {}}
+{"execute": "block-commit", "arguments": {"auto-finalize": false, "base-node": "node0", "device": "node4", "filter-node-name": "commit-filter", "job-id": "commit", "top-node": "node1"}}
+{"return": {}}
+{"execute": "block-stream", "arguments": {"base-node": "commit-filter", "device": "node3", "job-id": "stream"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "commit"}}
+{"return": {}}
+{"data": {"id": "commit", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit", "len": 67108864, "offset": 67108864, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream", "len": 67108864, "offset": 67108864, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+=== Commit and stream finish concurrently (letting commit write) ===
+
+{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "throttle-group": "tg"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "node-name": "node4"}}
+{"return": {}}
+{"execute": "block-commit", "arguments": {"auto-finalize": false, "base-node": "node0", "device": "node4", "filter-node-name": "commit-filter", "job-id": "commit", "top-node": "node1"}}
+{"return": {}}
+{"execute": "block-stream", "arguments": {"base-node": "commit-filter", "device": "node3", "job-id": "stream"}}
+{"return": {}}
+{"data": {"device": "stream", "len": 67108864, "offset": 67108864, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "commit"}}
+{"return": {}}
+{"data": {"id": "commit", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit", "len": 67108864, "offset": 67108864, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b34c8e3c0c..21bebc00e6 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -269,3 +269,4 @@
 254 rw auto backing quick
 255 rw auto quick
 256 rw auto quick
+258 rw auto
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 00/12] block: Fixes for concurrent block jobs

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3515 bytes --]

Ping

I suppose I’ll apply the patches I had in v1 if I don’t get reviews for
the new patches, because some fixes are better than none.

(And probably patch 2, because it’s obvious.)

Max

On 03.07.19 19:28, Max Reitz wrote:
> This is a v2 to “block: Add BDS.never_freeze”.
> 
> It depends on my “block: Delay poll when ending drained sections”
> series:
> 
> Depends-on: <20190619152603.5937-1-mreitz@redhat.com>
> 
> 
> It turned out that if you run 030 (or just the new test_overlapping_5
> case) sufficiently often, it breaks; which is why I’m hesitant to just
> merge the “add never_freeze” series as it is.
> 
> There are several reasons for why this test case breaks, I hope patches
> 3 to 6 fix them.  Patch 12 adds a test that is much more reliable than
> test_overlapping_5 at detecting the problems fixed by at least patches 4
> to 6.  (I think that 3 doesn’t really need a test.)
> 
> I’m sure there are other ways to see these problems, but well, coming
> from 030, concurrent commit/stream jobs are how I reproduced them.
> Hence the same of this series.
> 
> Patch 2 is for something I encountered on the way.  Patch 11 tests it.
> 
> 
> v2:
> - Added a bunch of more patches.
> 
> 
> git backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/12:[----] [--] 'block: Add BDS.never_freeze'
> 002/12:[down] 'block/stream: Fix error path'
> 003/12:[down] 'block/stream: Swap backing file change order'
> 004/12:[down] 'block: Keep subtree drained in drop_intermediate'
> 005/12:[down] 'block: Reduce (un)drains when replacing a child'
> 006/12:[down] 'block: Deep-clear inherits_from'
> 007/12:[----] [--] 'iotests: Fix throttling in 030'
> 008/12:[----] [--] 'iotests: Compare error messages in 030'
> 009/12:[----] [--] 'iotests: Add @use_log to VM.run_job()'
> 010/12:[----] [--] 'iotests: Add new case to 030'
> 011/12:[down] 'iotests: Add read-only test case to 030'
> 012/12:[down] 'iotests: Add test for concurrent stream/commit'
> 
> 
> 
> Max Reitz (12):
>   block: Add BDS.never_freeze
>   block/stream: Fix error path
>   block/stream: Swap backing file change order
>   block: Keep subtree drained in drop_intermediate
>   block: Reduce (un)drains when replacing a child
>   block: Deep-clear inherits_from
>   iotests: Fix throttling in 030
>   iotests: Compare error messages in 030
>   iotests: Add @use_log to VM.run_job()
>   iotests: Add new case to 030
>   iotests: Add read-only test case to 030
>   iotests: Add test for concurrent stream/commit
> 
>  include/block/block_int.h     |   3 +
>  block.c                       |  93 +++++++++++++------
>  block/commit.c                |   4 +
>  block/mirror.c                |   4 +
>  block/stream.c                |   4 +-
>  tests/qemu-iotests/030        | 150 +++++++++++++++++++++++++------
>  tests/qemu-iotests/030.out    |   4 +-
>  tests/qemu-iotests/258        | 163 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/258.out    |  33 +++++++
>  tests/qemu-iotests/group      |   1 +
>  tests/qemu-iotests/iotests.py |  18 ++--
>  11 files changed, 413 insertions(+), 64 deletions(-)
>  create mode 100755 tests/qemu-iotests/258
>  create mode 100644 tests/qemu-iotests/258.out
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 00/12] block: Fixes for concurrent block jobs

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1621 bytes --]

On 03.07.19 19:28, Max Reitz wrote:
> This is a v2 to “block: Add BDS.never_freeze”.
> 
> It depends on my “block: Delay poll when ending drained sections”
> series:
> 
> Depends-on: <20190619152603.5937-1-mreitz@redhat.com>
> 
> 
> It turned out that if you run 030 (or just the new test_overlapping_5
> case) sufficiently often, it breaks; which is why I’m hesitant to just
> merge the “add never_freeze” series as it is.
> 
> There are several reasons for why this test case breaks, I hope patches
> 3 to 6 fix them.  Patch 12 adds a test that is much more reliable than
> test_overlapping_5 at detecting the problems fixed by at least patches 4
> to 6.  (I think that 3 doesn’t really need a test.)
> 
> I’m sure there are other ways to see these problems, but well, coming
> from 030, concurrent commit/stream jobs are how I reproduced them.
> Hence the same of this series.
> 
> Patch 2 is for something I encountered on the way.  Patch 11 tests it.

Applied patches 1, 2, 3, and 6 through 11 to my block branch.


So what remains are patches 4, 5, and 12.

For 4, I don’t know whether the approach is too heavy-handed (maybe a
simple bdrv_drained_begin()/-end() would suffice), and whether it’s even
right to wrap the whole function in a drained section.  (Unless there
are objections, I’ll still take it for rc2.)

5 is just pretty complex, and it also depends on my other series in its
current form.  So I wouldn’t like to take it without any reviews.

12 is a test that will fail without 4 and 5, so I can’t take it without
those two.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Kevin Wolf]

Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
> BDS.inherits_from does not always point to an immediate parent node.
> When launching a block job with a filter node, for example, the node
> directly below the filter will not point to the filter, but keep its old
> pointee (above the filter).
> 
> If that pointee goes away while the job is still running, the node's
> inherits_from will not be updated and thus point to garbage.  To fix
> this, bdrv_unref_child() has to check not only the parent node's
> immediate children for nodes whose inherits_from needs to be cleared,
> but its whole subtree.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Isn't the real bug that we keep pointing to a node that isn't a parent
of the node any more? I think this will effectively disable option
inheritance in bdrv_reopen() as long as the filter node is present,
which is certainly not what we intended.

The intuitive thing would be that after inserting a filter, the image
now inherits from the filter node, and when the filter is removed, it
inherits from the filter's bs->inherit_from if that becomes a parent of
the node. (Though I'm not necessarily saying that my intuition is to be
trusted here.)

Kevin

Re: [Qemu-devel] [PATCH v2 04/12] block: Keep subtree drained in drop_intermediate

[Kevin Wolf]

Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
> bdrv_drop_intermediate() calls BdrvChildRole.update_filename().  That
> may poll, thus changing the graph, which potentially breaks the
> QLIST_FOREACH_SAFE() loop.
> 
> Just keep the whole subtree drained.  This is probably the right thing
> to do anyway (dropping nodes while the subtree is not drained seems
> wrong).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v2 05/12] block: Reduce (un)drains when replacing a child

[Kevin Wolf]

Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
> Currently, bdrv_replace_child_noperm() undrains the parent until it is
> completely undrained, then re-drains it after attaching the new child
> node.
> 
> This is a problem with bdrv_drop_intermediate(): We want to keep the
> whole subtree drained, including parents, while the operation is
> under way.  bdrv_replace_child_noperm() breaks this by allowing every
> parent to become unquiesced briefly, and then redraining it.
> 
> In fact, there is no reason why the parent should become unquiesced and
> be allowed to submit requests to the new child node if that new node is
> supposed to be kept drained.  So if anything, we have to drain the
> parent before detaching the old child node.  Conversely, we have to
> undrain it only after attaching the new child node.
> 
> Thus, change the whole drain algorithm here: Calculate the number of
> times we have to drain/undrain the parent before replacing the child
> node then drain it (if necessary), replace the child node, and then
> undrain it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

I think this looks good (and fixes a bug that Denis Plotnikov reported
before while trying to fix IDE submitting requests in a drained
section), but I would be even more confident if we could add tests for
the various cases to tests/test-bdrv-drain.c.

Kevin

Re: [Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2653 bytes --]

On 16.07.19 19:01, Kevin Wolf wrote:
> Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
>> BDS.inherits_from does not always point to an immediate parent node.
>> When launching a block job with a filter node, for example, the node
>> directly below the filter will not point to the filter, but keep its old
>> pointee (above the filter).
>>
>> If that pointee goes away while the job is still running, the node's
>> inherits_from will not be updated and thus point to garbage.  To fix
>> this, bdrv_unref_child() has to check not only the parent node's
>> immediate children for nodes whose inherits_from needs to be cleared,
>> but its whole subtree.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Isn't the real bug that we keep pointing to a node that isn't a parent
> of the node any more? I think this will effectively disable option
> inheritance in bdrv_reopen() as long as the filter node is present,
> which is certainly not what we intended.

Well, it breaks it while a block job is running.  I don’t know whether I
would advise doing a reopen across a block job filter.  It’s a case of
“Why wouldn’t it work?”, but I’m sure there’s something that doesn’t.
(Like this here, for example, but it at least has the decency of just
letting the reopen fail.)

> The intuitive thing would be that after inserting a filter, the image
> now inherits from the filter node, and when the filter is removed, it
> inherits from the filter's bs->inherit_from if that becomes a parent of
> the node. (Though I'm not necessarily saying that my intuition is to be
> trusted here.)

I tried that first, but I couldn’t get it to work.  I don’t remember
why, though.  I suppose my problem was that removing the filter node
make inherits_from NULL.  I guess I stopped at that point and went this
route instead.

I suppose we could add a flag to the BDS that says whether an heir
node’s inherits_from should be cleared or set to the bequeather’s
inherits_from, like so:

    if (parent->inherit_inherits_from) {
        child->bs->inherits_from = parent->inherits_from;
    } else {
        child->bs->inherits_from = NULL;
    }

And then, if you insert a node between a child and its inherits_from
parent, that node copies inherits_from from the child and gets its
inherit_inherits_from set to true.

The problem I see is that it doesn’t always appear clear to me that this
intermediate node should actually copy its inherits_from from the child.

So the same question applies here, too, I guess; should the filter node
even inherit its options from the parent?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4045 bytes --]

Am 17.07.2019 um 09:47 hat Max Reitz geschrieben:
> On 16.07.19 19:01, Kevin Wolf wrote:
> > Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
> >> BDS.inherits_from does not always point to an immediate parent node.
> >> When launching a block job with a filter node, for example, the node
> >> directly below the filter will not point to the filter, but keep its old
> >> pointee (above the filter).
> >>
> >> If that pointee goes away while the job is still running, the node's
> >> inherits_from will not be updated and thus point to garbage.  To fix
> >> this, bdrv_unref_child() has to check not only the parent node's
> >> immediate children for nodes whose inherits_from needs to be cleared,
> >> but its whole subtree.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> > Isn't the real bug that we keep pointing to a node that isn't a parent
> > of the node any more? I think this will effectively disable option
> > inheritance in bdrv_reopen() as long as the filter node is present,
> > which is certainly not what we intended.
> 
> Well, it breaks it while a block job is running.  I don’t know whether I
> would advise doing a reopen across a block job filter.  It’s a case of
> “Why wouldn’t it work?”, but I’m sure there’s something that doesn’t.
> (Like this here, for example, but it at least has the decency of just
> letting the reopen fail.)

Why would it let the reopen fail? Failing would be justifiable, but I
expect it just succeeds without updating the options of the child node.

> > The intuitive thing would be that after inserting a filter, the image
> > now inherits from the filter node, and when the filter is removed, it
> > inherits from the filter's bs->inherit_from if that becomes a parent of
> > the node. (Though I'm not necessarily saying that my intuition is to be
> > trusted here.)
> 
> I tried that first, but I couldn’t get it to work.  I don’t remember
> why, though.  I suppose my problem was that removing the filter node
> make inherits_from NULL.  I guess I stopped at that point and went this
> route instead.
> 
> I suppose we could add a flag to the BDS that says whether an heir
> node’s inherits_from should be cleared or set to the bequeather’s
> inherits_from, like so:
> 
>     if (parent->inherit_inherits_from) {
>         child->bs->inherits_from = parent->inherits_from;
>     } else {
>         child->bs->inherits_from = NULL;
>     }
> 
> And then, if you insert a node between a child and its inherits_from
> parent, that node copies inherits_from from the child and gets its
> inherit_inherits_from set to true.
> 
> The problem I see is that it doesn’t always appear clear to me that this
> intermediate node should actually copy its inherits_from from the child.
> 
> So the same question applies here, too, I guess; should the filter node
> even inherit its options from the parent?

An explicitly created filter node that is inserted with blockdev-reopen
certainly doesn't inherit from its parent. An automatically created
filter node where you didn't have control of its options - hm... good
question.

If we want to keep it simple, maybe it would be defensible if we just
break the inheritance relationship as soon as the parent is detached
(i.e. when inserting the filter). At least that would be more consistent
than silently disabling option inheritance while a filter is present and
then suddenly reenabling it when the filter goes away.


The other option would be making it actually work, i.e. the child node
keeps inheriting from its original parent node. This would not only
require modifications to bdrv_reopen(), but also to the data structures
so that the parent has a list of nodes that inherit from it: Nobody can
even guarantee that the child node always stays in the subtree of the
original parent.

This is in fact a reason why this patch isn't only ugly, but actually
wrong - you may still miss some inherits_from references.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 6314 bytes --]

On 17.07.19 10:17, Kevin Wolf wrote:
> Am 17.07.2019 um 09:47 hat Max Reitz geschrieben:
>> On 16.07.19 19:01, Kevin Wolf wrote:
>>> Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
>>>> BDS.inherits_from does not always point to an immediate parent node.
>>>> When launching a block job with a filter node, for example, the node
>>>> directly below the filter will not point to the filter, but keep its old
>>>> pointee (above the filter).
>>>>
>>>> If that pointee goes away while the job is still running, the node's
>>>> inherits_from will not be updated and thus point to garbage.  To fix
>>>> this, bdrv_unref_child() has to check not only the parent node's
>>>> immediate children for nodes whose inherits_from needs to be cleared,
>>>> but its whole subtree.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> Isn't the real bug that we keep pointing to a node that isn't a parent
>>> of the node any more? I think this will effectively disable option
>>> inheritance in bdrv_reopen() as long as the filter node is present,
>>> which is certainly not what we intended.
>>
>> Well, it breaks it while a block job is running.  I don’t know whether I
>> would advise doing a reopen across a block job filter.  It’s a case of
>> “Why wouldn’t it work?”, but I’m sure there’s something that doesn’t.
>> (Like this here, for example, but it at least has the decency of just
>> letting the reopen fail.)
> 
> Why would it let the reopen fail? Failing would be justifiable, but I
> expect it just succeeds without updating the options of the child node.

I actually don’t know what you’re referring to, because I’m not familiar
with either the inherits_from paths nor with bdrv_reopen.

I took it you meant the loop over the children in
bdrv_reopen_queue_child(), and the fact that it skips the children that
do not inherit from this node.

So I took it that options that do not get passed to children remain at
the parent level and would throw an error at some point, because options
that cannot be handled should throw an error at some point.  (Which does
happen, as far as I can tell.)

>>> The intuitive thing would be that after inserting a filter, the image
>>> now inherits from the filter node, and when the filter is removed, it
>>> inherits from the filter's bs->inherit_from if that becomes a parent of
>>> the node. (Though I'm not necessarily saying that my intuition is to be
>>> trusted here.)
>>
>> I tried that first, but I couldn’t get it to work.  I don’t remember
>> why, though.  I suppose my problem was that removing the filter node
>> make inherits_from NULL.  I guess I stopped at that point and went this
>> route instead.
>>
>> I suppose we could add a flag to the BDS that says whether an heir
>> node’s inherits_from should be cleared or set to the bequeather’s
>> inherits_from, like so:
>>
>>     if (parent->inherit_inherits_from) {
>>         child->bs->inherits_from = parent->inherits_from;
>>     } else {
>>         child->bs->inherits_from = NULL;
>>     }
>>
>> And then, if you insert a node between a child and its inherits_from
>> parent, that node copies inherits_from from the child and gets its
>> inherit_inherits_from set to true.
>>
>> The problem I see is that it doesn’t always appear clear to me that this
>> intermediate node should actually copy its inherits_from from the child.
>>
>> So the same question applies here, too, I guess; should the filter node
>> even inherit its options from the parent?
> 
> An explicitly created filter node that is inserted with blockdev-reopen
> certainly doesn't inherit from its parent. An automatically created
> filter node where you didn't have control of its options - hm... good
> question.
> 
> If we want to keep it simple, maybe it would be defensible if we just
> break the inheritance relationship as soon as the parent is detached
> (i.e. when inserting the filter). At least that would be more consistent
> than silently disabling option inheritance while a filter is present and
> then suddenly reenabling it when the filter goes away.

That sounds wrong to me.

As I said, doing a reopen across a block job filter sounds like there
can be many things to go wrong.  I don’t know why anyone would do so
(and live to tell the tale).

So from that perspective, if you do a reopen before or after, it would
work.  You don’t do anything while the filter is there because it’s a
bad idea anyway.  If it just failed while the job is running and then
started working again afterwards, that would actually sound good to me.


What does sound bad to me is breaking it just because you ran a block
job in the meantime.

Well, it doesn’t really sound bad, but it doesn’t sound ideal either.

> The other option would be making it actually work, i.e. the child node
> keeps inheriting from its original parent node. This would not only
> require modifications to bdrv_reopen(), but also to the data structures
> so that the parent has a list of nodes that inherit from it: Nobody can
> even guarantee that the child node always stays in the subtree of the
> original parent.

I don’t quite see how that cannot be guaranteed.  If the child goes out
of the subtree, we should clear inherits_from.

The only way the child can go out of the subtree is by virtue of it or
one of its recursive parents until the inherits_from pointee being detached.

I don’t see how that can happen without going through
bdrv_unref_child(), which will clear all of those inherits_from pointers.

bdrv_replace_node() comes to mind, but currently it’s actually only
called by bdrv_append(), by the block jobs to undo bdrv_append(), and by
mirror to take the to_replace node.  The last case may be an issue.

> This is in fact a reason why this patch isn't only ugly, but actually
> wrong - you may still miss some inherits_from references.

Well, the unfortunate thing is that this patch is in master now because
I preferred fixing access of a dangling pointer over being sure to keep
inherits_from working in all cases.

I know, that’s all the more reason for me to fix it now.  But as I said,
I don’t have much experience with bdrv_reopen.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 8320 bytes --]

Am 17.07.2019 um 11:07 hat Max Reitz geschrieben:
> On 17.07.19 10:17, Kevin Wolf wrote:
> > Am 17.07.2019 um 09:47 hat Max Reitz geschrieben:
> >> On 16.07.19 19:01, Kevin Wolf wrote:
> >>> Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
> >>>> BDS.inherits_from does not always point to an immediate parent node.
> >>>> When launching a block job with a filter node, for example, the node
> >>>> directly below the filter will not point to the filter, but keep its old
> >>>> pointee (above the filter).
> >>>>
> >>>> If that pointee goes away while the job is still running, the node's
> >>>> inherits_from will not be updated and thus point to garbage.  To fix
> >>>> this, bdrv_unref_child() has to check not only the parent node's
> >>>> immediate children for nodes whose inherits_from needs to be cleared,
> >>>> but its whole subtree.
> >>>>
> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>
> >>> Isn't the real bug that we keep pointing to a node that isn't a parent
> >>> of the node any more? I think this will effectively disable option
> >>> inheritance in bdrv_reopen() as long as the filter node is present,
> >>> which is certainly not what we intended.
> >>
> >> Well, it breaks it while a block job is running.  I don’t know whether I
> >> would advise doing a reopen across a block job filter.  It’s a case of
> >> “Why wouldn’t it work?”, but I’m sure there’s something that doesn’t.
> >> (Like this here, for example, but it at least has the decency of just
> >> letting the reopen fail.)
> > 
> > Why would it let the reopen fail? Failing would be justifiable, but I
> > expect it just succeeds without updating the options of the child node.
> 
> I actually don’t know what you’re referring to, because I’m not familiar
> with either the inherits_from paths nor with bdrv_reopen.
> 
> I took it you meant the loop over the children in
> bdrv_reopen_queue_child(), and the fact that it skips the children that
> do not inherit from this node.
> 
> So I took it that options that do not get passed to children remain at
> the parent level and would throw an error at some point, because options
> that cannot be handled should throw an error at some point.  (Which does
> happen, as far as I can tell.)

I'm talking about cases where you don't explicitly set an option for a
child, but it inherits the (possibly changed) option from its parent.

For example, consider a qcow2 image that was originally opened as a
backing file, so both its qcow2 node and its file-posix node are
read-only; the file-posix node inherits its read-only=on setting from
the qcow2 node. Now you reopen the qcow2 node read-write (starting a
commit job) and the expected result is that the file-posix node
automatically inherits the updated value and becomes read-write, too.

This happens indeed in bdrv_reopen_queue_child(), but the interesting
case is where an option isn't explicitly set in the recursive call for
the child node. This happens in the following line in the nested call:

    role->inherit_options(&flags, options, parent_flags, parent_options);

If child->bs->inherits_from didn't point to the parent that is being
reopened, we wouldn't even recurse, so .inherit_options would never be
called.

> >>> The intuitive thing would be that after inserting a filter, the image
> >>> now inherits from the filter node, and when the filter is removed, it
> >>> inherits from the filter's bs->inherit_from if that becomes a parent of
> >>> the node. (Though I'm not necessarily saying that my intuition is to be
> >>> trusted here.)
> >>
> >> I tried that first, but I couldn’t get it to work.  I don’t remember
> >> why, though.  I suppose my problem was that removing the filter node
> >> make inherits_from NULL.  I guess I stopped at that point and went this
> >> route instead.
> >>
> >> I suppose we could add a flag to the BDS that says whether an heir
> >> node’s inherits_from should be cleared or set to the bequeather’s
> >> inherits_from, like so:
> >>
> >>     if (parent->inherit_inherits_from) {
> >>         child->bs->inherits_from = parent->inherits_from;
> >>     } else {
> >>         child->bs->inherits_from = NULL;
> >>     }
> >>
> >> And then, if you insert a node between a child and its inherits_from
> >> parent, that node copies inherits_from from the child and gets its
> >> inherit_inherits_from set to true.
> >>
> >> The problem I see is that it doesn’t always appear clear to me that this
> >> intermediate node should actually copy its inherits_from from the child.
> >>
> >> So the same question applies here, too, I guess; should the filter node
> >> even inherit its options from the parent?
> > 
> > An explicitly created filter node that is inserted with blockdev-reopen
> > certainly doesn't inherit from its parent. An automatically created
> > filter node where you didn't have control of its options - hm... good
> > question.
> > 
> > If we want to keep it simple, maybe it would be defensible if we just
> > break the inheritance relationship as soon as the parent is detached
> > (i.e. when inserting the filter). At least that would be more consistent
> > than silently disabling option inheritance while a filter is present and
> > then suddenly reenabling it when the filter goes away.
> 
> That sounds wrong to me.
> 
> As I said, doing a reopen across a block job filter sounds like there
> can be many things to go wrong.  I don’t know why anyone would do so
> (and live to tell the tale).

You do reopen on a single node that could be way up the graph, not
really related to the job. The question is what this reopen can do to
the subtree below it, including the job filter.

> So from that perspective, if you do a reopen before or after, it would
> work.  You don’t do anything while the filter is there because it’s a
> bad idea anyway.  If it just failed while the job is running and then
> started working again afterwards, that would actually sound good to me.
> 
> 
> What does sound bad to me is breaking it just because you ran a block
> job in the meantime.
> 
> Well, it doesn’t really sound bad, but it doesn’t sound ideal either.

Fair enough (though I'm not completely convinced that a filter job
somewhere down the tree can reasonably block all of its parent nodes
from being reopened), but at the very least we must make sure that
reopen actually does report an error instead of returning success and
silently ignoring the subtrees that contain filter nodes.

> > The other option would be making it actually work, i.e. the child node
> > keeps inheriting from its original parent node. This would not only
> > require modifications to bdrv_reopen(), but also to the data structures
> > so that the parent has a list of nodes that inherit from it: Nobody can
> > even guarantee that the child node always stays in the subtree of the
> > original parent.
> 
> I don’t quite see how that cannot be guaranteed.  If the child goes out
> of the subtree, we should clear inherits_from.
> 
> The only way the child can go out of the subtree is by virtue of it or
> one of its recursive parents until the inherits_from pointee being detached.
> 
> I don’t see how that can happen without going through
> bdrv_unref_child(), which will clear all of those inherits_from pointers.
> 
> bdrv_replace_node() comes to mind, but currently it’s actually only
> called by bdrv_append(), by the block jobs to undo bdrv_append(), and by
> mirror to take the to_replace node.  The last case may be an issue.

Hm, okay. Maybe it's actually not possible. I'd have to have a closer
look at the code.

> > This is in fact a reason why this patch isn't only ugly, but actually
> > wrong - you may still miss some inherits_from references.
> 
> Well, the unfortunate thing is that this patch is in master now because
> I preferred fixing access of a dangling pointer over being sure to keep
> inherits_from working in all cases.
> 
> I know, that’s all the more reason for me to fix it now.  But as I said,
> I don’t have much experience with bdrv_reopen.

Yeah, let's see if we can improve the situation a bit at least for 4.1.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]