[Qemu-devel] [PATCH] throttle-groups: update tg->any_timer_armed[] on detach

[Qemu-devel] [PATCH] throttle-groups: update tg->any_timer_armed[] on detach

[Stefan Hajnoczi]

Clear tg->any_timer_armed[] when throttling timers are destroy during
AioContext attach/detach.  Failure to do so causes throttling to hang
because we believe the timer is already scheduled!

The following was broken at least since QEMU 2.10.0 with -drive
iops=100:

  $ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
  (qemu) stop
  (qemu) cont
  ...I/O is stuck...

Reported-by: Yongxue Hong <yhong@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/throttle-groups.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 6ba992c8d7..2bfd03faa0 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -592,7 +592,20 @@ void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
 void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
 {
     ThrottleTimers *tt = &tgm->throttle_timers;
+    ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
+
     throttle_timers_detach_aio_context(tt);
+
+    /* Forget about these timers, they have been destroyed */
+    qemu_mutex_lock(&tg->lock);
+    if (tg->tokens[0] == tgm) {
+        tg->any_timer_armed[0] = false;
+    }
+    if (tg->tokens[1] == tgm) {
+        tg->any_timer_armed[1] = false;
+    }
+    qemu_mutex_unlock(&tg->lock);
+
     tgm->aio_context = NULL;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH] throttle-groups: update tg->any_timer_armed[] on detach

[Eric Blake]

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

On 09/19/2017 10:50 AM, Stefan Hajnoczi wrote:
> Clear tg->any_timer_armed[] when throttling timers are destroy during

s/destroy/destroyed/

> AioContext attach/detach.  Failure to do so causes throttling to hang
> because we believe the timer is already scheduled!
> 
> The following was broken at least since QEMU 2.10.0 with -drive
> iops=100:
> 
>   $ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
>   (qemu) stop
>   (qemu) cont
>   ...I/O is stuck...
> 
> Reported-by: Yongxue Hong <yhong@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/throttle-groups.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

CC: qemu-stable@nongnu.org

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] throttle-groups: update tg->any_timer_armed[] on detach

[Stefan Hajnoczi]

On Tue, Sep 19, 2017 at 11:07:53AM -0500, Eric Blake wrote:
> On 09/19/2017 10:50 AM, Stefan Hajnoczi wrote:
> > Clear tg->any_timer_armed[] when throttling timers are destroy during
> 
> s/destroy/destroyed/

All your base are belong to us!

Re: [Qemu-devel] [PATCH] throttle-groups: update tg->any_timer_armed[] on detach

[Alberto Garcia]

On Tue 19 Sep 2017 05:50:25 PM CEST, Stefan Hajnoczi wrote:
> Clear tg->any_timer_armed[] when throttling timers are destroy during
> AioContext attach/detach.  Failure to do so causes throttling to hang
> because we believe the timer is already scheduled!
>
> The following was broken at least since QEMU 2.10.0 with -drive
> iops=100:
>
>   $ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
>   (qemu) stop
>   (qemu) cont
>   ...I/O is stuck...
>
> Reported-by: Yongxue Hong <yhong@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/throttle-groups.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 6ba992c8d7..2bfd03faa0 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -592,7 +592,20 @@ void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
>  void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
>  {
>      ThrottleTimers *tt = &tgm->throttle_timers;
> +    ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
> +
>      throttle_timers_detach_aio_context(tt);
> +
> +    /* Forget about these timers, they have been destroyed */
> +    qemu_mutex_lock(&tg->lock);
> +    if (tg->tokens[0] == tgm) {
> +        tg->any_timer_armed[0] = false;
> +    }
> +    if (tg->tokens[1] == tgm) {
> +        tg->any_timer_armed[1] = false;
> +    }
> +    qemu_mutex_unlock(&tg->lock);

I think I'd rather check the timers directly using timer_pending().

See https://github.com/qemu/qemu/blob/dbe824cc5/block/throttle-groups.c#L426

Berto

Re: [Qemu-devel] [PATCH] throttle-groups: update tg->any_timer_armed[] on detach

[Michael Roth]

Quoting Stefan Hajnoczi (2017-09-19 10:50:25)
> Clear tg->any_timer_armed[] when throttling timers are destroy during
> AioContext attach/detach.  Failure to do so causes throttling to hang
> because we believe the timer is already scheduled!
> 
> The following was broken at least since QEMU 2.10.0 with -drive
> iops=100:
> 
>   $ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
>   (qemu) stop
>   (qemu) cont
>   ...I/O is stuck...
> 
> Reported-by: Yongxue Hong <yhong@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

FYI: this patch has been tagged for stable 2.10.1, but is not yet
upstream. Patch freeze for 2.10.1 is September 27th.

> ---
>  block/throttle-groups.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 6ba992c8d7..2bfd03faa0 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -592,7 +592,20 @@ void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
>  void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
>  {
>      ThrottleTimers *tt = &tgm->throttle_timers;
> +    ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
> +
>      throttle_timers_detach_aio_context(tt);
> +
> +    /* Forget about these timers, they have been destroyed */
> +    qemu_mutex_lock(&tg->lock);
> +    if (tg->tokens[0] == tgm) {
> +        tg->any_timer_armed[0] = false;
> +    }
> +    if (tg->tokens[1] == tgm) {
> +        tg->any_timer_armed[1] = false;
> +    }
> +    qemu_mutex_unlock(&tg->lock);
> +
>      tgm->aio_context = NULL;
>  }
> 
> -- 
> 2.13.5
> 
>