[PATCH for-7.1 0/2] throttle-groups: use QEMU_CLOCK_REALTIME

[PATCH for-7.1 0/2] throttle-groups: use QEMU_CLOCK_REALTIME

[Vladimir Sementsov-Ogievskiy]

Hi all!

Honestly, I don't know why QEMU_CLOCK_VIRTUAL is used here. Comment say
that that's specially for throttle tests, but the simple test (patch 02)
just hangs because QEMU_CLOCK_VIRTUAL clock just doesn't tick in this
environment.. And if we change the clock to QEMU_CLOCK_REALTIME, new
test works, and old tests work as well.

What the relation between QEMU_CLOCK_VIRTUAL, qtest and throttle?
This code is here since "throttle: Add throttle group support" of 2015..

Vladimir Sementsov-Ogievskiy (2):
  block/throttle-groups: use QEMU_CLOCK_REALTIME for qtest too
  iotests: add throttle test

 block/throttle-groups.c               |  4 ---
 tests/qemu-iotests/tests/throttle     | 50 +++++++++++++++++++++++++++
 tests/qemu-iotests/tests/throttle.out |  0
 3 files changed, 50 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/throttle
 create mode 100644 tests/qemu-iotests/tests/throttle.out

-- 
2.35.1

[PATCH 1/2] block/throttle-groups: use QEMU_CLOCK_REALTIME for qtest too

[Vladimir Sementsov-Ogievskiy]

Virtual clock just doesn't tick for iotests, and throttling just not
work. Let's use realtime clock.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/throttle-groups.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index fb203c3ced..029158d797 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -753,10 +753,6 @@ static void throttle_group_obj_init(Object *obj)
     ThrottleGroup *tg = THROTTLE_GROUP(obj);
 
     tg->clock_type = QEMU_CLOCK_REALTIME;
-    if (qtest_enabled()) {
-        /* For testing block IO throttling only */
-        tg->clock_type = QEMU_CLOCK_VIRTUAL;
-    }
     tg->is_initialized = false;
     qemu_mutex_init(&tg->lock);
     throttle_init(&tg->ts);
-- 
2.35.1

[PATCH 2/2] iotests: add throttle test

[Vladimir Sementsov-Ogievskiy]

Add simple test for throttle filter driver.

Without previous
"block/throttle-groups: use QEMU_CLOCK_REALTIME for qtest too"
commit the test hangs forever, because previously used VIRTUAL clock
just doesn't tick for iotests.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 tests/qemu-iotests/tests/throttle     | 50 +++++++++++++++++++++++++++
 tests/qemu-iotests/tests/throttle.out |  0
 2 files changed, 50 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/throttle
 create mode 100644 tests/qemu-iotests/tests/throttle.out

diff --git a/tests/qemu-iotests/tests/throttle b/tests/qemu-iotests/tests/throttle
new file mode 100755
index 0000000000..63b802cee7
--- /dev/null
+++ b/tests/qemu-iotests/tests/throttle
@@ -0,0 +1,50 @@
+#!/usr/bin/env python3
+# group: auto
+#
+# Simple test for throttle filter driver.
+#
+# Copyright (c) 2022 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 <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+
+with iotests.VM() as vm:
+    vm.launch()
+
+    result = vm.qmp('object-add', {
+        'qom-type': 'throttle-group',
+        'id': 'group0',
+        'limits': {'bps-write': 512 * 1024}
+    })
+    assert result == {'return': {}}
+
+    result = vm.qmp('blockdev-add', {
+        'node-name': 'throt',
+        'driver': 'throttle',
+        'throttle-group': 'group0',
+        'file': {
+            'driver': 'null-co',
+            'size': 1024 * 1024
+        }
+    })
+    assert result == {'return': {}}
+
+    result = vm.hmp_qemu_io('throt', 'write 0 512K')
+    assert result == {'return': ''}
+
+    # We need second write to trigger throttling
+    result = vm.hmp_qemu_io('throt', 'write 512K 512K')
+    assert result == {'return': ''}
diff --git a/tests/qemu-iotests/tests/throttle.out b/tests/qemu-iotests/tests/throttle.out
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.35.1

Re: [PATCH 1/2] block/throttle-groups: use QEMU_CLOCK_REALTIME for qtest too

[Hanna Reitz]

On 06.04.22 17:32, Vladimir Sementsov-Ogievskiy wrote:
> Virtual clock just doesn't tick for iotests, and throttling just not
> work. Let's use realtime clock.

It does tick when you make it take, specifically with the clock_step 
qtest command.  093 does this, and so with this patch, it fails, because 
it is no longer deterministic.

So far, if I needed realtime throttling, I simply switched the 
accelerator to tcg (e.g. in stream-error-on-reset).

I’m not really opposed to this, but it does break 093, and without 
looking too closely into it, I would guess that it’d be difficult to 
rewrite 093 in a deterministic way without it relying on throttling 
using the virtual clock.  (A runtime option for the throttle-group 
object to choose the clock type might be an option.)

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/throttle-groups.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index fb203c3ced..029158d797 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -753,10 +753,6 @@ static void throttle_group_obj_init(Object *obj)
>       ThrottleGroup *tg = THROTTLE_GROUP(obj);
>   
>       tg->clock_type = QEMU_CLOCK_REALTIME;
> -    if (qtest_enabled()) {
> -        /* For testing block IO throttling only */
> -        tg->clock_type = QEMU_CLOCK_VIRTUAL;
> -    }
>       tg->is_initialized = false;
>       qemu_mutex_init(&tg->lock);
>       throttle_init(&tg->ts);

Re: [PATCH 1/2] block/throttle-groups: use QEMU_CLOCK_REALTIME for qtest too

[Vladimir Sementsov-Ogievskiy]

Thanks for explanation!

07.04.2022 09:42, Hanna Reitz wrote:
> On 06.04.22 17:32, Vladimir Sementsov-Ogievskiy wrote:
>> Virtual clock just doesn't tick for iotests, and throttling just not
>> work. Let's use realtime clock.
> 
> It does tick when you make it take, specifically with the clock_step qtest command.  093 does this, and so with this patch, it fails, because it is no longer deterministic.
> 
> So far, if I needed realtime throttling, I simply switched the accelerator to tcg (e.g. in stream-error-on-reset).

Hm, I tried but it doesn't help (Add vm.add_args('-accel', 'tcg') before vm.launch() in the test), as " -accel qtest" is kept anyway, and therefore do_configure_accelerator is called for qtest and finally qtest_allowed is set to true.

But using QEMUMachine class instead of VM helps.

> 
> I’m not really opposed to this, but it does break 093, and without looking too closely into it, I would guess that it’d be difficult to rewrite 093 in a deterministic way without it relying on throttling using the virtual clock.  (A runtime option for the throttle-group object to choose the clock type might be an option.)

OK, I don't think we need these patches now.

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/throttle-groups.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
>> index fb203c3ced..029158d797 100644
>> --- a/block/throttle-groups.c
>> +++ b/block/throttle-groups.c
>> @@ -753,10 +753,6 @@ static void throttle_group_obj_init(Object *obj)
>>       ThrottleGroup *tg = THROTTLE_GROUP(obj);
>>       tg->clock_type = QEMU_CLOCK_REALTIME;
>> -    if (qtest_enabled()) {
>> -        /* For testing block IO throttling only */
>> -        tg->clock_type = QEMU_CLOCK_VIRTUAL;
>> -    }
>>       tg->is_initialized = false;
>>       qemu_mutex_init(&tg->lock);
>>       throttle_init(&tg->ts);
> 


-- 
Best regards,
Vladimir