On Wed, Sep 4, 2013 at 12:16 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
Shriram Rajagopalan writes ("[Xen-devel] [PATCH 3 of 5 V2] tools/libxl: setup/teardown Remus network buffering"):
> tools/libxl: setup/teardown Remus network bufferingThanks.
I'm afraid the event handling, particularly with respect to
subprocesses, is not yet right.
Do you plan to fix this later (perhaps as part of a future series) ?
> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Thu Aug 29 14:36:25 2013 -0700
> +++ b/tools/libxl/libxl_dom.c Thu Aug 29 14:36:36 2013 -0700
> @@ -1259,7 +1259,7 @@ static void remus_checkpoint_dm_saved(li
> /* REMUS TODO: Wait for disk and memory ack, release network buffer */
> /* REMUS TODO: make this asynchronous */
> assert(!rc); /* REMUS TODO handle this error properly */
> - usleep(dss->interval * 1000);
> + usleep(dss->remus_ctx->interval * 1000);
> libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, 1);
Its actually fixed in the next patch in the same series.> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/libxl_netbuffer.c...
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/libxl/libxl_netbuffer.c Thu Aug 29 14:36:36 2013 -0700
> @@ -0,0 +1,434 @@
> +static void netbuf_script_child_death_cb(libxl__egc *egc,This can't possibly be right. You must always wait for your children
> + libxl__ev_child *child,
> + pid_t pid, int status)
> +{
> + /* No-op. hotplug-error will be read in setup/teardown_ifb */
> + return;
> +}
and you may not complete the ao until the child has finished. This
should all be documented in libxl_internal.h.
Thats true. But this was not meant to be an ao (see explanation atend of this email).Also I don't see where this other error handling code is.
What purpose does this serve ? This can be false if the death_cb is
> + if (!pid) {
> + /* child: Launch netbuf script */
> + libxl__exec(gc, -1, -1, -1, args[0], args, env);
> + /* notreached */
> + abort();
> + }
> +
> + assert(libxl__ev_child_inuse(&childw_out));
called from within libxl__ev_child_fork.
Sorry. This was copy pasted from code in device_hotplug(). I couldn't finda simple fork & exec abstraction. The libxl_ev_child_fork seemed to bethe only option.> + rc = libxl__exec_netbuf_script(gc, domid, devid, vif,This definitely isn't right - it needs to be asynchronous, with a
> + script, "setup");
> + if (rc) return rc;
callback.
This wait does not actually block the current execution.
> + /* Now wait for the result (XENBUS_PATH/hotplug-status).
> + * It doesnt matter what the result is. If the hotplug-status path
> + * appears, then we are good to go.
> + */
> + rc = libxl__wait_for_offspring(gc, domid, LIBXL_HOTPLUG_TIMEOUT,
> + script,
> + /* path */
> + GCSPRINTF("%s/hotplug-status",
> + out_path_base),
> + NULL /* state */,
> + NULL, script_done_cb, NULL);
It actually does. Atleast based on the code in libxl_exec.c which usesa select() call to wait on xenstore fds.It arranges
for the callback to be called. You need to break this function apart.
And this one.
> +static int libxl__netbuf_script_teardown(libxl__gc *gc, uint32_t domid,
> + int devid, char *vif,
> + char *script)
> +{
> + /* Nothing to wait for during tear down */
> + return libxl__exec_netbuf_script(gc, domid, devid, vif,
> + script, "teardown");
Does this run a script per netbuf ? Do they want to run in
> + for (i = 0; i < num_netbufs; ++i) {
> +
> + /* The setup script does the following
> + * find a free IFB to act as buffer for the vif.
> + * set up the plug qdisc on the IFB.
> + */
> + ret = libxl__netbuf_script_setup(gc, domid, i, vif_list[i],
> + (char *) remus_ctx->netbufscript,
> + &ifb_list[i]);
parallel ?
A script per netbuf. Running them in parallel is an overkill, since the remussetup is a one-time task and the average domain is expected to have few interfaces.Besides, the script has no waits. It just issues a bunch of shell commands that don't block> +int libxl__remus_netbuf_teardown(libxl__gc *gc, uint32_t domid,...
> + libxl__remus_ctx *remus_ctx)
> +{
> + for (i = 0; i < netbuf_ctx->num_netbufs; ++i)I think you should definitely do this asynchronously and in parallel.
> + libxl__netbuf_script_teardown(gc, domid, i, vif_list[i],
> + (char *) remus_ctx->netbufscript);
See answer above for running in parallel. In terms of running this asynchronously,I had a comment in this file explaining why I decided to go the synchronous route.Basically, the setup and teardown are part of the remus API call, which is alreadyasynchronous.The remus external API (libxl_domain_remus_start) returns after starting remus.It goes like this:setup scriptssuspend_domain (which basically launches save_domain helper process)return AO_IN_PROGRESSThe setup scripts are have no long running operations. They complete in jiffy(adding a bunch of tc rules to interfaces)The AO callback in libxl.c (named "remus_replication_failure_cb" [beginningof this patch]), is called when the backup fails, i.e. when the replicationoperations fail (such as write() calls).The AO callback calls the above teardown function. I remember reading in thecomments that an AO op should not be issued from an AO callback(please correct me if I am wrong).What happens if I use "xl destroy" to destroy a domain which is being
handled by remus.
One of the replication steps (domain suspend or domain resume) would fail.This would result in the AO callback being invoked, which in turn invokes theexternal scripts. The scripts fail silently if the vif interface cannot be found,but they go ahead and remove the IFB device, and so on.Thanks,
Ian.