[v2] qemu: send stop event after bdrv_flush_all

[PATCH v2] qemu: send stop event after bdrv_flush_all

Posted by tianren@smartx.com 11 months, 3 weeks ago

From: Tianren Zhang <tianren@smartx.com>

The stop process is not finished until bdrv_flush_all
is done. Some users (e.g., libvirt) detect the STOP
event and invokes some lock release logic to revoke
the disk lock held by current qemu when such event is
emitted. In such case, if the bdrv_flush_all is after
the stop event, it's possible that the disk lock is
released while the qemu is still waiting for I/O.
Therefore, it's better to have the stop event generated
after the whole stop process is done, so we can
guarantee to users that the stop process is finished
when they get the STOP event.

Change-Id: Ia2f95cd55edfdeb71ee2e04005ac216cfabffa22
Signed-off-by: Tianren Zhang <tianren@smartx.com>
---
v2: do not call runstate_is_running twice
---
 system/cpus.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/system/cpus.c b/system/cpus.c
index a444a747f0..49af0f92b5 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -262,21 +262,24 @@ void cpu_interrupt(CPUState *cpu, int mask)
 static int do_vm_stop(RunState state, bool send_stop)
 {
     int ret = 0;
+    bool do_send_stop = false;
 
     if (runstate_is_running()) {
         runstate_set(state);
         cpu_disable_ticks();
         pause_all_vcpus();
         vm_state_notify(0, state);
-        if (send_stop) {
-            qapi_event_send_stop();
-        }
+        do_send_stop = send_stop;
     }
 
     bdrv_drain_all();
     ret = bdrv_flush_all();
     trace_vm_stop_flush_all(ret);
 
+    if (do_send_stop) {
+        qapi_event_send_stop();
+    }
+
     return ret;
 }
 
-- 
2.41.0

Re: [PATCH v2] qemu: send stop event after bdrv_flush_all

Posted by Daniel P. Berrangé 11 months, 3 weeks ago

On Tue, Dec 05, 2023 at 01:48:26AM -0500, tianren@smartx.com wrote:
> From: Tianren Zhang <tianren@smartx.com>
> 
> The stop process is not finished until bdrv_flush_all
> is done. Some users (e.g., libvirt) detect the STOP
> event and invokes some lock release logic to revoke
> the disk lock held by current qemu when such event is
> emitted. In such case, if the bdrv_flush_all is after
> the stop event, it's possible that the disk lock is
> released while the qemu is still waiting for I/O.
> Therefore, it's better to have the stop event generated
> after the whole stop process is done, so we can
> guarantee to users that the stop process is finished
> when they get the STOP event.
>
> Change-Id: Ia2f95cd55edfdeb71ee2e04005ac216cfabffa22

Please don't include this in commit messages for patches posted.
QEMU does not use Gerrit and so this is irrelevant to upstream.

> Signed-off-by: Tianren Zhang <tianren@smartx.com>
> ---
> v2: do not call runstate_is_running twice
> ---
>  system/cpus.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/system/cpus.c b/system/cpus.c
> index a444a747f0..49af0f92b5 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -262,21 +262,24 @@ void cpu_interrupt(CPUState *cpu, int mask)
>  static int do_vm_stop(RunState state, bool send_stop)
>  {
>      int ret = 0;
> +    bool do_send_stop = false;
>  
>      if (runstate_is_running()) {
>          runstate_set(state);
>          cpu_disable_ticks();
>          pause_all_vcpus();
>          vm_state_notify(0, state);
> -        if (send_stop) {
> -            qapi_event_send_stop();
> -        }
> +        do_send_stop = send_stop;
>      }
>  
>      bdrv_drain_all();
>      ret = bdrv_flush_all();
>      trace_vm_stop_flush_all(ret);
>  
> +    if (do_send_stop) {
> +        qapi_event_send_stop();
> +    }
> +
>      return ret;
>  }

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] qemu: send stop event after bdrv_flush_all

Posted by Tianren Zhang 11 months, 3 weeks ago

Thanks for the update. I have sent the v3 removing the irrelevant
information:
https://patchew.org/QEMU/20231205091903.3640-1-tianren@smartx.com/

Best,
Tianren Zhang

On Tue, Dec 5, 2023 at 4:14 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Dec 05, 2023 at 01:48:26AM -0500, tianren@smartx.com wrote:
> > From: Tianren Zhang <tianren@smartx.com>
> >
> > The stop process is not finished until bdrv_flush_all
> > is done. Some users (e.g., libvirt) detect the STOP
> > event and invokes some lock release logic to revoke
> > the disk lock held by current qemu when such event is
> > emitted. In such case, if the bdrv_flush_all is after
> > the stop event, it's possible that the disk lock is
> > released while the qemu is still waiting for I/O.
> > Therefore, it's better to have the stop event generated
> > after the whole stop process is done, so we can
> > guarantee to users that the stop process is finished
> > when they get the STOP event.
> >
> > Change-Id: Ia2f95cd55edfdeb71ee2e04005ac216cfabffa22
>
> Please don't include this in commit messages for patches posted.
> QEMU does not use Gerrit and so this is irrelevant to upstream.
>
> > Signed-off-by: Tianren Zhang <tianren@smartx.com>
> > ---
> > v2: do not call runstate_is_running twice
> > ---
> >  system/cpus.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> >
> > diff --git a/system/cpus.c b/system/cpus.c
> > index a444a747f0..49af0f92b5 100644
> > --- a/system/cpus.c
> > +++ b/system/cpus.c
> > @@ -262,21 +262,24 @@ void cpu_interrupt(CPUState *cpu, int mask)
> >  static int do_vm_stop(RunState state, bool send_stop)
> >  {
> >      int ret = 0;
> > +    bool do_send_stop = false;
> >
> >      if (runstate_is_running()) {
> >          runstate_set(state);
> >          cpu_disable_ticks();
> >          pause_all_vcpus();
> >          vm_state_notify(0, state);
> > -        if (send_stop) {
> > -            qapi_event_send_stop();
> > -        }
> > +        do_send_stop = send_stop;
> >      }
> >
> >      bdrv_drain_all();
> >      ret = bdrv_flush_all();
> >      trace_vm_stop_flush_all(ret);
> >
> > +    if (do_send_stop) {
> > +        qapi_event_send_stop();
> > +    }
> > +
> >      return ret;
> >  }
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>