[libvirt] [PATCH 3/4] libxl: handle domain shutdown events in a thread
Michal Privoznik
mprivozn at redhat.com
Thu Feb 6 12:53:15 UTC 2014
On 05.02.2014 18:39, Jim Fehlig wrote:
> Handling the domain shutdown event within the event handler seems
> a bit unfair to libxl's event machinery. Domain "shutdown" could
> take considerable time. E.g. if the shutdown reason is reboot,
> the domain must be reaped and then started again.
>
> Spawn a shutdown handler thread to do this work, allowing libxl's
> event machinery to go about its business.
>
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
> src/libxl/libxl_driver.c | 132 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 89 insertions(+), 43 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d639011..a1c6c0f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -352,61 +352,107 @@ libxlVmReap(libxlDriverPrivatePtr driver,
> # define VIR_LIBXL_EVENT_CONST const
> #endif
>
> +struct libxlShutdownThreadInfo
> +{
> + virDomainObjPtr vm;
> + libxl_event *event;
> +};
> +
> +
> static void
> -libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
> +libxlDomainShutdownThread(void *opaque)
> {
> libxlDriverPrivatePtr driver = libxl_driver;
> - libxlDomainObjPrivatePtr priv = ((virDomainObjPtr)data)->privateData;
> - virDomainObjPtr vm = NULL;
> + struct libxlShutdownThreadInfo *shutdown_info = opaque;
> + virDomainObjPtr vm = shutdown_info->vm;
> + libxlDomainObjPrivatePtr priv = vm->privateData;
> + libxl_event *ev = shutdown_info->event;
> + libxl_ctx *ctx = priv->ctx;
> virObjectEventPtr dom_event = NULL;
> - libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
> -
> - if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> - virDomainShutoffReason reason;
> -
> - /*
> - * Similar to the xl implementation, ignore SUSPEND. Any actions needed
> - * after calling libxl_domain_suspend() are handled by it's callers.
> - */
> - if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> - goto cleanup;
> + libxl_shutdown_reason xl_reason = ev->u.domain_shutdown.shutdown_reason;
> + virDomainShutoffReason reason;
>
> - vm = virDomainObjListFindByID(driver->domains, event->domid);
> - if (!vm)
> - goto cleanup;
> + virObjectLock(vm);
>
> - switch (xl_reason) {
> - case LIBXL_SHUTDOWN_REASON_POWEROFF:
> - case LIBXL_SHUTDOWN_REASON_CRASH:
> - if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
> - dom_event = virDomainEventLifecycleNewFromObj(vm,
> - VIR_DOMAIN_EVENT_STOPPED,
> - VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> - reason = VIR_DOMAIN_SHUTOFF_CRASHED;
> - } else {
> - reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
> - }
> - libxlVmReap(driver, vm, reason);
> - if (!vm->persistent) {
> - virDomainObjListRemove(driver->domains, vm);
> - vm = NULL;
> - }
> - break;
> - case LIBXL_SHUTDOWN_REASON_REBOOT:
> - libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> - libxlVmStart(driver, vm, 0, -1);
> - break;
> - default:
> - VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> - break;
> - }
> + switch (xl_reason) {
> + case LIBXL_SHUTDOWN_REASON_POWEROFF:
> + case LIBXL_SHUTDOWN_REASON_CRASH:
> + if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
> + dom_event = virDomainEventLifecycleNewFromObj(vm,
> + VIR_DOMAIN_EVENT_STOPPED,
> + VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> + reason = VIR_DOMAIN_SHUTOFF_CRASHED;
> + } else {
> + reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
> + }
> + libxlVmReap(driver, vm, reason);
> + if (!vm->persistent) {
> + virDomainObjListRemove(driver->domains, vm);
> + vm = NULL;
> + }
> + break;
> + case LIBXL_SHUTDOWN_REASON_REBOOT:
> + libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> + libxlVmStart(driver, vm, 0, -1);
> + break;
> + default:
> + VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> + break;
> }
>
> -cleanup:
> if (vm)
> virObjectUnlock(vm);
> if (dom_event)
> libxlDomainEventQueue(driver, dom_event);
> + libxl_event_free(ctx, ev);
> + VIR_FREE(shutdown_info);
> +}
> +
> +static void
> +libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
> +{
> + virDomainObjPtr vm = data;
> + libxlDomainObjPrivatePtr priv = vm->privateData;
> + libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
> + struct libxlShutdownThreadInfo *shutdown_info;
> + virThread thread;
> +
> + if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> + VIR_INFO("Unhandled event type %d", event->type);
> + goto cleanup;
> + }
> +
> + /*
> + * Similar to the xl implementation, ignore SUSPEND. Any actions needed
> + * after calling libxl_domain_suspend() are handled by it's callers.
> + */
> + if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> + goto cleanup;
> +
> + /*
> + * Start a thread to handle shutdown. We don't want to be tying up
> + * libxl's event machinery by doing a potentially lengthy shutdown.
> + */
> + if (VIR_ALLOC(shutdown_info) < 0)
> + goto cleanup;
> +
> + shutdown_info->vm = data;
> + shutdown_info->event = (libxl_event *)event;
> + if (virThreadCreate(&thread, true, libxlDomainShutdownThread,
> + shutdown_info) < 0) {
The 2nd argument 'true' tells, if @thread is joinable. Since you are not
joining it anywhere, I guess it should be rather 'false'. Moreover, it
will avoid some memory leaks, as pthread free()-s memory needed for
thread itself (otherwise the memory would be free()-d in phtread_join -
which again is not called anywhere).
> + /*
> + * Not much we can do on error here except log it.
> + */
> + VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
> + goto cleanup;
> + }
> +
> + /*
> + * libxl_event freed in shutdown thread
> + */
> + return;
> +
> +cleanup:
Since this is in fact an error path, I suggest renaming the label to
'error'.
> /* Cast away any const */
> libxl_event_free(priv->ctx, (libxl_event *)event);
> }
>
ACK if you fix these two issues.
Michal
More information about the libvir-list
mailing list