From 57f1030b1373455c531bf01e005fd62edcf5005f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2020 20:02:01 +0100 Subject: [PATCH 1/5] cgroup: use log_warning_errno() where possible --- src/core/cgroup.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index af2efd46fa..4e9e59ac82 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -651,30 +651,31 @@ static int lookup_block_device(const char *p, dev_t *ret) { r = device_path_parse_major_minor(p, &mode, &rdev); if (r == -ENODEV) { /* not a parsable device node, need to go to disk */ struct stat st; + if (stat(p, &st) < 0) return log_warning_errno(errno, "Couldn't stat device '%s': %m", p); + rdev = (dev_t)st.st_rdev; dev = (dev_t)st.st_dev; mode = st.st_mode; } else if (r < 0) return log_warning_errno(r, "Failed to parse major/minor from path '%s': %m", p); - if (S_ISCHR(mode)) { - log_warning("Device node '%s' is a character device, but block device needed.", p); - return -ENOTBLK; - } else if (S_ISBLK(mode)) + if (S_ISCHR(mode)) + return log_warning_errno(SYNTHETIC_ERRNO(ENOTBLK), + "Device node '%s' is a character device, but block device needed.", p); + if (S_ISBLK(mode)) *ret = rdev; else if (major(dev) != 0) *ret = dev; /* If this is not a device node then use the block device this file is stored on */ else { /* If this is btrfs, getting the backing block device is a bit harder */ r = btrfs_get_block_device(p, ret); - if (r < 0 && r != -ENOTTY) + if (r == -ENOTTY) + return log_warning_errno(SYNTHETIC_ERRNO(ENODEV), + "'%s' is not a block device node, and file system block device cannot be determined or is not local.", p); + if (r < 0) return log_warning_errno(r, "Failed to determine block device backing btrfs file system '%s': %m", p); - if (r == -ENOTTY) { - log_warning("'%s' is not a block device node, and file system block device cannot be determined or is not local.", p); - return -ENODEV; - } } /* If this is a LUKS device, try to get the originating block device */ From a0d6590c4e8fe0cd0f97d03db20a77236db523a3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2020 20:04:38 +0100 Subject: [PATCH 2/5] cgroup: no need to cast dev_t to dev_t --- src/core/cgroup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 4e9e59ac82..8b111b9197 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -655,9 +655,9 @@ static int lookup_block_device(const char *p, dev_t *ret) { if (stat(p, &st) < 0) return log_warning_errno(errno, "Couldn't stat device '%s': %m", p); - rdev = (dev_t)st.st_rdev; - dev = (dev_t)st.st_dev; mode = st.st_mode; + rdev = st.st_rdev; + dev = st.st_dev; } else if (r < 0) return log_warning_errno(r, "Failed to parse major/minor from path '%s': %m", p); From 95ae4d142072da5c4091c6e03fbdeabf82fd81f7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2020 20:04:58 +0100 Subject: [PATCH 3/5] cgroup: drop unnecessary {} --- src/core/cgroup.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 8b111b9197..148ea1a8b1 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1527,10 +1527,9 @@ CGroupMask unit_get_members_mask(Unit *u) { Unit *member; Iterator i; - HASHMAP_FOREACH_KEY(v, member, u->dependencies[UNIT_BEFORE], i) { + HASHMAP_FOREACH_KEY(v, member, u->dependencies[UNIT_BEFORE], i) if (UNIT_DEREF(member->slice) == u) u->cgroup_members_mask |= unit_get_subtree_mask(member); /* note that this calls ourselves again, for the children */ - } } u->cgroup_members_mask_valid = true; @@ -3513,10 +3512,9 @@ void unit_invalidate_cgroup_bpf(Unit *u) { Iterator i; void *v; - HASHMAP_FOREACH_KEY(v, member, u->dependencies[UNIT_BEFORE], i) { + HASHMAP_FOREACH_KEY(v, member, u->dependencies[UNIT_BEFORE], i) if (UNIT_DEREF(member->slice) == u) unit_invalidate_cgroup_bpf(member); - } } } From e1e98911a818ad3b46c6a1c26d759df590bef476 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2020 20:06:39 +0100 Subject: [PATCH 4/5] cgroup: update only siblings that got realized once Fixes: #14475 Replaces: #14554 --- src/core/cgroup.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 148ea1a8b1..21a63a2d0d 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2335,7 +2335,15 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) { Unit *slice; /* This adds the siblings of the specified unit and the siblings of all parent units to the cgroup - * queue. (But neither the specified unit itself nor the parents.) */ + * queue. (But neither the specified unit itself nor the parents.) + * + * Propagation of realization "side-ways" (i.e. towards siblings) is in relevant on cgroup-v1 where + * scheduling become very weird if two units that own processes reside in the same slice, but one is + * realized in the "cpu" hierarchy and once is not (for example because one has CPUWeight= set and + * the other does not), because that means processes need to be scheduled against groups. Let's avoid + * this asymmetry by always ensuring that units below a slice that are realized at all are hence + * always realized in *all* their hierarchies, and it is sufficient for a unit's sibling to be + * realized for a unit to be realized too. */ while ((slice = UNIT_DEREF(u->slice))) { Iterator i; @@ -2343,6 +2351,7 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) { void *v; HASHMAP_FOREACH_KEY(v, m, slice->dependencies[UNIT_BEFORE], i) { + /* Skip units that have a dependency on the slice but aren't actually in it. */ if (UNIT_DEREF(m->slice) != slice) continue; @@ -2351,6 +2360,11 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) { if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(m))) continue; + /* We only enqueue siblings if they were realized once at least, in the main + * hierarchy. */ + if (!m->cgroup_realized) + continue; + /* If the unit doesn't need any new controllers and has current ones realized, it * doesn't need any changes. */ if (unit_has_mask_realized(m, @@ -2648,6 +2662,7 @@ void unit_add_to_cgroup_empty_queue(Unit *u) { /* Let's verify that the cgroup is really empty */ if (!u->cgroup_path) return; + r = cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path); if (r < 0) { log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty: %m", u->cgroup_path); From 48fd01e5f3bf752bd5b160c75e5ccb97409ed0de Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2020 20:11:56 +0100 Subject: [PATCH 5/5] cgroup: drop redundant if check --- src/core/cgroup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 21a63a2d0d..fdb134b99f 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3621,8 +3621,8 @@ int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name) { return r; if (r == 0) return -ENODATA; - if (r > 0) - r = cg_get_attribute("cpuset", u->cgroup_path, name, &v); + + r = cg_get_attribute("cpuset", u->cgroup_path, name, &v); if (r == -ENOENT) return -ENODATA; if (r < 0)