[Qemu-devel] [PATCH v4 0/5] This series adds iotest case for IO throttling.

[Qemu-devel] [PATCH v4 0/5] This series adds iotest case for IO throttling.

[Fam Zheng]

There is a change in qemu-io sub-commands "aio_read" and "aio_write", which
makes the aio requests accounted and the statistics reflected in blockstats.

Note that IO throttling implementation allows overcommiting of requests, so the
actual IO happened in a time unit may be a bit larger than given limits. In the
test case, the stats numbers are compared against 110%, to make room for such
flexibility in order to improve determinism.

v4: Rebase to master. Add Benoit's rev-by lines to all the patches.

Fam

Fam Zheng (5):
  qemu-io: Account IO by aio_read and aio_write
  qtest: Add scripts/qtest/qtest.py
  qemu-iotests: Add VM method qtest() to iotests.py
  qemu-iotests: Allow caller to disable underscore convertion for qmp
  qemu-iotests: Add 093 for IO throttling

 qemu-io-cmds.c                |  10 +++
 scripts/qtest                 |   5 --
 scripts/qtest/qtest.py        |  74 +++++++++++++++++++
 tests/qemu-iotests/093        | 164 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out    |   5 ++
 tests/qemu-iotests/iotests.py |  24 +++++--
 6 files changed, 273 insertions(+), 9 deletions(-)
 delete mode 100755 scripts/qtest
 create mode 100644 scripts/qtest/qtest.py
 create mode 100755 tests/qemu-iotests/093
 create mode 100644 tests/qemu-iotests/093.out

-- 
2.0.0

[Qemu-devel] [PATCH v4 1/5] qemu-io: Account IO by aio_read and aio_write

[Fam Zheng]

This will enable accounting of aio requests issued from qemu-io aio
read/write commands.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 qemu-io-cmds.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 60c1ceb..ae6c299 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1346,6 +1346,7 @@ out:
 }
 
 struct aio_ctx {
+    BlockDriverState *bs;
     QEMUIOVector qiov;
     int64_t offset;
     char *buf;
@@ -1353,6 +1354,7 @@ struct aio_ctx {
     int vflag;
     int Cflag;
     int Pflag;
+    BlockAcctCookie acct;
     int pattern;
     struct timeval t1;
 };
@@ -1370,6 +1372,8 @@ static void aio_write_done(void *opaque, int ret)
         goto out;
     }
 
+    bdrv_acct_done(ctx->bs, &ctx->acct);
+
     if (ctx->qflag) {
         goto out;
     }
@@ -1407,6 +1411,8 @@ static void aio_read_done(void *opaque, int ret)
         g_free(cmp_buf);
     }
 
+    bdrv_acct_done(ctx->bs, &ctx->acct);
+
     if (ctx->qflag) {
         goto out;
     }
@@ -1462,6 +1468,7 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
     int nr_iov, c;
     struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
 
+    ctx->bs = bs;
     while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
         switch (c) {
         case 'C':
@@ -1515,6 +1522,7 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
     }
 
     gettimeofday(&ctx->t1, NULL);
+    bdrv_acct_start(bs, &ctx->acct, ctx->qiov.size, BDRV_ACCT_READ);
     bdrv_aio_readv(bs, ctx->offset >> 9, &ctx->qiov,
                    ctx->qiov.size >> 9, aio_read_done, ctx);
     return 0;
@@ -1558,6 +1566,7 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
     int pattern = 0xcd;
     struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
 
+    ctx->bs = bs;
     while ((c = getopt(argc, argv, "CqP:")) != EOF) {
         switch (c) {
         case 'C':
@@ -1607,6 +1616,7 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
     }
 
     gettimeofday(&ctx->t1, NULL);
+    bdrv_acct_start(bs, &ctx->acct, ctx->qiov.size, BDRV_ACCT_WRITE);
     bdrv_aio_writev(bs, ctx->offset >> 9, &ctx->qiov,
                     ctx->qiov.size >> 9, aio_write_done, ctx);
     return 0;
-- 
2.0.0

[Qemu-devel] [PATCH v4 2/5] qtest: Add scripts/qtest/qtest.py

[Fam Zheng]

This removes the dummy scripts/qtest and adds scripts/qtest/qtest.py as
a python library for qtest protocol.

This is a skeleton with a basic "cmd" method to execute a command,
reading and parsing of qtest output will be added later on demand.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 scripts/qtest          |  5 ----
 scripts/qtest/qtest.py | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 5 deletions(-)
 delete mode 100755 scripts/qtest
 create mode 100644 scripts/qtest/qtest.py

diff --git a/scripts/qtest b/scripts/qtest
deleted file mode 100755
index 4ef6c1c..0000000
--- a/scripts/qtest
+++ /dev/null
@@ -1,5 +0,0 @@
-#!/bin/sh
-
-export QTEST_QEMU_BINARY=$1
-shift
-"$@"
diff --git a/scripts/qtest/qtest.py b/scripts/qtest/qtest.py
new file mode 100644
index 0000000..16c6713
--- /dev/null
+++ b/scripts/qtest/qtest.py
@@ -0,0 +1,74 @@
+# QEMU qtest library
+#
+# Copyright (C) 2014 Red Hat Inc.
+#
+# Authors:
+#  Fam Zheng <famz@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+#
+# Based on qmp.py.
+#
+
+import errno
+import socket
+
+class QEMUQtestProtocol:
+    def __init__(self, address, server=False):
+        """
+        Create a QEMUQtestProtocol object.
+
+        @param address: QEMU address, can be either a unix socket path (string)
+                        or a tuple in the form ( address, port ) for a TCP
+                        connection
+        @param server: server mode listens on the socket (bool)
+        @raise socket.error on socket connection errors
+        @note No connection is established, this is done by the connect() or
+              accept() methods
+        """
+        self.__address = address
+        self.__sock = self.__get_sock()
+        if server:
+            self.__sock.bind(self.__address)
+            self.__sock.listen(1)
+
+    def __get_sock(self):
+        if isinstance(self.__address, tuple):
+            family = socket.AF_INET
+        else:
+            family = socket.AF_UNIX
+        return socket.socket(family, socket.SOCK_STREAM)
+
+    def connect(self):
+        """
+        Connect to the qtest socket.
+
+        @raise socket.error on socket connection errors
+        """
+        self.__sock.connect(self.__address)
+        self.__sockfile = self.__sock.makefile()
+
+    def accept(self):
+        """
+        Await connection from QEMU.
+
+        @raise socket.error on socket connection errors
+        """
+        self.__sock, _ = self.__sock.accept()
+        self.__sockfile = self.__sock.makefile()
+
+    def cmd(self, qtest_cmd):
+        """
+        Send a qtest command on the wire.
+
+        @param qtest_cmd: qtest command text to be sent
+        """
+        self.__sock.sendall(qtest_cmd + "\n")
+
+    def close(self):
+        self.__sockfile.close()
+        self.__sock.close()
+
+    def settimeout(self, timeout):
+        self.__sock.settimeout(timeout)
-- 
2.0.0

[Qemu-devel] [PATCH v4 3/5] qemu-iotests: Add VM method qtest() to iotests.py

[Fam Zheng]

This will allow test case to run command in qtest protocol. It's
write-only for now.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 tests/qemu-iotests/iotests.py | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f6c437c..914b4d3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -21,9 +21,13 @@ import re
 import subprocess
 import string
 import unittest
-import sys; sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts', 'qmp'))
+import sys
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts', 'qmp'))
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts', 'qtest'))
 import qmp
+import qtest
 import struct
+import socket
 
 __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
            'VM', 'QMPTestCase', 'notrun', 'main']
@@ -80,10 +84,12 @@ class VM(object):
     def __init__(self):
         self._monitor_path = os.path.join(test_dir, 'qemu-mon.%d' % os.getpid())
         self._qemu_log_path = os.path.join(test_dir, 'qemu-log.%d' % os.getpid())
+        self._qtest_path = os.path.join(test_dir, 'qemu-qtest.%d' % os.getpid())
         self._args = qemu_args + ['-chardev',
                      'socket,id=mon,path=' + self._monitor_path,
                      '-mon', 'chardev=mon,mode=control',
-                     '-qtest', 'stdio', '-machine', 'accel=qtest',
+                     '-qtest', 'unix:path=' + self._qtest_path,
+                     '-machine', 'accel=qtest',
                      '-display', 'none', '-vga', 'none']
         self._num_drives = 0
 
@@ -159,9 +165,11 @@ class VM(object):
         qemulog = open(self._qemu_log_path, 'wb')
         try:
             self._qmp = qmp.QEMUMonitorProtocol(self._monitor_path, server=True)
+            self._qtest = qtest.QEMUQtestProtocol(self._qtest_path, server=True)
             self._popen = subprocess.Popen(self._args, stdin=devnull, stdout=qemulog,
                                            stderr=subprocess.STDOUT)
             self._qmp.accept()
+            self._qtest.accept()
         except:
             os.remove(self._monitor_path)
             raise
@@ -172,6 +180,7 @@ class VM(object):
             self._qmp.cmd('quit')
             self._popen.wait()
             os.remove(self._monitor_path)
+            os.remove(self._qtest_path)
             os.remove(self._qemu_log_path)
             self._popen = None
 
@@ -184,6 +193,10 @@ class VM(object):
 
         return self._qmp.cmd(cmd, args=qmp_args)
 
+    def qtest(self, cmd):
+        '''Send a qtest command to guest'''
+        return self._qtest.cmd(cmd)
+
     def get_qmp_event(self, wait=False):
         '''Poll for one queued QMP events and return it'''
         return self._qmp.pull_event(wait=wait)
-- 
2.0.0

[Qemu-devel] [PATCH v4 4/5] qemu-iotests: Allow caller to disable underscore convertion for qmp

[Fam Zheng]

QMP command "block_set_io_throttle" expects underscores in parameters
instead of dashes: {iops,bps}_{rd,wr,max}.

Add optional argument conv_keys (defaults to True, backward compatible),
it will be used in IO throttling test case.

Reviewed-by: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 914b4d3..0b696fb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -185,11 +185,14 @@ class VM(object):
             self._popen = None
 
     underscore_to_dash = string.maketrans('_', '-')
-    def qmp(self, cmd, **args):
+    def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the result dict'''
         qmp_args = dict()
         for k in args.keys():
-            qmp_args[k.translate(self.underscore_to_dash)] = args[k]
+            if conv_keys:
+                qmp_args[k.translate(self.underscore_to_dash)] = args[k]
+            else:
+                qmp_args[k] = args[k]
 
         return self._qmp.cmd(cmd, args=qmp_args)
 
-- 
2.0.0

[Qemu-devel] [PATCH v4 5/5] qemu-iotests: Add 093 for IO throttling

[Fam Zheng]

This case utilizes qemu-io command "aio_{read,write} -a -q" to verify
the effectiveness of IO throttling options.

The "-a" option will cause qemu-io requests to be accounted.

It's implemented by driving the vm timer from qtest protocol, so the
throttling timers are signaled with determinied time duration. Then we
verify the completed IO requests are within 110% of bps and iops limits.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 tests/qemu-iotests/093     | 164 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out |   5 ++
 2 files changed, 169 insertions(+)
 create mode 100755 tests/qemu-iotests/093
 create mode 100644 tests/qemu-iotests/093.out

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
new file mode 100755
index 0000000..222bb37
--- /dev/null
+++ b/tests/qemu-iotests/093
@@ -0,0 +1,164 @@
+#!/usr/bin/env python
+#
+# Tests for IO throttling
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+
+class ThrottleTestCase(iotests.QMPTestCase):
+    image_len = 80 * 1024 * 1024 # MB
+
+    def blockstats(self, device):
+        result = self.vm.qmp("query-blockstats")
+        for r in result['return']:
+            if r['device'] == device:
+                stat = r['stats']
+                return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
+        raise Exception("Device not found for blockstats: %s" % device)
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img, "1G")
+        #self.vm = iotests.VM().add_drive(test_img, "bps=1024,bps_max=1")
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+
+    def do_test_throttle(self, seconds=10, **limits):
+        def check_limit(limit, num):
+            # IO throttling algorithm is discrete, allow 10% error so the test
+            # is more deterministic
+            return limit == 0 or num < seconds * limit * 1.1
+
+        nsec_per_sec = 1000000000
+
+        limits['bps_max'] = 1
+        limits['iops_max'] = 1
+
+        # Enqueue many requests to throttling queue
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **limits)
+        self.assert_qmp(result, 'return', {})
+
+        # Set vm clock to a known value
+        ns = nsec_per_sec
+        self.vm.qtest_cmd("clock_step %d" % ns)
+
+        # Append many requests into the throttle queue
+        # They drain bps_max and iops_max
+        # The rest requests won't get executed until qtest clock is driven
+        for i in range(1000):
+            self.vm.hmp_qemu_io("drive0", "aio_read -a -q 0 512")
+            self.vm.hmp_qemu_io("drive0", "aio_write -a -q 0 512")
+
+        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
+
+        ns += seconds * nsec_per_sec
+        self.vm.qtest_cmd("clock_step %d" % ns)
+        # wait for a while to let requests take off
+        time.sleep(1)
+        end_rd_bytes, end_rd_iops, end_wr_bytes, end_wr_iops = self.blockstats('drive0')
+
+        rd_bytes = end_rd_bytes - start_rd_bytes
+        rd_iops = end_rd_iops - start_rd_iops
+        wr_bytes = end_wr_bytes - start_wr_bytes
+        wr_iops = end_wr_iops - start_wr_iops
+
+        assert check_limit(limits['bps'], rd_bytes)
+        assert check_limit(limits['bps_rd'], rd_bytes)
+        assert check_limit(limits['bps'], wr_bytes)
+        assert check_limit(limits['bps_wr'], wr_bytes)
+        assert check_limit(limits['iops'], rd_iops)
+        assert check_limit(limits['iops_rd'], rd_iops)
+        assert check_limit(limits['iops'], wr_iops)
+        assert check_limit(limits['iops_wr'], wr_iops)
+
+    def test_bps(self):
+        self.do_test_throttle(**{
+            'device': 'drive0',
+            'bps': 1000,
+            'bps_rd': 0,
+            'bps_wr': 0,
+            'iops': 0,
+            'iops_rd': 0,
+            'iops_wr': 0,
+            })
+
+    def test_bps_rd(self):
+        self.do_test_throttle(**{
+            'device': 'drive0',
+            'bps': 0,
+            'bps_rd': 1000,
+            'bps_wr': 0,
+            'iops': 0,
+            'iops_rd': 0,
+            'iops_wr': 0,
+            })
+
+    def test_bps_wr(self):
+        self.do_test_throttle(**{
+            'device': 'drive0',
+            'bps': 0,
+            'bps_rd': 0,
+            'bps_wr': 1000,
+            'iops': 0,
+            'iops_rd': 0,
+            'iops_wr': 0,
+            })
+
+    def test_iops(self):
+        self.do_test_throttle(**{
+            'device': 'drive0',
+            'bps': 0,
+            'bps_rd': 0,
+            'bps_wr': 0,
+            'iops': 10,
+            'iops_rd': 0,
+            'iops_wr': 0,
+            })
+
+    def test_iops_rd(self):
+        self.do_test_throttle(**{
+            'device': 'drive0',
+            'bps': 0,
+            'bps_rd': 0,
+            'bps_wr': 0,
+            'iops': 0,
+            'iops_rd': 10,
+            'iops_wr': 0,
+            })
+
+    def test_iops_wr(self):
+        self.do_test_throttle(**{
+            'device': 'drive0',
+            'bps': 0,
+            'bps_rd': 0,
+            'bps_wr': 0,
+            'iops': 0,
+            'iops_rd': 0,
+            'iops_wr': 10,
+            })
+
+if __name__ == '__main__':
+    iotests.main()
diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out
new file mode 100644
index 0000000..3f8a935
--- /dev/null
+++ b/tests/qemu-iotests/093.out
@@ -0,0 +1,5 @@
+......
+----------------------------------------------------------------------
+Ran 6 tests
+
+OK
-- 
2.0.0

Re: [Qemu-devel] [PATCH v4 0/5] This series adds iotest case for IO throttling.

[Fam Zheng]

On Thu, 06/05 16:47, Fam Zheng wrote:
> There is a change in qemu-io sub-commands "aio_read" and "aio_write", which
> makes the aio requests accounted and the statistics reflected in blockstats.
> 
> Note that IO throttling implementation allows overcommiting of requests, so the
> actual IO happened in a time unit may be a bit larger than given limits. In the
> test case, the stats numbers are compared against 110%, to make room for such
> flexibility in order to improve determinism.
> 
> v4: Rebase to master. Add Benoit's rev-by lines to all the patches.

Ping?

Thanks,
Fam

> 
> Fam
> 
> Fam Zheng (5):
>   qemu-io: Account IO by aio_read and aio_write
>   qtest: Add scripts/qtest/qtest.py
>   qemu-iotests: Add VM method qtest() to iotests.py
>   qemu-iotests: Allow caller to disable underscore convertion for qmp
>   qemu-iotests: Add 093 for IO throttling
> 
>  qemu-io-cmds.c                |  10 +++
>  scripts/qtest                 |   5 --
>  scripts/qtest/qtest.py        |  74 +++++++++++++++++++
>  tests/qemu-iotests/093        | 164 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/093.out    |   5 ++
>  tests/qemu-iotests/iotests.py |  24 +++++--
>  6 files changed, 273 insertions(+), 9 deletions(-)
>  delete mode 100755 scripts/qtest
>  create mode 100644 scripts/qtest/qtest.py
>  create mode 100755 tests/qemu-iotests/093
>  create mode 100644 tests/qemu-iotests/093.out
> 
> -- 
> 2.0.0
> 
> 

Re: [Qemu-devel] [PATCH v4 0/5] This series adds iotest case for IO throttling.

[Fam Zheng]

On Tue, 06/17 10:45, Fam Zheng wrote:
> On Thu, 06/05 16:47, Fam Zheng wrote:
> > There is a change in qemu-io sub-commands "aio_read" and "aio_write", which
> > makes the aio requests accounted and the statistics reflected in blockstats.
> > 
> > Note that IO throttling implementation allows overcommiting of requests, so the
> > actual IO happened in a time unit may be a bit larger than given limits. In the
> > test case, the stats numbers are compared against 110%, to make room for such
> > flexibility in order to improve determinism.
> > 
> > v4: Rebase to master. Add Benoit's rev-by lines to all the patches.
> 
> Ping?

Ping.

Re: [Qemu-devel] [PATCH v4 2/5] qtest: Add scripts/qtest/qtest.py

[Stefan Hajnoczi]

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

On Thu, Jun 05, 2014 at 04:47:43PM +0800, Fam Zheng wrote:
> diff --git a/scripts/qtest b/scripts/qtest
> deleted file mode 100755
> index 4ef6c1c..0000000
> --- a/scripts/qtest
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -#!/bin/sh
> -
> -export QTEST_QEMU_BINARY=$1
> -shift
> -"$@"

Why delete this?  The script is unrelated to qtest.py.

I think nothing uses scripts/qtest so it could be deleted in a separate
commit if you feel that is worthwhile.

> +import errno
> +import socket
> +
> +class QEMUQtestProtocol:

Using new-style classes is probably a good idea:

  class QEMUQtestProtocol(object):

https://docs.python.org/2.7/reference/datamodel.html#newstyle

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

Re: [Qemu-devel] [PATCH v4 5/5] qemu-iotests: Add 093 for IO throttling

[Stefan Hajnoczi]

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

On Thu, Jun 05, 2014 at 04:47:46PM +0800, Fam Zheng wrote:
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, test_img, "1G")
> +        #self.vm = iotests.VM().add_drive(test_img, "bps=1024,bps_max=1")

Commented out lines should be dropped

> +        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(test_img)
> +
> +    def do_test_throttle(self, seconds=10, **limits):
> +        def check_limit(limit, num):
> +            # IO throttling algorithm is discrete, allow 10% error so the test
> +            # is more deterministic
> +            return limit == 0 or num < seconds * limit * 1.1
> +
> +        nsec_per_sec = 1000000000
> +
> +        limits['bps_max'] = 1
> +        limits['iops_max'] = 1
> +
> +        # Enqueue many requests to throttling queue

This comment is wrong, it actually happens further down

> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **limits)
> +        self.assert_qmp(result, 'return', {})
> +
> +        # Set vm clock to a known value
> +        ns = nsec_per_sec
> +        self.vm.qtest_cmd("clock_step %d" % ns)
> +
> +        # Append many requests into the throttle queue
> +        # They drain bps_max and iops_max
> +        # The rest requests won't get executed until qtest clock is driven
> +        for i in range(1000):
> +            self.vm.hmp_qemu_io("drive0", "aio_read -a -q 0 512")
> +            self.vm.hmp_qemu_io("drive0", "aio_write -a -q 0 512")
> +
> +        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
> +
> +        ns += seconds * nsec_per_sec
> +        self.vm.qtest_cmd("clock_step %d" % ns)
> +        # wait for a while to let requests take off
> +        time.sleep(1)

This is not a reliable testing approach.  If the system is under heavy
load maybe only a few requests completed.  We don't know whether that is
due to I/O throttling or not.

A reliable test would not perform real disk I/O so the test is
independent of disk/system speed.  And it would not use time.sleep(1) to
"wait" since there is no guarantee that anything happened in the
meantime.

Do you think this can be improved?

> +        end_rd_bytes, end_rd_iops, end_wr_bytes, end_wr_iops = self.blockstats('drive0')
> +
> +        rd_bytes = end_rd_bytes - start_rd_bytes
> +        rd_iops = end_rd_iops - start_rd_iops
> +        wr_bytes = end_wr_bytes - start_wr_bytes
> +        wr_iops = end_wr_iops - start_wr_iops
> +
> +        assert check_limit(limits['bps'], rd_bytes)
> +        assert check_limit(limits['bps_rd'], rd_bytes)
> +        assert check_limit(limits['bps'], wr_bytes)
> +        assert check_limit(limits['bps_wr'], wr_bytes)
> +        assert check_limit(limits['iops'], rd_iops)
> +        assert check_limit(limits['iops_rd'], rd_iops)
> +        assert check_limit(limits['iops'], wr_iops)
> +        assert check_limit(limits['iops_wr'], wr_iops)

Please use TestCase.assert*() methods instead of plain assert.  They
produce humand-readable error messages including the failing values.

> +
> +    def test_bps(self):
> +        self.do_test_throttle(**{
> +            'device': 'drive0',
> +            'bps': 1000,
> +            'bps_rd': 0,
> +            'bps_wr': 0,
> +            'iops': 0,
> +            'iops_rd': 0,
> +            'iops_wr': 0,
> +            })

Keyword argument syntax is more concise:

self.do_test_throttle(device='drive0',
                      bps=1000,
		      bps_rd=0,
		      ...)

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

Re: [Qemu-devel] [PATCH v4 2/5] qtest: Add scripts/qtest/qtest.py

[Fam Zheng]

On Tue, 08/26 14:21, Stefan Hajnoczi wrote:
> On Thu, Jun 05, 2014 at 04:47:43PM +0800, Fam Zheng wrote:
> > diff --git a/scripts/qtest b/scripts/qtest
> > deleted file mode 100755
> > index 4ef6c1c..0000000
> > --- a/scripts/qtest
> > +++ /dev/null
> > @@ -1,5 +0,0 @@
> > -#!/bin/sh
> > -
> > -export QTEST_QEMU_BINARY=$1
> > -shift
> > -"$@"
> 
> Why delete this?  The script is unrelated to qtest.py.
> 
> I think nothing uses scripts/qtest so it could be deleted in a separate
> commit if you feel that is worthwhile.

OK. I'll send a separate patch.

> 
> > +import errno
> > +import socket
> > +
> > +class QEMUQtestProtocol:
> 
> Using new-style classes is probably a good idea:
> 
>   class QEMUQtestProtocol(object):
> 
> https://docs.python.org/2.7/reference/datamodel.html#newstyle

OK.

Re: [Qemu-devel] [PATCH v4 5/5] qemu-iotests: Add 093 for IO throttling

[Fam Zheng]

On Tue, 08/26 14:50, Stefan Hajnoczi wrote:
> On Thu, Jun 05, 2014 at 04:47:46PM +0800, Fam Zheng wrote:
> > +    def setUp(self):
> > +        qemu_img('create', '-f', iotests.imgfmt, test_img, "1G")
> > +        #self.vm = iotests.VM().add_drive(test_img, "bps=1024,bps_max=1")
> 
> Commented out lines should be dropped
> 
> > +        self.vm = iotests.VM().add_drive(test_img)
> > +        self.vm.launch()
> > +
> > +    def tearDown(self):
> > +        self.vm.shutdown()
> > +        os.remove(test_img)
> > +
> > +    def do_test_throttle(self, seconds=10, **limits):
> > +        def check_limit(limit, num):
> > +            # IO throttling algorithm is discrete, allow 10% error so the test
> > +            # is more deterministic
> > +            return limit == 0 or num < seconds * limit * 1.1
> > +
> > +        nsec_per_sec = 1000000000
> > +
> > +        limits['bps_max'] = 1
> > +        limits['iops_max'] = 1
> > +
> > +        # Enqueue many requests to throttling queue
> 
> This comment is wrong, it actually happens further down
> 
> > +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **limits)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        # Set vm clock to a known value
> > +        ns = nsec_per_sec
> > +        self.vm.qtest_cmd("clock_step %d" % ns)
> > +
> > +        # Append many requests into the throttle queue
> > +        # They drain bps_max and iops_max
> > +        # The rest requests won't get executed until qtest clock is driven
> > +        for i in range(1000):
> > +            self.vm.hmp_qemu_io("drive0", "aio_read -a -q 0 512")
> > +            self.vm.hmp_qemu_io("drive0", "aio_write -a -q 0 512")
> > +
> > +        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
> > +
> > +        ns += seconds * nsec_per_sec
> > +        self.vm.qtest_cmd("clock_step %d" % ns)
> > +        # wait for a while to let requests take off
> > +        time.sleep(1)
> 
> This is not a reliable testing approach.  If the system is under heavy
> load maybe only a few requests completed.  We don't know whether that is
> due to I/O throttling or not.
> 
> A reliable test would not perform real disk I/O so the test is
> independent of disk/system speed.  And it would not use time.sleep(1) to
> "wait" since there is no guarantee that anything happened in the
> meantime.
> 
> Do you think this can be improved?

Throttling only sets the upper limit of IO, this test checks the IO doesn't
cross it: when the test fails, something must be wrong with the throttling;
when the check passes, we can't guarantee that "everything is correct". That's
not uncommon across many other test cases we have. The other half is very hard,
because of host load, etc., which are out of control of this script.

Regarding to disk IO, I've submitted a separate patch, the "null://" protocol,
which can be used to sidestep the host disk load uncertainty. I can base this
test on top.

> 
> > +        end_rd_bytes, end_rd_iops, end_wr_bytes, end_wr_iops = self.blockstats('drive0')
> > +
> > +        rd_bytes = end_rd_bytes - start_rd_bytes
> > +        rd_iops = end_rd_iops - start_rd_iops
> > +        wr_bytes = end_wr_bytes - start_wr_bytes
> > +        wr_iops = end_wr_iops - start_wr_iops
> > +
> > +        assert check_limit(limits['bps'], rd_bytes)
> > +        assert check_limit(limits['bps_rd'], rd_bytes)
> > +        assert check_limit(limits['bps'], wr_bytes)
> > +        assert check_limit(limits['bps_wr'], wr_bytes)
> > +        assert check_limit(limits['iops'], rd_iops)
> > +        assert check_limit(limits['iops_rd'], rd_iops)
> > +        assert check_limit(limits['iops'], wr_iops)
> > +        assert check_limit(limits['iops_wr'], wr_iops)
> 
> Please use TestCase.assert*() methods instead of plain assert.  They
> produce humand-readable error messages including the failing values.

OK.

> 
> > +
> > +    def test_bps(self):
> > +        self.do_test_throttle(**{
> > +            'device': 'drive0',
> > +            'bps': 1000,
> > +            'bps_rd': 0,
> > +            'bps_wr': 0,
> > +            'iops': 0,
> > +            'iops_rd': 0,
> > +            'iops_wr': 0,
> > +            })
> 
> Keyword argument syntax is more concise:
> 
> self.do_test_throttle(device='drive0',
>                       bps=1000,
> 		      bps_rd=0,
> 		      ...)

OK, will change.

Thanks,
Fam

Re: [Qemu-devel] [PATCH v4 5/5] qemu-iotests: Add 093 for IO throttling

[Stefan Hajnoczi]

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

On Wed, Aug 27, 2014 at 02:19:44PM +0800, Fam Zheng wrote:
> On Tue, 08/26 14:50, Stefan Hajnoczi wrote:
> > On Thu, Jun 05, 2014 at 04:47:46PM +0800, Fam Zheng wrote:
> > > +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **limits)
> > > +        self.assert_qmp(result, 'return', {})
> > > +
> > > +        # Set vm clock to a known value
> > > +        ns = nsec_per_sec
> > > +        self.vm.qtest_cmd("clock_step %d" % ns)
> > > +
> > > +        # Append many requests into the throttle queue
> > > +        # They drain bps_max and iops_max
> > > +        # The rest requests won't get executed until qtest clock is driven
> > > +        for i in range(1000):
> > > +            self.vm.hmp_qemu_io("drive0", "aio_read -a -q 0 512")
> > > +            self.vm.hmp_qemu_io("drive0", "aio_write -a -q 0 512")
> > > +
> > > +        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
> > > +
> > > +        ns += seconds * nsec_per_sec
> > > +        self.vm.qtest_cmd("clock_step %d" % ns)
> > > +        # wait for a while to let requests take off
> > > +        time.sleep(1)
> > 
> > This is not a reliable testing approach.  If the system is under heavy
> > load maybe only a few requests completed.  We don't know whether that is
> > due to I/O throttling or not.
> > 
> > A reliable test would not perform real disk I/O so the test is
> > independent of disk/system speed.  And it would not use time.sleep(1) to
> > "wait" since there is no guarantee that anything happened in the
> > meantime.
> > 
> > Do you think this can be improved?
> 
> Throttling only sets the upper limit of IO, this test checks the IO doesn't
> cross it: when the test fails, something must be wrong with the throttling;
> when the check passes, we can't guarantee that "everything is correct". That's
> not uncommon across many other test cases we have. The other half is very hard,
> because of host load, etc., which are out of control of this script.
> 
> Regarding to disk IO, I've submitted a separate patch, the "null://" protocol,
> which can be used to sidestep the host disk load uncertainty. I can base this
> test on top.

If both a fake disk and fake timers are used then the execution is
deterministic.  I'll take a look at the null:// protocol you have
posted.

Although I would love this test to be deterministic, I understand it is
tricky to achieve that.  I'd like to discuss making this deterministic
but I'm not opposed to merging the current approach.

Stefan

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