Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
roger.pau at entel
Sep 16, 2011, 8:36 AM
Post #1 of 9 (413 views)
Permalink
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1316187362 -7200
# Node ID aff3960421768180410c8d553acf8881435bc3b4
# Parent 00949e363f6f2c70001da548403475628df93b97
libxl: add support for booting PV domains from NetBSD using raw files as disks.

Used the hotplug-status attribute in xenstore for vbd to check when the device is offline.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Fri Sep 16 17:29:45 2011 +0200
+++ b/tools/libxl/libxl_device.c Fri Sep 16 17:36:02 2011 +0200
@@ -136,15 +136,20 @@ static int disk_try_backend(disk_try_bac
a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
goto bad_format;
}
- if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
- !S_ISBLK(a->stab.st_mode)) {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
- " unsuitable as phys path not a block device",
- a->disk->vdev);
- return 0;
- }
-
- return backend;
+ if (S_ISBLK(a->stab.st_mode))
+ return backend;
+#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
+ if (S_ISREG(a->stab.st_mode))
+ return backend;
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+ " unsuitable as phys path not a block device or"
+ " raw image", a->disk->vdev);
+#else
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+ " unsuitable as phys path not a block device",
+ a->disk->vdev);
+#endif
+ return 0;

case LIBXL_DISK_BACKEND_TAP:
if (!libxl__blktap_enabled(a->gc)) {
@@ -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);
char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+ char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path);
int rc = 0;

if (!state)
goto out;
- if (atoi(state) != 4) {
- xs_rm(ctx->xsh, XBT_NULL, be_path);
- goto out;
+
+ if (!strstr(be_path, "vbd")) {
+ if (atoi(state) != 4) {
+ xs_rm(ctx->xsh, XBT_NULL, be_path);
+ goto out;
+ }
+ } else {
+ if (!hotplug)
+ goto out;
+ if (!strcmp(hotplug, "disconnected")) {
+ xs_rm(ctx->xsh, XBT_NULL, be_path);
+ goto out;
+ }
}

retry_transaction:
@@ -389,8 +406,13 @@ retry_transaction:
}
}
if (!force) {
- xs_watch(ctx->xsh, state_path, be_path);
- rc = 1;
+ if (strstr(be_path, "vbd")) {
+ xs_watch(ctx->xsh, hotplug_path, be_path);
+ rc = 1;
+ } else {
+ xs_watch(ctx->xsh, state_path, be_path);
+ rc = 1;
+ }
} else {
xs_rm(ctx->xsh, XBT_NULL, be_path);
}
@@ -405,6 +427,7 @@ static int wait_for_dev_destroy(libxl__g
unsigned int n;
fd_set rfds;
char **l1 = NULL;
+ char *state, *hotplug;

rc = 1;
nfds = xs_fileno(ctx->xsh) + 1;
@@ -413,15 +436,29 @@ static int wait_for_dev_destroy(libxl__g
if (select(nfds, &rfds, NULL, NULL, tv) > 0) {
l1 = xs_read_watch(ctx->xsh, &n);
if (l1 != NULL) {
- char *state = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
- if (!state || atoi(state) == 6) {
- xs_unwatch(ctx->xsh, l1[0], l1[1]);
- xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend at %s", l1[XS_WATCH_TOKEN]);
- rc = 0;
+ if (strstr(l1[XS_WATCH_PATH], "hotplug-status")) {
+ hotplug = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
+ if (!hotplug || !strcmp(hotplug, "disconnected")) {
+ xs_unwatch(ctx->xsh, l1[0], l1[1]);
+ xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend at %s hotplug-status: %s",
+ l1[XS_WATCH_TOKEN], hotplug);
+ rc = 0;
+ }
+ } else {
+ state = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
+ if (!state || atoi(state) == 6) {
+ xs_unwatch(ctx->xsh, l1[0], l1[1]);
+ xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend at %s", l1[XS_WATCH_TOKEN]);
+ rc = 0;
+ }
}
free(l1);
}
+ } else {
+ /* timeout reached */
+ rc = 0;
}
return rc;
}
@@ -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--;
}
diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h Fri Sep 16 17:29:45 2011 +0200
+++ b/tools/libxl/libxl_osdeps.h Fri Sep 16 17:36:02 2011 +0200
@@ -25,6 +25,7 @@

#if defined(__NetBSD__) || defined(__OpenBSD__)
#include <util.h>
+#define HAVE_PHY_BACKEND_FILE_SUPPORT
#elif defined(__linux__)
#include <pty.h>
#elif defined(__sun__)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
Ian.Campbell at citrix
Sep 21, 2011, 2:49 AM
Post #2 of 9 (313 views)
Permalink
On Fri, 2011-09-16 at 16:36 +0100, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@entel.upc.edu>
> # Date 1316187362 -7200
> # Node ID aff3960421768180410c8d553acf8881435bc3b4
> # Parent 00949e363f6f2c70001da548403475628df93b97
> libxl: add support for booting PV domains from NetBSD using raw files as disks.
>
> Used the hotplug-status attribute in xenstore for vbd to check when the device is offline.
>
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

Again this generally looks good but I think the functional split between
the patches isn't quite right.

>
> diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c Fri Sep 16 17:29:45 2011 +0200
> +++ b/tools/libxl/libxl_device.c Fri Sep 16 17:36:02 2011 +0200
> @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,


This bit (and most of this patch) goes along with the hotplug script
changes in patch 1/2, doesn't it?

> 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);
> char *state = libxl__xs_read(gc, XBT_NULL, state_path);
> + char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path);
> int rc = 0;
>
> if (!state)
> goto out;
> - if (atoi(state) != 4) {
> - xs_rm(ctx->xsh, XBT_NULL, be_path);
> - goto out;
> +
> + if (!strstr(be_path, "vbd")) {

I've got a WIP patch which passes a libxl__device to functions like this
which will be helpful for removing these strstr things, since you can
just use the DEVICE_* enum.

> + if (atoi(state) != 4) {
> + xs_rm(ctx->xsh, XBT_NULL, be_path);
> + goto out;
> + }
> + } else {
> + if (!hotplug)
> + goto out;
> + if (!strcmp(hotplug, "disconnected")) {
> + xs_rm(ctx->xsh, XBT_NULL, be_path);
> + goto out;
> + }
> }
>
> retry_transaction:

> @@ -389,8 +406,13 @@ retry_transaction:
> }
> }
> if (!force) {
> - xs_watch(ctx->xsh, state_path, be_path);
> - rc = 1;
> + if (strstr(be_path, "vbd")) {
> + xs_watch(ctx->xsh, hotplug_path, be_path);
> + rc = 1;
> + } else {
> + xs_watch(ctx->xsh, state_path, be_path);
> + rc = 1;
> + }
> } else {
> xs_rm(ctx->xsh, XBT_NULL, be_path);
> }
> @@ -405,6 +427,7 @@ static int wait_for_dev_destroy(libxl__g
> unsigned int n;
> fd_set rfds;
> char **l1 = NULL;
> + char *state, *hotplug;
>
> rc = 1;
> nfds = xs_fileno(ctx->xsh) + 1;
> @@ -413,15 +436,29 @@ static int wait_for_dev_destroy(libxl__g
> if (select(nfds, &rfds, NULL, NULL, tv) > 0) {
> l1 = xs_read_watch(ctx->xsh, &n);
> if (l1 != NULL) {
> - char *state = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> - if (!state || atoi(state) == 6) {
> - xs_unwatch(ctx->xsh, l1[0], l1[1]);
> - xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend at %s", l1[XS_WATCH_TOKEN]);
> - rc = 0;
> + if (strstr(l1[XS_WATCH_PATH], "hotplug-status")) {
> + hotplug = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> + if (!hotplug || !strcmp(hotplug, "disconnected")) {
> + xs_unwatch(ctx->xsh, l1[0], l1[1]);
> + xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend at %s hotplug-status: %s",
> + l1[XS_WATCH_TOKEN], hotplug);
> + rc = 0;
> + }
> + } else {
> + state = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> + if (!state || atoi(state) == 6) {
> + xs_unwatch(ctx->xsh, l1[0], l1[1]);
> + xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend at %s", l1[XS_WATCH_TOKEN]);
> + rc = 0;
> + }
> }
> free(l1);
> }
> + } else {
> + /* timeout reached */
> + rc = 0;
> }
> return rc;
> }
> @@ -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--;
> }

This looks like an independent bug fix?

> diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_osdeps.h
> --- a/tools/libxl/libxl_osdeps.h Fri Sep 16 17:29:45 2011 +0200
> +++ b/tools/libxl/libxl_osdeps.h Fri Sep 16 17:36:02 2011 +0200
> @@ -25,6 +25,7 @@
>
> #if defined(__NetBSD__) || defined(__OpenBSD__)
> #include <util.h>
> +#define HAVE_PHY_BACKEND_FILE_SUPPORT
> #elif defined(__linux__)
> #include <pty.h>
> #elif defined(__sun__)
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
Ian.Jackson at eu
Sep 27, 2011, 10:01 AM
Post #3 of 9 (306 views)
Permalink
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.

> @@ -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.

How does that fit with NetBSD's disk backend approach ?
What goes wrong on NetBSD without this additional code ?

> @@ -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.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
roger.pau at entel
Sep 28, 2011, 1:07 AM
Post #4 of 9 (303 views)
Permalink
2011/9/27 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> 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:

This is an old version of the patch, I've split this change across
several patches, that are on the list also, and include more exact
descriptions of every change.

>
>> +        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.

Ok, I don't like ifdefs also.

>
>> @@ -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.
>
> How does that fit with NetBSD's disk backend approach ?
> What goes wrong on NetBSD without this additional code ?

NetBSD still uses the "loop" ("vnd" on NetBSD) device for files,
because it doesn't have support for qdisk or blktap. If we don't check
the hotplug-status before removing the vbd from xenstore (and only
look at state) it might be removed before the hotplug scripts are
executed, and the disk is never unmounted. This is why we need to
check the hotplug-status before removing vbd from xenstore. Of course,
I could call the hotplug scripts from libxl directly (for disk and
inet interfaces), and we could get rid of xenbackendd.

>
>> @@ -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 change is more explained in the series, basically libxl was not
waiting for all devices to disconnect, because when it returned from
wait_for_dev_destroy exited the loop immediately, even if we where
watching for more than one device to disconnect.

>
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
Ian.Campbell at citrix
Sep 28, 2011, 1:13 AM
Post #5 of 9 (302 views)
Permalink
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@lists.xensource.com
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
Ian.Jackson at eu
Sep 28, 2011, 6:28 AM
Post #6 of 9 (307 views)
Permalink
Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
> NetBSD still uses the "loop" ("vnd" on NetBSD) device for files,
> because it doesn't have support for qdisk or blktap. If we don't check
> the hotplug-status before removing the vbd from xenstore (and only
> look at state) it might be removed before the hotplug scripts are
> executed, and the disk is never unmounted. This is why we need to
> check the hotplug-status before removing vbd from xenstore. Of course,
> I could call the hotplug scripts from libxl directly (for disk and
> inet interfaces), and we could get rid of xenbackendd.

Right.

Yes, I think you should call the hotplug scripts from libxl and get
rid of xenbackendd. That is the way we want the Linux world to go,
although of course Linux needs to call hotplug scripts in fewer cases.

> >> @@ -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 change is more explained in the series, basically libxl was not
> waiting for all devices to disconnect, because when it returned from
> wait_for_dev_destroy exited the loop immediately, even if we where
> watching for more than one device to disconnect.

Right.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
Ian.Jackson at eu
Sep 28, 2011, 6:32 AM
Post #7 of 9 (303 views)
Permalink
Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
> 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?

Yes.

The other elements of the block hotplug scripts don't do anything any
more on Linux I think, and currently libxl does not cope with script=
being set, so from a Linux pov this is a new feature for libxl.

> Would we then need special handling to turn "file:<X>" into
> "phy:<X>,script=block-loop"?

I think this should be done behind the scenes. The backend-specific
code in libxl should call some kind of "invoke this script" function
which would also be used for explicit "script=".

On NetBSD, how do "more exciting" script= things work ? Eg, iscsi or
nbd. On Linux the idea is that the hotplug script sets up your nbd
which causes a real block device to appear, and that block device is
used by blkback.

> 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.

We would need to do that to support (eg) pygrub over nbd.

> 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).

I'm not a fan of these approaches with a separate daemon. We should
avoid that if we can as it produces a lot of complication.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
roger.pau at entel
Sep 29, 2011, 1:25 AM
Post #8 of 9 (302 views)
Permalink
> Yes.
>
> The other elements of the block hotplug scripts don't do anything any
> more on Linux I think, and currently libxl does not cope with script=
> being set, so from a Linux pov this is a new feature for libxl.
>
>> Would we then need special handling to turn "file:<X>" into
>> "phy:<X>,script=block-loop"?
>
> I think this should be done behind the scenes.  The backend-specific
> code in libxl should call some kind of "invoke this script" function
> which would also be used for explicit "script=".
>
> On NetBSD, how do "more exciting" script= things work ?  Eg, iscsi or
> nbd.

NetBSD only has block and vif scripts currently, nothing fancy.

> On Linux the idea is that the hotplug script sets up your nbd
> which causes a real block device to appear, and that block device is
> used by blkback.

NetBSD uses this approach to mount virtual images, the image is
mounted on the vnd device, and then this physical device is handled as
a regular 'phy' backend.

>
>> 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.
>
> We would need to do that to support (eg) pygrub over nbd.
>
>> 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).
>
> I'm not a fan of these approaches with a separate daemon.  We should
> avoid that if we can as it produces a lot of complication.

I also think it's stupid to have two different programs watching
xenstore, because that's what xenbackendd does. Instead, the code in
xenbackendd could be move to libxl without much problem. The proper
calls to the block, vif and other scripts can be added to
libxl__device_destroy function to unplug vbd and network interfaces,
but I don't know if that's the best place to put them, because I still
don't have enough knowledge of libxl to decide that. As for the
startup script, I have to look at the source to decide where to put
them. Some advice about where would be the best place to put this
calls is welcome.

Regards, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks [ In reply to ]
Ian.Campbell at eu
Sep 29, 2011, 2:05 AM
Post #9 of 9 (303 views)
Permalink
On Thu, 2011-09-29 at 09:25 +0100, Roger Pau Monné wrote:
> I also think it's stupid to have two different programs watching
> xenstore, because that's what xenbackendd does. Instead, the code in
> xenbackendd could be move to libxl without much problem. The proper
> calls to the block, vif and other scripts can be added to
> libxl__device_destroy function to unplug vbd and network interfaces,
> but I don't know if that's the best place to put them, because I still
> don't have enough knowledge of libxl to decide that.

I think libxl__device_destroy is where they have to go right now,
because there are destroy code paths which don't go through code with
device-specific knowledge (specifically libxl__devices_destroy).

Now, I'd like to change that in the long run (so all destroy paths knows
about device specifics) but for now libxl__device_destroy will do.

> As for the
> startup script, I have to look at the source to decide where to put
> them. Some advice about where would be the best place to put this
> calls is welcome.

I think libxl_device_disk_add() and libxl_device_nic_add() are the right
places.

The disk case is pretty trivial, I think, since you run the script
before setting up the backend and after tearing it down again.

The network case is a little trickier since you need to run the script
after the backend driver has created the actual device in dom0 so the
script can configure it (I presume this is much the same on NetBSD as
Linux). The existing hotplug scripts are obviously pretty good for this
(at least on Linux) since they are called at precisely the right time.
How does NetBSD manage that synchronisation today in xenbackendd? I'm
not sure how this can be done in an OS independent way and/or compatibly
with the existing backend implementations for each platform .

I'd have no problem with just tackling the block half now since it is
the more critical bit for NetBSD users.

There's a secondary issue of doing the right thing for
libxl_device_disk_local_(attach|detach) (to allow e.g. pygrub) but again
I think that could be left for the time being, fixing block devices for
regular use by domains is more important.

With regard to disabling the hotplug scripts/xenbackendd when this last
came up we decided that /etc/init.d/xencommons should be opting in on a
per-toolstack basis by touching a file (in /var/run or so) which the
hotplug script check before actually doing anything. In the NetBSD case
instead of touching a file I guess you start xenbackendd or not as
appropriate (or pass the right parameters etc).

That thread is "add a way to disable xen's udev script." from June,
you'll probably want to skip to the end, specifically
<19963.36234.366877.468293@mariner.uk.xensource.com>

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal