xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
To: xen-devel@lists.xenproject.org
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	"Ian Jackson" <iwj@xenproject.org>, "Wei Liu" <wl@xen.org>,
	"Anthony PERARD" <anthony.perard@citrix.com>
Subject: [RFC PATCH 2/2] libxl: allow to skip block script completely
Date: Tue, 27 Apr 2021 02:22:32 +0200	[thread overview]
Message-ID: <25fbb81f8f7805a390613521dadedaeab4cd4563.1619482896.git-series.marmarek@invisiblethingslab.com> (raw)
In-Reply-To: <cover.3a5d506462133586bd805b72a226916af6a33799.1619482896.git-series.marmarek@invisiblethingslab.com>

Default block script is quite slow and requires global lock which slows
it down even more (for domains with multiple disks). The common case of
a block device-based disk is trivial to handle and does not require
locking. This can be handled directly within libxl, to avoid slow script
execution and waiting for it to finish. This, depending on the hardware,
may save about 0.5s of domain start time per disk.

Allow setting script name to empty string to skip executing the script
at all, and use target name as a block device path directly.

This does skip two functions of the block script:
 - checking if device isn't used anywhere else (including mounted in
   dom0)
 - setting up loop device for a file-based disk

The former is expected to be done by the operator manually (or by a
higher level management stack, that calls into libxl). The later is a
limitation of the current implementation, but should be possible to
extend in the future.

The code to fill 'physical-device' xenstore node is added via
libxl__hotplug_disk() (in libxl_linux.c) intentionally. This code is
called in device backend domain (which may be not dom0), contrary to
device_disk_add() which fills all the other xenstore entries, but is
called always in the toolstack domain (dom0).
libxl__hoplug_disk() is called from libxl__get_hotplug_script_info(),
which may not sound like the most logical place to actually change
some state, but it is called when we need it.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 docs/man/xl-disk-configuration.5.pod.in |  4 ++-
 tools/libs/light/libxl_disk.c           |  7 ++-
 tools/libs/light/libxl_linux.c          | 62 ++++++++++++++++++++++++++-
 3 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl-disk-configuration.5.pod.in b/docs/man/xl-disk-configuration.5.pod.in
index 71d0e86e3d63..3b3b3c329e13 100644
--- a/docs/man/xl-disk-configuration.5.pod.in
+++ b/docs/man/xl-disk-configuration.5.pod.in
@@ -261,6 +261,10 @@ information to be interpreted by the executable program I<SCRIPT>,
 
 These scripts are normally called "block-I<SCRIPT>".
 
+Setting empty script avoids calling the hoplug script at all (including
+the default one). It skips device sharing safety check and
+requires the target to point at a block device. Empty script value is supported
+on Linux backend domain only.
 
 =item B<direct-io-safe>
 
diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
index 411ffeaca6ce..4278db449a2f 100644
--- a/tools/libs/light/libxl_disk.c
+++ b/tools/libs/light/libxl_disk.c
@@ -324,8 +324,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                 flexarray_append(back, "params");
                 flexarray_append(back, dev);
 
-                script = libxl__abs_path(gc, disk->script?: "block",
-                                         libxl__xen_script_dir_path());
+                if (disk->script && !disk->script[0])
+                    script = "";
+                else
+                    script = libxl__abs_path(gc, disk->script?: "block",
+                                             libxl__xen_script_dir_path());
                 flexarray_append_pair(back, "script", script);
 
                 assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
diff --git a/tools/libs/light/libxl_linux.c b/tools/libs/light/libxl_linux.c
index cc8baf5c3eae..8832f2c6483a 100644
--- a/tools/libs/light/libxl_linux.c
+++ b/tools/libs/light/libxl_linux.c
@@ -160,6 +160,62 @@ out:
     return rc;
 }
 
+static int libxl__hotplug_disk_direct_add(libxl__gc *gc, libxl__device *dev,
+                                      const char *be_path)
+{
+    char *params;
+    xs_transaction_t t = XBT_NULL;
+    struct stat st;
+    int rc = 0;
+    char *xs_kvs[] = { "physical-device", NULL,
+                       "physical-device-path", NULL,
+                       "hotplug-status", "connected",
+                       NULL, NULL, };
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto error;
+
+        params = libxl__xs_read(gc, t,
+                                GCSPRINTF("%s/%s", be_path, "params"));
+        if (!params) {
+            rc = ERROR_FAIL;
+            goto error;
+        }
+
+        if (stat(params, &st) == -1) {
+            LOGED(ERROR, dev->domid, "failed to stat %s", params);
+            rc = ERROR_FAIL;
+            goto error;
+        }
+
+        if ((st.st_mode & S_IFMT) != S_IFBLK) {
+            LOGD(ERROR, dev->domid, "not a block device: %s", params);
+            rc = ERROR_FAIL;
+            goto error;
+        }
+
+        xs_kvs[1] = GCSPRINTF("%x:%x", major(st.st_rdev), minor(st.st_rdev));
+        xs_kvs[3] = params;
+        rc = libxl__xs_writev(gc, t, be_path, xs_kvs);
+        if (rc) goto error;
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto error;
+    }
+
+    return 0;
+
+error:
+    libxl__xs_transaction_abort(gc, &t);
+    if (libxl__xs_write_checked(gc, XBT_NULL,
+                                GCSPRINTF("%s/%s", be_path, "hotplug-status"),
+                                "error"))
+        LOGD(ERROR, dev->domid, "failed to write 'hotplug-status'");
+    return rc;
+}
+
 static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
                                char ***args, char ***env,
                                libxl__device_action action)
@@ -177,6 +233,12 @@ static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
         goto out;
     }
 
+    if (!script[0]) {
+        if (action == LIBXL__DEVICE_ACTION_ADD)
+            rc = libxl__hotplug_disk_direct_add(gc, dev, be_path);
+        goto out;
+    }
+
     *env = get_hotplug_env(gc, script, dev);
     if (!*env) {
         LOGD(ERROR, dev->domid, "Failed to get hotplug environment");
-- 
git-series 0.9.1


  parent reply	other threads:[~2021-04-27  0:24 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-27  0:22 [RFC PATCH 0/2] libxl: support common cases without block script Marek Marczykowski-Górecki
2021-04-27  0:22 ` [RFC PATCH 1/2] libxl: rename 'error' label to 'out' as it is used for success too Marek Marczykowski-Górecki
2021-04-27  0:22 ` Marek Marczykowski-Górecki [this message]
2021-04-28  6:48 ` [RFC PATCH 0/2] libxl: support common cases without block script Demi Marie Obenour
2021-04-28 12:26   ` Jason Andryuk

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=25fbb81f8f7805a390613521dadedaeab4cd4563.1619482896.git-series.marmarek@invisiblethingslab.com \
    --to=marmarek@invisiblethingslab.com \
    --cc=anthony.perard@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.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).