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

[Fam Zheng <famz@redhat.com>]

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