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

[Qemu-devel] [PATCH v2] 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.

For example in bdrv_drain_all, all requests are resumed immediately
without taking throttling limit into account. This means whenever it is
called, IO throttling goes ineffective (examples: system reset,
migration and many block job operations.).

This might be some loophole that guest could exploit.

If we use the host clock, we can later just trust the nested poll when
waiting for requests.

Note that for qemu-iotests case 093, which sets up qtest when running
QEMU, we still use vm clock so the script can control the clock stepping
in order to be deterministic.

Signed-off-by: Fam Zheng <famz@redhat.com>

---
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 v2] block: Switch to host monotonic clock for IO throttling

[Paolo Bonzini]

On 23/03/2015 06:24, 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.
> 
> For example in bdrv_drain_all, all requests are resumed immediately
> without taking throttling limit into account. This means whenever it is
> called, IO throttling goes ineffective (examples: system reset,
> migration and many block job operations.).
> 
> This might be some loophole that guest could exploit.
> 
> If we use the host clock, we can later just trust the nested poll when
> waiting for requests.
> 
> Note that for qemu-iotests case 093, which sets up qtest when running
> QEMU, we still use vm clock so the script can control the clock stepping
> in order to be deterministic.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

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

[Alberto Garcia]

On Mon, Mar 23, 2015 at 01:24:15PM +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.
> 
> For example in bdrv_drain_all, all requests are resumed immediately
> without taking throttling limit into account. This means whenever it is
> called, IO throttling goes ineffective (examples: system reset,
> migration and many block job operations.).
> 
> This might be some loophole that guest could exploit.
> 
> If we use the host clock, we can later just trust the nested poll when
> waiting for requests.
> 
> Note that for qemu-iotests case 093, which sets up qtest when running
> QEMU, we still use vm clock so the script can control the clock stepping
> in order to be deterministic.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Stefan Hajnoczi]

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

On Mon, Mar 23, 2015 at 01:24:15PM +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.
> 
> For example in bdrv_drain_all, all requests are resumed immediately
> without taking throttling limit into account. This means whenever it is
> called, IO throttling goes ineffective (examples: system reset,
> migration and many block job operations.).
> 
> This might be some loophole that guest could exploit.
> 
> If we use the host clock, we can later just trust the nested poll when
> waiting for requests.
> 
> Note that for qemu-iotests case 093, which sets up qtest when running
> QEMU, we still use vm clock so the script can control the clock stepping
> in order to be deterministic.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> v2: Don't break qemu-iotests 093.
> ---
>  block.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Should we make an exception for live migration to reduce downtime?

I'm concerned that now vm_stop() can take even longer since we'll wait
for throttling.

Stefan

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

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

[Paolo Bonzini]

On 23/03/2015 14:56, Stefan Hajnoczi wrote:
> On Mon, Mar 23, 2015 at 01:24:15PM +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.
>>
>> For example in bdrv_drain_all, all requests are resumed immediately
>> without taking throttling limit into account. This means whenever it is
>> called, IO throttling goes ineffective (examples: system reset,
>> migration and many block job operations.).
>>
>> This might be some loophole that guest could exploit.
>>
>> If we use the host clock, we can later just trust the nested poll when
>> waiting for requests.
>>
>> Note that for qemu-iotests case 093, which sets up qtest when running
>> QEMU, we still use vm clock so the script can control the clock stepping
>> in order to be deterministic.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>
>> ---
>> v2: Don't break qemu-iotests 093.
>> ---
>>  block.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> Should we make an exception for live migration to reduce downtime?
> 
> I'm concerned that now vm_stop() can take even longer since we'll wait
> for throttling.

What would have prevented the wait before?  (In fact I'm not sure why we
would have even terminated bdrv_drain_all, since we run it when
QEMU_CLOCK_VIRTUAL is not advancing anymore!).

Paolo

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

[Stefan Hajnoczi]

On Mon, Mar 23, 2015 at 1:58 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 23/03/2015 14:56, Stefan Hajnoczi wrote:
>> On Mon, Mar 23, 2015 at 01:24:15PM +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.
>>>
>>> For example in bdrv_drain_all, all requests are resumed immediately
>>> without taking throttling limit into account. This means whenever it is
>>> called, IO throttling goes ineffective (examples: system reset,
>>> migration and many block job operations.).
>>>
>>> This might be some loophole that guest could exploit.
>>>
>>> If we use the host clock, we can later just trust the nested poll when
>>> waiting for requests.
>>>
>>> Note that for qemu-iotests case 093, which sets up qtest when running
>>> QEMU, we still use vm clock so the script can control the clock stepping
>>> in order to be deterministic.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>
>>> ---
>>> v2: Don't break qemu-iotests 093.
>>> ---
>>>  block.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> Should we make an exception for live migration to reduce downtime?
>>
>> I'm concerned that now vm_stop() can take even longer since we'll wait
>> for throttling.
>
> What would have prevented the wait before?  (In fact I'm not sure why we
> would have even terminated bdrv_drain_all, since we run it when
> QEMU_CLOCK_VIRTUAL is not advancing anymore!).

Hang on, I just noticed that bdrv_drain_one() disables throttling:

    bs->io_limits_enabled = false;

    for (i = 0; i < 2; i++) {
        while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
            drained = true;
        }
    }

    bs->io_limits_enabled = enabled;

So this patch does not make bdrv_drain_all() honor I/O throttling
limits.  I find the commit description confusing when it mentions
bdrv_drain_all().

What this patch really does is to all throttled I/O requests after
vm_stop() or when the CPU was never running in the first place.

The reason why the virtual clock was chosen for throttling is to
guarantee that we never violate the assumption that device backends
are stopped when vcpus are stopped.

I'd like to know what problems exactly this patch fixes.  The test
case for throttling actually relies on the virtual clock so it can be
stepped deterministically.  What other legitimate cases are there
where throttling is used outside a running qemu-system process?

Stefan

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

[Paolo Bonzini]

On 23/03/2015 15:28, Stefan Hajnoczi wrote:
> I'd like to know what problems exactly this patch fixes.  The test
> case for throttling actually relies on the virtual clock so it can be
> stepped deterministically.  What other legitimate cases are there
> where throttling is used outside a running qemu-system process?

Alberto brought up block jobs.  It is debatable whether block jobs
should be stopped while the VM is.

Paolo

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

[Stefan Hajnoczi]

On Mon, Mar 23, 2015 at 2:49 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 23/03/2015 15:28, Stefan Hajnoczi wrote:
>> I'd like to know what problems exactly this patch fixes.  The test
>> case for throttling actually relies on the virtual clock so it can be
>> stepped deterministically.  What other legitimate cases are there
>> where throttling is used outside a running qemu-system process?
>
> Alberto brought up block jobs.  It is debatable whether block jobs
> should be stopped while the VM is.

This patch is unrelated to block jobs.  Block jobs use their own
ratelimiting mechanism and the realtime clock (not vm clock).

Stefan

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

[Paolo Bonzini]

On 23/03/2015 16:00, Stefan Hajnoczi wrote:
> On Mon, Mar 23, 2015 at 2:49 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 23/03/2015 15:28, Stefan Hajnoczi wrote:
>>> I'd like to know what problems exactly this patch fixes.  The test
>>> case for throttling actually relies on the virtual clock so it can be
>>> stepped deterministically.  What other legitimate cases are there
>>> where throttling is used outside a running qemu-system process?
>>
>> Alberto brought up block jobs.  It is debatable whether block jobs
>> should be stopped while the VM is.
> 
> This patch is unrelated to block jobs.  Block jobs use their own
> ratelimiting mechanism and the realtime clock (not vm clock).

Block jobs are also influenced by throttling though, if their underlying
BDS is throttled.  For example, streaming works by doing reads on a BDS
that is also visible to the VM and might be throttled.  Same for
mirroring or active-layer commit.

Paolo

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

[Fam Zheng]

On Mon, 03/23 14:28, Stefan Hajnoczi wrote:
> On Mon, Mar 23, 2015 at 1:58 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 23/03/2015 14:56, Stefan Hajnoczi wrote:
> >> On Mon, Mar 23, 2015 at 01:24:15PM +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.
> >>>
> >>> For example in bdrv_drain_all, all requests are resumed immediately
> >>> without taking throttling limit into account. This means whenever it is
> >>> called, IO throttling goes ineffective (examples: system reset,
> >>> migration and many block job operations.).
> >>>
> >>> This might be some loophole that guest could exploit.
> >>>
> >>> If we use the host clock, we can later just trust the nested poll when
> >>> waiting for requests.
> >>>
> >>> Note that for qemu-iotests case 093, which sets up qtest when running
> >>> QEMU, we still use vm clock so the script can control the clock stepping
> >>> in order to be deterministic.
> >>>
> >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>>
> >>> ---
> >>> v2: Don't break qemu-iotests 093.
> >>> ---
> >>>  block.c | 9 ++++++++-
> >>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> Should we make an exception for live migration to reduce downtime?
> >>
> >> I'm concerned that now vm_stop() can take even longer since we'll wait
> >> for throttling.
> >
> > What would have prevented the wait before?  (In fact I'm not sure why we
> > would have even terminated bdrv_drain_all, since we run it when
> > QEMU_CLOCK_VIRTUAL is not advancing anymore!).
> 
> Hang on, I just noticed that bdrv_drain_one() disables throttling:
> 
>     bs->io_limits_enabled = false;
> 
>     for (i = 0; i < 2; i++) {
>         while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
>             drained = true;
>         }
>     }
> 
>     bs->io_limits_enabled = enabled;
> 
> So this patch does not make bdrv_drain_all() honor I/O throttling
> limits.  I find the commit description confusing when it mentions
> bdrv_drain_all().
> 
> What this patch really does is to all throttled I/O requests after
> vm_stop() or when the CPU was never running in the first place.

This patch has no effect even on that - before vm_stop we have
bdrv_drain_all(), right :)

See below for the reason of this change.

> 
> The reason why the virtual clock was chosen for throttling is to
> guarantee that we never violate the assumption that device backends
> are stopped when vcpus are stopped.

I don't get this. Doesn't bdrv_drain_all() in do_vm_stop guarantees that
already?

> 
> I'd like to know what problems exactly this patch fixes.

This patch is necessary, though not enough, to make bdrv_drain_all honor IO
throttling. It currently disables throttling, which is bad.

A malicious guest could use a loop that keep triggering bdrv_drain_all() so
that many requests can go beyond throttling, for example live snapshot.

In order to fix that we should remove the throttling disabling code above, but
it's not possible without this patch. Otherwise vm_stop and many other things
would not work.

Alberto saw that this patch also fixes the odd behavior: block jobs, which need
to R/W a throttled BDS, will not make progress if VCPU is not running. If we
don't consider this as a bug, we should document the inconsistency (confusion):
if no throttling, they DO make progress after VCPU stopped.

Fam

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

[Stefan Hajnoczi]

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

On Tue, Mar 24, 2015 at 09:17:38AM +0800, Fam Zheng wrote:
> Alberto saw that this patch also fixes the odd behavior: block jobs, which need
> to R/W a throttled BDS, will not make progress if VCPU is not running. If we
> don't consider this as a bug, we should document the inconsistency (confusion):
> if no throttling, they DO make progress after VCPU stopped.

This is a strong justification for the patch.  Please make this the main
point in the commit description.

Stefan

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