From: George Dunlap <george.dunlap@citrix.com>
To: xen-devel@lists.xen.org
Cc: Wei Liu <wei.liu2@citrix.com>,
Ian Jackson <Ian.Jackson@eu.citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Roger Pau Monne <roger.pau@citrix.com>
Subject: [PATCH v2 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path
Date: Mon, 21 Mar 2016 18:17:01 +0000 [thread overview]
Message-ID: <1458584221-24426-10-git-send-email-george.dunlap@citrix.com> (raw)
In-Reply-To: <1458584221-24426-1-git-send-email-george.dunlap@citrix.com>
From: Ian Jackson <ian.jackson@eu.citrix.com>
DO NOT APPLY. This is provided for future reference, as a starting
point to clean up the disk path. A simple fix will make it "work",
but will introduce a subtle race condition.
* Change block-common.sh on Linux to only write physical-device-path
with the path of the device node, rather than also writing
physical-device with its major:minor numbers.
* Have the libxl Linux hotplug script scheduler convert this, by
reading physical-device-path and writing physical-device. This
happens just when we have decided that there is no more script to
run.
(As a reminder: Many hotplug scripts are called multiple times; so
libxl__get_hotplug_script_info() is called repeatedly until it returns
'0'. Block scripts are only called once; but this gives us the
opportunity to do the translation at any point when
libxl__get_hotplug_script_info() *would* return zero.)
* Add an xxx about the fact that the sharing check in
tools/hotplug/Linux/block needs adjusting. Note that WITHOUT THIS
OTHER FIX THE CHANGE TO BLOCK-COMMON.SH IS BROKEN.
* This has been tested (with the xxx commented out) and works.
The reason the block script is broken with this change is that
block:check_sharing() reads physical-device to check the major:minor
for duplicates rather than checking the path. But since this is (now)
written by libxl without the block script lock held, it's possible
that a duplicate instance will run the check_sharing() check before
libxl gets a chance to write that node.
It should be sufficient to modify check_sharing() to read that node if
it's avialable, and if it's not available, to instead read the
major/minor from physical-device-path.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Chances since rfc v1:
- Fixed two bugs in the patch
- Use backend xenstore node rather than frondend
- Correctly interpret return value for libxl__device_physdisk_major_minor()
- Remove erroneous comment about libxl__device_physdisk_major_minor()'s return value
- Port to libxl script
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
docs/misc/block-scripts.txt | 38 ++++++--------------
tools/hotplug/Linux/block-common.sh | 15 ++------
tools/libxl/libxl_linux.c | 70 +++++++++++++++++++++++++++++++++++--
3 files changed, 82 insertions(+), 41 deletions(-)
diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
index 6dd5d48..58c6955 100644
--- a/docs/misc/block-scripts.txt
+++ b/docs/misc/block-scripts.txt
@@ -48,18 +48,18 @@ Output
Block scripts are responsible for making sure that if a file is
provided to a VM read/write, that it is not provided to any other VM.
-FreeBSD block hotplug scripts must write
-"$XENBUS_PATH/physical-device-path" with the path to the physical
-device or file. Linux and NetBSD block hotplug scripts *should* also
-write this node.
+Block hotplug scripts must write "$XENBUS_PATH/physical-device-path"
+with the path to the physical device or file.
-For the time being, Linux and NetBSD block hotplug scripts must write
-"$XENBUS_PATH/physical-device" with the device's major and minor
-numbers, written in hex, and separated by a colon.
+Linux scripts used to write "$XENBUS_PATH/physical-device" with the
+major and minor number of a block device, separated by a colon,
+instead of physical-device-path. This interface style is deprecated
+but still supported. However, some functionality, such as emulated
+devices for HVM guests, may not work.
Scripts which include block-common.sh can simply call write_dev "$dev"
with a path to the device, and write_dev will do the right thing, now
-and going forward. (See the discussion below.)
+and going forward.
Rationale and future work
-------------------------
@@ -74,25 +74,9 @@ major/minor numbers, and can give direct access to a file without
going through loopback; so its driver will consume
physical-device-path.
-On Linux, the device model (qemu) needs access to a file it can
-interpret to provide emulated disks before paravirtualized drivers are
-marked as up. The easiest way to accomplish this is to allow qemu to
-consume physical-device-path (rather than, say, having dom0 act as
-both a frontend and a backend).
-
-Going forward, the plan is at some point to have all block scripts
-simply write "physical-device-path", and then have libxl write the
-other nodes. The reason we haven't done this yet is that the main
-block script wants to check to make sure the *major/minor* number
-hasn't been re-used, rather than just checking that the *specific
-device node* isn't re-used. To do this it currently uses
-physical-device; and to do this *safely* it needs physical-device to
-be written with the lock held.
-
-The simplest solution for sorting this out would be to have the block
-script use physical-device if it's present, but if not, to directly
-stat physical-device-path. But there's not time before the 4.7
-release to make sure all that works.
+Rather than have different interfaces for different operating systems,
+we just have the block script write the path, and libxl do the
+tranlation when necessary.
Another possibility would be to only call out to scripts when using
custom hotplug scripts; and when doing files or physical devices, to
diff --git a/tools/hotplug/Linux/block-common.sh b/tools/hotplug/Linux/block-common.sh
index 35110b4..2fcbf76 100644
--- a/tools/hotplug/Linux/block-common.sh
+++ b/tools/hotplug/Linux/block-common.sh
@@ -50,23 +50,14 @@ device_major_minor()
##
-# Write physical-device = MM,mm to the store, where MM and mm are the major
-# and minor numbers of device respectively.
+# Write physical-device-path = "$1" to the store
#
# @param device The device from which major and minor numbers are read, which
# will be written into the store.
#
write_dev() {
- local mm
-
- mm=$(device_major_minor "$1")
-
- if [ -z $mm ]
- then
- fatal "Backend device does not exist"
- fi
-
- xenstore_write "$XENBUS_PATH/physical-device" "$mm"
+ xxx check_sharing needs to be fixed or this introduces a race
+
xenstore_write "$XENBUS_PATH/physical-device-path" "$1"
success
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index be4afc6..9526dbd 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -233,6 +233,65 @@ error:
return rc;
}
+static int disk_copy_block_device(libxl__gc *gc, libxl__device *dev,
+ libxl__device_action action)
+{
+ int rc;
+ xs_transaction_t t = 0;
+ const char *be_path = libxl__device_backend_path(gc, dev);
+ const char *majmin_path = GCSPRINTF("%s/physical-device", be_path);
+ const char *path_path = GCSPRINTF("%s/physical-device-path", be_path);
+ int major, minor;
+
+ if (action != LIBXL__DEVICE_ACTION_ADD)
+ return 0;
+
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ const char *majmin;
+ rc = libxl__xs_read_checked(gc,t, majmin_path, &majmin);
+ if (rc) goto out;
+ if (majmin) {
+ /* Old-style Linux-only hotplug script wrote physical-device */
+ rc = 0;
+ goto out;
+ }
+
+ const char *bdev;
+ rc = libxl__xs_read_checked(gc,t, path_path, &bdev);
+ if (rc) goto out;
+ if (!bdev) {
+ LOGE(ERROR, "nothing wrote physical device to %s", path_path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (libxl__device_physdisk_major_minor(bdev, &major, &minor)<0) {
+ LOG(ERROR, "libxl__device_physdisk_major_minor failed (on %s)",
+ bdev);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ majmin = GCSPRINTF("%x:%x", major, minor);
+ rc = libxl__xs_write_checked(gc,t, majmin_path, majmin);
+ if (rc) goto out;
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc<0) goto out;
+ }
+
+ return rc;
+
+ out:
+ libxl__xs_transaction_abort(gc, &t);
+ return rc;
+}
+
+
int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
libxl__device_action action,
@@ -245,9 +304,16 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
if (num_exec != 0) {
LOG(DEBUG, "num_exec %d, not running hotplug scripts", num_exec);
rc = 0;
- goto out;
+ } else {
+ rc = libxl__hotplug_disk(gc, dev, args, env, action);
+ }
+ if (!rc) {
+ /* No more hotplug scripts to run. But, the hotplug
+ * scripts write physical-device-path but we blkback wants
+ * physical-device (major:minor). So now we adjust
+ * that. */
+ rc = disk_copy_block_device(gc, dev, action);
}
- rc = libxl__hotplug_disk(gc, dev, args, env, action);
break;
case LIBXL__DEVICE_KIND_VIF:
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-21 18:17 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-21 18:16 [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts George Dunlap
2016-03-21 18:16 ` [PATCH v2 1/9] tools/hotplug: Add a "dummy" hotplug script for testing George Dunlap
2016-03-21 18:16 ` [PATCH v2 2/9] libxl: Remove redundant setting of phyical-device George Dunlap
2016-03-21 18:16 ` [PATCH v2 3/9] tools/hotplug: Write physical-device-path in addition to physical-device George Dunlap
2016-03-21 18:16 ` [PATCH v2 4/9] libxl: Move check for local access to a funciton George Dunlap
2016-03-21 18:16 ` [PATCH v2 5/9] libxl: Rearrange qemu upstream disk argument code George Dunlap
2016-03-21 18:16 ` [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub George Dunlap
2016-03-22 10:38 ` George Dunlap
2016-03-21 18:16 ` [PATCH v2 7/9] libxl: Allow local access for block devices with hotplug scripts George Dunlap
2016-03-21 18:17 ` [PATCH v2 8/9] docs: Document block-script protocol George Dunlap
2016-03-22 12:52 ` Roger Pau Monné
2016-03-23 11:19 ` George Dunlap
2016-03-23 12:14 ` Roger Pau Monné
2016-03-24 16:29 ` George Dunlap
2016-03-24 16:51 ` Roger Pau Monné
2016-03-21 18:17 ` George Dunlap [this message]
2016-03-23 10:10 ` [PATCH v2 0/9] tools: Allow HVM domains emulated access to disks provided by hotplug scripts Roger Pau Monné
2016-03-23 10:36 ` George Dunlap
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1458584221-24426-10-git-send-email-george.dunlap@citrix.com \
--to=george.dunlap@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=roger.pau@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).