Re: [systemd-devel] [correct PATCH v2] dev-root.device is not active, results in an umount spree
Lennart Poettering [2015-05-18 23:04 +0200]: On Mon, 18.05.15 16:08, Martin Pitt (martin.p...@ubuntu.com) wrote: Martin Pitt [2015-05-17 15:54 +0200]: 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. Eek, I attached the wrong 0002- patch, sorry about that. The above is fixed by the attached patch, please ignore 0002- from the previous mail. For completeness I also re-attach 0001- again (unchanged). Still not getting what the purpose of the 0002 patch is, even in this revision... Please elaborate! I'll try to explain step by step: Say you are booting with plan9 fs, a container, or some other thing with a mount that references a device /dev/foo which isn't actually available as /dev/foo and in udev. Note that you can't reproduce this in nspawn, as it forcefully mounts /sys as r/o, which triggers the unit_type_supported(UNIT_DEVICE) safety check in unit_add_node_link(); this happens in environments with writable /sys. - Boot, dev-foo.device becomes DEVICE_FOUND_MOUNT/tentative - Do some more mounts from /dev/foo, e. g. mkdir /tmp/etc /tmp/boot mount -o bind /etc /tmp/etc mount -o bind /boot /tmp/boot (In practice, you'd probably do such bind mounts with nspawn --bind or lxc.mount.entry) tmp-etc.mount and tmp-boot.mount will be BindsTo=dev-foo.device, and dev-foo.device's status is still unchanged (DEVICE_FOUND_MOUNT/tentative) - Now do umount /tmp/boot. This *also* umounts /tmp/etc! Journal says systemd[1]: Requested transaction contradicts existing jobs: Resource deadlock avoided systemd[1]: Unmounted /tmp/etc. and dev-sda3.device is now inactive/dead, which tears down the bound tmp-etc.mount. Boom! Reason: Unmounting /tmp/boot triggers device_update_found_one(found==DEVICE_FOUND_MOUNT, add==0). d-found previously was DEVICE_FOUND_MOUNT, and this n = add ? (d-found | found) : (d-found ~found); computes the new state to 0 (i. e. DEVICE_NOT_FOUND), and calls device_set_state(DEVICE_DEAD). Thus here we (1) don't consider that dev-foo.device is still bound by other units (tmp-etc.mount) and (2) lose the information that dev-foo.device is tentative. So the problem is that this tentative → dead transition only works if a device is referenced once, but causes overzealous unmounts if there are more references. We need a reference count, or check if the device is bound by other device still. Come to think of it now, we actually already have that: Id=dev-foo.device BoundBy=tmp-boot.mount tmp-etc.mount But my patch doesn't take that into account yet. Devices that show up in /proc/self/mountinfo or /proc/swap, and then disappear again, without ever showing up in udev, need to be cleaned up. That's right, I forgot about that, thanks for catching! So current master is too overzealous, and my current patch never cleans up. So this needs some more work. Thanks, Martin -- Martin Pitt| http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org) signature.asc Description: Digital signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [correct PATCH v2] dev-root.device is not active, results in an umount spree
Martin Pitt [2015-05-17 15:54 +0200]:
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.
Eek, I attached the wrong 0002- patch, sorry about that. The above is
fixed by the attached patch, please ignore 0002- from the previous
mail. For completeness I also re-attach 0001- again (unchanged).
Martin
--
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
From bc080c3a0ddd24fabd94d11dc609967d557d044d 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/3] 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/102
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))
Re: [systemd-devel] [correct PATCH v2] dev-root.device is not active, results in an umount spree
On Mon, 18.05.15 16:08, Martin Pitt (martin.p...@ubuntu.com) wrote:
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.
Ah, I see now. This makes sense!
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;
+}
Why move this down? In order to keep the patch small and easy to grok,
can this stay up where it was, but simply be enclosed in the if (dev) {}
check?
-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();
Hmm, why are all these errors supposed to be OOM?
udev_device_new_from_devnum sets errno correctly, hence we should just
print what it really is set to with log_error_errno(), unless of
course it is ENOENT.
-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);
The if else { and if (errn... lines could be merged into one
else if (errno != ..., no?
From c18dbd432ecd16c57123b5fc04313d625e8a8e88 Mon Sep 17 00:00:00 2001
From: Martin Pitt martin.p...@ubuntu.com
Date: Sun, 17 May 2015 15:17:58 +0200
Subject: [PATCH 2/3] device: never transition from tentative to dead
Only set a device to state dead if it was previously plugged through udev.
We
must never transition from tentative to dead, as there is no way to be
sure
that this is actually true.
This fixes systemd unmounting everything from a tentative device as soon as
mountinfo changes.
Not following on this patch: what's the rationale here? your patch
basically means we would never ever clean up tentative devices, if
they once were referenced in mountinfo or /proc/swaps, but no longer
are.
Can you elaborate on what this patch is supposed to achieve? How could
it be problematic to deactivate device units that are neither
announced anywhere in /proc nor in udev?
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [correct PATCH v2] dev-root.device is not active, results in an umount spree
On Mon, 18.05.15 16:08, Martin Pitt (martin.p...@ubuntu.com) wrote: Martin Pitt [2015-05-17 15:54 +0200]: 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. Eek, I attached the wrong 0002- patch, sorry about that. The above is fixed by the attached patch, please ignore 0002- from the previous mail. For completeness I also re-attach 0001- again (unchanged). Still not getting what the purpose of the 0002 patch is, even in this revision... Please elaborate! --- a/src/core/device.c +++ b/src/core/device.c @@ -465,12 +465,13 @@ static void device_update_found_one(Device *d, bool add, DeviceFound found, bool * now referenced by the kernel, then we assume the * kernel knows it now, and udev might soon too. */ device_set_state(d, DEVICE_TENTATIVE); -else -/* If nobody sees the device, or if the device was - * previously seen by udev and now is only referenced - * from the kernel, then we consider the device is +else if (previous DEVICE_FOUND_UDEV) +/* If the device was previously seen by udev and now is only + * referenced from the kernel, then we consider the device is * gone, the kernel just hasn't noticed it yet. */ device_set_state(d, DEVICE_DEAD); +/* We never move from TENTATIVE to DEAD, as we can only determine this + * status update with udev, not with mountinfo */ } Devices that show up in /proc/self/mountinfo or /proc/swap, and then disappear again, without ever showing up in udev, need to be cleaned up. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
