[Qemu-devel] [PATCH v4] block: Switch to host monotonic clock for IO throttling

[Qemu-devel] [PATCH v4] block: Switch to host monotonic clock for IO throttling

[Fam Zheng]

Currently, throttle timers won't make any progress when VCPU is not
running, which would stall the request queue in utils, qtest, vm
suspending, and live migration, without special handling.

Block jobs are confusingly inconsistent between with and without
throttling: if user sets a bps limit, stops the vm, then start a block
job, the block job will not make any progress; in contrary, if user
unsets the bps limit, or if it's not set, the block job will run
normally.

After this patch, with the host clock, even if the VCPUs are stopped,
the throttle queues will be processed.

This patch also enables potential to add throttle to bdrv_drain_all.
Currently all requests are drained immediately. In other words whenever
it is called, IO throttling goes ineffective (examples: system reset,
migration and many block job operations.). This is a loophole that guest
could exploit. If we use the host clock, we can later just trust the
nested poll. This could be done on top.

Note that for qemu-iotests case 093, which uses qtest, we still keep vm
clock so the script can control the clock stepping in order to be
deterministic.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Fam Zheng <famz@redhat.com>

---
v4: Fix the description. [Alberto]
v3: More justification in commit message. [Stefan]
    Add Paolo's and Alberto's rev-bys.
v2: Don't break qemu-iotests 093.
---
 block.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 0fe97de..89a1d5b 100644
--- a/block.c
+++ b/block.c
@@ -30,6 +30,7 @@
 #include "qapi/qmp/qjson.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/qtest.h"
 #include "qemu/notify.h"
 #include "block/coroutine.h"
 #include "block/qapi.h"
@@ -181,10 +182,16 @@ static void bdrv_throttle_write_timer_cb(void *opaque)
 /* should be called before bdrv_set_io_limits if a limit is set */
 void bdrv_io_limits_enable(BlockDriverState *bs)
 {
+    int clock_type = QEMU_CLOCK_REALTIME;
+
+    if (qtest_enabled()) {
+        /* For testing block IO throttling only */
+        clock_type = QEMU_CLOCK_VIRTUAL;
+    }
     assert(!bs->io_limits_enabled);
     throttle_init(&bs->throttle_state,
                   bdrv_get_aio_context(bs),
-                  QEMU_CLOCK_VIRTUAL,
+                  clock_type,
                   bdrv_throttle_read_timer_cb,
                   bdrv_throttle_write_timer_cb,
                   bs);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4] block: Switch to host monotonic clock for IO throttling

[Stefan Hajnoczi]

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

On Wed, Mar 25, 2015 at 03:27:26PM +0800, Fam Zheng wrote:
> Currently, throttle timers won't make any progress when VCPU is not
> running, which would stall the request queue in utils, qtest, vm
> suspending, and live migration, without special handling.
> 
> Block jobs are confusingly inconsistent between with and without
> throttling: if user sets a bps limit, stops the vm, then start a block
> job, the block job will not make any progress; in contrary, if user
> unsets the bps limit, or if it's not set, the block job will run
> normally.
> 
> After this patch, with the host clock, even if the VCPUs are stopped,
> the throttle queues will be processed.
> 
> This patch also enables potential to add throttle to bdrv_drain_all.
> Currently all requests are drained immediately. In other words whenever
> it is called, IO throttling goes ineffective (examples: system reset,
> migration and many block job operations.). This is a loophole that guest
> could exploit. If we use the host clock, we can later just trust the
> nested poll. This could be done on top.
> 
> Note that for qemu-iotests case 093, which uses qtest, we still keep vm
> clock so the script can control the clock stepping in order to be
> deterministic.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> v4: Fix the description. [Alberto]
> v3: More justification in commit message. [Stefan]
>     Add Paolo's and Alberto's rev-bys.
> v2: Don't break qemu-iotests 093.
> ---
>  block.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Queued for QEMU 2.4.  Please let me know if you think it's worth risking
adding it in QEMU 2.3.

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

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

Re: [Qemu-devel] [PATCH v4] block: Switch to host monotonic clock for IO throttling

[Fam Zheng]

On Wed, 03/25 15:31, Stefan Hajnoczi wrote:
> Queued for QEMU 2.4.  Please let me know if you think it's worth risking
> adding it in QEMU 2.3.

Thanks, 2.4 is okay.

Fam