Skip to contentSkip to conversation

libxl: wait for QEMU exit before restarting a domain

PATCH In review Jiaqing Zhao 2 participants 4 messages Nov 10 – Nov 11

lore
Share & tools
lore

Jiaqing Zhao

author
#0 lore raw

PATCH v2 1/1 libxl: wait for QEMU exit before restarting a domain

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after signaling it for at most 10 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

Signed-off-by Jiaqing Zhao <[email protected]>

---
v2:
* Share the fork, error handling and child exit handling between UID and PID-based destruction paths.
* Reduce the device-model stop timeout from 60 to 10 seconds.
* Handle unexpected errors from kill(pid, 0) explicitly.
* Fix misspelled "signaling" in commit message.
* Coding style fixes against tools/libs/light/CODING_STYLE.
---
Cc: Anthony PERARD <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Roger Pau Monné <[email protected]>
Cc: Stefano Stabellini <[email protected]>
Cc: Penny Zheng <[email protected]>

---
 tools/libs/light/libxl_aoutils.c  |  8 ++-
 tools/libs/light/libxl_dm.c       | 98 +++++++++++++++++++++----------
 tools/libs/light/libxl_domain.c   |  2 +-
 tools/libs/light/libxl_internal.h |  3 +-
 4 files changed, 75 insertions(+), 36 deletions(-)
tools/libs/light/libxl_aoutils.c +6 -2
index c2d42e7cac..977a2705a8 100644
@@ -628,10 +628,11 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what)
 
 /* Generic function to signal (HUP) a pid stored in xenstore */
 int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
-                        const char *what)
+                        const char *what, pid_t *pid_r)
 {
     const char *xs_pid;
-    int ret, pid;
+    int ret;
+    pid_t pid;
 
     ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
     if (ret || !xs_pid) {

@@ -640,6 +641,9 @@ int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
         goto out;
     }
     pid = atoi(xs_pid);
+    if (pid_r) {
+        *pid_r = pid;
+    }
 
     ret = kill(pid, SIGHUP);
     if (ret < 0 && errno == ESRCH) {
tools/libs/light/libxl_dm.c +66 -32

147 lines not on this page · raw message

tools/libs/light/libxl_domain.c +1 -1
index 37fcd92871..6e332cfed7 100644
@@ -1270,7 +1270,7 @@ static void dm_destroy_cb(libxl__egc *egc,
         libxl__stubdomain_is_linux_running(gc, target_domid)) {
         char *path = GCSPRINTF("/local/domain/%d/image/qmp-proxy-pid", domid);
 
-        libxl__kill_xs_path(gc, path, "QMP Proxy");
+        libxl__kill_xs_path(gc, path, "QMP Proxy", NULL);
         /* qmp-proxy for stubdom registers target_domid's QMP sockets. */
         libxl__qmp_cleanup(gc, target_domid);
     }
tools/libs/light/libxl_internal.h +2 -1
index b65e0064b9..b4a8273b3e 100644
@@ -97,6 +97,7 @@
 /* QEMU may be slow to load and start due to a bug in Linux where the I/O
  * subsystem sometime produce high latency under load. */
 #define LIBXL_DEVICE_MODEL_START_TIMEOUT 60
+#define LIBXL_DEVICE_MODEL_STOP_TIMEOUT 10
 #define LIBXL_DEVICE_MODEL_SAVE_FILE XEN_LIB_DIR "/qemu-save" /* .$domid */
 #define LIBXL_DEVICE_MODEL_RESTORE_FILE XEN_LIB_DIR "/qemu-resume" /* .$domid */
 #define LIBXL_QMP_CMD_TIMEOUT 10

@@ -2659,7 +2660,7 @@ bool libxl__async_exec_inuse(const libxl__async_exec_state *aes);
 _hidden void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what);
 /* kill SIGHUP a pid stored in xenstore */
 _hidden int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
-                                const char *what);
+                                const char *what, pid_t *pid_r);
 
 /*----- device addition/removal -----*/
Signature · 1 line
2.53.0
#1 lore raw
Quoted 9 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after signaling it for at most 10 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

Quoted 6 lines
Signed-off-by Jiaqing Zhao <[email protected]>

---
v2:
* Share the fork, error handling and child exit handling between UID and PID-based destruction paths.
* Reduce the device-model stop timeout from 60 to 10 seconds.

As per my v1 comments I continue to question this.

Quoted 7 lines

* Handle unexpected errors from kill(pid, 0) explicitly.
* Fix misspelled "signaling" in commit message.
* Coding style fixes against tools/libs/light/CODING_STYLE.
---
Cc: Anthony PERARD <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Roger Pau Monné <[email protected]>

You will want to use a correct address here (.org rather than .com).

Jan

author
#2 lore raw
Quoted 10 lines · Jan Beulich
Quoted 9 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after signaling it for at most 10 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

Quoted 8 lines
Quoted 6 lines
Signed-off-by Jiaqing Zhao <[email protected]>

---
v2:
* Share the fork, error handling and child exit handling between UID and PID-based destruction paths.
* Reduce the device-model stop timeout from 60 to 10 seconds.

As per my v1 comments I continue to question this.

I looked further into QEMU's shutdown path. After receiving SIGHUP, QEMU calls bdrv_drain_all() to wait for outstanding block I/O requests and bdrv_flush_all() to flush its block devices. These could take longer than 10 seconds on slow storage.

While this does not identify the timeout you observed with SR-IOV, my earlier measurements are insufficient to prove that 10s is enough. I'll restore the 60s timeout in v3.

Quoted 11 lines
Quoted 7 lines

* Handle unexpected errors from kill(pid, 0) explicitly.
* Fix misspelled "signaling" in commit message.
* Coding style fixes against tools/libs/light/CODING_STYLE.
---
Cc: Anthony PERARD <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Roger Pau Monné <[email protected]>

You will want to use a correct address here (.org rather than .com).

Jan

That was my mistake, sorry.

By the way, on MAINTAINERS:390, Roger's address uses .com, maybe it need to be fixed?

#3 lore raw
Quoted 7 lines · Jiaqing Zhao
Quoted 6 lines · Jan Beulich
Quoted 4 lines · Jiaqing Zhao

---
Cc: Anthony PERARD <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Roger Pau Monné <[email protected]>

Quoted 6 lines
Quoted 1 line

You will want to use a correct address here (.org rather than .com).

That was my mistake, sorry.

By the way, on MAINTAINERS:390, Roger's address uses .com, maybe it need to be fixed?

Indeed - Roger?

Jan