From patchwork Thu Mar 24 17:22:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: George Dunlap X-Patchwork-Id: 8663211 Return-Path: X-Original-To: patchwork-xen-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 77557C0553 for ; Thu, 24 Mar 2016 17:25:32 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3883F203B4 for ; Thu, 24 Mar 2016 17:25:31 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4BD26201FE for ; Thu, 24 Mar 2016 17:25:29 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aj8yH-0000Mo-7b; Thu, 24 Mar 2016 17:22:57 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aj8yG-0000MM-9i for xen-devel@lists.xen.org; Thu, 24 Mar 2016 17:22:56 +0000 Received: from [85.158.139.211] by server-12.bemta-5.messagelabs.com id 56/D1-16378-F6224F65; Thu, 24 Mar 2016 17:22:55 +0000 X-Env-Sender: prvs=884475cec=George.Dunlap@citrix.com X-Msg-Ref: server-10.tower-206.messagelabs.com!1458840164!14025837!3 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 8.11; banners=-,-,- X-VirusChecked: Checked Received: (qmail 1287 invoked from network); 24 Mar 2016 17:22:54 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-10.tower-206.messagelabs.com with RC4-SHA encrypted SMTP; 24 Mar 2016 17:22:54 -0000 X-IronPort-AV: E=Sophos;i="5.24,386,1454976000"; d="scan'208";a="348474000" From: George Dunlap To: Date: Thu, 24 Mar 2016 17:22:40 +0000 Message-ID: <1458840160-26733-10-git-send-email-george.dunlap@citrix.com> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1458840160-26733-1-git-send-email-george.dunlap@citrix.com> References: <1458840160-26733-1-git-send-email-george.dunlap@citrix.com> MIME-Version: 1.0 X-DLP: MIA2 Cc: Wei Liu , Ian Jackson , George Dunlap , Roger Pau Monne Subject: [Xen-devel] [PATCH v3 9/9] DO NOT APPLY libxl: Change hotplug script interface to use physical-device-path X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Ian Jackson 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 Signed-off-by: George Dunlap --- 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 CC: Wei Liu --- 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 f9a001e..5390e7c 100644 --- a/docs/misc/block-scripts.txt +++ b/docs/misc/block-scripts.txt @@ -59,18 +59,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 ------------------------- @@ -85,25 +85,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: /*