Roger Pau Monne
2011-Sep-16 15:36 UTC
[Xen-devel] [PATCH 0 of 2] Add NetBSD support for libxl PV DomU
This series of patches adds NetBSD support to libxl, using hotplug-status to synchronize the unplugging of vbd devices. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Roger Pau Monne
2011-Sep-16 15:36 UTC
[Xen-devel] [PATCH 1 of 2] hotplug: add hotplug-status disconnected
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1316186985 -7200
# Node ID 00949e363f6f2c70001da548403475628df93b97
# Parent 63e254468d6e8d81baeafaed68f05791dc21eb4e
hotplug: add hotplug-status disconnected
Added hotplug-status disconnected when vbd are removed.
Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>
diff -r 63e254468d6e -r 00949e363f6f tools/hotplug/Linux/block
--- a/tools/hotplug/Linux/block Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/hotplug/Linux/block Fri Sep 16 17:29:45 2011 +0200
@@ -321,6 +321,7 @@ mount it read-write in a guest domain."
remove)
case $t in
phy)
+ xenstore_write "$XENBUS_PATH/hotplug-status"
"disconnected"
exit 0
;;
@@ -329,6 +330,7 @@ mount it read-write in a guest domain."
node=$(xenstore_read "$XENBUS_PATH/node")
losetup -d "$node"
release_lock "block"
+ xenstore_write "$XENBUS_PATH/hotplug-status"
"disconnected"
exit 0
;;
diff -r 63e254468d6e -r 00949e363f6f tools/hotplug/NetBSD/block
--- a/tools/hotplug/NetBSD/block Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/hotplug/NetBSD/block Fri Sep 16 17:29:45 2011 +0200
@@ -19,7 +19,7 @@ error() {
xpath=$1
xstatus=$2
-xtype=$(xenstore-read "$xpath/type")
+xtype=$3
xparams=$(xenstore-read "$xpath/params")
case $xstatus in
@@ -38,6 +38,8 @@ 6)
echo "unknown type $xtype" >&2
;;
esac
+ echo xenstore-write $xpath/hotplug-status disconnected
+ xenstore-write $xpath/hotplug-status disconnected
xenstore-rm $xpath
exit 0
;;
diff -r 63e254468d6e -r 00949e363f6f tools/xenbackendd/xenbackendd.c
--- a/tools/xenbackendd/xenbackendd.c Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/xenbackendd/xenbackendd.c Fri Sep 16 17:29:45 2011 +0200
@@ -89,15 +89,15 @@ dodebug(const char *fmt, ...)
}
static void
-doexec(const char *cmd, const char *arg1, const char *arg2)
+doexec(const char *cmd, const char *arg1, const char *arg2, const char *arg3)
{
- dodebug("exec %s %s %s", cmd, arg1, arg2);
+ dodebug("exec %s %s %s %s", cmd, arg1, arg2, arg3);
switch(vfork()) {
case -1:
dolog(LOG_ERR, "can''t vfork: %s", strerror(errno));
break;
case 0:
- execl(cmd, cmd, arg1, arg2, NULL);
+ execl(cmd, cmd, arg1, arg2, arg3, NULL);
dolog(LOG_ERR, "can''t exec %s: %s", cmd, strerror(errno));
exit(EXIT_FAILURE);
/* NOTREACHED */
@@ -145,11 +145,14 @@ xen_setup(void)
int
main(int argc, char * const argv[])
{
+ struct stat stab;
char **vec;
unsigned int num;
char *s;
int state;
char *sstate;
+ char *stype;
+ char *params;
char *p;
char buf[80];
int type;
@@ -169,7 +172,7 @@ main(int argc, char * const argv[])
log_file = optarg;
break;
case ''p'':
- pidfile = pidfile;
+ pidfile = optarg;
case ''s'':
vbd_script = optarg;
break;
@@ -297,11 +300,38 @@ main(int argc, char * const argv[])
strerror(errno));
goto next2;
}
- doexec(s, vec[XS_WATCH_PATH], sstate);
+ doexec(s, vec[XS_WATCH_PATH], sstate, NULL);
break;
case DEVTYPE_VBD:
- doexec(vbd_script, vec[XS_WATCH_PATH], sstate);
+ /* check if given file is a block device or a raw image */
+ snprintf(buf, sizeof(buf), "%s/params", vec[XS_WATCH_PATH]);
+ params = xs_read(xs, XBT_NULL, buf, 0);
+ if(params == NULL) {
+ dolog(LOG_ERR,
+ "Failed to read %s (%s)", buf, strerror(errno));
+ goto next2;
+ }
+ if (stat(params, &stab) < 0) {
+ dolog(LOG_ERR,
+ "Failed to get info about %s (%s)", params,
+ strerror(errno));
+ goto next3;
+ }
+ stype = NULL;
+ if (S_ISBLK(stab.st_mode))
+ stype = "phy";
+ if (S_ISREG(stab.st_mode))
+ stype = "file";
+ if (stype == NULL) {
+ dolog(LOG_ERR,
+ "Failed to attach %s (not a block device or raw image)",
+ params, strerror(errno));
+ goto next3;
+ }
+ doexec(vbd_script, vec[XS_WATCH_PATH], sstate, stype);
+next3:
+ free(params);
break;
default:
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Roger Pau Monne
2011-Sep-16 15:36 UTC
[Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
# 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
Ian Campbell
2011-Sep-21 09:41 UTC
Re: [Xen-devel] [PATCH 1 of 2] hotplug: add hotplug-status disconnected
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 1316186985 -7200 > # Node ID 00949e363f6f2c70001da548403475628df93b97 > # Parent 63e254468d6e8d81baeafaed68f05791dc21eb4e > hotplug: add hotplug-status disconnected > > Added hotplug-status disconnected when vbd are removed. > > Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu> >Generally looks good, but I don''t think you''ve got the logical separation between the two patches quite right:> diff -r 63e254468d6e -r 00949e363f6f tools/xenbackendd/xenbackendd.c > --- a/tools/xenbackendd/xenbackendd.c Wed Sep 14 14:18:40 2011 +0200 > +++ b/tools/xenbackendd/xenbackendd.c Fri Sep 16 17:29:45 2011 +0200> @@ -169,7 +172,7 @@ main(int argc, char * const argv[]) > log_file = optarg; > break; > case ''p'': > - pidfile = pidfile; > + pidfile = optarg;This is an unrelated bugfix? If so please post it as such.> case ''s'': > vbd_script = optarg; > break; > @@ -297,11 +300,38 @@ main(int argc, char * const argv[]) > strerror(errno)); > goto next2; > } > - doexec(s, vec[XS_WATCH_PATH], sstate); > + doexec(s, vec[XS_WATCH_PATH], sstate, NULL); > break; > > case DEVTYPE_VBD: > - doexec(vbd_script, vec[XS_WATCH_PATH], sstate); > + /* check if given file is a block device or a raw image */ > + snprintf(buf, sizeof(buf), "%s/params", vec[XS_WATCH_PATH]); > + params = xs_read(xs, XBT_NULL, buf, 0); > + if(params == NULL) { > + dolog(LOG_ERR, > + "Failed to read %s (%s)", buf, strerror(errno)); > + goto next2; > + } > + if (stat(params, &stab) < 0) { > + dolog(LOG_ERR, > + "Failed to get info about %s (%s)", params, > + strerror(errno)); > + goto next3; > + } > + stype = NULL; > + if (S_ISBLK(stab.st_mode)) > + stype = "phy"; > + if (S_ISREG(stab.st_mode)) > + stype = "file"; > + if (stype == NULL) { > + dolog(LOG_ERR, > + "Failed to attach %s (not a block device or raw image)", > + params, strerror(errno)); > + goto next3; > + } > + doexec(vbd_script, vec[XS_WATCH_PATH], sstate, stype); > +next3: > + free(params);Isn''t this more like part of patch 2/2? It doesn''t seem to relate to the addition of the hotplug-status xenstore node. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-21 09:49 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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
Ian Jackson
2011-Sep-27 17:01 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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
Roger Pau Monné
2011-Sep-28 08:07 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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
Ian Campbell
2011-Sep-28 08:13 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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
Ian Jackson
2011-Sep-28 13:28 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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
Ian Jackson
2011-Sep-28 13:32 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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 scriptbeing 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
Roger Pau Monné
2011-Sep-29 08:25 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
> 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
Ian Campbell
2011-Sep-29 09:05 UTC
Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks
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