[Qemu-devel] [PATCH v6 0/5] block: Add a qemu-iotests case for IO throttling

[Qemu-devel] [PATCH v6 0/5] block: Add a qemu-iotests case for IO throttling

[Fam Zheng]

v6: Less resource demanding patch 5. (Max)
    Add rev-by of Max to other patches.

v5: Rebase and improve the test. Please review again.

    Patch dependencies:

    This test depends on the qtest timer fix to run correctly.
    http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01865.html

    Also depends on the os check fix to run at all:
    http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01848.html

Original cover letter
---------------------

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 with a 10% error tolerance, to make
room for such flexibility in order to improve determinism.

Fam


Fam Zheng (5):
  qemu-io: Account IO by aio_read and aio_write
  qtest: Add scripts/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                |  11 ++++
 scripts/qtest.py              |  71 +++++++++++++++++++++++++
 tests/qemu-iotests/093        | 120 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out    |   5 ++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  23 ++++++--
 6 files changed, 227 insertions(+), 4 deletions(-)
 create mode 100644 scripts/qtest.py
 create mode 100755 tests/qemu-iotests/093
 create mode 100644 tests/qemu-iotests/093.out

-- 
1.9.3

[Qemu-devel] [PATCH v6 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: Max Reitz <mreitz@redhat.com>
---
 qemu-io-cmds.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e708552..29377cd 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -13,6 +13,7 @@
 #include "block/qapi.h"
 #include "qemu/main-loop.h"
 #include "qemu/timer.h"
+#include "sysemu/block-backend.h"
 
 #define CMD_NOFILE_OK   0x01
 
@@ -1337,6 +1338,7 @@ out:
 }
 
 struct aio_ctx {
+    BlockDriverState *bs;
     QEMUIOVector qiov;
     int64_t offset;
     char *buf;
@@ -1344,6 +1346,7 @@ struct aio_ctx {
     int vflag;
     int Cflag;
     int Pflag;
+    BlockAcctCookie acct;
     int pattern;
     struct timeval t1;
 };
@@ -1361,6 +1364,8 @@ static void aio_write_done(void *opaque, int ret)
         goto out;
     }
 
+    block_acct_done(&ctx->bs->stats, &ctx->acct);
+
     if (ctx->qflag) {
         goto out;
     }
@@ -1398,6 +1403,8 @@ static void aio_read_done(void *opaque, int ret)
         g_free(cmp_buf);
     }
 
+    block_acct_done(&ctx->bs->stats, &ctx->acct);
+
     if (ctx->qflag) {
         goto out;
     }
@@ -1453,6 +1460,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':
@@ -1506,6 +1514,7 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
     }
 
     gettimeofday(&ctx->t1, NULL);
+    block_acct_start(&bs->stats, &ctx->acct, ctx->qiov.size, BLOCK_ACCT_READ);
     bdrv_aio_readv(bs, ctx->offset >> 9, &ctx->qiov,
                    ctx->qiov.size >> 9, aio_read_done, ctx);
     return 0;
@@ -1549,6 +1558,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':
@@ -1598,6 +1608,7 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
     }
 
     gettimeofday(&ctx->t1, NULL);
+    block_acct_start(&bs->stats, &ctx->acct, ctx->qiov.size, BLOCK_ACCT_WRITE);
     bdrv_aio_writev(bs, ctx->offset >> 9, &ctx->qiov,
                     ctx->qiov.size >> 9, aio_write_done, ctx);
     return 0;
-- 
1.9.3

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

[Fam Zheng]

This adds scripts/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 could be added later on demand.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 scripts/qtest.py | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 scripts/qtest.py

diff --git a/scripts/qtest.py b/scripts/qtest.py
new file mode 100644
index 0000000..a971445
--- /dev/null
+++ b/scripts/qtest.py
@@ -0,0 +1,71 @@
+# QEMU qtest library
+#
+# Copyright (C) 2015 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(object):
+    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)
+
+    def accept(self):
+        """
+        Await connection from QEMU.
+
+        @raise socket.error on socket connection errors
+        """
+        self._sock, _ = self._sock.accept()
+
+    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._sock.close()
+
+    def settimeout(self, timeout):
+        self._sock.settimeout(timeout)
-- 
1.9.3

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

[Fam Zheng]

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

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 241b5ee..85cb9a5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -21,8 +21,11 @@ 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'))
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts', 'qmp'))
 import qmp
+import qtest
 import struct
 
 __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
@@ -81,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
 
@@ -160,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
@@ -173,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
 
@@ -185,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)
-- 
1.9.3

[Qemu-devel] [PATCH v6 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>
Reviewed-by: Max Reitz <mreitz@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 85cb9a5..1402854 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)
 
-- 
1.9.3

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

[Fam Zheng]

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

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 10% error of bps and iops
limits.

"null" protocol is used as the disk backend so that no actual disk IO is
performed on host, this will make the blockstats much more
deterministic. Both "null-aio" and "null-co" are covered, which is also
a simple cross validation test for the driver code.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/093     | 120 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 126 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..2866536
--- /dev/null
+++ b/tests/qemu-iotests/093
@@ -0,0 +1,120 @@
+#!/usr/bin/env python
+#
+# Tests for IO throttling
+#
+# Copyright (C) 2015 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 iotests
+
+class ThrottleTestCase(iotests.QMPTestCase):
+    test_img = "null-aio://"
+
+    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):
+        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+
+    def do_test_throttle(self, seconds, params):
+        def check_limit(limit, num):
+            # IO throttling algorithm is discrete, allow 10% error so the test
+            # is more robust
+            return limit == 0 or \
+                   (num < seconds * limit * 1.1
+                   and num > seconds * limit * 0.9)
+
+        nsec_per_sec = 1000000000
+
+        params['device'] = 'drive0'
+
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params)
+        self.assert_qmp(result, 'return', {})
+
+        # Set vm clock to a known value
+        ns = seconds * nsec_per_sec
+        self.vm.qtest("clock_step %d" % ns)
+
+        # Submit enough requests. They will drain bps_max and iops_max, but the
+        # rest requests won't get executed until we advance the virtual clock
+        # with qtest interface
+        rq_size = 512
+        rd_nr = max(params['bps'] / rq_size / 2,
+                    params['bps_rd'] / rq_size,
+                    params['iops'] / 2,
+                    params['iops_rd']) + \
+                params['bps_max'] / rq_size / 2 + \
+                params['iops_max']
+        rd_nr *= seconds * 2
+        wr_nr = max(params['bps'] / rq_size / 2,
+                    params['bps_wr'] / rq_size,
+                    params['iops'] / 2,
+                    params['iops_wr']) + \
+                params['bps_max'] / rq_size / 2 + \
+                params['iops_max']
+        wr_nr *= seconds * 2
+        for i in range(rd_nr):
+            self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % (i * rq_size, rq_size))
+        for i in range(wr_nr):
+            self.vm.hmp_qemu_io("drive0", "aio_write %d %d" % (i * rq_size, rq_size))
+
+        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
+
+        self.vm.qtest("clock_step %d" % ns)
+        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
+
+        self.assertTrue(check_limit(params['bps'], rd_bytes + wr_bytes))
+        self.assertTrue(check_limit(params['bps_rd'], rd_bytes))
+        self.assertTrue(check_limit(params['bps_wr'], wr_bytes))
+        self.assertTrue(check_limit(params['iops'], rd_iops + wr_iops))
+        self.assertTrue(check_limit(params['iops_rd'], rd_iops))
+        self.assertTrue(check_limit(params['iops_wr'], wr_iops))
+
+    def test_all(self):
+        params = {"bps": 4096,
+                  "bps_rd": 4096,
+                  "bps_wr": 4096,
+                  "bps_max": 4096,
+                  "iops": 10,
+                  "iops_rd": 10,
+                  "iops_wr": 10,
+                  "iops_max": 10,
+                 }
+        # Pick each out of all possible params and test
+        for tk in params:
+            limits = dict([(k, 0) for k in params])
+            limits[tk] = params[tk]
+            self.do_test_throttle(5, limits)
+
+class ThrottleTestCoroutine(ThrottleTestCase):
+    test_img = "null-co://"
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=["raw"])
diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out
new file mode 100644
index 0000000..fbc63e6
--- /dev/null
+++ b/tests/qemu-iotests/093.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index f8bf354..0272c9a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -99,6 +99,7 @@
 090 rw auto quick
 091 rw auto
 092 rw auto quick
+093 auto
 095 rw auto quick
 097 rw auto backing
 098 rw auto backing quick
-- 
1.9.3

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

[Max Reitz]

On 2015-01-27 at 21:28, Fam Zheng wrote:
> This case utilizes qemu-io command "aio_{read,write} -q" to verify the
> effectiveness of IO throttling options.
>
> 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 10% error of bps and iops
> limits.
>
> "null" protocol is used as the disk backend so that no actual disk IO is
> performed on host, this will make the blockstats much more
> deterministic. Both "null-aio" and "null-co" are covered, which is also
> a simple cross validation test for the driver code.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   tests/qemu-iotests/093     | 120 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/093.out |   5 ++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 126 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..2866536
> --- /dev/null
> +++ b/tests/qemu-iotests/093
> @@ -0,0 +1,120 @@
> +#!/usr/bin/env python
> +#
> +# Tests for IO throttling
> +#
> +# Copyright (C) 2015 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 iotests
> +
> +class ThrottleTestCase(iotests.QMPTestCase):
> +    test_img = "null-aio://"
> +
> +    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):
> +        self.vm = iotests.VM().add_drive(self.test_img)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +
> +    def do_test_throttle(self, seconds, params):
> +        def check_limit(limit, num):
> +            # IO throttling algorithm is discrete, allow 10% error so the test
> +            # is more robust
> +            return limit == 0 or \
> +                   (num < seconds * limit * 1.1
> +                   and num > seconds * limit * 0.9)
> +
> +        nsec_per_sec = 1000000000
> +
> +        params['device'] = 'drive0'
> +
> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params)
> +        self.assert_qmp(result, 'return', {})
> +
> +        # Set vm clock to a known value
> +        ns = seconds * nsec_per_sec
> +        self.vm.qtest("clock_step %d" % ns)
> +
> +        # Submit enough requests. They will drain bps_max and iops_max, but the
> +        # rest requests won't get executed until we advance the virtual clock
> +        # with qtest interface
> +        rq_size = 512
> +        rd_nr = max(params['bps'] / rq_size / 2,
> +                    params['bps_rd'] / rq_size,
> +                    params['iops'] / 2,
> +                    params['iops_rd']) + \
> +                params['bps_max'] / rq_size / 2 + \
> +                params['iops_max']

I guess the divisions by two are because those values represent read and 
write operations combined. Shouldn't iops_max be divided by two, too, then?

> +        rd_nr *= seconds * 2
> +        wr_nr = max(params['bps'] / rq_size / 2,
> +                    params['bps_wr'] / rq_size,
> +                    params['iops'] / 2,
> +                    params['iops_wr']) + \
> +                params['bps_max'] / rq_size / 2 + \
> +                params['iops_max']
> +        wr_nr *= seconds * 2
> +        for i in range(rd_nr):
> +            self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % (i * rq_size, rq_size))
> +        for i in range(wr_nr):
> +            self.vm.hmp_qemu_io("drive0", "aio_write %d %d" % (i * rq_size, rq_size))
> +
> +        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
> +
> +        self.vm.qtest("clock_step %d" % ns)
> +        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
> +
> +        self.assertTrue(check_limit(params['bps'], rd_bytes + wr_bytes))
> +        self.assertTrue(check_limit(params['bps_rd'], rd_bytes))
> +        self.assertTrue(check_limit(params['bps_wr'], wr_bytes))
> +        self.assertTrue(check_limit(params['iops'], rd_iops + wr_iops))
> +        self.assertTrue(check_limit(params['iops_rd'], rd_iops))
> +        self.assertTrue(check_limit(params['iops_wr'], wr_iops))

Hm, you're not checking bps_max and iops_max here. Should you be?

Apart from these two (minor) things: Works for me! :-)

Max

> +
> +    def test_all(self):
> +        params = {"bps": 4096,
> +                  "bps_rd": 4096,
> +                  "bps_wr": 4096,
> +                  "bps_max": 4096,
> +                  "iops": 10,
> +                  "iops_rd": 10,
> +                  "iops_wr": 10,
> +                  "iops_max": 10,
> +                 }
> +        # Pick each out of all possible params and test
> +        for tk in params:
> +            limits = dict([(k, 0) for k in params])
> +            limits[tk] = params[tk]
> +            self.do_test_throttle(5, limits)
> +
> +class ThrottleTestCoroutine(ThrottleTestCase):
> +    test_img = "null-co://"
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=["raw"])
> diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out
> new file mode 100644
> index 0000000..fbc63e6
> --- /dev/null
> +++ b/tests/qemu-iotests/093.out
> @@ -0,0 +1,5 @@
> +..
> +----------------------------------------------------------------------
> +Ran 2 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index f8bf354..0272c9a 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -99,6 +99,7 @@
>   090 rw auto quick
>   091 rw auto
>   092 rw auto quick
> +093 auto
>   095 rw auto quick
>   097 rw auto backing
>   098 rw auto backing quick

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

[Fam Zheng]

On Wed, 01/28 11:22, Max Reitz wrote:
> On 2015-01-27 at 21:28, Fam Zheng wrote:
> >This case utilizes qemu-io command "aio_{read,write} -q" to verify the
> >effectiveness of IO throttling options.
> >
> >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 10% error of bps and iops
> >limits.
> >
> >"null" protocol is used as the disk backend so that no actual disk IO is
> >performed on host, this will make the blockstats much more
> >deterministic. Both "null-aio" and "null-co" are covered, which is also
> >a simple cross validation test for the driver code.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  tests/qemu-iotests/093     | 120 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/093.out |   5 ++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 126 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..2866536
> >--- /dev/null
> >+++ b/tests/qemu-iotests/093
> >@@ -0,0 +1,120 @@
> >+#!/usr/bin/env python
> >+#
> >+# Tests for IO throttling
> >+#
> >+# Copyright (C) 2015 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 iotests
> >+
> >+class ThrottleTestCase(iotests.QMPTestCase):
> >+    test_img = "null-aio://"
> >+
> >+    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):
> >+        self.vm = iotests.VM().add_drive(self.test_img)
> >+        self.vm.launch()
> >+
> >+    def tearDown(self):
> >+        self.vm.shutdown()
> >+
> >+    def do_test_throttle(self, seconds, params):
> >+        def check_limit(limit, num):
> >+            # IO throttling algorithm is discrete, allow 10% error so the test
> >+            # is more robust
> >+            return limit == 0 or \
> >+                   (num < seconds * limit * 1.1
> >+                   and num > seconds * limit * 0.9)
> >+
> >+        nsec_per_sec = 1000000000
> >+
> >+        params['device'] = 'drive0'
> >+
> >+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        # Set vm clock to a known value
> >+        ns = seconds * nsec_per_sec
> >+        self.vm.qtest("clock_step %d" % ns)
> >+
> >+        # Submit enough requests. They will drain bps_max and iops_max, but the
> >+        # rest requests won't get executed until we advance the virtual clock
> >+        # with qtest interface
> >+        rq_size = 512
> >+        rd_nr = max(params['bps'] / rq_size / 2,
> >+                    params['bps_rd'] / rq_size,
> >+                    params['iops'] / 2,
> >+                    params['iops_rd']) + \
> >+                params['bps_max'] / rq_size / 2 + \
> >+                params['iops_max']
> 
> I guess the divisions by two are because those values represent read and
> write operations combined. Shouldn't iops_max be divided by two, too, then?
> 
> >+        rd_nr *= seconds * 2
> >+        wr_nr = max(params['bps'] / rq_size / 2,
> >+                    params['bps_wr'] / rq_size,
> >+                    params['iops'] / 2,
> >+                    params['iops_wr']) + \
> >+                params['bps_max'] / rq_size / 2 + \
> >+                params['iops_max']
> >+        wr_nr *= seconds * 2
> >+        for i in range(rd_nr):
> >+            self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % (i * rq_size, rq_size))
> >+        for i in range(wr_nr):
> >+            self.vm.hmp_qemu_io("drive0", "aio_write %d %d" % (i * rq_size, rq_size))
> >+
> >+        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
> >+
> >+        self.vm.qtest("clock_step %d" % ns)
> >+        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
> >+
> >+        self.assertTrue(check_limit(params['bps'], rd_bytes + wr_bytes))
> >+        self.assertTrue(check_limit(params['bps_rd'], rd_bytes))
> >+        self.assertTrue(check_limit(params['bps_wr'], wr_bytes))
> >+        self.assertTrue(check_limit(params['iops'], rd_iops + wr_iops))
> >+        self.assertTrue(check_limit(params['iops_rd'], rd_iops))
> >+        self.assertTrue(check_limit(params['iops_wr'], wr_iops))
> 
> Hm, you're not checking bps_max and iops_max here. Should you be?

I never really liked these two parameters, but now that you asked, probably
yes (to this question and above). :)

Fam


> 
> Apart from these two (minor) things: Works for me! :-)
> 
> Max
> 
> >+
> >+    def test_all(self):
> >+        params = {"bps": 4096,
> >+                  "bps_rd": 4096,
> >+                  "bps_wr": 4096,
> >+                  "bps_max": 4096,
> >+                  "iops": 10,
> >+                  "iops_rd": 10,
> >+                  "iops_wr": 10,
> >+                  "iops_max": 10,
> >+                 }
> >+        # Pick each out of all possible params and test
> >+        for tk in params:
> >+            limits = dict([(k, 0) for k in params])
> >+            limits[tk] = params[tk]
> >+            self.do_test_throttle(5, limits)
> >+
> >+class ThrottleTestCoroutine(ThrottleTestCase):
> >+    test_img = "null-co://"
> >+
> >+if __name__ == '__main__':
> >+    iotests.main(supported_fmts=["raw"])
> >diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out
> >new file mode 100644
> >index 0000000..fbc63e6
> >--- /dev/null
> >+++ b/tests/qemu-iotests/093.out
> >@@ -0,0 +1,5 @@
> >+..
> >+----------------------------------------------------------------------
> >+Ran 2 tests
> >+
> >+OK
> >diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> >index f8bf354..0272c9a 100644
> >--- a/tests/qemu-iotests/group
> >+++ b/tests/qemu-iotests/group
> >@@ -99,6 +99,7 @@
> >  090 rw auto quick
> >  091 rw auto
> >  092 rw auto quick
> >+093 auto
> >  095 rw auto quick
> >  097 rw auto backing
> >  098 rw auto backing quick
> 

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

[Fam Zheng]

On Thu, 01/29 08:53, Fam Zheng wrote:
> On Wed, 01/28 11:22, Max Reitz wrote:
> > On 2015-01-27 at 21:28, Fam Zheng wrote:
> > >This case utilizes qemu-io command "aio_{read,write} -q" to verify the
> > >effectiveness of IO throttling options.
> > >
> > >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 10% error of bps and iops
> > >limits.
> > >
> > >"null" protocol is used as the disk backend so that no actual disk IO is
> > >performed on host, this will make the blockstats much more
> > >deterministic. Both "null-aio" and "null-co" are covered, which is also
> > >a simple cross validation test for the driver code.
> > >
> > >Signed-off-by: Fam Zheng <famz@redhat.com>
> > >---
> > >  tests/qemu-iotests/093     | 120 +++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/qemu-iotests/093.out |   5 ++
> > >  tests/qemu-iotests/group   |   1 +
> > >  3 files changed, 126 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..2866536
> > >--- /dev/null
> > >+++ b/tests/qemu-iotests/093
> > >@@ -0,0 +1,120 @@
> > >+#!/usr/bin/env python
> > >+#
> > >+# Tests for IO throttling
> > >+#
> > >+# Copyright (C) 2015 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 iotests
> > >+
> > >+class ThrottleTestCase(iotests.QMPTestCase):
> > >+    test_img = "null-aio://"
> > >+
> > >+    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):
> > >+        self.vm = iotests.VM().add_drive(self.test_img)
> > >+        self.vm.launch()
> > >+
> > >+    def tearDown(self):
> > >+        self.vm.shutdown()
> > >+
> > >+    def do_test_throttle(self, seconds, params):
> > >+        def check_limit(limit, num):
> > >+            # IO throttling algorithm is discrete, allow 10% error so the test
> > >+            # is more robust
> > >+            return limit == 0 or \
> > >+                   (num < seconds * limit * 1.1
> > >+                   and num > seconds * limit * 0.9)
> > >+
> > >+        nsec_per_sec = 1000000000
> > >+
> > >+        params['device'] = 'drive0'
> > >+
> > >+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params)
> > >+        self.assert_qmp(result, 'return', {})
> > >+
> > >+        # Set vm clock to a known value
> > >+        ns = seconds * nsec_per_sec
> > >+        self.vm.qtest("clock_step %d" % ns)
> > >+
> > >+        # Submit enough requests. They will drain bps_max and iops_max, but the
> > >+        # rest requests won't get executed until we advance the virtual clock
> > >+        # with qtest interface
> > >+        rq_size = 512
> > >+        rd_nr = max(params['bps'] / rq_size / 2,
> > >+                    params['bps_rd'] / rq_size,
> > >+                    params['iops'] / 2,
> > >+                    params['iops_rd']) + \
> > >+                params['bps_max'] / rq_size / 2 + \
> > >+                params['iops_max']
> > 
> > I guess the divisions by two are because those values represent read and
> > write operations combined. Shouldn't iops_max be divided by two, too, then?
> > 
> > >+        rd_nr *= seconds * 2
> > >+        wr_nr = max(params['bps'] / rq_size / 2,
> > >+                    params['bps_wr'] / rq_size,
> > >+                    params['iops'] / 2,
> > >+                    params['iops_wr']) + \
> > >+                params['bps_max'] / rq_size / 2 + \
> > >+                params['iops_max']
> > >+        wr_nr *= seconds * 2
> > >+        for i in range(rd_nr):
> > >+            self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % (i * rq_size, rq_size))
> > >+        for i in range(wr_nr):
> > >+            self.vm.hmp_qemu_io("drive0", "aio_write %d %d" % (i * rq_size, rq_size))
> > >+
> > >+        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
> > >+
> > >+        self.vm.qtest("clock_step %d" % ns)
> > >+        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
> > >+
> > >+        self.assertTrue(check_limit(params['bps'], rd_bytes + wr_bytes))
> > >+        self.assertTrue(check_limit(params['bps_rd'], rd_bytes))
> > >+        self.assertTrue(check_limit(params['bps_wr'], wr_bytes))
> > >+        self.assertTrue(check_limit(params['iops'], rd_iops + wr_iops))
> > >+        self.assertTrue(check_limit(params['iops_rd'], rd_iops))
> > >+        self.assertTrue(check_limit(params['iops_wr'], wr_iops))
> > 
> > Hm, you're not checking bps_max and iops_max here. Should you be?
> 
> I never really liked these two parameters, but now that you asked, probably
> yes (to this question and above). :)
> 
> Fam

OK, messed for some time with *_max here and I'm giving up:

/* fix bucket parameters */
static void throttle_fix_bucket(LeakyBucket *bkt)
{
    double min;

    /* zero bucket level */
    bkt->level = 0;

    /* The following is done to cope with the Linux CFQ block scheduler
     * which regroup reads and writes by block of 100ms in the guest.
     * When they are two process one making reads and one making writes cfq
     * make a pattern looking like the following:
     * WWWWWWWWWWWRRRRRRRRRRRRRRWWWWWWWWWWWWWwRRRRRRRRRRRRRRRRR
     * Having a max burst value of 100ms of the average will help smooth the
     * throttling
     */
    min = bkt->avg / 10;
    if (bkt->avg && !bkt->max) {
        bkt->max = min;
    }
}

This is some magic that cannot be tested.  So are you happy with this version?

Fam

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

[Max Reitz]

On 2015-01-28 at 21:06, Fam Zheng wrote:
> On Thu, 01/29 08:53, Fam Zheng wrote:
>> On Wed, 01/28 11:22, Max Reitz wrote:
>>> On 2015-01-27 at 21:28, Fam Zheng wrote:
>>>> This case utilizes qemu-io command "aio_{read,write} -q" to verify the
>>>> effectiveness of IO throttling options.
>>>>
>>>> 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 10% error of bps and iops
>>>> limits.
>>>>
>>>> "null" protocol is used as the disk backend so that no actual disk IO is
>>>> performed on host, this will make the blockstats much more
>>>> deterministic. Both "null-aio" and "null-co" are covered, which is also
>>>> a simple cross validation test for the driver code.
>>>>
>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/093     | 120 +++++++++++++++++++++++++++++++++++++++++++++
>>>>   tests/qemu-iotests/093.out |   5 ++
>>>>   tests/qemu-iotests/group   |   1 +
>>>>   3 files changed, 126 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..2866536
>>>> --- /dev/null
>>>> +++ b/tests/qemu-iotests/093
>>>> @@ -0,0 +1,120 @@
>>>> +#!/usr/bin/env python
>>>> +#
>>>> +# Tests for IO throttling
>>>> +#
>>>> +# Copyright (C) 2015 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 iotests
>>>> +
>>>> +class ThrottleTestCase(iotests.QMPTestCase):
>>>> +    test_img = "null-aio://"
>>>> +
>>>> +    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):
>>>> +        self.vm = iotests.VM().add_drive(self.test_img)
>>>> +        self.vm.launch()
>>>> +
>>>> +    def tearDown(self):
>>>> +        self.vm.shutdown()
>>>> +
>>>> +    def do_test_throttle(self, seconds, params):
>>>> +        def check_limit(limit, num):
>>>> +            # IO throttling algorithm is discrete, allow 10% error so the test
>>>> +            # is more robust
>>>> +            return limit == 0 or \
>>>> +                   (num < seconds * limit * 1.1
>>>> +                   and num > seconds * limit * 0.9)
>>>> +
>>>> +        nsec_per_sec = 1000000000
>>>> +
>>>> +        params['device'] = 'drive0'
>>>> +
>>>> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params)
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        # Set vm clock to a known value
>>>> +        ns = seconds * nsec_per_sec
>>>> +        self.vm.qtest("clock_step %d" % ns)
>>>> +
>>>> +        # Submit enough requests. They will drain bps_max and iops_max, but the
>>>> +        # rest requests won't get executed until we advance the virtual clock
>>>> +        # with qtest interface
>>>> +        rq_size = 512
>>>> +        rd_nr = max(params['bps'] / rq_size / 2,
>>>> +                    params['bps_rd'] / rq_size,
>>>> +                    params['iops'] / 2,
>>>> +                    params['iops_rd']) + \
>>>> +                params['bps_max'] / rq_size / 2 + \
>>>> +                params['iops_max']
>>> I guess the divisions by two are because those values represent read and
>>> write operations combined. Shouldn't iops_max be divided by two, too, then?
>>>
>>>> +        rd_nr *= seconds * 2
>>>> +        wr_nr = max(params['bps'] / rq_size / 2,
>>>> +                    params['bps_wr'] / rq_size,
>>>> +                    params['iops'] / 2,
>>>> +                    params['iops_wr']) + \
>>>> +                params['bps_max'] / rq_size / 2 + \
>>>> +                params['iops_max']
>>>> +        wr_nr *= seconds * 2
>>>> +        for i in range(rd_nr):
>>>> +            self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % (i * rq_size, rq_size))
>>>> +        for i in range(wr_nr):
>>>> +            self.vm.hmp_qemu_io("drive0", "aio_write %d %d" % (i * rq_size, rq_size))
>>>> +
>>>> +        start_rd_bytes, start_rd_iops, start_wr_bytes, start_wr_iops = self.blockstats('drive0')
>>>> +
>>>> +        self.vm.qtest("clock_step %d" % ns)
>>>> +        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
>>>> +
>>>> +        self.assertTrue(check_limit(params['bps'], rd_bytes + wr_bytes))
>>>> +        self.assertTrue(check_limit(params['bps_rd'], rd_bytes))
>>>> +        self.assertTrue(check_limit(params['bps_wr'], wr_bytes))
>>>> +        self.assertTrue(check_limit(params['iops'], rd_iops + wr_iops))
>>>> +        self.assertTrue(check_limit(params['iops_rd'], rd_iops))
>>>> +        self.assertTrue(check_limit(params['iops_wr'], wr_iops))
>>> Hm, you're not checking bps_max and iops_max here. Should you be?
>> I never really liked these two parameters, but now that you asked, probably
>> yes (to this question and above). :)
>>
>> Fam
> OK, messed for some time with *_max here and I'm giving up:
>
> /* fix bucket parameters */
> static void throttle_fix_bucket(LeakyBucket *bkt)
> {
>      double min;
>
>      /* zero bucket level */
>      bkt->level = 0;
>
>      /* The following is done to cope with the Linux CFQ block scheduler
>       * which regroup reads and writes by block of 100ms in the guest.
>       * When they are two process one making reads and one making writes cfq
>       * make a pattern looking like the following:
>       * WWWWWWWWWWWRRRRRRRRRRRRRRWWWWWWWWWWWWWwRRRRRRRRRRRRRRRRR
>       * Having a max burst value of 100ms of the average will help smooth the
>       * throttling
>       */
>      min = bkt->avg / 10;
>      if (bkt->avg && !bkt->max) {
>          bkt->max = min;
>      }
> }
>
> This is some magic that cannot be tested.  So are you happy with this version?

Would it be possible to just remove {b,io}ps_max completely from this 
test? I'd be fine with that.

Max

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

[Fam Zheng]

On Thu, 01/29 09:29, Max Reitz wrote:
> Would it be possible to just remove {b,io}ps_max completely from this test?
> I'd be fine with that.

Sure, will resend.

Fam

> 
> Max