[Qemu-devel] [PATCH 0/4] iotests: Make 245 faster and more reliable

[Qemu-devel] [PATCH 0/4] iotests: Make 245 faster and more reliable

[Max Reitz]

245 is a bit flakey for me, because it uses block jobs that copy 1 MB of
data but have a buffer size of 512 kB, so they may be done before the
test gets to do the things it wants to do while the check is running.
(Rate limiting doesn’t change this.)

The boring way to fix this would be to increase the amount of data.

The interesting way to fix this is to make use of auto_finalize=false
and thus keep the jobs around until the test is done with them.
However, this has one problem: In one case, 245 tries to make the target
node of a stream job read-only.  If the job is still copying data, doing
so will fail because the target node is in COR mode.  Otherwise, we get
a cryptic “Block node is read-only” message.

What the message means is “After reopening, the node will be read-only,
and that won’t work, because there is a writer on it.”  It doesn’t say
that, though, but it should.  So patch 1 makes it say something to that
effect (“Cannot make block node read-only, there is a writer on it”).

245 doesn’t care about the actual error message, both reflect that qemu
correctly detects that this node cannot be made read-only at this time.
So the other thing we have to do is let assert_qmp() accept an array of
valid error messages and choose the one that matches (if any).  Then we
can just pass both error messages to it and everything works.


Nice side effect: For me, the test duration goes down from about 12 s to
about 6 s.
(That’s because the test forgot to disable rate limiting on the jobs
before waiting for their completion.)


Max Reitz (4):
  block: Improve "Block node is read-only" message
  iotests.py: Let assert_qmp() accept an array
  iotests.py: Fix VM.run_job
  iotests: Make 245 faster and more reliable

 block.c                       | 17 ++++++++++++++++-
 tests/qemu-iotests/245        | 22 ++++++++++++++--------
 tests/qemu-iotests/245.out    | 12 ++++++++++++
 tests/qemu-iotests/iotests.py | 20 +++++++++++++++++---
 4 files changed, 59 insertions(+), 12 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/4] block: Improve "Block node is read-only" message

[Max Reitz]

This message does not make any sense when it appears as the response to
making an R/W node read-only.  We should detect that case and emit a
different message, then.

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

diff --git a/block.c b/block.c
index 16ef5edfd8..af662d5f17 100644
--- a/block.c
+++ b/block.c
@@ -1689,6 +1689,8 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  GSList *ignore_children, Error **errp);
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
+static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
+                                     uint64_t *shared_perm);
 
 typedef struct BlockReopenQueueEntry {
      bool prepared;
@@ -1775,7 +1777,20 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
     if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
         !bdrv_is_writable_after_reopen(bs, q))
     {
-        error_setg(errp, "Block node is read-only");
+        if (!bdrv_is_writable_after_reopen(bs, NULL)) {
+            error_setg(errp, "Block node is read-only");
+        } else {
+            uint64_t current_perms, current_shared;
+            bdrv_get_cumulative_perm(bs, &current_perms, &current_shared);
+            if (current_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) {
+                error_setg(errp, "Cannot make block node read-only, there is "
+                           "a writer on it");
+            } else {
+                error_setg(errp, "Cannot make block node read-only and create "
+                           "a writer on it");
+            }
+        }
+
         return -EPERM;
     }
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH 1/4] block: Improve "Block node is read-only" message

[Alberto Garcia]

On Wed 15 May 2019 10:15:00 PM CEST, Max Reitz wrote:
> This message does not make any sense when it appears as the response to
> making an R/W node read-only.  We should detect that case and emit a
> different message, then.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

[Qemu-devel] [PATCH 2/4] iotests.py: Let assert_qmp() accept an array

[Max Reitz]

Sometimes we cannot tell which error message qemu will emit, and we do
not care.  With this change, we can then just pass an array of all
possible messages to assert_qmp() and it will choose the right one.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f811f69135..d96ba1f63c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -596,9 +596,23 @@ class QMPTestCase(unittest.TestCase):
         self.fail('path "%s" has value "%s"' % (path, str(result)))
 
     def assert_qmp(self, d, path, value):
-        '''Assert that the value for a specific path in a QMP dict matches'''
+        '''Assert that the value for a specific path in a QMP dict
+           matches.  When given a list of values, assert that any of
+           them matches.'''
+
         result = self.dictpath(d, path)
-        self.assertEqual(result, value, 'values not equal "%s" and "%s"' % (str(result), str(value)))
+
+        # [] makes no sense as a list of valid values, so treat it as
+        # an actual single value.
+        if isinstance(value, list) and value != []:
+            for v in value:
+                if result == v:
+                    return
+            self.fail('no match for "%s" in %s' % (str(result), str(value)))
+        else:
+            self.assertEqual(result, value,
+                             'values not equal "%s" and "%s"'
+                                 % (str(result), str(value)))
 
     def assert_no_active_block_jobs(self):
         result = self.vm.qmp('query-block-jobs')
-- 
2.21.0

Re: [Qemu-devel] [PATCH 2/4] iotests.py: Let assert_qmp() accept an array

[Alberto Garcia]

On Wed 15 May 2019 10:15:01 PM CEST, Max Reitz wrote:
> Sometimes we cannot tell which error message qemu will emit, and we do
> not care.  With this change, we can then just pass an array of all
> possible messages to assert_qmp() and it will choose the right one.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

[Qemu-devel] [PATCH 3/4] iotests.py: Fix VM.run_job

[Max Reitz]

log() is in the current module, there is no need to prefix it.  In fact,
doing so may make VM.run_job() unusable in tests that never use
iotests.log() themselves.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d96ba1f63c..7bde380d96 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -552,7 +552,7 @@ class VM(qtest.QEMUQtestMachine):
                     elif status == 'null':
                         return error
                 else:
-                    iotests.log(ev)
+                    log(ev)
 
     def node_info(self, node_name):
         nodes = self.qmp('query-named-block-nodes')
-- 
2.21.0

Re: [Qemu-devel] [PATCH 3/4] iotests.py: Fix VM.run_job

[Alberto Garcia]

On Wed 15 May 2019 10:15:02 PM CEST, Max Reitz wrote:
> log() is in the current module, there is no need to prefix it.  In fact,
> doing so may make VM.run_job() unusable in tests that never use
> iotests.log() themselves.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

[Qemu-devel] [PATCH 4/4] iotests: Make 245 faster and more reliable

[Max Reitz]

Sometimes, 245 fails for me because some stream job has already finished
while the test expects it to still be active.  (With -c none, it fails
basically every time.) The most reliable way to fix this is to simply
set auto_finalize=false so the job will remain in the block graph as
long as we need it.  This allows us to drop the rate limiting, too,
which makes the test faster.

The only problem with this is that there is a single place that yields a
different error message depending on whether the stream job is still
copying data (so COR is enabled) or not (COR has been disabled, but the
job still has the WRITE_UNCHANGED permission on the target node).  We
can easily address that by expecting either error message.

Note that we do not need auto_finalize=false (or rate limiting) for the
active commit job, because It never completes without an explicit
block-job-complete anyway.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/245     | 22 ++++++++++++++--------
 tests/qemu-iotests/245.out | 12 ++++++++++++
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a04c6235c1..349b94aace 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -862,7 +862,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
         # hd2 <- hd0
         result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0',
-                             device = 'hd0', base_node = 'hd2', speed = 512 * 1024)
+                             device = 'hd0', base_node = 'hd2',
+                             auto_finalize = False)
         self.assert_qmp(result, 'return', {})
 
         # We can't remove hd2 while the stream job is ongoing
@@ -873,7 +874,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         opts['backing'] = None
         self.reopen(opts, {}, "Cannot change 'backing' link from 'hd0' to 'hd1'")
 
-        self.wait_until_completed(drive = 'stream0')
+        self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True)
 
     # Reopen the chain during a block-stream job (from hd2 to hd1)
     def test_block_stream_4(self):
@@ -886,12 +887,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
         # hd1 <- hd0
         result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0',
-                             device = 'hd1', speed = 512 * 1024)
+                             device = 'hd1', auto_finalize = False)
         self.assert_qmp(result, 'return', {})
 
         # We can't reopen with the original options because that would
         # make hd1 read-only and block-stream requires it to be read-write
-        self.reopen(opts, {}, "Can't set node 'hd1' to r/o with copy-on-read enabled")
+        # (Which error message appears depends on whether the stream job is
+        # already done with copying at this point.)
+        self.reopen(opts, {},
+            ["Can't set node 'hd1' to r/o with copy-on-read enabled",
+             "Cannot make block node read-only, there is a writer on it"])
 
         # We can't remove hd2 while the stream job is ongoing
         opts['backing']['backing'] = None
@@ -901,7 +906,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         opts['backing'] = None
         self.reopen(opts)
 
-        self.wait_until_completed(drive = 'stream0')
+        self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True)
 
     # Reopen the chain during a block-commit job (from hd0 to hd2)
     def test_block_commit_1(self):
@@ -913,7 +918,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-commit', conv_keys = True, job_id = 'commit0',
-                             device = 'hd0', speed = 1024 * 1024)
+                             device = 'hd0')
         self.assert_qmp(result, 'return', {})
 
         # We can't remove hd2 while the commit job is ongoing
@@ -944,7 +949,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-commit', conv_keys = True, job_id = 'commit0',
-                             device = 'hd0', top_node = 'hd1', speed = 1024 * 1024)
+                             device = 'hd0', top_node = 'hd1',
+                             auto_finalize = False)
         self.assert_qmp(result, 'return', {})
 
         # We can't remove hd2 while the commit job is ongoing
@@ -956,7 +962,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file")
 
         # hd2 <- hd0
-        self.wait_until_completed(drive = 'commit0')
+        self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True)
 
         self.assert_qmp(self.get_node('hd0'), 'ro', False)
         self.assertEqual(self.get_node('hd1'), None)
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 71009c239f..a19de5214d 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -3,3 +3,15 @@
 Ran 18 tests
 
 OK
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-- 
2.21.0

Re: [Qemu-devel] [PATCH 4/4] iotests: Make 245 faster and more reliable

[Alberto Garcia]

On Wed 15 May 2019 10:15:03 PM CEST, Max Reitz wrote:
> Sometimes, 245 fails for me because some stream job has already finished
> while the test expects it to still be active.  (With -c none, it fails
> basically every time.) The most reliable way to fix this is to simply
> set auto_finalize=false so the job will remain in the block graph as
> long as we need it.  This allows us to drop the rate limiting, too,
> which makes the test faster.
>
> The only problem with this is that there is a single place that yields a
> different error message depending on whether the stream job is still
> copying data (so COR is enabled) or not (COR has been disabled, but the
> job still has the WRITE_UNCHANGED permission on the target node).  We
> can easily address that by expecting either error message.
>
> Note that we do not need auto_finalize=false (or rate limiting) for the
> active commit job, because It never completes without an explicit
> block-job-complete anyway.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

Re: [Qemu-devel] [PATCH 0/4] iotests: Make 245 faster and more reliable

[Kevin Wolf]

Am 15.05.2019 um 22:14 hat Max Reitz geschrieben:
> 245 is a bit flakey for me, because it uses block jobs that copy 1 MB of
> data but have a buffer size of 512 kB, so they may be done before the
> test gets to do the things it wants to do while the check is running.
> (Rate limiting doesn’t change this.)
> 
> The boring way to fix this would be to increase the amount of data.
> 
> The interesting way to fix this is to make use of auto_finalize=false
> and thus keep the jobs around until the test is done with them.
> However, this has one problem: In one case, 245 tries to make the target
> node of a stream job read-only.  If the job is still copying data, doing
> so will fail because the target node is in COR mode.  Otherwise, we get
> a cryptic “Block node is read-only” message.
> 
> What the message means is “After reopening, the node will be read-only,
> and that won’t work, because there is a writer on it.”  It doesn’t say
> that, though, but it should.  So patch 1 makes it say something to that
> effect (“Cannot make block node read-only, there is a writer on it”).
> 
> 245 doesn’t care about the actual error message, both reflect that qemu
> correctly detects that this node cannot be made read-only at this time.
> So the other thing we have to do is let assert_qmp() accept an array of
> valid error messages and choose the one that matches (if any).  Then we
> can just pass both error messages to it and everything works.
> 
> 
> Nice side effect: For me, the test duration goes down from about 12 s to
> about 6 s.
> (That’s because the test forgot to disable rate limiting on the jobs
> before waiting for their completion.)

Thanks, applied to the block branch.

Kevin