[Xen-devel] [PATCH v2 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path

[George Dunlap <george.dunlap@xxxxxxxxxx>]

From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
---
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@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
---
 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel