[PATCH 0/2] monitor: Shutdown fixes

[PATCH 0/2] monitor: Shutdown fixes

[Kevin Wolf]

This fixes the bug(s) that Markus reported here:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07719.html

Kevin Wolf (2):
  monitor: Fix assertion failure on shutdown
  monitor/qmp: Stop processing requests when shutdown is requested

 monitor/monitor.c | 25 +++++++++++++++----------
 monitor/qmp.c     |  5 +++++
 2 files changed, 20 insertions(+), 10 deletions(-)

-- 
2.29.2

[PATCH 1/2] monitor: Fix assertion failure on shutdown

[Kevin Wolf]

Commit 357bda95 already tried to fix the order in monitor_cleanup() by
moving shutdown of the dispatcher coroutine further to the start.
However, it didn't go far enough:

iothread_stop() makes sure that all pending work (bottom halves) in the
AioContext of the monitor iothread is completed. iothread_destroy()
depends on this and fails an assertion if there is still a pending BH.

While the dispatcher coroutine is running, it will try to resume the
monitor after taking a request out of the queue, which involves a BH.
The dispatcher is run until it terminates in the AIO_WAIT_WHILE() loop.
However, adding new BHs between iothread_stop() and iothread_destroy()
is forbidden.

Fix this by stopping the dispatcher first before shutting down the other
parts of the monitor. This means we can now receive requests that aren't
handled any more when QEMU is shutting down, but this is unlikely to be
a problem for QMP clients.

Fixes: 357bda9590784ff75803d52de43150d4107ed98e
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/monitor.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 1e4a6b3f20..e94f532cf5 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -618,16 +618,6 @@ void monitor_data_destroy(Monitor *mon)
 
 void monitor_cleanup(void)
 {
-    /*
-     * We need to explicitly stop the I/O thread (but not destroy it),
-     * clean up the monitor resources, then destroy the I/O thread since
-     * we need to unregister from chardev below in
-     * monitor_data_destroy(), and chardev is not thread-safe yet
-     */
-    if (mon_iothread) {
-        iothread_stop(mon_iothread);
-    }
-
     /*
      * The dispatcher needs to stop before destroying the monitor and
      * the I/O thread.
@@ -637,6 +627,11 @@ void monitor_cleanup(void)
      * eventually terminates.  qemu_aio_context is automatically
      * polled by calling AIO_WAIT_WHILE on it, but we must poll
      * iohandler_ctx manually.
+     *
+     * Letting the iothread continue while shutting down the dispatcher
+     * means that new requests may still be coming in. This is okay,
+     * we'll just leave them in the queue without sending a response
+     * and monitor_data_destroy() will free them.
      */
     qmp_dispatcher_co_shutdown = true;
     if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) {
@@ -647,6 +642,16 @@ void monitor_cleanup(void)
                    (aio_poll(iohandler_get_aio_context(), false),
                     qatomic_mb_read(&qmp_dispatcher_co_busy)));
 
+    /*
+     * We need to explicitly stop the I/O thread (but not destroy it),
+     * clean up the monitor resources, then destroy the I/O thread since
+     * we need to unregister from chardev below in
+     * monitor_data_destroy(), and chardev is not thread-safe yet
+     */
+    if (mon_iothread) {
+        iothread_stop(mon_iothread);
+    }
+
     /* Flush output buffers and destroy monitors */
     qemu_mutex_lock(&monitor_lock);
     monitor_destroyed = true;
-- 
2.29.2

[PATCH 2/2] monitor/qmp: Stop processing requests when shutdown is requested

[Kevin Wolf]

monitor_qmp_dispatcher_co() used to check whether shutdown is requested
only when it would have to wait for new requests. If there were still
some queued requests, it would try to execute all of them before
shutting down.

This can be surprising when the queued QMP commands take long or hang
because Ctrl-C may not actually exit QEMU as soon as possible.

Change monitor_qmp_dispatcher_co() so that it additionally checks
whether shutdown is request before it gets a new request from the queue.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/qmp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 43880fa623..2326bd7f9b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -227,6 +227,11 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
          */
         qatomic_mb_set(&qmp_dispatcher_co_busy, false);
 
+        /* On shutdown, don't take any more requests from the queue */
+        if (qmp_dispatcher_co_shutdown) {
+            return;
+        }
+
         while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
             /*
              * No more requests to process.  Wait to be reentered from
-- 
2.29.2

Re: [PATCH 2/2] monitor/qmp: Stop processing requests when shutdown is requested

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> monitor_qmp_dispatcher_co() used to check whether shutdown is requested

"used to": until when?

> only when it would have to wait for new requests. If there were still
> some queued requests, it would try to execute all of them before
> shutting down.
>
> This can be surprising when the queued QMP commands take long or hang
> because Ctrl-C may not actually exit QEMU as soon as possible.
>
> Change monitor_qmp_dispatcher_co() so that it additionally checks
> whether shutdown is request before it gets a new request from the queue.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  monitor/qmp.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 43880fa623..2326bd7f9b 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -227,6 +227,11 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
>           */
>          qatomic_mb_set(&qmp_dispatcher_co_busy, false);
>  
> +        /* On shutdown, don't take any more requests from the queue */
> +        if (qmp_dispatcher_co_shutdown) {
> +            return;
> +        }
> +
>          while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
>              /*
>               * No more requests to process.  Wait to be reentered from

Re: [PATCH 0/2] monitor: Shutdown fixes

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This fixes the bug(s) that Markus reported here:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07719.html

Tested-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 2/2] monitor/qmp: Stop processing requests when shutdown is requested

[Kevin Wolf]

Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested
> 
> "used to": until when?

Until right before this patch. Do you prefer present tense to describe
the old state?

Also, does your Tested-by imply that you expect someone else to merge
this series? I'm planning to send a pull request today, so if that was
your intention, I can include it there.

Kevin

> > only when it would have to wait for new requests. If there were still
> > some queued requests, it would try to execute all of them before
> > shutting down.
> >
> > This can be surprising when the queued QMP commands take long or hang
> > because Ctrl-C may not actually exit QEMU as soon as possible.
> >
> > Change monitor_qmp_dispatcher_co() so that it additionally checks
> > whether shutdown is request before it gets a new request from the queue.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  monitor/qmp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 43880fa623..2326bd7f9b 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -227,6 +227,11 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
> >           */
> >          qatomic_mb_set(&qmp_dispatcher_co_busy, false);
> >  
> > +        /* On shutdown, don't take any more requests from the queue */
> > +        if (qmp_dispatcher_co_shutdown) {
> > +            return;
> > +        }
> > +
> >          while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
> >              /*
> >               * No more requests to process.  Wait to be reentered from

Re: [PATCH 2/2] monitor/qmp: Stop processing requests when shutdown is requested

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested
>> 
>> "used to": until when?
>
> Until right before this patch. Do you prefer present tense to describe
> the old state?

I've done it both ways.  I think present tense works better for me.
Case in point: I wasn't confident I read your commit message correctly,
so I asked.

> Also, does your Tested-by imply that you expect someone else to merge
> this series? I'm planning to send a pull request today, so if that was
> your intention, I can include it there.

I didn't mean to imply anything beyond "I ran my reproducer, and your
patches fix it."

I don't mind you including the fixes in your pull request.  I also don't
mind doing a pull request for your fixes.  Up to you!

Re: [PATCH 0/2] monitor: Shutdown fixes

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This fixes the bug(s) that Markus reported here:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07719.html

Mostly thanks to the helpful commit messages and code comments:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 2/2] monitor/qmp: Stop processing requests when shutdown is requested

[Kevin Wolf]

Am 15.02.2021 um 13:42 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested
> >> 
> >> "used to": until when?
> >
> > Until right before this patch. Do you prefer present tense to describe
> > the old state?
> 
> I've done it both ways.  I think present tense works better for me.
> Case in point: I wasn't confident I read your commit message correctly,
> so I asked.

I guess I can just add "Before this patch, ..." to clarify.

> > Also, does your Tested-by imply that you expect someone else to merge
> > this series? I'm planning to send a pull request today, so if that was
> > your intention, I can include it there.
> 
> I didn't mean to imply anything beyond "I ran my reproducer, and your
> patches fix it."
> 
> I don't mind you including the fixes in your pull request.  I also
> don't mind doing a pull request for your fixes.  Up to you!

Ok, if you don't mind either way, I'll just fix up the commit message
and include it.

I just asked because it has happened before that everyone was waiting
for someone else to merge a patch. So I made it a habit to ask when a
maintainer replies with some kind of approval without mentioning that
they queued the patch, because that could be a sign that they expect it
to go through a different tree.

Kevin

Re: [PATCH 2/2] monitor/qmp: Stop processing requests when shutdown is requested

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 15.02.2021 um 13:42 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> 
>> >> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested
>> >> 
>> >> "used to": until when?
>> >
>> > Until right before this patch. Do you prefer present tense to describe
>> > the old state?
>> 
>> I've done it both ways.  I think present tense works better for me.
>> Case in point: I wasn't confident I read your commit message correctly,
>> so I asked.
>
> I guess I can just add "Before this patch, ..." to clarify.

Yes, please.

>> > Also, does your Tested-by imply that you expect someone else to merge
>> > this series? I'm planning to send a pull request today, so if that was
>> > your intention, I can include it there.
>> 
>> I didn't mean to imply anything beyond "I ran my reproducer, and your
>> patches fix it."
>> 
>> I don't mind you including the fixes in your pull request.  I also
>> don't mind doing a pull request for your fixes.  Up to you!
>
> Ok, if you don't mind either way, I'll just fix up the commit message
> and include it.
>
> I just asked because it has happened before that everyone was waiting
> for someone else to merge a patch. So I made it a habit to ask when a
> maintainer replies with some kind of approval without mentioning that
> they queued the patch, because that could be a sign that they expect it
> to go through a different tree.

Not a bad habit.

Thanks!

Re: [PATCH 0/2] monitor: Shutdown fixes

[Vladimir Sementsov-Ogievskiy]

12.02.2021 20:20, Kevin Wolf wrote:
> This fixes the bug(s) that Markus reported here:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07719.html
> 
> Kevin Wolf (2):
>    monitor: Fix assertion failure on shutdown
>    monitor/qmp: Stop processing requests when shutdown is requested
> 
>   monitor/monitor.c | 25 +++++++++++++++----------
>   monitor/qmp.c     |  5 +++++
>   2 files changed, 20 insertions(+), 10 deletions(-)
> 


Hi!

Now we faced this bug after rebasing Virtuozzo qemu package onto qemu-kvm-5.2.0-16.module+el8.4.0+10806+b7d97207.src.rpm

So, I'm backporting these patches.

This probably should go to stable and/or to further Rhel packages.

-- 
Best regards,
Vladimir

Re: [PATCH 0/2] monitor: Shutdown fixes

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 12.02.2021 20:20, Kevin Wolf wrote:
>> This fixes the bug(s) that Markus reported here:
>> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07719.html
>> Kevin Wolf (2):
>>    monitor: Fix assertion failure on shutdown
>>    monitor/qmp: Stop processing requests when shutdown is requested
>>   monitor/monitor.c | 25 +++++++++++++++----------
>>   monitor/qmp.c     |  5 +++++
>>   2 files changed, 20 insertions(+), 10 deletions(-)
>> 
>
>
> Hi!
>
> Now we faced this bug after rebasing Virtuozzo qemu package onto qemu-kvm-5.2.0-16.module+el8.4.0+10806+b7d97207.src.rpm
>
> So, I'm backporting these patches.
>
> This probably should go to stable and/or to further Rhel packages.

Cc'ing qemu-stable, so it gets considered.