Hello again,
Martin Pitt [2015-05-17 13:02 +0200]:
> - Alternative: Fix device_found_node() to create a .device with the
> correct ("tentative") state if the device doesn't exist in /dev/
> or udev (yet). Then manager_load_unit() would not create a "dead"
> stub .device any more, but use the existing "tentative" .device
> and mounts would also stop being auto-unmounted.
The attached patches implement this now. The whole device state logic
with the "tentative" handling isn't that easy to understand, and
indeed there are still some "nice" traps.
The first patch fixes the incomplete handling of tentative devices
that got picked up by mountinfo -- you NACK'ed the first patch which
would avoid creating the stub "dead" .device units for those, so this
now makes sure that this comment in device_found_node():
/* This is called whenever we find a device referenced in
* /proc/swaps or /proc/self/mounts. Such a device might be
* mounted/enabled at a time where udev has not finished
* probing it yet, and we thus haven't learned about it
* yet. In this case we will set the device unit to
* "tentative" state. */
will actually become true: i. e. with this patch this now creates a
.device unit with found == DEVICE_FOUND_MOUNT and thus state ==
DEVICE_TENTATIVE. This avoids the aforementioned trap of
manager_load_unit() creating bogus units later on, but keeps the idea
of having a real .device unit in "tentative" state for non-udev
devices.
This fixes the original "systemd immediately unmounts my mounts" bug,
but not for very long: If you remount or unmount just one mount on a
tentative device, mountinfo changes, and device_found_node() now calls
device_update_found_one() with "add == 0". Then
n = add ? (d->found | found) : (d->found & ~found);
would unset the previous DEVICE_FOUND_MOUNT flag, leaving 0 (i. e.
DEVICE_NOT_FOUND). Thus the previously "tentative" device would once
again be set to "dead", and systemd would unmount all other mounts
from it. This must never happen, we simply can't declare tentative
devices as dead and clean up their unmounts, this only works for
"plugged" ones that we know via udev.
I tested this now with various container setups (with and without
udev/writable /sys), real iron, VM, and the infamous "remove mounted
CD"; the latter still cleans up the stale mount point as intended.
Perhaps it's time to stick our heads together again, write down the
use cases where this stuff should actually do something (e. g. cleanup
mounts and track .device units), and see if we can simplify this
again?
Thanks,
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
From 827d258b8640b6137f8c388e90016d8738be4a2d Mon Sep 17 00:00:00 2001 From: Martin Pitt <martin.p...@ubuntu.com> Date: Sun, 17 May 2015 15:07:47 +0200 Subject: [PATCH 1/2] device: create units with intended "found" value Change device_found_node() to also create a .device unit if a device is not known by udev; this is the case for "tentative" devices picked up by mountinfo (DEVICE_FOUND_MOUNT). With that we can record the "found" attribute on the unit. Change device_setup_unit() to also accept a NULL udev_device, and don't add the extra udev information in that case. Previously device_found_node() would not create a .device unit, and unit_add_node_link() would then create a "dead" stub one via manager_load_unit(), so we lost the "found" attribute and unmounted everything from that device. https://launchpad.net/bugs/1444402 http://lists.freedesktop.org/archives/systemd-devel/2015-May/031658.html --- src/core/device.c | 53 ++++++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 8eca4c2..3fddc62 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -292,26 +292,28 @@ static int device_add_udev_wants(Unit *u, struct udev_device *dev) { static int device_setup_unit(Manager *m, struct udev_device *dev, const char *path, bool main) { _cleanup_free_ char *e = NULL; - const char *sysfs; + const char *sysfs = NULL; Unit *u = NULL; bool delete; int r; assert(m); - assert(dev); assert(path); - sysfs = udev_device_get_syspath(dev); - if (!sysfs) - return 0; - r = unit_name_from_path(path, ".device", &e); if (r < 0) return log_error_errno(r, "Failed to generate unit name from device path: %m"); u = manager_get_unit(m, e); + if (dev) { + sysfs = udev_device_get_syspath(dev); + if (!sysfs) + return 0; + } + if (u && + sysfs && DEVICE(u)->sysfs && !path_equal(DEVICE(u)->sysfs, sysfs)) { log_unit_debug(u, "Device %s appeared twice with different sysfs paths %s and %s", e, DEVICE(u)->sysfs, sysfs); @@ -336,17 +338,19 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa /* If this was created via some dependency and has not * actually been seen yet ->sysfs will not be * initialized. Hence initialize it if necessary. */ + if (sysfs) { + r = device_set_sysfs(DEVICE(u), sysfs); + if (r < 0) + goto fail; - r = device_set_sysfs(DEVICE(u), sysfs); - if (r < 0) - goto fail; + (void) device_update_description(u, dev, path); - (void) device_update_description(u, dev, path); + /* The additional systemd udev properties we only interpret + * for the main object */ + if (main) + (void) device_add_udev_wants(u, dev); + } - /* The additional systemd udev properties we only interpret - * for the main object */ - if (main) - (void) device_add_udev_wants(u, dev); /* Note that this won't dispatch the load queue, the caller * has to do that if needed and appropriate */ @@ -788,22 +792,17 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, * will still have a device node even when the medium * is not there... */ - if (stat(node, &st) < 0) { - if (errno == ENOENT) + if (stat(node, &st) >= 0) { + if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) return 0; - return log_error_errno(errno, "Failed to stat device node file %s: %m", node); - } - - if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) - return 0; + dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); + if (!dev && errno != ENOENT) + return log_oom(); - dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); - if (!dev) { - if (errno == ENOENT) - return 0; - - return log_oom(); + } else { + if (errno != ENOENT) + return log_error_errno(errno, "Failed to stat device node file %s: %m", node); } /* If the device is known in the kernel and newly -- 2.1.4
From 7f9c2d8f1d38fda86dfc389f0b38e3f5706338cb Mon Sep 17 00:00:00 2001 From: Martin Pitt <martin.p...@ubuntu.com> Date: Thu, 14 May 2015 12:36:44 +0200 Subject: [PATCH] device: Don't bind to nonexisting devices In containers, with remote/NFS/other non-device file systems, or with /dev/root it is common to have mountinfo entries which don't have an actual device node in /dev, and thus device_found_node() will not create a .device unit for them (not even a tentative one). When binding a .mount to a .device, only do that if the .device actually exists; if not, it would be immediately (attempted to) get unmounted again. The previously used manager_load_unit() would create stub .device units with state "dead", causing the above effect; so use manager_get_unit() instead. Also revert commit 7ba2711d; it's a special case of this, and a rather ugly hardcoding of a device name. https://launchpad.net/bugs/1444402 http://lists.freedesktop.org/archives/systemd-devel/2015-May/031658.html --- src/core/mount.c | 6 ------ src/core/unit.c | 8 +++++--- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 8ef3d17..9beff35 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -317,12 +317,6 @@ static int mount_add_device_links(Mount *m) { if (!is_device_path(p->what)) return 0; - /* /dev/root is a really weird thing, it's not a real device, - * but just a path the kernel exports for the root file system - * specified on the kernel command line. Ignore it here. */ - if (path_equal(p->what, "/dev/root")) - return 0; - if (path_equal(m->where, "/")) return 0; diff --git a/src/core/unit.c b/src/core/unit.c index 1c1d9cf..9f028c4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2832,9 +2832,11 @@ int unit_add_node_link(Unit *u, const char *what, bool wants) { if (r < 0) return r; - r = manager_load_unit(u->manager, e, NULL, NULL, &device); - if (r < 0) - return r; + device = manager_get_unit(u->manager, e); + if (!device) { + log_unit_debug(u, "Device %s does not exist, not linking to it", e); + return 0; + } r = unit_add_two_dependencies(u, UNIT_AFTER, u->manager->running_as == MANAGER_SYSTEM ? UNIT_BINDS_TO : UNIT_WANTS, device, true); if (r < 0) -- 2.1.4
signature.asc
Description: Digital signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
