[Qemu-devel] [PATCH 0/4] qemu-iotests: Test case for IO throttling

[Qemu-devel] [PATCH 0/4] qemu-iotests: Test case for IO throttling

[Fam Zheng]

This series adds iotest case for IO throttling.

There is a new option "-a" for 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 11%, to make room for such
flexibility in order to improve determinism.


Fam Zheng (4):
  qemu-io: New option "-a" to aio_read and aio_write
  qemu-iotests: Add VM method qtest_cmd() to iotests.py
  qemu-iotests: Allow caller to disable underscore convertion for qmp
  qemu-iotests: Add 080 for IO throttling

 qemu-io-cmds.c                |  35 +++++++--
 tests/qemu-iotests/080        | 164 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/080.out    |   5 ++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  14 ++--
 5 files changed, 211 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/080
 create mode 100644 tests/qemu-iotests/080.out

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Fam Zheng]

This option will enable accounting of aio requests.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-io-cmds.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index f1de24c..d6e20e6 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,8 @@ struct aio_ctx {
     int vflag;
     int Cflag;
     int Pflag;
+    int aflag;
+    BlockAcctCookie acct;
     int pattern;
     struct timeval t1;
 };
@@ -1370,6 +1373,10 @@ static void aio_write_done(void *opaque, int ret)
         goto out;
     }
 
+    if (ctx->aflag) {
+        bdrv_acct_done(ctx->bs, &ctx->acct);
+    }
+
     if (ctx->qflag) {
         goto out;
     }
@@ -1407,6 +1414,10 @@ static void aio_read_done(void *opaque, int ret)
         g_free(cmp_buf);
     }
 
+    if (ctx->aflag) {
+        bdrv_acct_done(ctx->bs, &ctx->acct);
+    }
+
     if (ctx->qflag) {
         goto out;
     }
@@ -1442,6 +1453,7 @@ static void aio_read_help(void)
 " -P, -- use a pattern to verify read data\n"
 " -v, -- dump buffer to standard output\n"
 " -q, -- quiet mode, do not show I/O statistics\n"
+" -a, -- account IO\n"
 "\n");
 }
 
@@ -1452,7 +1464,7 @@ static const cmdinfo_t aio_read_cmd = {
     .cfunc      = aio_read_f,
     .argmin     = 2,
     .argmax     = -1,
-    .args       = "[-Cqv] [-P pattern ] off len [len..]",
+    .args       = "[-Cqva] [-P pattern ] off len [len..]",
     .oneline    = "asynchronously reads a number of bytes",
     .help       = aio_read_help,
 };
@@ -1462,7 +1474,8 @@ 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);
 
-    while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
+    ctx->bs = bs;
+    while ((c = getopt(argc, argv, "CP:qva")) != EOF) {
         switch (c) {
         case 'C':
             ctx->Cflag = 1;
@@ -1481,6 +1494,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
         case 'v':
             ctx->vflag = 1;
             break;
+        case 'a':
+            ctx->aflag = 1;
+            break;
         default:
             g_free(ctx);
             return qemuio_command_usage(&aio_read_cmd);
@@ -1515,6 +1531,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
     }
 
     gettimeofday(&ctx->t1, NULL);
+    if (ctx->aflag) {
+        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;
@@ -1537,6 +1556,7 @@ static void aio_write_help(void)
 " -P, -- use different pattern to fill file\n"
 " -C, -- report statistics in a machine parsable format\n"
 " -q, -- quiet mode, do not show I/O statistics\n"
+" -a, -- account IO\n"
 "\n");
 }
 
@@ -1547,7 +1567,7 @@ static const cmdinfo_t aio_write_cmd = {
     .cfunc      = aio_write_f,
     .argmin     = 2,
     .argmax     = -1,
-    .args       = "[-Cq] [-P pattern ] off len [len..]",
+    .args       = "[-Cqa] [-P pattern ] off len [len..]",
     .oneline    = "asynchronously writes a number of bytes",
     .help       = aio_write_help,
 };
@@ -1558,7 +1578,8 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
     int pattern = 0xcd;
     struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
 
-    while ((c = getopt(argc, argv, "CqP:")) != EOF) {
+    ctx->bs = bs;
+    while ((c = getopt(argc, argv, "CqP:a")) != EOF) {
         switch (c) {
         case 'C':
             ctx->Cflag = 1;
@@ -1573,6 +1594,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
                 return 0;
             }
             break;
+        case 'a':
+            ctx->aflag = 1;
+            break;
         default:
             g_free(ctx);
             return qemuio_command_usage(&aio_write_cmd);
@@ -1607,6 +1631,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
     }
 
     gettimeofday(&ctx->t1, NULL);
+    if (ctx->aflag) {
+        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;
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/4] qemu-iotests: Add VM method qtest_cmd() 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>
---
 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 e4fa9af..ca79d09 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -153,13 +153,16 @@ class VM(object):
                              stderr=sys.stderr)
         return p.wait()
 
+    def qtest_cmd(self, cmd):
+        self._popen.stdin.write(cmd + "\n")
+
     def launch(self):
         '''Launch the VM and establish a QMP connection'''
-        devnull = open('/dev/null', 'rb')
         qemulog = open(self._qemu_log_path, 'wb')
         try:
             self._qmp = qmp.QEMUMonitorProtocol(self._monitor_path, server=True)
-            self._popen = subprocess.Popen(self._args, stdin=devnull, stdout=qemulog,
+            self._popen = subprocess.Popen(self._args, stdin=subprocess.PIPE,
+                                           stdout=qemulog,
                                            stderr=subprocess.STDOUT)
             self._qmp.accept()
         except:
-- 
1.8.5.3

[Qemu-devel] [PATCH 3/4] 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.

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 ca79d09..c1f5427 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -179,11 +179,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.8.5.3

[Qemu-devel] [PATCH 4/4] qemu-iotests: Add 080 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>
---
 tests/qemu-iotests/080     | 164 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/080.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 170 insertions(+)
 create mode 100755 tests/qemu-iotests/080
 create mode 100644 tests/qemu-iotests/080.out

diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
new file mode 100755
index 0000000..222bb37
--- /dev/null
+++ b/tests/qemu-iotests/080
@@ -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/080.out b/tests/qemu-iotests/080.out
new file mode 100644
index 0000000..3f8a935
--- /dev/null
+++ b/tests/qemu-iotests/080.out
@@ -0,0 +1,5 @@
+......
+----------------------------------------------------------------------
+Ran 6 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 03c762f..013a7ef 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -82,3 +82,4 @@
 073 rw auto
 074 rw auto
 077 rw auto
+080 rw auto
-- 
1.8.5.3

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

[Eric Blake]

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

On 01/29/2014 01:40 AM, Fam Zheng wrote:

s/convertion/conversion/ in the subject

> 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.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

You know, it might be nice to get the '-'/'_' looseness into QMP itself
when invoked via JSON strings over the monitor, rather than just the
testsuite wrapper, since we're already inconsistent in several commands.
 But that's a bigger project for another day.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Stefan Hajnoczi]

On Wed, Jan 29, 2014 at 04:40:40PM +0800, Fam Zheng wrote:
> @@ -1442,6 +1453,7 @@ static void aio_read_help(void)
>  " -P, -- use a pattern to verify read data\n"
>  " -v, -- dump buffer to standard output\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -a, -- account IO\n"
>  "\n");
>  }

This is too brief and probably won't help users.  Maybe "use BlockStats
I/O accounting"?

(I can fix this up when merging if you do not want to respin.)

Re: [Qemu-devel] [PATCH 2/4] qemu-iotests: Add VM method qtest_cmd() to iotests.py

[Stefan Hajnoczi]

On Wed, Jan 29, 2014 at 04:40:41PM +0800, Fam Zheng wrote:
> 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>
> ---
>  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 e4fa9af..ca79d09 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -153,13 +153,16 @@ class VM(object):
>                               stderr=sys.stderr)
>          return p.wait()
>  
> +    def qtest_cmd(self, cmd):
> +        self._popen.stdin.write(cmd + "\n")
> +
>      def launch(self):
>          '''Launch the VM and establish a QMP connection'''
> -        devnull = open('/dev/null', 'rb')
>          qemulog = open(self._qemu_log_path, 'wb')
>          try:
>              self._qmp = qmp.QEMUMonitorProtocol(self._monitor_path, server=True)
> -            self._popen = subprocess.Popen(self._args, stdin=devnull, stdout=qemulog,
> +            self._popen = subprocess.Popen(self._args, stdin=subprocess.PIPE,
> +                                           stdout=qemulog,
>                                             stderr=subprocess.STDOUT)

Commit 0fd05e8dd1ee7ae143fba3d6bcc6abe3fbeaeb34 ("qemu-iotests: start
vms in qtest mode") put qtest on stdio.  I think that was a mistake and
it should be fixed if you want to drive qtest.

Let's not mix qtest output with QEMU stderr.  If you need to drive
qtest, put it on a dedicated UNIX domain socket
(just like QMP).

Implement the qtest protocol as documented in qtest.c:

 * Line based protocol, request/response based.  Server can send async messages
 * so clients should always handle many async messages before the response
 * comes in.

Stefan

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

[Stefan Hajnoczi]

On Wed, Jan 29, 2014 at 04:40:42PM +0800, Fam Zheng wrote:
> 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.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] qemu-iotests: Add 080 for IO throttling

[Stefan Hajnoczi]

On Wed, Jan 29, 2014 at 04:40:43PM +0800, Fam Zheng wrote:
> 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

s/determinied/determined/

> +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")

Commented out line?

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

I *think* this trick is to force throttling not to allow bursts?  Maybe
a comment would be good to clarify.  IIRC when max is 0, burst behavior
is different.

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

sleep() is evil but I'm not sure there is a better way either... :(

Re: [Qemu-devel] [PATCH 4/4] qemu-iotests: Add 080 for IO throttling

[Stefan Hajnoczi]

On Wed, Jan 29, 2014 at 04:40:43PM +0800, Fam Zheng wrote:
> +    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,
> +            })

I guess you'll get more stable behavior with 1024 instead of 1000.  The
request themselves are 512 bytes.

As a side note, I think we'll wait for all 1000 pending requests each
time QEMU shuts down.  Probably not a huge time waste but even 100
requests exceed the I/O limits you've specified.  Perhaps using just 100
will make the test slightly faster.

Re: [Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Benoît Canet]

Le Wednesday 29 Jan 2014 à 16:40:40 (+0800), Fam Zheng a écrit :
> This option will enable accounting of aio requests.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-io-cmds.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index f1de24c..d6e20e6 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,8 @@ struct aio_ctx {
>      int vflag;
>      int Cflag;
>      int Pflag;
> +    int aflag;
> +    BlockAcctCookie acct;
>      int pattern;
>      struct timeval t1;
>  };
> @@ -1370,6 +1373,10 @@ static void aio_write_done(void *opaque, int ret)
>          goto out;
>      }
>  
> +    if (ctx->aflag) {
> +        bdrv_acct_done(ctx->bs, &ctx->acct);
> +    }
> +
>      if (ctx->qflag) {
>          goto out;
>      }
> @@ -1407,6 +1414,10 @@ static void aio_read_done(void *opaque, int ret)
>          g_free(cmp_buf);
>      }
>  
> +    if (ctx->aflag) {
> +        bdrv_acct_done(ctx->bs, &ctx->acct);
> +    }
> +
>      if (ctx->qflag) {
>          goto out;
>      }
> @@ -1442,6 +1453,7 @@ static void aio_read_help(void)
>  " -P, -- use a pattern to verify read data\n"
>  " -v, -- dump buffer to standard output\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -a, -- account IO\n"
>  "\n");
>  }
>  
> @@ -1452,7 +1464,7 @@ static const cmdinfo_t aio_read_cmd = {
>      .cfunc      = aio_read_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-Cqv] [-P pattern ] off len [len..]",
> +    .args       = "[-Cqva] [-P pattern ] off len [len..]",
>      .oneline    = "asynchronously reads a number of bytes",
>      .help       = aio_read_help,
>  };
> @@ -1462,7 +1474,8 @@ 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);
>  
> -    while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
> +    ctx->bs = bs;
> +    while ((c = getopt(argc, argv, "CP:qva")) != EOF) {
>          switch (c) {
>          case 'C':
>              ctx->Cflag = 1;
> @@ -1481,6 +1494,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
>          case 'v':
>              ctx->vflag = 1;
>              break;
> +        case 'a':
> +            ctx->aflag = 1;
> +            break;
>          default:
>              g_free(ctx);
>              return qemuio_command_usage(&aio_read_cmd);
> @@ -1515,6 +1531,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
>      }
>  
>      gettimeofday(&ctx->t1, NULL);
> +    if (ctx->aflag) {
> +        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;
> @@ -1537,6 +1556,7 @@ static void aio_write_help(void)
>  " -P, -- use different pattern to fill file\n"
>  " -C, -- report statistics in a machine parsable format\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -a, -- account IO\n"
>  "\n");
>  }
>  
> @@ -1547,7 +1567,7 @@ static const cmdinfo_t aio_write_cmd = {
>      .cfunc      = aio_write_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-Cq] [-P pattern ] off len [len..]",
> +    .args       = "[-Cqa] [-P pattern ] off len [len..]",
>      .oneline    = "asynchronously writes a number of bytes",
>      .help       = aio_write_help,
>  };
> @@ -1558,7 +1578,8 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
>      int pattern = 0xcd;
>      struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
>  
> -    while ((c = getopt(argc, argv, "CqP:")) != EOF) {
> +    ctx->bs = bs;
> +    while ((c = getopt(argc, argv, "CqP:a")) != EOF) {
>          switch (c) {
>          case 'C':
>              ctx->Cflag = 1;
> @@ -1573,6 +1594,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
>                  return 0;
>              }
>              break;
> +        case 'a':
> +            ctx->aflag = 1;
> +            break;
>          default:
>              g_free(ctx);
>              return qemuio_command_usage(&aio_write_cmd);
> @@ -1607,6 +1631,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
>      }
>  
>      gettimeofday(&ctx->t1, NULL);
> +    if (ctx->aflag) {
> +        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;
> -- 
> 1.8.5.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

Le Wednesday 29 Jan 2014 à 16:40:42 (+0800), Fam Zheng a écrit :
> 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.
> 
> 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 ca79d09..c1f5427 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -179,11 +179,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.8.5.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 4/4] qemu-iotests: Add 080 for IO throttling

[Benoît Canet]

Le Wednesday 29 Jan 2014 à 16:40:43 (+0800), Fam Zheng a écrit :
> 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>
> ---
>  tests/qemu-iotests/080     | 164 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/080.out |   5 ++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 170 insertions(+)
>  create mode 100755 tests/qemu-iotests/080
>  create mode 100644 tests/qemu-iotests/080.out
> 
> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
> new file mode 100755
> index 0000000..222bb37
> --- /dev/null
> +++ b/tests/qemu-iotests/080
> @@ -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/080.out b/tests/qemu-iotests/080.out
> new file mode 100644
> index 0000000..3f8a935
> --- /dev/null
> +++ b/tests/qemu-iotests/080.out
> @@ -0,0 +1,5 @@
> +......
> +----------------------------------------------------------------------
> +Ran 6 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 03c762f..013a7ef 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -82,3 +82,4 @@
>  073 rw auto
>  074 rw auto
>  077 rw auto
> +080 rw auto
> -- 
> 1.8.5.3
> 
> 

I like the way it's done.
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Paolo Bonzini]

Il 29/01/2014 09:40, Fam Zheng ha scritto:
> This option will enable accounting of aio requests.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-io-cmds.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index f1de24c..d6e20e6 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,8 @@ struct aio_ctx {
>      int vflag;
>      int Cflag;
>      int Pflag;
> +    int aflag;
> +    BlockAcctCookie acct;
>      int pattern;
>      struct timeval t1;
>  };
> @@ -1370,6 +1373,10 @@ static void aio_write_done(void *opaque, int ret)
>          goto out;
>      }
>
> +    if (ctx->aflag) {
> +        bdrv_acct_done(ctx->bs, &ctx->acct);
> +    }
> +
>      if (ctx->qflag) {
>          goto out;
>      }
> @@ -1407,6 +1414,10 @@ static void aio_read_done(void *opaque, int ret)
>          g_free(cmp_buf);
>      }
>
> +    if (ctx->aflag) {
> +        bdrv_acct_done(ctx->bs, &ctx->acct);
> +    }
> +
>      if (ctx->qflag) {
>          goto out;
>      }
> @@ -1442,6 +1453,7 @@ static void aio_read_help(void)
>  " -P, -- use a pattern to verify read data\n"
>  " -v, -- dump buffer to standard output\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -a, -- account IO\n"
>  "\n");
>  }
>
> @@ -1452,7 +1464,7 @@ static const cmdinfo_t aio_read_cmd = {
>      .cfunc      = aio_read_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-Cqv] [-P pattern ] off len [len..]",
> +    .args       = "[-Cqva] [-P pattern ] off len [len..]",
>      .oneline    = "asynchronously reads a number of bytes",
>      .help       = aio_read_help,
>  };
> @@ -1462,7 +1474,8 @@ 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);
>
> -    while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
> +    ctx->bs = bs;
> +    while ((c = getopt(argc, argv, "CP:qva")) != EOF) {
>          switch (c) {
>          case 'C':
>              ctx->Cflag = 1;
> @@ -1481,6 +1494,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
>          case 'v':
>              ctx->vflag = 1;
>              break;
> +        case 'a':
> +            ctx->aflag = 1;
> +            break;
>          default:
>              g_free(ctx);
>              return qemuio_command_usage(&aio_read_cmd);
> @@ -1515,6 +1531,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
>      }
>
>      gettimeofday(&ctx->t1, NULL);
> +    if (ctx->aflag) {
> +        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;
> @@ -1537,6 +1556,7 @@ static void aio_write_help(void)
>  " -P, -- use different pattern to fill file\n"
>  " -C, -- report statistics in a machine parsable format\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -a, -- account IO\n"
>  "\n");
>  }
>
> @@ -1547,7 +1567,7 @@ static const cmdinfo_t aio_write_cmd = {
>      .cfunc      = aio_write_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-Cq] [-P pattern ] off len [len..]",
> +    .args       = "[-Cqa] [-P pattern ] off len [len..]",
>      .oneline    = "asynchronously writes a number of bytes",
>      .help       = aio_write_help,
>  };
> @@ -1558,7 +1578,8 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
>      int pattern = 0xcd;
>      struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
>
> -    while ((c = getopt(argc, argv, "CqP:")) != EOF) {
> +    ctx->bs = bs;
> +    while ((c = getopt(argc, argv, "CqP:a")) != EOF) {
>          switch (c) {
>          case 'C':
>              ctx->Cflag = 1;
> @@ -1573,6 +1594,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
>                  return 0;
>              }
>              break;
> +        case 'a':
> +            ctx->aflag = 1;
> +            break;
>          default:
>              g_free(ctx);
>              return qemuio_command_usage(&aio_write_cmd);
> @@ -1607,6 +1631,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
>      }
>
>      gettimeofday(&ctx->t1, NULL);
> +    if (ctx->aflag) {
> +        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;
>

Why can't it be enabled unconditionally?

Paolo

Re: [Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Fam Zheng]

On Wed, 01/29 16:58, Paolo Bonzini wrote:
> Il 29/01/2014 09:40, Fam Zheng ha scritto:
> >This option will enable accounting of aio requests.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> > qemu-io-cmds.c | 35 +++++++++++++++++++++++++++++++----
> > 1 file changed, 31 insertions(+), 4 deletions(-)
> >
> >diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> >index f1de24c..d6e20e6 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,8 @@ struct aio_ctx {
> >     int vflag;
> >     int Cflag;
> >     int Pflag;
> >+    int aflag;
> >+    BlockAcctCookie acct;
> >     int pattern;
> >     struct timeval t1;
> > };
> >@@ -1370,6 +1373,10 @@ static void aio_write_done(void *opaque, int ret)
> >         goto out;
> >     }
> >
> >+    if (ctx->aflag) {
> >+        bdrv_acct_done(ctx->bs, &ctx->acct);
> >+    }
> >+
> >     if (ctx->qflag) {
> >         goto out;
> >     }
> >@@ -1407,6 +1414,10 @@ static void aio_read_done(void *opaque, int ret)
> >         g_free(cmp_buf);
> >     }
> >
> >+    if (ctx->aflag) {
> >+        bdrv_acct_done(ctx->bs, &ctx->acct);
> >+    }
> >+
> >     if (ctx->qflag) {
> >         goto out;
> >     }
> >@@ -1442,6 +1453,7 @@ static void aio_read_help(void)
> > " -P, -- use a pattern to verify read data\n"
> > " -v, -- dump buffer to standard output\n"
> > " -q, -- quiet mode, do not show I/O statistics\n"
> >+" -a, -- account IO\n"
> > "\n");
> > }
> >
> >@@ -1452,7 +1464,7 @@ static const cmdinfo_t aio_read_cmd = {
> >     .cfunc      = aio_read_f,
> >     .argmin     = 2,
> >     .argmax     = -1,
> >-    .args       = "[-Cqv] [-P pattern ] off len [len..]",
> >+    .args       = "[-Cqva] [-P pattern ] off len [len..]",
> >     .oneline    = "asynchronously reads a number of bytes",
> >     .help       = aio_read_help,
> > };
> >@@ -1462,7 +1474,8 @@ 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);
> >
> >-    while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
> >+    ctx->bs = bs;
> >+    while ((c = getopt(argc, argv, "CP:qva")) != EOF) {
> >         switch (c) {
> >         case 'C':
> >             ctx->Cflag = 1;
> >@@ -1481,6 +1494,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
> >         case 'v':
> >             ctx->vflag = 1;
> >             break;
> >+        case 'a':
> >+            ctx->aflag = 1;
> >+            break;
> >         default:
> >             g_free(ctx);
> >             return qemuio_command_usage(&aio_read_cmd);
> >@@ -1515,6 +1531,9 @@ static int aio_read_f(BlockDriverState *bs, int argc, char **argv)
> >     }
> >
> >     gettimeofday(&ctx->t1, NULL);
> >+    if (ctx->aflag) {
> >+        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;
> >@@ -1537,6 +1556,7 @@ static void aio_write_help(void)
> > " -P, -- use different pattern to fill file\n"
> > " -C, -- report statistics in a machine parsable format\n"
> > " -q, -- quiet mode, do not show I/O statistics\n"
> >+" -a, -- account IO\n"
> > "\n");
> > }
> >
> >@@ -1547,7 +1567,7 @@ static const cmdinfo_t aio_write_cmd = {
> >     .cfunc      = aio_write_f,
> >     .argmin     = 2,
> >     .argmax     = -1,
> >-    .args       = "[-Cq] [-P pattern ] off len [len..]",
> >+    .args       = "[-Cqa] [-P pattern ] off len [len..]",
> >     .oneline    = "asynchronously writes a number of bytes",
> >     .help       = aio_write_help,
> > };
> >@@ -1558,7 +1578,8 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
> >     int pattern = 0xcd;
> >     struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
> >
> >-    while ((c = getopt(argc, argv, "CqP:")) != EOF) {
> >+    ctx->bs = bs;
> >+    while ((c = getopt(argc, argv, "CqP:a")) != EOF) {
> >         switch (c) {
> >         case 'C':
> >             ctx->Cflag = 1;
> >@@ -1573,6 +1594,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
> >                 return 0;
> >             }
> >             break;
> >+        case 'a':
> >+            ctx->aflag = 1;
> >+            break;
> >         default:
> >             g_free(ctx);
> >             return qemuio_command_usage(&aio_write_cmd);
> >@@ -1607,6 +1631,9 @@ static int aio_write_f(BlockDriverState *bs, int argc, char **argv)
> >     }
> >
> >     gettimeofday(&ctx->t1, NULL);
> >+    if (ctx->aflag) {
> >+        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;
> >
> 
> Why can't it be enabled unconditionally?
> 

So the default behavior is unchanged.

Fam

Re: [Qemu-devel] [PATCH 2/4] qemu-iotests: Add VM method qtest_cmd() to iotests.py

[Fam Zheng]

On Wed, 01/29 15:22, Stefan Hajnoczi wrote:
> On Wed, Jan 29, 2014 at 04:40:41PM +0800, Fam Zheng wrote:
> > 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>
> > ---
> >  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 e4fa9af..ca79d09 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -153,13 +153,16 @@ class VM(object):
> >                               stderr=sys.stderr)
> >          return p.wait()
> >  
> > +    def qtest_cmd(self, cmd):
> > +        self._popen.stdin.write(cmd + "\n")
> > +
> >      def launch(self):
> >          '''Launch the VM and establish a QMP connection'''
> > -        devnull = open('/dev/null', 'rb')
> >          qemulog = open(self._qemu_log_path, 'wb')
> >          try:
> >              self._qmp = qmp.QEMUMonitorProtocol(self._monitor_path, server=True)
> > -            self._popen = subprocess.Popen(self._args, stdin=devnull, stdout=qemulog,
> > +            self._popen = subprocess.Popen(self._args, stdin=subprocess.PIPE,
> > +                                           stdout=qemulog,
> >                                             stderr=subprocess.STDOUT)
> 
> Commit 0fd05e8dd1ee7ae143fba3d6bcc6abe3fbeaeb34 ("qemu-iotests: start
> vms in qtest mode") put qtest on stdio.  I think that was a mistake and
> it should be fixed if you want to drive qtest.
> 
> Let's not mix qtest output with QEMU stderr.  If you need to drive
> qtest, put it on a dedicated UNIX domain socket
> (just like QMP).
> 
> Implement the qtest protocol as documented in qtest.c:
> 
>  * Line based protocol, request/response based.  Server can send async messages
>  * so clients should always handle many async messages before the response
>  * comes in.
> 

OK, good idea, I'll respin for a better qtest support.

Fam

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

[Fam Zheng]

On Wed, 01/29 07:09, Eric Blake wrote:
> On 01/29/2014 01:40 AM, Fam Zheng wrote:
> 
> s/convertion/conversion/ in the subject
> 
> > 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.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  tests/qemu-iotests/iotests.py | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> 
> You know, it might be nice to get the '-'/'_' looseness into QMP itself
> when invoked via JSON strings over the monitor, rather than just the
> testsuite wrapper, since we're already inconsistent in several commands.
>  But that's a bigger project for another day.
> 

I agree. Let's stick to this now and leave the loosing in QMP for later.

Fam

Re: [Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Paolo Bonzini]

Il 01/02/2014 15:31, Fam Zheng ha scritto:
>> Why can't it be enabled unconditionally?
>>
>
> So the default behavior is unchanged.

For the stand-alone executable there is no difference, is there anything 
that breaks for the monitor if you always do the accounting?

Paolo

Re: [Qemu-devel] [PATCH 1/4] qemu-io: New option "-a" to aio_read and aio_write

[Fam Zheng]

On Sun, 02/02 01:20, Paolo Bonzini wrote:
> Il 01/02/2014 15:31, Fam Zheng ha scritto:
> >>Why can't it be enabled unconditionally?
> >>
> >
> >So the default behavior is unchanged.
> 
> For the stand-alone executable there is no difference, is there anything
> that breaks for the monitor if you always do the accounting?
> 

I don't see anything broken, it just affacts the numbers in block stats. So it
should be fine to enable it unconditionally, will do a respin for that. Thanks.

Fam