[Qemu-devel] [PATCH 0/7] next round of qemu-iotests fixes

[Qemu-devel] [PATCH 0/7] next round of qemu-iotests fixes

[Sascha Silbe]

With these fixes, qemu-iotests complete successfully on my test
systems (s390x, x86_64) when used with QCOW2 or raw image formats.

These are purely bug fixes for tests and most are trivial, so they
should be safe even for hard freeze.


Sascha Silbe (7):
  qemu-iotests: check: don't place files with predictable names in /tmp
  qemu-iotests: fix 051 on non-PC architectures
  qemu-iotests: iotests.VM: remove qtest socket on error
  qemu-iotests: 148: properly skip test if quorum support is missing
  qemu-iotests: 068: don't require KVM
  qemu-iotests: 141: reduce likelihood of race condition on systems with
    fast IO
  qemu-iotests: iotests.py: get rid of __all__

 tests/qemu-iotests/051.out       | 10 +++++-----
 tests/qemu-iotests/068           |  2 +-
 tests/qemu-iotests/141           | 11 ++++++-----
 tests/qemu-iotests/141.out       | 24 ++++++++++++------------
 tests/qemu-iotests/148           |  4 +---
 tests/qemu-iotests/check         | 21 +++++++++++----------
 tests/qemu-iotests/common.filter |  1 +
 tests/qemu-iotests/iotests.py    | 22 +++++++++++++++++-----
 8 files changed, 54 insertions(+), 41 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/7] qemu-iotests: check: don't place files with predictable names in /tmp

[Sascha Silbe]

Placing files with predictable or even hard-coded names in /tmp is a
security risk and can prevent or disturb operation on a multi-user
machine. Place them inside the "scratch" directory instead, as we
already do for most other test-related files.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/check | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index c350f16..4cba215 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -19,7 +19,6 @@
 # Control script for QA
 #
 
-tmp=/tmp/$$
 status=0
 needwrap=true
 try=0
@@ -130,6 +129,8 @@ fi
 #    exit 1
 #fi
 
+tmp="${TEST_DIR}"/$$
+
 _wallclock()
 {
     date "+%H %M %S" | $AWK_PROG '{ print $1*3600 + $2*60 + $3 }'
@@ -146,8 +147,8 @@ _wrapup()
     # for hangcheck ...
     # remove files that were used by hangcheck
     #
-    [ -f /tmp/check.pid ] && rm -rf /tmp/check.pid
-    [ -f /tmp/check.sts ] && rm -rf /tmp/check.sts
+    [ -f "${TEST_DIR}"/check.pid ] && rm -rf "${TEST_DIR}"/check.pid
+    [ -f "${TEST_DIR}"/check.sts ] && rm -rf "${TEST_DIR}"/check.sts
 
     if $showme
     then
@@ -197,8 +198,8 @@ END        { if (NR > 0) {
         needwrap=false
     fi
 
-    rm -f /tmp/*.out /tmp/*.err /tmp/*.time
-    rm -f /tmp/check.pid /tmp/check.sts
+    rm -f "${TEST_DIR}"/*.out "${TEST_DIR}"/*.err "${TEST_DIR}"/*.time
+    rm -f "${TEST_DIR}"/check.pid "${TEST_DIR}"/check.sts
     rm -f $tmp.*
 }
 
@@ -208,16 +209,16 @@ trap "_wrapup; exit \$status" 0 1 2 3 15
 # Save pid of check in a well known place, so that hangcheck can be sure it
 # has the right pid (getting the pid from ps output is not reliable enough).
 #
-rm -rf /tmp/check.pid
-echo $$ >/tmp/check.pid
+rm -rf "${TEST_DIR}"/check.pid
+echo $$ > "${TEST_DIR}"/check.pid
 
 # for hangcheck ...
 # Save the status of check in a well known place, so that hangcheck can be
 # sure to know where check is up to (getting test number from ps output is
 # not reliable enough since the trace stuff has been introduced).
 #
-rm -rf /tmp/check.sts
-echo "preamble" >/tmp/check.sts
+rm -rf "${TEST_DIR}"/check.sts
+echo "preamble" > "${TEST_DIR}"/check.sts
 
 # don't leave old full output behind on a clean run
 rm -f check.full
@@ -285,7 +286,7 @@ do
         rm -f core $seq.notrun
 
         # for hangcheck ...
-        echo "$seq" >/tmp/check.sts
+        echo "$seq" > "${TEST_DIR}"/check.sts
 
         start=`_wallclock`
         $timestamp && echo -n "        ["`date "+%T"`"]"
-- 
1.9.1

[Qemu-devel] [PATCH 2/7] qemu-iotests: fix 051 on non-PC architectures

[Sascha Silbe]

Commit 61de4c68 [block: Remove BDRV_O_CACHE_WB] updated the reference
output for PCs, but neglected to do the same for the generic reference
output file. Fix 051 on all non-PC architectures by applying the same
change to the generic output file.

Fixes: 61de4c68 ("block: Remove BDRV_O_CACHE_WB")
Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/051.out | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index c1291ff..408d613 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -145,7 +145,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=null-co,cache=invalid_value
 QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -165,7 +165,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -185,7 +185,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -205,8 +205,8 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0: invalid cache option
+Testing: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0: invalid cache option
 
 
 === Specifying the protocol layer ===
-- 
1.9.1

[Qemu-devel] [PATCH 3/7] qemu-iotests: iotests.VM: remove qtest socket on error

[Sascha Silbe]

On error, VM.launch() cleaned up the monitor unix socket, but left the
qtest unix socket behind. This caused the remaining sub-tests to fail
with EADDRINUSE:

+======================================================================
+ERROR: testQuorum (__main__.TestFifoQuorumEvents)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "148", line 63, in setUp
+    self.vm.launch()
+  File "/home6/silbe/qemu/tests/qemu-iotests/iotests.py", line 247, in launch
+    self._qmp.accept()
+  File "/home6/silbe/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 141, in accept
+    return self.__negotiate_capabilities()
+  File "/home6/silbe/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 57, in __negotiate_capabilities
+    raise QMPConnectError
+QMPConnectError
+
+======================================================================
+ERROR: testQuorum (__main__.TestQuorumEvents)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "148", line 63, in setUp
+    self.vm.launch()
+  File "/home6/silbe/qemu/tests/qemu-iotests/iotests.py", line 244, in launch
+    self._qtest = qtest.QEMUQtestProtocol(self._qtest_path, server=True)
+  File "/home6/silbe/qemu/tests/qemu-iotests/../../scripts/qtest.py", line 33, in __init__
+    self._sock.bind(self._address)
+  File "/usr/lib64/python2.7/socket.py", line 224, in meth
+    return getattr(self._sock,name)(*args)
+error: [Errno 98] Address already in use

Fix this by cleaning up both the monitor socket and the qtest socket iff
they exist.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/iotests.py | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8499e1b..fb5c482 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import errno
 import os
 import re
 import subprocess
@@ -247,7 +248,8 @@ class VM(object):
             self._qmp.accept()
             self._qtest.accept()
         except:
-            os.remove(self._monitor_path)
+            _remove_if_exists(self._monitor_path)
+            _remove_if_exists(self._qtest_path)
             raise
 
     def shutdown(self):
@@ -409,6 +411,15 @@ class QMPTestCase(unittest.TestCase):
         event = self.wait_until_completed(drive=drive)
         self.assert_qmp(event, 'data/type', 'mirror')
 
+def _remove_if_exists(path):
+    '''Remove file object at path if it exists'''
+    try:
+        os.remove(path)
+    except OSError as exception:
+        if exception.errno == errno.ENOENT:
+           return
+        raise
+
 def notrun(reason):
     '''Skip this test suite'''
     # Each test in qemu-iotests has a number ("seq")
-- 
1.9.1

[Qemu-devel] [PATCH 4/7] qemu-iotests: 148: properly skip test if quorum support is missing

[Sascha Silbe]

qemu-iotests test case 148 already had some code for skipping the test
if quorum support is missing, but it didn't work in all
cases. TestQuorumEvents.setUp() gets run before the actual test class
(which contains the skipping code) and tries to start qemu with a drive
using the quorum driver. For some reason this works fine when using
qcow2, but fails for raw.

As the entire test case requires quorum, just check for availability
before even starting the test suite. Introduce a verify_quorum()
function in iotests.py for this purpose so future test cases can make
use of it.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/148        | 4 +---
 tests/qemu-iotests/iotests.py | 5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/148 b/tests/qemu-iotests/148
index d066ec3..e01b061 100644
--- a/tests/qemu-iotests/148
+++ b/tests/qemu-iotests/148
@@ -79,9 +79,6 @@ sector = "%d"
                 self.assert_qmp(event, 'data/sector-num', sector)
 
     def testQuorum(self):
-        if not 'quorum' in iotests.qemu_img_pipe('--help'):
-            return
-
         # Generate an error and get an event
         self.vm.hmp_qemu_io("drive0", "aio_read %d %d" %
                             (offset * sector_size, sector_size))
@@ -139,4 +136,5 @@ class TestFifoQuorumEvents(TestQuorumEvents):
     read_pattern = 'fifo'
 
 if __name__ == '__main__':
+    iotests.verify_quorum()
     iotests.main(supported_fmts=["raw"])
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fb5c482..bf31ec8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -437,6 +437,11 @@ def verify_platform(supported_oses=['linux']):
     if True not in [sys.platform.startswith(x) for x in supported_oses]:
         notrun('not suitable for this OS: %s' % sys.platform)
 
+def verify_quorum():
+    '''Skip test suite if quorum support is not available'''
+    if 'quorum' not in qemu_img_pipe('--help'):
+        notrun('quorum support missing')
+
 def main(supported_fmts=[], supported_oses=['linux']):
     '''Run tests'''
 
-- 
1.9.1

[Qemu-devel] [PATCH 5/7] qemu-iotests: 068: don't require KVM

[Sascha Silbe]

None of the other test cases explicitly enable KVM and there's no
obvious reason for 068 to require it. Drop this so all test cases can be
executed in environments where KVM is not available (e.g. because the
user doesn't have sufficient permissions to access /dev/kvm).

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/068 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index 58d1d80..7562dd7 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -53,7 +53,7 @@ _make_test_img $IMG_SIZE
 
 case "$QEMU_DEFAULT_MACHINE" in
   s390-ccw-virtio)
-      platform_parm="-no-shutdown -machine accel=kvm"
+      platform_parm="-no-shutdown"
       ;;
   *)
       platform_parm=""
-- 
1.9.1

[Qemu-devel] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Sascha Silbe]

On systems with fast IO, qemu-io may write more than 1 MiB before
receiving the block-job-cancel command, causing the test case to fail.

141 is inherently racy, but we can at least reduce the likelihood of the
job completing before the cancel command arrives by bumping the size of
the data getting written; we'll try 32 MiB for a start.

Once we actually move enough data around for the block job not to
complete prematurely, the test will still fail because the offset value
in the BLOCK_JOB_CANCELLED event will vary. Since this is more or less
inherent to the nature of this event, we just replace it with a fixed
value globally (in _filter_qmp), the same way we handle timestamps.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/141           | 11 ++++++-----
 tests/qemu-iotests/141.out       | 24 ++++++++++++------------
 tests/qemu-iotests/common.filter |  1 +
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index f7c28b4..a06dc72 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -83,9 +83,10 @@ test_blockjob()
 }
 
 
-TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img 1M
-TEST_IMG="$TEST_DIR/m.$IMGFMT" _make_test_img -b "$TEST_DIR/b.$IMGFMT" 1M
-_make_test_img -b "$TEST_DIR/m.$IMGFMT" 1M
+IMGSIZE=32M
+TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img "${IMGSIZE}"
+TEST_IMG="$TEST_DIR/m.$IMGFMT" _make_test_img -b "$TEST_DIR/b.$IMGFMT" "${IMGSIZE}"
+_make_test_img -b "$TEST_DIR/m.$IMGFMT" "${IMGSIZE}"
 
 _launch_qemu -nodefaults
 
@@ -147,7 +148,7 @@ echo
 # immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
 # fine without the block job still running.
 
-$QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io
+$QEMU_IO -c "write 0 ${IMGSIZE}" "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io
 
 test_blockjob \
     "{'execute': 'block-commit',
@@ -165,7 +166,7 @@ echo
 # immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
 # fine without the block job still running.
 
-$QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
+$QEMU_IO -c "write 0 ${IMGSIZE}" "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
 
 # With some data to stream (and @speed set to 1), block-stream will not complete
 # until we send the block-job-cancel command. Therefore, no event other than
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index adceac1..14e457f 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -1,23 +1,23 @@
 QA output created by 141
-Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/b.IMGFMT
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m.IMGFMT
+Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=33554432
+Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/b.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/m.IMGFMT
 {"return": {}}
 
 === Testing drive-backup ===
 
 {"return": {}}
-Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 33554432, "offset": OFFSET, "speed": 0, "type": "backup"}}
 {"return": {}}
 
 === Testing drive-mirror ===
 
 {"return": {}}
-Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}}
@@ -37,23 +37,23 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
 
 === Testing non-active block-commit ===
 
-wrote 1048576/1048576 bytes at offset 0
-1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 33554432, "offset": OFFSET, "speed": 1, "type": "commit"}}
 {"return": {}}
 
 === Testing block-stream ===
 
-wrote 1048576/1048576 bytes at offset 0
-1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 33554432, "offset": OFFSET, "speed": 1, "type": "stream"}}
 {"return": {}}
 *** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 84b7434..1698d7b 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -173,6 +173,7 @@ _filter_qmp()
 {
     _filter_win32 | \
     sed -e 's#\("\(micro\)\?seconds": \)[0-9]\+#\1 TIMESTAMP#g' \
+        -e '/^{"timestamp": {[^}]*}, "event": "BLOCK_JOB_CANCELLED"/ s#\("offset": \)[0-9]\+#\1 OFFSET#' \
         -e 's#^{"QMP":.*}$#QMP_VERSION#' \
         -e '/^    "QMP": {\s*$/, /^    }\s*$/ c\' \
         -e '    QMP_VERSION'
-- 
1.9.1

[Qemu-devel] [PATCH 7/7] qemu-iotests: iotests.py: get rid of __all__

[Sascha Silbe]

The __all__ list contained a typo for as long as the iotests module
existed. That typo prevented "from iotests import *" (which is the
only case where iotests.__all__ is used at all) from ever working.

The names used by iotests are highly prone to name collisions, so
importing them all unconditionally is a bad idea anyway. Since __all__
is not adding any value, let's just get rid of it.

Fixes: f345cfd0 ("qemu-iotests: add iotests Python module")
Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/iotests.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bf31ec8..0c0b533 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -29,10 +29,6 @@ import qmp
 import qtest
 import struct
 
-__all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
-           'VM', 'QMPTestCase', 'notrun', 'main', 'verify_image_format',
-           'verify_platform', 'filter_test_dir', 'filter_win32',
-           'filter_qemu_io', 'filter_chown', 'log']
 
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
-- 
1.9.1

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qemu-iotests: check: don't place files with predictable names in /tmp

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> Placing files with predictable or even hard-coded names in /tmp is a
> security risk and can prevent or disturb operation on a multi-user
> machine. Place them inside the "scratch" directory instead, as we
> already do for most other test-related files.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/check | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)

Nice side effect: With this patch it's possible to run multiple
instances of the iotests in parallel (for different formats/protocols)
without them interfering with each other.

Grepping for '/tmp' in the iotests directory yields more occurrences,
however: Many tests set the tmp variable to /tmp/$$. Let's see whether
we can just remove that or have to replace it by "${TEST_DIR}"/$$.

"common.filter" evaluates $tmp, but the single filter that does so is
actually never used any more. Other than that, only "common" evaluates
it, but "common" is sourced by "check". Thus I think those tests setting
$tmp is superfluous and dropping it should be fine.

For this patch:

Reviewed-by: Max Reitz <mreitz@redhat.com>

You decide whether you want to drop the tmp=/tmp/$$ lines in the tests
in a dedicated (follow-up) patch or include it here.

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] qemu-iotests: fix 051 on non-PC architectures

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> Commit 61de4c68 [block: Remove BDRV_O_CACHE_WB] updated the reference
> output for PCs, but neglected to do the same for the generic reference
> output file. Fix 051 on all non-PC architectures by applying the same
> change to the generic output file.
> 
> Fixes: 61de4c68 ("block: Remove BDRV_O_CACHE_WB")
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/051.out | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Well, that's interesting. I did note that in
http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg06123.html
and thus didn't give an R-b. But in v2,
(http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg06353.html)
my R-b magically had appeared there without the patch being changed. I
guess I need to be more careful with reviewing revised patch series from
now on, whoever they are from. :-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] qemu-iotests: iotests.VM: remove qtest socket on error

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> On error, VM.launch() cleaned up the monitor unix socket, but left the
> qtest unix socket behind. This caused the remaining sub-tests to fail
> with EADDRINUSE:
> 
> +======================================================================
> +ERROR: testQuorum (__main__.TestFifoQuorumEvents)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "148", line 63, in setUp
> +    self.vm.launch()
> +  File "/home6/silbe/qemu/tests/qemu-iotests/iotests.py", line 247, in launch
> +    self._qmp.accept()
> +  File "/home6/silbe/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 141, in accept
> +    return self.__negotiate_capabilities()
> +  File "/home6/silbe/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 57, in __negotiate_capabilities
> +    raise QMPConnectError
> +QMPConnectError
> +
> +======================================================================
> +ERROR: testQuorum (__main__.TestQuorumEvents)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "148", line 63, in setUp
> +    self.vm.launch()
> +  File "/home6/silbe/qemu/tests/qemu-iotests/iotests.py", line 244, in launch
> +    self._qtest = qtest.QEMUQtestProtocol(self._qtest_path, server=True)
> +  File "/home6/silbe/qemu/tests/qemu-iotests/../../scripts/qtest.py", line 33, in __init__
> +    self._sock.bind(self._address)
> +  File "/usr/lib64/python2.7/socket.py", line 224, in meth
> +    return getattr(self._sock,name)(*args)
> +error: [Errno 98] Address already in use
> 
> Fix this by cleaning up both the monitor socket and the qtest socket iff
> they exist.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/iotests.py | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Not sure about the leading underscore (it appears to be the only such
function in iotests.py besides __init__()), but I guess it's a test so
it doesn't really matter anyway.

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] qemu-iotests: 148: properly skip test if quorum support is missing

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> qemu-iotests test case 148 already had some code for skipping the test
> if quorum support is missing, but it didn't work in all
> cases. TestQuorumEvents.setUp() gets run before the actual test class
> (which contains the skipping code) and tries to start qemu with a drive
> using the quorum driver. For some reason this works fine when using
> qcow2, but fails for raw.
> 
> As the entire test case requires quorum, just check for availability
> before even starting the test suite. Introduce a verify_quorum()
> function in iotests.py for this purpose so future test cases can make
> use of it.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/148        | 4 +---
>  tests/qemu-iotests/iotests.py | 5 +++++
>  2 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] qemu-iotests: 068: don't require KVM

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> None of the other test cases explicitly enable KVM and there's no
> obvious reason for 068 to require it. Drop this so all test cases can be
> executed in environments where KVM is not available (e.g. because the
> user doesn't have sufficient permissions to access /dev/kvm).
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/068 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> On systems with fast IO, qemu-io may write more than 1 MiB before
> receiving the block-job-cancel command, causing the test case to fail.
> 
> 141 is inherently racy, but we can at least reduce the likelihood of the
> job completing before the cancel command arrives by bumping the size of
> the data getting written; we'll try 32 MiB for a start.

Hm, interesting. I tried to prevent this by setting the block jobs'
speed to 1, which should make it stop after the block job has processed
the first block of data.

I won't oppose this patch, because if it fixes things for you, that's
good. But I don't think it should be necessary.

Max

> 
> Once we actually move enough data around for the block job not to
> complete prematurely, the test will still fail because the offset value
> in the BLOCK_JOB_CANCELLED event will vary. Since this is more or less
> inherent to the nature of this event, we just replace it with a fixed
> value globally (in _filter_qmp), the same way we handle timestamps.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/141           | 11 ++++++-----
>  tests/qemu-iotests/141.out       | 24 ++++++++++++------------
>  tests/qemu-iotests/common.filter |  1 +
>  3 files changed, 19 insertions(+), 17 deletions(-)


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

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] qemu-iotests: iotests.py: get rid of __all__

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> The __all__ list contained a typo for as long as the iotests module
> existed. That typo prevented "from iotests import *" (which is the
> only case where iotests.__all__ is used at all) from ever working.

Hahaha :D

Nice one.

> The names used by iotests are highly prone to name collisions, so
> importing them all unconditionally is a bad idea anyway. Since __all__
> is not adding any value, let's just get rid of it.
> 
> Fixes: f345cfd0 ("qemu-iotests: add iotests Python module")
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/iotests.py | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Kevin Wolf]

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

Am 06.04.2016 um 18:15 hat Max Reitz geschrieben:
> On 05.04.2016 11:21, Sascha Silbe wrote:
> > On systems with fast IO, qemu-io may write more than 1 MiB before
> > receiving the block-job-cancel command, causing the test case to fail.
> > 
> > 141 is inherently racy, but we can at least reduce the likelihood of the
> > job completing before the cancel command arrives by bumping the size of
> > the data getting written; we'll try 32 MiB for a start.
> 
> Hm, interesting. I tried to prevent this by setting the block jobs'
> speed to 1, which should make it stop after the block job has processed
> the first block of data.
> 
> I won't oppose this patch, because if it fixes things for you, that's
> good. But I don't think it should be necessary.

We don't generally change test cases when they fail. Making a test case
pass doesn't "fix things" per se. It only helps when the failure is a
false positive.

In this case, it looks like there might be a problem with block job
throttling, so maybe we should look into that before changing the test?

Kevin

> > Once we actually move enough data around for the block job not to
> > complete prematurely, the test will still fail because the offset value
> > in the BLOCK_JOB_CANCELLED event will vary. Since this is more or less
> > inherent to the nature of this event, we just replace it with a fixed
> > value globally (in _filter_qmp), the same way we handle timestamps.
> > 
> > Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> > Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> > ---
> >  tests/qemu-iotests/141           | 11 ++++++-----
> >  tests/qemu-iotests/141.out       | 24 ++++++++++++------------
> >  tests/qemu-iotests/common.filter |  1 +
> >  3 files changed, 19 insertions(+), 17 deletions(-)
> 




[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qemu-iotests: check: don't place files with predictable names in /tmp

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

> You decide whether you want to drop the tmp=/tmp/$$ lines in the tests
> in a dedicated (follow-up) patch or include it here.

Let's land this one first; I'll address the other occurrences in a
separate patch. I've put it on my todo list.

Thanks for the reviews!

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] qemu-iotests: iotests.VM: remove qtest socket on error

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

> On 05.04.2016 11:21, Sascha Silbe wrote:
[...]
[_remove_if_exists()]
> Not sure about the leading underscore (it appears to be the only such
> function in iotests.py besides __init__()), but I guess it's a test so
> it doesn't really matter anyway.

I considered it an internal implementation detail of VM.launch(), hence
the underscore. Thinking about it some more, it may be a useful helper
for the tests themselves. But we can easily promote it whenever one of
the tests actually wants to make use of it.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

> On 05.04.2016 11:21, Sascha Silbe wrote:
>> On systems with fast IO, qemu-io may write more than 1 MiB before
>> receiving the block-job-cancel command, causing the test case to fail.
>> 
>> 141 is inherently racy, but we can at least reduce the likelihood of the
>> job completing before the cancel command arrives by bumping the size of
>> the data getting written; we'll try 32 MiB for a start.
>
> Hm, interesting. I tried to prevent this by setting the block jobs'
> speed to 1, which should make it stop after the block job has processed
> the first block of data.

Hmm, seems like I didn't quite understand the way the test works
then.

@Kevin: Please do NOT queue this patch.

@Max: From a cursory glance at the code, maybe your 1 *byte* per second
rate limit is being rounded down to 0 *blocks* per second, with 0
meaning no limit? See e.g. mirror_set_speed(). Though I must admit I
don't understand how speed=0 translates to unlimited (like
qapi/block-core.json:block-job-set-speed says). My understanding of
ratelimit_calculate_delay() is that speed=0 means "1 quantum per time
slice", with time slice usually being 100ms; not sure about the
quantum.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Kevin Wolf]

Am 07.04.2016 um 22:27 hat Sascha Silbe geschrieben:
> Dear Max,
> 
> Max Reitz <mreitz@redhat.com> writes:
> 
> > On 05.04.2016 11:21, Sascha Silbe wrote:
> >> On systems with fast IO, qemu-io may write more than 1 MiB before
> >> receiving the block-job-cancel command, causing the test case to fail.
> >> 
> >> 141 is inherently racy, but we can at least reduce the likelihood of the
> >> job completing before the cancel command arrives by bumping the size of
> >> the data getting written; we'll try 32 MiB for a start.
> >
> > Hm, interesting. I tried to prevent this by setting the block jobs'
> > speed to 1, which should make it stop after the block job has processed
> > the first block of data.
> 
> Hmm, seems like I didn't quite understand the way the test works
> then.
> 
> @Kevin: Please do NOT queue this patch.

No problem. I expect this series to go through Max's tree anyway.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Sascha Silbe]

Dear Max,

Sascha Silbe <silbe@linux.vnet.ibm.com> writes:

> @Max: From a cursory glance at the code, maybe your 1 *byte* per second
> rate limit is being rounded down to 0 *blocks* per second, with 0
> meaning no limit? See e.g. mirror_set_speed(). Though I must admit I
> don't understand how speed=0 translates to unlimited (like
> qapi/block-core.json:block-job-set-speed says). My understanding of
> ratelimit_calculate_delay() is that speed=0 means "1 quantum per time
> slice", with time slice usually being 100ms; not sure about the
> quantum.

I think I've understood the issue now.

The backup, commit, mirror and stream actions operate in on full chunks,
with chunk size depending on the action and backing device. For
e.g. commit that means it always bursts at least 0.5MiB; that's where
the value the reference output comes from.

ratelimit_calculate_delay() lets through at least one burst per time
slice. This means the minimum rate is chunk size per time slice (always
100ms). So for commit and stream one will always get at least 5 MiB/s. A
surprisingly large value for something specified in bytes per second,
BTW. (I.e. it should probably be documented in qmp-commands.hx if it
stays this way).

On a busy or slow host, it may take the shell longer than the time slice
of 100ms to send the cancel command to qemu. When that happens,
additional chunks will get written before the job gets cancelled. That's
why I sometimes see 1 or even 1.5 MiB as offset, especially when running
CPU intensive workloads in parallel.

The best approach probably would be to fix up the rate limit code to
delay for multiple time slices if necessary. We should get rid of the
artificial BDRV_SECTOR_SIZE granularity at the same time, always using
bytes as the quantum unit.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Kevin Wolf]

Am 08.04.2016 um 14:01 hat Sascha Silbe geschrieben:
> Dear Max,
> 
> Sascha Silbe <silbe@linux.vnet.ibm.com> writes:
> 
> > @Max: From a cursory glance at the code, maybe your 1 *byte* per second
> > rate limit is being rounded down to 0 *blocks* per second, with 0
> > meaning no limit? See e.g. mirror_set_speed(). Though I must admit I
> > don't understand how speed=0 translates to unlimited (like
> > qapi/block-core.json:block-job-set-speed says). My understanding of
> > ratelimit_calculate_delay() is that speed=0 means "1 quantum per time
> > slice", with time slice usually being 100ms; not sure about the
> > quantum.
> 
> I think I've understood the issue now.
> 
> The backup, commit, mirror and stream actions operate in on full chunks,
> with chunk size depending on the action and backing device. For
> e.g. commit that means it always bursts at least 0.5MiB; that's where
> the value the reference output comes from.
> 
> ratelimit_calculate_delay() lets through at least one burst per time
> slice. This means the minimum rate is chunk size per time slice (always
> 100ms). So for commit and stream one will always get at least 5 MiB/s. A
> surprisingly large value for something specified in bytes per second,
> BTW. (I.e. it should probably be documented in qmp-commands.hx if it
> stays this way).
> 
> On a busy or slow host, it may take the shell longer than the time slice
> of 100ms to send the cancel command to qemu. When that happens,
> additional chunks will get written before the job gets cancelled. That's
> why I sometimes see 1 or even 1.5 MiB as offset, especially when running
> CPU intensive workloads in parallel.
> 
> The best approach probably would be to fix up the rate limit code to
> delay for multiple time slices if necessary. We should get rid of the
> artificial BDRV_SECTOR_SIZE granularity at the same time, always using
> bytes as the quantum unit.

In the 2.7 time frame we might actually be able to reuse the normal I/O
throttling code for block jobs as the jobs will be using their own
BlockBackend and can therefore set their own throttling limits.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO

[Sascha Silbe]

Dear Kevin,

Kevin Wolf <kwolf@redhat.com> writes:

> Am 08.04.2016 um 14:01 hat Sascha Silbe geschrieben:
[...]
>> The best approach probably would be to fix up the rate limit code to
>> delay for multiple time slices if necessary. We should get rid of the
>> artificial BDRV_SECTOR_SIZE granularity at the same time, always using
>> bytes as the quantum unit.
>
> In the 2.7 time frame we might actually be able to reuse the normal I/O
> throttling code for block jobs as the jobs will be using their own
> BlockBackend and can therefore set their own throttling limits.

Sounds good. Then I suggest we just drop this patch for now. I've added
a reminder to check the situation again at the end of June (i.e. before
soft feature freeze of 2.7).

Will give documenting the minimum rate a try, but that shouldn't hold up
the fixes.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [Qemu-block] [PATCH 0/7] next round of qemu-iotests fixes

[Max Reitz]

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

On 05.04.2016 11:21, Sascha Silbe wrote:
> With these fixes, qemu-iotests complete successfully on my test
> systems (s390x, x86_64) when used with QCOW2 or raw image formats.
> 
> These are purely bug fixes for tests and most are trivial, so they
> should be safe even for hard freeze.

Thanks, applied everything except for patch 6 to my block tree:

https://github.com/XanClic/qemu/commits/block

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/7] next round of qemu-iotests fixes

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

> On 05.04.2016 11:21, Sascha Silbe wrote:
>> With these fixes, qemu-iotests complete successfully on my test
>> systems (s390x, x86_64) when used with QCOW2 or raw image formats.
>> 
>> These are purely bug fixes for tests and most are trivial, so they
>> should be safe even for hard freeze.
>
> Thanks, applied everything except for patch 6 to my block tree:
>
> https://github.com/XanClic/qemu/commits/block

Thanks!

I've cooked up patches for the remaining /tmp usage and documenting the
minimum block speed speed; will send them out next week.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641