From 5427b6afdb7514be83297411b42ee9d565c84ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Nov 2017 21:28:43 +0100 Subject: [PATCH 1/6] Update TODO --- TODO | 2 -- 1 file changed, 2 deletions(-) diff --git a/TODO b/TODO index 61fb73025d..c885639d4d 100644 --- a/TODO +++ b/TODO @@ -209,8 +209,6 @@ Features: then use that for the setting used in user@.service. It should be understood relative to the configured default value. -* on cgroupsv2 add DelegateControllers=, to pick the precise cgroup controllers to delegate - * in networkd, when matching device types, fix up DEVTYPE rubbish the kernel passes to us * enable LockMLOCK to take a percentage value relative to physical memory From 47a78d4102596c154cb3eeea6df16bf6ee1a4e61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Nov 2017 21:29:40 +0100 Subject: [PATCH 2/6] Use plural DelegateControllers= consistently --- src/core/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 6872da1f89..903c275359 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -214,7 +214,7 @@ void cgroup_context_dump(CGroupContext *c, FILE* f, const char *prefix) { (void) cg_mask_to_string(c->delegate_controllers, &t); - fprintf(f, "%sDelegateController=%s\n", + fprintf(f, "%sDelegateControllers=%s\n", prefix, strempty(t)); } From 9aa2113365d59c0fa4a866bb4a47b8553bf98288 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Nov 2017 10:04:29 +0100 Subject: [PATCH 3/6] util-lib: add debug messages when checking cgroup layout This has become very complex, let's make it a bit easier to diagnose. --- src/basic/cgroup-util.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 0100fc6dbf..23d4f4a309 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -2449,24 +2449,31 @@ static int cg_unified_update(void) { return 0; if (statfs("/sys/fs/cgroup/", &fs) < 0) - return -errno; + return log_debug_errno(errno, "statfs(\"/sys/fs/cgroup/\" failed: %m"); - if (F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC)) + if (F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC)) { + log_debug("Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy"); unified_cache = CGROUP_UNIFIED_ALL; - else if (F_TYPE_EQUAL(fs.f_type, TMPFS_MAGIC)) { + } else if (F_TYPE_EQUAL(fs.f_type, TMPFS_MAGIC)) { if (statfs("/sys/fs/cgroup/unified/", &fs) == 0 && F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC)) { + log_debug("Found cgroup2 on /sys/fs/cgroup/unified, unified hierarchy for systemd controller"); unified_cache = CGROUP_UNIFIED_SYSTEMD; unified_systemd_v232 = false; } else if (statfs("/sys/fs/cgroup/systemd/", &fs) == 0 && F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC)) { + log_debug("Found cgroup2 on /sys/fs/cgroup/systemd, unified hierarchy for systemd controller (v232 variant)"); unified_cache = CGROUP_UNIFIED_SYSTEMD; unified_systemd_v232 = true; } else { if (statfs("/sys/fs/cgroup/systemd/", &fs) < 0) - return -errno; - if (!F_TYPE_EQUAL(fs.f_type, CGROUP_SUPER_MAGIC)) + return log_debug_errno(errno, "statfs(\"/sys/fs/cgroup/systemd\" failed: %m"); + if (!F_TYPE_EQUAL(fs.f_type, CGROUP_SUPER_MAGIC)) { + log_debug("Unexpected filesystem type %llx mounted on /sys/fs/cgroup/systemd.", + (unsigned long long) fs.f_type); return -ENOMEDIUM; + } + log_debug("Found cgroup on /sys/fs/cgroup/systemd, legacy hierarchy"); unified_cache = CGROUP_UNIFIED_NONE; } } else { From 073e8f0956deae9bb07d32a6360473262b137928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Nov 2017 10:17:10 +0100 Subject: [PATCH 4/6] test-cgroup-util: add basic test for cg_all_unified/cg_hybrid_unified/cg_unified_controller --- src/test/test-cgroup-util.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/test/test-cgroup-util.c b/src/test/test-cgroup-util.c index d5bc73feff..2c220578a3 100644 --- a/src/test/test-cgroup-util.c +++ b/src/test/test-cgroup-util.c @@ -371,6 +371,32 @@ static void test_is_wanted(void) { test_is_wanted_print(false); } +static void test_cg_tests(void) { + int all, hybrid, systemd; + + assert_se(cg_unified_flush() == 0); + + all = cg_all_unified(); + assert_se(IN_SET(all, 0, 1)); + + hybrid = cg_hybrid_unified(); + assert_se(IN_SET(hybrid, 0, 1)); + + systemd = cg_unified_controller(SYSTEMD_CGROUP_CONTROLLER); + assert_se(IN_SET(systemd, 0, 1)); + + if (all) { + assert_se(systemd); + assert_se(!hybrid); + + } else if (hybrid) { + assert_se(systemd); + assert_se(!all); + + } else + assert_se(!systemd); +} + int main(void) { log_set_max_level(LOG_DEBUG); log_parse_environment(); @@ -395,6 +421,7 @@ int main(void) { test_is_wanted_print(true); test_is_wanted_print(false); /* run twice to test caching */ test_is_wanted(); + test_cg_tests(); return 0; } From d4c819ed2311a8334045f0d88924fc2e2f7dcc00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Nov 2017 10:19:45 +0100 Subject: [PATCH 5/6] core: fix message about detected memory hierarchy Just the error check and message were wrong, otherwise the logic was OK. --- src/core/cgroup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 903c275359..3c0d22a691 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1978,9 +1978,9 @@ int manager_setup_cgroup(Manager *m) { return log_error_errno(r, "Couldn't determine if we are running in the unified hierarchy: %m"); all_unified = cg_all_unified(); - if (r < 0) - return log_error_errno(r, "Couldn't determine whether we are in all unified mode: %m"); - if (r > 0) + if (all_unified < 0) + return log_error_errno(all_unified, "Couldn't determine whether we are in all unified mode: %m"); + if (all_unified > 0) log_debug("Unified cgroup hierarchy is located at %s.", path); else { r = cg_unified_controller(SYSTEMD_CGROUP_CONTROLLER); From 17f01ace62fb24cee8a06c0bee64dd0db9aac4e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Nov 2017 10:38:04 +0100 Subject: [PATCH 6/6] core/manager: just return an error if we fail halfway We would continue, but still return an error at the end. This isn't useful because we'd still error-out in main(). Also, add a missing error message when we fail to mkdir. --- src/core/manager.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index c89dadc108..9978be7a48 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1309,7 +1309,7 @@ static void manager_distribute_fds(Manager *m, FDSet *fds) { } int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { - int r, q; + int r; assert(m); @@ -1329,7 +1329,8 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { * in the search path */ r = mkdir_p_label(m->lookup_paths.transient, 0755); if (r < 0) - return r; + return log_error_errno(r, "Failed to create transient generator directory \"%s\": %m", + m->lookup_paths.transient); dual_timestamp_get(&m->generators_start_timestamp); r = manager_run_generators(m); @@ -1341,9 +1342,10 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { m->unit_file_scope == UNIT_FILE_SYSTEM && !m->test_run_flags) { - q = unit_file_preset_all(UNIT_FILE_SYSTEM, 0, NULL, UNIT_FILE_PRESET_ENABLE_ONLY, NULL, 0); - if (q < 0) - log_full_errno(q == -EEXIST ? LOG_NOTICE : LOG_WARNING, q, "Failed to populate /etc with preset unit settings, ignoring: %m"); + r = unit_file_preset_all(UNIT_FILE_SYSTEM, 0, NULL, UNIT_FILE_PRESET_ENABLE_ONLY, NULL, 0); + if (r < 0) + log_full_errno(r == -EEXIST ? LOG_NOTICE : LOG_WARNING, r, + "Failed to populate /etc with preset unit settings, ignoring: %m"); else log_info("Populated /etc with preset unit settings."); } @@ -1366,7 +1368,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { if (serialization) { r = manager_deserialize(m, serialization, fds); if (r < 0) - log_error_errno(r, "Deserialization failed: %m"); + return log_error_errno(r, "Deserialization failed: %m"); } /* Any fds left? Find some unit which wants them. This is @@ -1377,17 +1379,20 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { /* We might have deserialized the notify fd, but if we didn't * then let's create the bus now */ - q = manager_setup_notify(m); - if (q < 0 && r == 0) - r = q; + r = manager_setup_notify(m); + if (r < 0) + /* No sense to continue without notifications, our children would fail anyway. */ + return r; - q = manager_setup_cgroups_agent(m); - if (q < 0 && r == 0) - r = q; + r = manager_setup_cgroups_agent(m); + if (r < 0) + /* Likewise, no sense to continue without empty cgroup notifications. */ + return r; - q = manager_setup_user_lookup_fd(m); - if (q < 0 && r == 0) - r = q; + r = manager_setup_user_lookup_fd(m); + if (r < 0) + /* This shouldn't fail, except if things are really broken. */ + return r; /* Let's connect to the bus now. */ (void) manager_connect_bus(m, !!serialization); @@ -1415,7 +1420,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { m->send_reloading_done = true; } - return r; + return 0; } int manager_add_job(Manager *m, JobType type, Unit *unit, JobMode mode, sd_bus_error *e, Job **_ret) {