On Tue, 2011-09-27 at 18:01 +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[Xen-devel] [PATCH 2 of 2] libxl: add support for
> booting PV domains from NetBSD using raw files as disks"):
> > libxl: add support for booting PV domains from NetBSD using raw files as
> > disks.
>
> Thanks, I have some comments:
>
> > + if (S_ISBLK(a->stab.st_mode))
> > + return backend;
> > +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
> > + if (S_ISREG(a->stab.st_mode))
> > + return backend;
>
> I think we would prefer not to have #ifdefs in the code. That can
> make the logic quite hard to follow. Instead, invent a helper
> function which answers the "does the phy backend support files" which
> is provided in two versions, from osdep.c.
This was my suggestion but you are right that a helper function is a
much better idea.
> > @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,
> > libxl_ctx *ctx = libxl__gc_owner(gc);
> > xs_transaction_t t;
> > char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> > + char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);
>
> We want to get away from the hotplug scripts for disks at least on
> Linux with libxl. Rather, any scripts that are needed should be run
> from libxl directly.
xenbackendd is the component in NetBSD which runs the "hotplug" scripts,
triggered off the xenstore state node transitions. (I presume the NetBSD
kernel driver doesn't generate hotplug events)
AIUI the issue is the synchronisation between the kernel device, libxl
and NetBSD's xenbackendd. Effectively libxl and xenbackendd are racing
on the teardown (both are watching the state node in xenstore). If
xenbackendd loses then it cannot do its cleanup because libxl has
already nuked the necessary info from xenstore. The fix which Roger has
made is to have only xenbackendd watch "state" and set
"hotplug-status=disconnected" and libxl to watch "hotplug-status". On
Linux the equivalent is to have the hotplug script write the
"hotplug-status=disconnected".
I think strictly speaking the Linux hotplug scripts have a similar race
but it just happens that there is no actual work on the teardown path so
the race is benign.
> How does that fit with NetBSD's disk backend approach ?
I expect that if we get rid of hotplug scripts on Linux that this will
be equivalent to removing xenbackendd on NetBSD, the same approximate
scheme should work for both, I think.
I think you've explained the scheme you have in mind for deprecating
hotplug scripts before but could you remind me (and for the lists
benefit)? Is it simply a case of shelling out to the vbd's configured
"script=" at the right points on attach and detach?
Would we then need special handling to turn "file:<X>" into
"phy:<X>,script=block-loop"?
I seem to remember something about setting up a faked out backend area
for each backend and running the scripts against that instead of the
real one.
> What goes wrong on NetBSD without this additional code ?
NetBSD doesn't have the option of falling back to blktap for file://
type disk devices so these simply don't work at the moment. This is a
pretty serious blocker for NetBSD moving to xl.
There was a subtle difference between NetBSD's and Linux's handling of
these with xend. On Linux xend used to setup and manage the loopback
device and create a simple phy backend referencing it. On NetBSD xend
just sets up a file or phy backend as appropriate and the scripts which
run out of xenbackendd take care of setting up the loopback as necessary
and filing in the real device in xenstore. On teardown the loopback
device needs to be cleaned up (and this is what currently fails).
For libxl on Linux we decided to avoid managing loopback devices in
libxl and instead just used blktap to meet that need but as I say this
isn't an option on BSD.
Roger has indicated that he is working on blktap-in-userspace
functionality, which would solve this problem longer term, so I think we
just need a stopgap measure to allow NetBSD users to switch to xl in the
meantime. How much work do you expect deprecating the hotplug scripts to
be, is it (or at least the subset necessary to solve this issue)
something which can be achieved in the short term?
> > @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
> > tv.tv_usec = 0;
> > while (n_watches > 0) {
> > if (wait_for_dev_destroy(gc, &tv)) {
> > - break;
> > + continue;
> > } else {
> > n_watches--;
> > }
>
> I'm not sure I understand this change, or why it's needed.
This was an unrelated fixup. Roger subsequently posted it again as a
separate change. (as he did this whole series with clearer separation of
different fixes)
Ian.
>
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home