[Qemu-devel] [PATCH 0/5] block: Add BDS.never_freeze

[Qemu-devel] [PATCH 0/5] block: Add BDS.never_freeze

[Max Reitz]

As it is, this series depends on my block branch, most importantly
Andrey’s series “block/stream: get rid of the base”.

Depends-on: <1559152576-281803-1-git-send-email-andrey.shinkevich@virtuozzo.com>

This series made the problem apparent to me, but it existed before:
Namely, the commit and mirror block jobs create filter nodes which they
just try to drop with an &error_fatal.  If any of its parent links is
frozen, that will abort qemu.  Patch 1 fixes this by adding a
BDS.never_freeze flag, which prevents bdrv_freeze_backing_chain() from
freezing any links to a BDS that has this flag set.

Now, after Andrey’s series, you can reproduce this problem by first
launching a commit job and then launching a stream job from the commit’s
top node.  This is actually something that iotest 030 tests, but I
didn’t notice the problem because 030 has two problems of itself:
(1) It fails to do proper throttling, so usually, the commit job is
    already gone by the time the stream job starts.  (Fixed by patch 2.)
(2) It doesn’t check the error message, but just the error class.  If it
    had checked the error message, we would have seen that it just says
    that the stream’s base node cannot be found, and thus would have
    fixed problem #1 earlier.  (Fixed by patch 3.)

If you apply patches 2 and 3 to my block branch, you’ll see that both
jobs can be created.  If you remove the check that creating the stream
job should fail and replace the self.wait_until_completed() by a
self.vm.run_job('drive0'), you will see that qemu aborts.

So that appears to be a new problem introduced by Andrey’s series,
right?  But it actually isn’t new.  Patch 5 adds a test for a
pre-existing case:

If on master you launch a commit job and then a stream job from the
commit’s filter node, qemu will abort (if the commit job completes
before the stream job, at least).  So the problem existed well before
Andrey’s series, it just moved it down one node.
(You can test that by applying patches 2 through 5 on master.  You will
get a failure in test_overlapping_4, and a qemu abort in
test_overlapping_5.)

In turn, thanks to said series, this test case now fully works.  (So you
can stream from a commit’s filter node.)


So either way, the problem addressed by patch 1 is visible both on
master and on my block branch.  It just appears in different places,
hence the dependency.


Max Reitz (5):
  block: Add BDS.never_freeze
  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

 include/block/block_int.h     |   3 +
 block.c                       |   8 +++
 block/commit.c                |   4 ++
 block/mirror.c                |   4 ++
 tests/qemu-iotests/030        | 121 ++++++++++++++++++++++++++--------
 tests/qemu-iotests/030.out    |   4 +-
 tests/qemu-iotests/iotests.py |  18 +++--
 7 files changed, 127 insertions(+), 35 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/5] 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>
---
 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 d6415b53c1..50902531b7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -885,6 +885,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 c139540f2b..6565192b91 100644
--- a/block.c
+++ b/block.c
@@ -4416,6 +4416,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 2/5] 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>
---
 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 3/5] 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>
---
 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 4/5] 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 5/5] 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>
---
 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

Re: [Qemu-devel] [PATCH 2/5] iotests: Fix throttling in 030

[Andrey Shinkevich]

On 28/06/2019 01:32, Max Reitz wrote:
> 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>
> ---
>   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:
> 

Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH 3/5] iotests: Compare error messages in 030

[Andrey Shinkevich]

On 28/06/2019 01:32, Max Reitz wrote:
> 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>
> ---
>   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)
>   
> 

Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH 5/5] iotests: Add new case to 030

[Andrey Shinkevich]

On 28/06/2019 01:32, Max Reitz wrote:
> 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>
> ---
>   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
> 

Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH 1/5] block: Add BDS.never_freeze

[Andrey Shinkevich]

On 28/06/2019 01:32, Max Reitz wrote:
> 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>
> ---
>   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 d6415b53c1..50902531b7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -885,6 +885,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 c139540f2b..6565192b91 100644
> --- a/block.c
> +++ b/block.c
> @@ -4416,6 +4416,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 |
> 

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH 4/5] iotests: Add @use_log to VM.run_job()

[John Snow]

On 6/27/19 6:32 PM, Max Reitz wrote:
> 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>

Wondering out loud:

can log() (and by extension qmp_log, and run_job) be made to use the
python logging module and we can configure the logging environment
instead of bespoke arguments to avoid ever engaging the log?

We could theoretically just pre-disable iotests log output for unittest
style tests, unless you run in debug mode where we allow it.

I don't have a specific proposal for how to accomplish this, I think
there are some nuances to Python logging that I don't quite understand.
Maybe Cleber Rosa can help advise?

I'd like to toy with this idea; it seems like this won't be the last
time we want to turn output on/off.

--js

> ---
>  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
>  
> 

Re: [Qemu-devel] [PATCH 3/5] iotests: Compare error messages in 030

[Alberto Garcia]

On Fri 28 Jun 2019 12:32:53 AM CEST, Max Reitz wrote:
> 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>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 1/5] block: Add BDS.never_freeze

[Alberto Garcia]

On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote:
> @@ -4416,6 +4416,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;
> +        }
> +    }

How about adding this to bdrv_is_backing_chain_frozen() instead?

Berto

Re: [Qemu-devel] [PATCH 5/5] iotests: Add new case to 030

[Alberto Garcia]

On Fri 28 Jun 2019 12:32:55 AM CEST, Max Reitz wrote:
> 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>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 2/5] iotests: Fix throttling in 030

[Alberto Garcia]

On Fri 28 Jun 2019 12:32:52 AM CEST, Max Reitz wrote:
> 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>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 1/5] block: Add BDS.never_freeze

[Max Reitz]

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

On 02.07.19 16:02, Alberto Garcia wrote:
> On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote:
>> @@ -4416,6 +4416,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;
>> +        }
>> +    }
> 
> How about adding this to bdrv_is_backing_chain_frozen() instead?

But that’s the wrong place.  For example, that function is called by
bdrv_set_backing_hd() to check whether the backing BDS can be changed.
But the point of never_freeze is to ensure that links to the BDS can be
changed.

never_freeze only becomes relevant when trying to freeze the backing
chain, in that it should prevent it.  So I think putting the check here
is correct.

Max


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

Re: [Qemu-devel] [PATCH 1/5] block: Add BDS.never_freeze

[Alberto Garcia]

On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 1/5] block: Add BDS.never_freeze

[Max Reitz]

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

On 02.07.19 17:36, Alberto Garcia wrote:
> On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote:
>> 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: Alberto Garcia <berto@igalia.com>

Thanks! :-)

Max


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

Re: [Qemu-devel] [PATCH 4/5] iotests: Add @use_log to VM.run_job()

[Max Reitz]

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

On 02.07.19 00:59, John Snow wrote:
> 
> 
> On 6/27/19 6:32 PM, Max Reitz wrote:
>> 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>
> 
> Wondering out loud:
> 
> can log() (and by extension qmp_log, and run_job) be made to use the
> python logging module and we can configure the logging environment
> instead of bespoke arguments to avoid ever engaging the log?
> 
> We could theoretically just pre-disable iotests log output for unittest
> style tests, unless you run in debug mode where we allow it.
> 
> I don't have a specific proposal for how to accomplish this, I think
> there are some nuances to Python logging that I don't quite understand.
> Maybe Cleber Rosa can help advise?
> 
> I'd like to toy with this idea; it seems like this won't be the last
> time we want to turn output on/off.

Sounds good.  But considering this is just test infrastructure, I’ll
leave that for when someone(TM) gets around to doing it.  (Hopefully
when the next function is about to get a @use_log parameter.)

Max


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

Re: [Qemu-devel] [PATCH 4/5] iotests: Add @use_log to VM.run_job()

[John Snow]

On 7/2/19 12:19 PM, Max Reitz wrote:
> On 02.07.19 00:59, John Snow wrote:
>>
>>
>> On 6/27/19 6:32 PM, Max Reitz wrote:
>>> 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>
>>
>> Wondering out loud:
>>
>> can log() (and by extension qmp_log, and run_job) be made to use the
>> python logging module and we can configure the logging environment
>> instead of bespoke arguments to avoid ever engaging the log?
>>
>> We could theoretically just pre-disable iotests log output for unittest
>> style tests, unless you run in debug mode where we allow it.
>>
>> I don't have a specific proposal for how to accomplish this, I think
>> there are some nuances to Python logging that I don't quite understand.
>> Maybe Cleber Rosa can help advise?
>>
>> I'd like to toy with this idea; it seems like this won't be the last
>> time we want to turn output on/off.
> 
> Sounds good.  But considering this is just test infrastructure, I’ll
> leave that for when someone(TM) gets around to doing it.  (Hopefully
> when the next function is about to get a @use_log parameter.)
> 
> Max
> 

Yes, understood.

I just noticed that our pal run_job is getting a little long in the
arguments list, which is a good hint that we're doing something wrong.

I'll try to look into this as part of my next bitmap set.

--js