|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH 4 of 7 v2] blktap3/tapback: Introduce back-end XenStore path handler
This patch introduces the handler executed when the back-end XenStore path gets
modified. A back-end XenStore path is modified as a result of a device
creation/removal request. The device is created/removed depending on whether
its path exists/does not exist in XenStore.
Creating a device comprises creating the in-memory representation of it and
adding it to the list of devices, locating the tapdisk designated to serve
this VBD, and setting a XenStore watch to the front-end path of the
newly-created device.
Deleting a device comprises removing that XenStore watch and deallocating its
in-memory representation.
The main issues in this patch are the following:
* Function tapback_backend_handle_backend_watch calls tapback_backend_scan
even when we know in which domain a device changed, wouldn't it make sense
from a performance perspective to only probe the devices of that domain
instead of probing ALL devices of ALL domains?
* Is there a race condition between the tapback daemon and the toolstack
regarding partially brought up devices? I'm trying to figure out what the
"serial" thing does (check functions tapback_backend_create_device and
tapback_backend_probe_device) but I don't get it. Even if there is such a
race condition, I'm not convinced that the way that "serial" thing is
implemented fixes it.
diff -r a9beb303b541 -r 8094db37a249 tools/blktap3/tapback/backend.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/backend.c Fri Jan 04 12:09:05 2013 +0000
@@ -0,0 +1,434 @@
+/*
+ * Copyright (C) 2012 Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
+ * USA.
+ *
+ * This file contains the handler executed when the back-end XenStore path gets
+ * modified.
+ */
+
+#include "tapback.h"
+#include "xenstore.h"
+#include "tap-ctl-info.h"
+
+/**
+ * Removes the XenStore watch from the front-end.
+ *
+ * @param device the VBD whose front-end XenStore path should stop being
+ * watched
+ */
+static void
+tapback_device_unwatch_frontend_state(vbd_t * const device)
+{
+ assert(device);
+
+ if (device->frontend_state_path)
+ xs_unwatch(blktap3_daemon.xs, device->frontend_state_path,
+ BLKTAP3_FRONTEND_TOKEN);
+
+ free(device->frontend_state_path);
+ device->frontend_state_path = NULL;
+}
+
+/**
+ * Destroys and deallocates the back-end part of a VBD.
+ *
+ * @param device the VBD to destroy
+ */
+static void
+tapback_backend_destroy_device(vbd_t * const device)
+{
+ assert(device);
+
+ TAILQ_REMOVE(&blktap3_daemon.devices, device, backend_entry);
+
+ tapback_device_unwatch_frontend_state(device);
+
+ if (device->frontend_path) {
+ free(device->frontend_path);
+ device->frontend_path = NULL;
+ }
+
+ if (device->name) {
+ free(device->name);
+ device->name = NULL;
+ }
+
+ /*
+ * XXX device->bdev is expected to have been freed
+ */
+
+ free(device);
+}
+
+/**
+ * Retrieves the tapdisk designated to serve this device, storing this
+ * information in the supplied VBD handle.
+ *
+ * @param bdev the VBD handle for whose tapdisk handle should be retrieved
+ * @returns 0 on success, an error code otherwise
+ *
+ * FIXME How does this thing work since bdev->dev is never initialised?
+ * Probably because they're both initialised to 0 and we use one tapdisk per
+ * VBD?
+ *
+ * XXX Only called by blkback_probe.
+ */
+static inline int
+blkback_find_tapdisk(vbd_t * const bdev)
+{
+ struct tqh_tap_list list;
+ tap_list_t *tap = NULL;
+ int err = 0;
+
+ assert(bdev);
+
+ if ((err = tap_ctl_list(&list))) {
+ WARN("error listing tapdisks: %s\n", strerror(err));
+ return err;
+ }
+
+ err = ESRCH;
+ if (!TAILQ_EMPTY(&list)) {
+ tap_list_for_each_entry(tap, &list)
+ if (tap->minor == minor(bdev->dev)) {
+ err = 0;
+ memcpy(&bdev->tap, tap, sizeof(bdev->tap));
+ break;
+ }
+ tap_ctl_list_free(&list);
+ } else
+ WARN("no tapdisks\n");
+
+ return err;
+}
+
+/**
+ * Creates a device and adds it to the list of devices.
+ * Initiates a XenStore watch to the front-end state.
+ *
+ * Creating the device implies initializing the handle and retrieving all the
+ * information of the tapdisk serving this VBD.
+ *
+ * @param domid the ID of the domain where the VBD is created
+ * @param name the name of the device
+ * @returns 0 on success, an error code otherwise
+ */
+static inline int
+tapback_backend_create_device(const domid_t domid, const char * const name)
+{
+ vbd_t *device = NULL;
+ int err = 0;
+ char *end = NULL;
+
+ assert(name);
+
+ DBG("creating device %d/%s\n", domid, name);
+
+ if (!(device = calloc(1, sizeof(*device)))) {
+ WARN("error allocating memory\n");
+ err = -errno;
+ goto fail;
+ }
+
+ /*
+ * TODO replace with alloc_serial() and check for overflow
+ */
+ device->serial = blktap3_daemon.serial++;
+ device->domid = domid;
+
+ TAILQ_INSERT_TAIL(&blktap3_daemon.devices, device, backend_entry);
+
+ if (!(device->name = strdup(name))) {
+ err = -errno;
+ goto fail;
+ }
+
+ /*
+ * Get the front-end path from XenStore. We need this to talk to front-end.
+ */
+ if (!(device->frontend_path = tapback_device_read(device, "frontend"))) {
+ err = -errno;
+ goto fail;
+ }
+
+ /*
+ * Write to XenStore tapback-serial.
+ */
+ if ((err = tapback_device_printf(device, BLKTAP3_SERIAL, false, "%lld",
+ device->serial)))
+ goto fail;
+
+ /*
+ * Get the tapdisk that is serving this virtual block device, along with
+ * it's parameters.
+ */
+ device->devid = strtoul(name, &end, 0);
+ if (*end != 0 || end == name) {
+ err = -EINVAL;
+ goto fail;
+ }
+
+ if ((err = blkback_find_tapdisk(device))) {
+ WARN("error looking for tapdisk: %s", strerror(err));
+ goto fail;
+ }
+
+ /*
+ * get the VBD parameters from the tapdisk
+ */
+ if ((err = tap_ctl_info(device->tap.pid, device->tap.minor,
+ &device->sectors, &device->sector_size, &device->info))) {
+ WARN("error retrieving disk characteristics: %s\n", strerror(err));
+ goto fail;
+ }
+
+ DBG("got %s-%d-%d with tapdev %d/%d\n", BLKTAP3_BACKEND_NAME,
+ device->domid, device->devid, device->tap.pid, device->tap.minor);
+
+ /*
+ * Finally, watch the front-end path in XenStore for changes, i.e.
+ * /local/domain/<domid>/device/vbd/<devname>/state
+ * After this, we wait for the front-end to switch state to continue with
+ * the initialisation.
+ */
+ if (!(device->frontend_state_path= mprintf("%s/state",
+ device->frontend_path))) {
+ err = -errno;
+ goto fail;
+ }
+ /*
+ * We use the same token for all front-end watches. We don't have to use a
+ * unique token for each front-end watch because when a front-end watch
+ * fires we are given the XenStore path that changed.
+ */
+ if (!xs_watch(blktap3_daemon.xs, device->frontend_state_path,
+ BLKTAP3_FRONTEND_TOKEN)) {
+ free(device->frontend_state_path);
+ err = -errno;
+ goto fail;
+ }
+
+ return 0;
+
+fail:
+ WARN("error creating device: domid=%d name=%s err=%d (%s)\n",
+ domid, name, err, strerror(-err));
+ if (device)
+ tapback_backend_destroy_device(device);
+
+ return err;
+}
+
+/**
+ * Creates (removes) a device depending on the existence (non-existence) of the
+ * "backend/<backend name>/@domid/@devname" XenStore path.
+ *
+ * @param domid the ID of the domain where the VBD is created
+ * @param devname device name
+ * @returns 0 on success, an error code otherwise
+ */
+static int
+tapback_backend_probe_device(const domid_t domid, const char * const devname)
+{
+ int should_exist = 0, create = 0, remove = 0;
+ vbd_t *device = NULL;
+ char * s = NULL;
+
+ assert(devname);
+
+ DBG("probe device domid=%d name=%s\n", domid, devname);
+
+ /*
+ * Ask XenStore if the device _should_ exist.
+ */
+ s = tapback_xs_read(blktap3_daemon.xs, blktap3_daemon.xst, "%s/%d/%s",
+ BLKTAP3_BACKEND_PATH, domid, devname);
+ should_exist = s != NULL;
+ free(s);
+
+ /*
+ * Search the device list for this specific device.
+ */
+ tapback_backend_find_device(device,
+ device->domid == domid && !strcmp(device->name, devname));
+
+ /*
+ * If XenStore says that the device should exist but it's not in our
device
+ * list, we must create it. If it's the other way round, this is a removal.
+ * If XenStore says that the device should exist and it's already in our
+ * device list, we must compare the serials in case the device got removed
+ * and re-created.
+ */
+ remove = device && !should_exist;
+ create = !device && should_exist;
+
+ DBG("should exist=%d device=%p remove=%d create=%d\n",
+ should_exist, device, remove, create);
+
+ /*
+ * The device exists in our list of device and XenStore says it should
+ * exist, we need to check if the device has been removed and re-added to
+ * XenStore. If this has happened the device's serial in XenStore will be
+ * different from the one in our list (sync with fast remove/re-create
+ * cycles).
+ */
+ if (device && should_exist) {
+ char *s = NULL;
+ long long serial = 0;
+
+ /*
+ * read the serial
+ */
+ if (!(s = tapback_device_read(device, BLKTAP3_SERIAL))) {
+ WARN("error reading serial for %s: %s\n", devname,
+ strerror(errno));
+ return -errno;
+ }
+ serial = atoll(s);
+ free(s);
+
+ remove = create = serial != device->serial;
+
+ /*
+ * TODO Is it possible that neither create nor remove are true? If so,
+ * why was this function called in the first place?
+ */
+ WARN_ON(!remove && !create, "%d/%s neither created nor removed\n",
+ domid, devname);
+ }
+
+ /*
+ * Remember that remove and create may both be true at the same time, as
+ * this indicates that the device has been removed and re-created too fast.
+ * In this case, we too need to remove and re-create the device,
+ * respectively.
+ */
+
+ if (remove)
+ tapback_backend_destroy_device(device);
+
+ if (create) {
+ const int err = tapback_backend_create_device(domid, devname);
+ if (0 != err) {
+ WARN("error creating device %d on domain %d: %s\n", devname, domid,
+ strerror(-err));
+ return err;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * Scans XenStore for all blktap3 devices and probes each one of them.
+ *
+ * XXX Only called by tapback_backend_handle_backend_watch.
+ */
+static int
+tapback_backend_scan(void)
+{
+ vbd_t *device = NULL, *next = NULL;
+ unsigned int i = 0, j = 0, n = 0, m = 0;
+ char **dir = NULL;
+
+ /*
+ * scrap all non-existent devices
+ * FIXME Why do we do this?
+ * FIXME Is this costly?
+ */
+
+ tapback_backend_for_each_device(device, next)
+ tapback_backend_probe_device(device->domid, device->name);
+
+ /*
+ * probe the new ones
+ *
+ * FIXME We're checking each and every device in each and every domain,
+ * could there be a performance issue in the presence of many VMs/VBDs?
+ * (e.g. boot-storm)
+ */
+ if (!(dir = xs_directory(blktap3_daemon.xs, blktap3_daemon.xst,
+ BLKTAP3_BACKEND_PATH, &n)))
+ return 0;
+
+ for (i = 0; i < n; i++) { /* for each domain */
+ char *path = NULL, **sub = NULL, *end = NULL;
+ domid_t domid = 0;
+
+ /*
+ * Get the domain ID.
+ */
+ domid = strtoul(dir[i], &end, 0);
+ if (*end != 0 || end == dir[i])
+ continue;
+
+ /*
+ * Read the devices of this domain.
+ */
+ path = mprintf("%s/%d", BLKTAP3_BACKEND_PATH, domid);
+ assert(path != NULL);
+ sub = xs_directory(blktap3_daemon.xs, blktap3_daemon.xst, path, &m);
+ free(path);
+
+ /*
+ * Probe each device.
+ */
+ for (j = 0; j < m; j++)
+ tapback_backend_probe_device(domid, sub[j]);
+
+ free(sub);
+ }
+
+ free(dir);
+ return 0;
+}
+
+int
+tapback_backend_handle_backend_watch(char * const path)
+{
+ char *s = NULL, *end = NULL, *name = NULL;
+ domid_t domid = 0;
+
+ assert(path);
+
+ s = strtok(path, "/");
+ assert(!strcmp(s, XENSTORE_BACKEND));
+ if (!(s = strtok(NULL, "/")))
+ return tapback_backend_scan();
+
+ assert(!strcmp(s, BLKTAP3_BACKEND_NAME));
+ if (!(s = strtok(NULL, "/")))
+ return tapback_backend_scan();
+
+ domid = strtoul(s, &end, 0);
+ if (*end != 0 || end == s) {
+ WARN("invalid domain ID \'%s\'\n", s);
+ return -EINVAL;
+ }
+
+ /*
+ * FIXME since we know which domain changed, why scan the whole thing?
+ * Add the domid as an optional parameter to tapback_backend_scan.
+ */
+ if (!(name = strtok(NULL, "/")))
+ return tapback_backend_scan();
+
+ /*
+ * Create or remove a specific device.
+ */
+ return tapback_backend_probe_device(domid, name);
+}
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |