Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Denis Plotnikov]

On 07.11.2019 19:30, Cleber Rosa wrote:
> On Wed, Nov 06, 2019 at 04:26:41PM -0300, Eduardo Habkost wrote:
>> On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
>>> On 06.11.19 10:24, Stefan Hajnoczi wrote:
 On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> It tests proper queue size settings for all available machine types.
>
> Signed-off-by: Denis Plotnikov 
> ---
>   tests/qemu-iotests/267 | 154 +
>   tests/qemu-iotests/267.out |   1 +
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 156 insertions(+)
>   create mode 100755 tests/qemu-iotests/267
>   create mode 100644 tests/qemu-iotests/267.out
 The qemu-iotests maintainers might prefer for this to be at the
 top-level in tests/ since it's not really an iotest, but the code itself
 looks fine to me:

 Reviewed-by: Stefan Hajnoczi 
>>> Good question.  I don’t really mind, but it would be weird if started
>>> adding all kinds of “external” qemu tests (i.e. that use QMP) in the
>>> iotests directory.
>>>
>>> What is the alternative?  Just putting it in a different directory
>>> doesn’t sound that appealing to me either, because it would still depend
>>> on the iotests infrastructure, right?  (i.e., iotests.py and check)
>> We do have tests/acceptance for simple test cases written in
>> Python.  What's the reason for this test case to depend on the
>> iotests infrastructure?
>>
>> -- 
>> Eduardo
> This test does look similar in spirit to "tests/acceptance/virtio_version.py".
>
> Denis,
>
> If you think this is more of a generic test than an IO test, and would
> rather want to have it a more agnostic location, I can provide you
> with tips (or a patch) to do so.

It would be great! Thanks!

Denis

>
> Cheers,
> - Cleber.
>

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Cleber Rosa]

On Wed, Nov 06, 2019 at 04:26:41PM -0300, Eduardo Habkost wrote:
> On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
> > On 06.11.19 10:24, Stefan Hajnoczi wrote:
> > > On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> > >> It tests proper queue size settings for all available machine types.
> > >>
> > >> Signed-off-by: Denis Plotnikov 
> > >> ---
> > >>  tests/qemu-iotests/267 | 154 +
> > >>  tests/qemu-iotests/267.out |   1 +
> > >>  tests/qemu-iotests/group   |   1 +
> > >>  3 files changed, 156 insertions(+)
> > >>  create mode 100755 tests/qemu-iotests/267
> > >>  create mode 100644 tests/qemu-iotests/267.out
> > > 
> > > The qemu-iotests maintainers might prefer for this to be at the
> > > top-level in tests/ since it's not really an iotest, but the code itself
> > > looks fine to me:
> > > 
> > > Reviewed-by: Stefan Hajnoczi 
> > 
> > Good question.  I don’t really mind, but it would be weird if started
> > adding all kinds of “external” qemu tests (i.e. that use QMP) in the
> > iotests directory.
> > 
> > What is the alternative?  Just putting it in a different directory
> > doesn’t sound that appealing to me either, because it would still depend
> > on the iotests infrastructure, right?  (i.e., iotests.py and check)
> 
> We do have tests/acceptance for simple test cases written in
> Python.  What's the reason for this test case to depend on the
> iotests infrastructure?
> 
> -- 
> Eduardo

This test does look similar in spirit to "tests/acceptance/virtio_version.py".

Denis,

If you think this is more of a generic test than an IO test, and would
rather want to have it a more agnostic location, I can provide you
with tips (or a patch) to do so.

Cheers,
- Cleber.

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Eduardo Habkost]

On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
> On 06.11.19 10:24, Stefan Hajnoczi wrote:
> > On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> >> It tests proper queue size settings for all available machine types.
> >>
> >> Signed-off-by: Denis Plotnikov 
> >> ---
> >>  tests/qemu-iotests/267 | 154 +
> >>  tests/qemu-iotests/267.out |   1 +
> >>  tests/qemu-iotests/group   |   1 +
> >>  3 files changed, 156 insertions(+)
> >>  create mode 100755 tests/qemu-iotests/267
> >>  create mode 100644 tests/qemu-iotests/267.out
> > 
> > The qemu-iotests maintainers might prefer for this to be at the
> > top-level in tests/ since it's not really an iotest, but the code itself
> > looks fine to me:
> > 
> > Reviewed-by: Stefan Hajnoczi 
> 
> Good question.  I don’t really mind, but it would be weird if started
> adding all kinds of “external” qemu tests (i.e. that use QMP) in the
> iotests directory.
> 
> What is the alternative?  Just putting it in a different directory
> doesn’t sound that appealing to me either, because it would still depend
> on the iotests infrastructure, right?  (i.e., iotests.py and check)

We do have tests/acceptance for simple test cases written in
Python.  What's the reason for this test case to depend on the
iotests infrastructure?

-- 
Eduardo

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Max Reitz]

On 06.11.19 10:24, Stefan Hajnoczi wrote:
> On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
>> It tests proper queue size settings for all available machine types.
>>
>> Signed-off-by: Denis Plotnikov 
>> ---
>>  tests/qemu-iotests/267 | 154 +
>>  tests/qemu-iotests/267.out |   1 +
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 156 insertions(+)
>>  create mode 100755 tests/qemu-iotests/267
>>  create mode 100644 tests/qemu-iotests/267.out
> 
> The qemu-iotests maintainers might prefer for this to be at the
> top-level in tests/ since it's not really an iotest, but the code itself
> looks fine to me:
> 
> Reviewed-by: Stefan Hajnoczi 

Good question.  I don’t really mind, but it would be weird if started
adding all kinds of “external” qemu tests (i.e. that use QMP) in the
iotests directory.

What is the alternative?  Just putting it in a different directory
doesn’t sound that appealing to me either, because it would still depend
on the iotests infrastructure, right?  (i.e., iotests.py and check)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Stefan Hajnoczi]

On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> It tests proper queue size settings for all available machine types.
> 
> Signed-off-by: Denis Plotnikov 
> ---
>  tests/qemu-iotests/267 | 154 +
>  tests/qemu-iotests/267.out |   1 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 156 insertions(+)
>  create mode 100755 tests/qemu-iotests/267
>  create mode 100644 tests/qemu-iotests/267.out

The qemu-iotests maintainers might prefer for this to be at the
top-level in tests/ since it's not really an iotest, but the code itself
looks fine to me:

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

[PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Denis Plotnikov]

It tests proper queue size settings for all available machine types.

Signed-off-by: Denis Plotnikov 
---
 tests/qemu-iotests/267 | 154 +
 tests/qemu-iotests/267.out |   1 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 156 insertions(+)
 create mode 100755 tests/qemu-iotests/267
 create mode 100644 tests/qemu-iotests/267.out

diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
new file mode 100755
index 00..6d3cc574b3
--- /dev/null
+++ b/tests/qemu-iotests/267
@@ -0,0 +1,154 @@
+#!/usr/bin/env python
+#
+# Test virtio-scsi and virtio-blk queue settings for all machine types
+#
+# Copyright (c) 2019 Virtuozzo International GmbH
+#
+# 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 .
+#
+
+import sys
+import os
+import re
+import iotests
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
+import qemu
+
+# list of device types and virtqueue properties to test
+# this is a generalized approach
+# for now we just check queue length
+# more properties can be added in each list
+virtio_scsi_props = {'vq_size': 'virtqueue_size'}
+virtio_blk_props = {'vq_size': 'queue-size'}
+
+dev_types = {'virtio-scsi-pci': virtio_scsi_props,
+ 'virtio-blk-pci': virtio_blk_props}
+
+vm_dev_params = {'virtio-scsi-pci': ['-device', 'virtio-scsi-pci,id=scsi0'],
+ 'virtio-blk-pci': ['-device',
+'virtio-blk-pci,id=scsi0,drive=drive0',
+'-drive',
+'driver=null-co,id=drive0,if=none']}
+
+def make_pattern(props):
+ pattern_items = ['{0} = \d+'.format(prop) for prop in props]
+ return '|'.join(pattern_items)
+
+
+def query_virtqueue_props(vm, dev_type_name):
+output = vm.qmp('human-monitor-command', command_line='info qtree')
+output = output['return']
+
+props_list = dev_types[dev_type_name].values();
+
+pattern = make_pattern(props_list)
+
+res = re.findall(pattern, output)
+
+if len(res) != len(props_list):
+not_found = props_list.difference(set(res))
+
+ret = (0, '({0}): The following properties not found: {1}'
+  .format(dev_type_name, ', '.join(not_found)))
+else:
+props = dict()
+for prop in res:
+p = prop.split(' = ')
+props[p[0]] = int(p[1])
+ret = (1, props)
+
+return ret
+
+
+def check_mt(mt, dev_type_name):
+vm_params = ['-machine', mt['name']] + vm_dev_params[dev_type_name]
+
+vm = qemu.QEMUMachine(iotests.qemu_prog, vm_params)
+vm.launch()
+ret = query_virtqueue_props(vm, dev_type_name)
+vm.shutdown()
+
+if ret[0] == 0:
+print('Error ({0}): {1}'.format(mt['name'], ret[1]))
+return 1
+
+errors = 0
+props = ret[1]
+
+for prop_name, prop_val in props.items():
+if mt[prop_name] != prop_val:
+print('Error [{0}, {1}]: {2}={3} (expected {4})'.
+  format(mt['name'], dev_type_name, prop_name, prop_val,
+ mt[prop_name]))
+errors += 1
+
+return errors
+
+def is_256_virtqueue_size_mt(mt):
+mt = mt.split("-")
+
+# machine types like pc-x.x
+if len(mt) == 2:
+return False
+
+# machine types like pc--x.x[.x]
+ver = mt[2]
+
+ver = ver.split(".");
+
+# all versions greater than 4.0.1 goes with 256 queue size
+if int(ver[0]) >= 4:
+major = int(ver[1])
+minor = 0
+if len(ver) > 2:
+minor = int(ver[2])
+
+if major > 0 or minor > 1:
+ return True
+
+return False
+
+
+# collect all machine types except 'none'
+vm = iotests.VM()
+vm.launch()
+machines = [m['name'] for m in vm.qmp('query-machines')['return']]
+vm.shutdown()
+machines.remove('none')
+machines.remove('isapc')
+
+failed = 0
+
+for dev_type in dev_types:
+# create a list of machine types and their parameters
+# machine types vz8.X.X must have virtqueue_length=256
+# others must have virtqueue_length=128
+mtypes = list()
+for m in machines:
+if is_256_virtqueue_size_mt(m):
+vq_size = 256
+else:
+vq_size = 128
+
+mtypes.append({'name': m, dev_types[dev_type]['vq_size']: vq_size})
+
+# test each machine type
+for mt in mtypes:
+failed += check_mt(mt,