From 032b3afbf4abe3cf760469d9d1cd2e4162829441 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:24:55 +0100 Subject: [PATCH 1/7] shared: split out ioprio related stuff into ioprio-util.[ch] No actual code changes, just some splitting out. --- src/basic/ioprio-util.c | 31 +++++++++++++++++++++++++++++++ src/basic/ioprio-util.h | 18 ++++++++++++++++++ src/basic/meson.build | 2 ++ src/basic/process-util.c | 25 ------------------------- src/basic/process-util.h | 14 -------------- src/core/dbus-execute.c | 1 + src/core/execute.c | 1 + src/core/load-fragment.c | 1 + src/shared/bus-unit-util.c | 1 + src/test/test-process-util.c | 1 + 10 files changed, 56 insertions(+), 39 deletions(-) create mode 100644 src/basic/ioprio-util.c create mode 100644 src/basic/ioprio-util.h diff --git a/src/basic/ioprio-util.c b/src/basic/ioprio-util.c new file mode 100644 index 0000000000..b63650b80b --- /dev/null +++ b/src/basic/ioprio-util.c @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "ioprio-util.h" +#include "parse-util.h" +#include "string-table.h" + +int ioprio_parse_priority(const char *s, int *ret) { + int i, r; + + assert(s); + assert(ret); + + r = safe_atoi(s, &i); + if (r < 0) + return r; + + if (!ioprio_priority_is_valid(i)) + return -EINVAL; + + *ret = i; + return 0; +} + +static const char *const ioprio_class_table[] = { + [IOPRIO_CLASS_NONE] = "none", + [IOPRIO_CLASS_RT] = "realtime", + [IOPRIO_CLASS_BE] = "best-effort", + [IOPRIO_CLASS_IDLE] = "idle", +}; + +DEFINE_STRING_TABLE_LOOKUP_WITH_FALLBACK(ioprio_class, int, IOPRIO_N_CLASSES); diff --git a/src/basic/ioprio-util.h b/src/basic/ioprio-util.h new file mode 100644 index 0000000000..ffb3e9c890 --- /dev/null +++ b/src/basic/ioprio-util.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include "macro.h" +#include "missing_ioprio.h" + +int ioprio_class_to_string_alloc(int i, char **s); +int ioprio_class_from_string(const char *s); + +static inline bool ioprio_class_is_valid(int i) { + return IN_SET(i, IOPRIO_CLASS_NONE, IOPRIO_CLASS_RT, IOPRIO_CLASS_BE, IOPRIO_CLASS_IDLE); +} + +static inline bool ioprio_priority_is_valid(int i) { + return i >= 0 && i < IOPRIO_BE_NR; +} + +int ioprio_parse_priority(const char *s, int *ret); diff --git a/src/basic/meson.build b/src/basic/meson.build index f0d5a1d2ea..bc90100c34 100644 --- a/src/basic/meson.build +++ b/src/basic/meson.build @@ -82,6 +82,8 @@ basic_sources = files(''' inotify-util.h io-util.c io-util.h + ioprio-util.c + ioprio-util.h limits-util.c limits-util.h linux/btrfs.h diff --git a/src/basic/process-util.c b/src/basic/process-util.c index bea53ccb7d..1dd2414bb2 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1132,23 +1132,6 @@ int pid_compare_func(const pid_t *a, const pid_t *b) { return CMP(*a, *b); } -int ioprio_parse_priority(const char *s, int *ret) { - int i, r; - - assert(s); - assert(ret); - - r = safe_atoi(s, &i); - if (r < 0) - return r; - - if (!ioprio_priority_is_valid(i)) - return -EINVAL; - - *ret = i; - return 0; -} - /* The cached PID, possible values: * * == UNSET [0] → cache not initialized yet @@ -1627,14 +1610,6 @@ _noreturn_ void freeze(void) { pause(); } -static const char *const ioprio_class_table[] = { - [IOPRIO_CLASS_NONE] = "none", - [IOPRIO_CLASS_RT] = "realtime", - [IOPRIO_CLASS_BE] = "best-effort", - [IOPRIO_CLASS_IDLE] = "idle", -}; - -DEFINE_STRING_TABLE_LOOKUP_WITH_FALLBACK(ioprio_class, int, IOPRIO_N_CLASSES); static const char *const sigchld_code_table[] = { [CLD_EXITED] = "exited", diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 25dea35ce6..c312e05d9c 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -14,7 +14,6 @@ #include "alloc-util.h" #include "format-util.h" #include "macro.h" -#include "missing_ioprio.h" #include "time-util.h" #define procfs_file_alloca(pid, field) \ @@ -97,9 +96,6 @@ const char *personality_to_string(unsigned long); int safe_personality(unsigned long p); int opinionated_personality(unsigned long *ret); -int ioprio_class_to_string_alloc(int i, char **s); -int ioprio_class_from_string(const char *s); - const char *sigchld_code_to_string(int i) _const_; int sigchld_code_from_string(const char *s) _pure_; @@ -130,20 +126,10 @@ static inline bool sched_priority_is_valid(int i) { return i >= 0 && i <= sched_get_priority_max(SCHED_RR); } -static inline bool ioprio_class_is_valid(int i) { - return IN_SET(i, IOPRIO_CLASS_NONE, IOPRIO_CLASS_RT, IOPRIO_CLASS_BE, IOPRIO_CLASS_IDLE); -} - -static inline bool ioprio_priority_is_valid(int i) { - return i >= 0 && i < IOPRIO_BE_NR; -} - static inline bool pid_is_valid(pid_t p) { return p > 0; } -int ioprio_parse_priority(const char *s, int *ret); - pid_t getpid_cached(void); void reset_cached_pid(void); diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index afb2d8f10f..c8323d84ba 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -24,6 +24,7 @@ #include "fileio.h" #include "hexdecoct.h" #include "io-util.h" +#include "ioprio-util.h" #include "journal-file.h" #include "missing_ioprio.h" #include "mountpoint-util.h" diff --git a/src/core/execute.c b/src/core/execute.c index ba66b9e6ec..e578bace6f 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -63,6 +63,7 @@ #include "glob-util.h" #include "hexdecoct.h" #include "io-util.h" +#include "ioprio-util.h" #include "label.h" #include "log.h" #include "macro.h" diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 6efcba3265..0e93ddfbc0 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -40,6 +40,7 @@ #include "fs-util.h" #include "hexdecoct.h" #include "io-util.h" +#include "ioprio-util.h" #include "ip-protocol-list.h" #include "journal-file.h" #include "limits-util.h" diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index b8fd85a191..c58394e63e 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -19,6 +19,7 @@ #include "hexdecoct.h" #include "hostname-util.h" #include "in-addr-util.h" +#include "ioprio-util.h" #include "ip-protocol-list.h" #include "libmount-util.h" #include "locale-util.h" diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index b87a24b342..dc45712937 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -19,6 +19,7 @@ #include "errno-list.h" #include "errno-util.h" #include "fd-util.h" +#include "ioprio-util.h" #include "log.h" #include "macro.h" #include "missing_sched.h" From 0692548c73fd2736f855cec7b44b13a818b7a560 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:30:22 +0100 Subject: [PATCH 2/7] ioprio-util: add macro for default ioprio settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IOPRIO_CLASS_NONE with any priority value actually is an alias for IOPRIO_CLASS_BE with priority value 4 – which is the default ioprio for all processes. We got this right at one place, but wrong at three others (where we assumed the default value was 0, not 4). Let's add a macro that encodes this properly, and use it everywhere. --- src/basic/ioprio-util.h | 4 ++++ src/core/execute.c | 4 ++-- src/core/load-fragment.c | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/basic/ioprio-util.h b/src/basic/ioprio-util.h index ffb3e9c890..0d3f70a220 100644 --- a/src/basic/ioprio-util.h +++ b/src/basic/ioprio-util.h @@ -16,3 +16,7 @@ static inline bool ioprio_priority_is_valid(int i) { } int ioprio_parse_priority(const char *s, int *ret); + +/* IOPRIO_CLASS_NONE with any prio value is another way to say IOPRIO_CLASS_BE with level 4. Encode that in a + * proper macro. */ +#define IOPRIO_DEFAULT_CLASS_AND_PRIO ioprio_prio_value(IOPRIO_CLASS_BE, 4) diff --git a/src/core/execute.c b/src/core/execute.c index e578bace6f..c30e15f614 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -5069,7 +5069,7 @@ void exec_context_init(ExecContext *c) { assert(c); c->umask = 0022; - c->ioprio = ioprio_prio_value(IOPRIO_CLASS_BE, 0); + c->ioprio = IOPRIO_DEFAULT_CLASS_AND_PRIO; c->cpu_sched_policy = SCHED_OTHER; c->syslog_priority = LOG_DAEMON|LOG_INFO; c->syslog_level_prefix = true; @@ -6021,7 +6021,7 @@ int exec_context_get_effective_ioprio(const ExecContext *c) { p = ioprio_get(IOPRIO_WHO_PROCESS, 0); if (p < 0) - return ioprio_prio_value(IOPRIO_CLASS_BE, 4); + return IOPRIO_DEFAULT_CLASS_AND_PRIO; return p; } diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 0e93ddfbc0..71434a4bee 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -1417,7 +1417,7 @@ int config_parse_exec_io_class(const char *unit, if (isempty(rvalue)) { c->ioprio_set = false; - c->ioprio = ioprio_prio_value(IOPRIO_CLASS_BE, 0); + c->ioprio = IOPRIO_DEFAULT_CLASS_AND_PRIO; return 0; } @@ -1454,7 +1454,7 @@ int config_parse_exec_io_priority(const char *unit, if (isempty(rvalue)) { c->ioprio_set = false; - c->ioprio = ioprio_prio_value(IOPRIO_CLASS_BE, 0); + c->ioprio = IOPRIO_DEFAULT_CLASS_AND_PRIO; return 0; } From ba7772fee445dd88438fb84477648a67821843d0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:38:10 +0100 Subject: [PATCH 3/7] ioprio: normalize io priority values in configuration Let's always say IOPRIO_CLASS_BE when IOPRIO_CALSS_NONE is set. --- src/basic/ioprio-util.h | 5 +++++ src/core/dbus-execute.c | 4 ++-- src/core/load-fragment.c | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/basic/ioprio-util.h b/src/basic/ioprio-util.h index 0d3f70a220..b8c9b7dd59 100644 --- a/src/basic/ioprio-util.h +++ b/src/basic/ioprio-util.h @@ -20,3 +20,8 @@ int ioprio_parse_priority(const char *s, int *ret); /* IOPRIO_CLASS_NONE with any prio value is another way to say IOPRIO_CLASS_BE with level 4. Encode that in a * proper macro. */ #define IOPRIO_DEFAULT_CLASS_AND_PRIO ioprio_prio_value(IOPRIO_CLASS_BE, 4) + +static inline int ioprio_normalize(int v) { + /* Converts IOPRIO_CLASS_NONE to what it actually means */ + return ioprio_prio_class(v) == IOPRIO_CLASS_NONE ? IOPRIO_DEFAULT_CLASS_AND_PRIO : v; +} diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index c8323d84ba..5c499e5d06 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2811,7 +2811,7 @@ int bus_exec_context_set_transient_property( if (r < 0) return r; - c->ioprio = ioprio_prio_value(q, ioprio_prio_data(c->ioprio)); + c->ioprio = ioprio_normalize(ioprio_prio_value(q, ioprio_prio_data(c->ioprio))); c->ioprio_set = true; unit_write_settingf(u, flags, name, "IOSchedulingClass=%s", s); @@ -2830,7 +2830,7 @@ int bus_exec_context_set_transient_property( return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid IO scheduling priority: %i", p); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - c->ioprio = ioprio_prio_value(ioprio_prio_class(c->ioprio), p); + c->ioprio = ioprio_normalize(ioprio_prio_value(ioprio_prio_class(c->ioprio), p)); c->ioprio_set = true; unit_write_settingf(u, flags, name, "IOSchedulingPriority=%i", p); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 71434a4bee..92a52819e2 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -1427,7 +1427,7 @@ int config_parse_exec_io_class(const char *unit, return 0; } - c->ioprio = ioprio_prio_value(x, ioprio_prio_data(c->ioprio)); + c->ioprio = ioprio_normalize(ioprio_prio_value(x, ioprio_prio_data(c->ioprio))); c->ioprio_set = true; return 0; @@ -1464,7 +1464,7 @@ int config_parse_exec_io_priority(const char *unit, return 0; } - c->ioprio = ioprio_prio_value(ioprio_prio_class(c->ioprio), i); + c->ioprio = ioprio_normalize(ioprio_prio_value(ioprio_prio_class(c->ioprio), i)); c->ioprio_set = true; return 0; From 8b330d7d75b5dd861bb4f9abde98d517cdc77229 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:39:43 +0100 Subject: [PATCH 4/7] core: normalize ioprio values we acquire from kernel Linux 5.15 broke API in ioprio_get(): instead of returning IOPRIO_CLASS_NONE when that's set it now returns IOPRIO_CLASS_BE, which is what this actually is (the former is just an alias for the latter with a priority value of 4). Let's hide the differences between old and new kernels here, and always normalize to what the new kernels do. --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index c30e15f614..aa1193e067 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -6023,7 +6023,7 @@ int exec_context_get_effective_ioprio(const ExecContext *c) { if (p < 0) return IOPRIO_DEFAULT_CLASS_AND_PRIO; - return p; + return ioprio_normalize(p); } bool exec_context_get_effective_mount_apivfs(const ExecContext *c) { From 543497fa34ccd754d9278124ca0c8e45fe1d3005 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:52:13 +0100 Subject: [PATCH 5/7] test: add test for ioprio normalization --- src/test/test-process-util.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index dc45712937..cc0f870e57 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -726,32 +726,38 @@ static void test_pid_to_ptr(void) { assert_se(PTR_TO_PID(PID_TO_PTR(INT32_MIN)) == INT32_MIN); } -static void test_ioprio_class_from_to_string_one(const char *val, int expected) { +static void test_ioprio_class_from_to_string_one(const char *val, int expected, int normalized) { assert_se(ioprio_class_from_string(val) == expected); if (expected >= 0) { _cleanup_free_ char *s = NULL; unsigned ret; + int combined; assert_se(ioprio_class_to_string_alloc(expected, &s) == 0); - /* We sometimes get a class number and sometimes a number back */ + /* We sometimes get a class number and sometimes a name back */ assert_se(streq(s, val) || safe_atou(val, &ret) == 0); + + /* Make sure normalization works, i.e. NONE → BE gets normalized */ + combined = ioprio_normalize(ioprio_prio_value(expected, 0)); + assert_se(ioprio_prio_class(combined) == normalized); + assert_se(expected != IOPRIO_CLASS_NONE || ioprio_prio_data(combined) == 4); } } static void test_ioprio_class_from_to_string(void) { log_info("/* %s */", __func__); - test_ioprio_class_from_to_string_one("none", IOPRIO_CLASS_NONE); - test_ioprio_class_from_to_string_one("realtime", IOPRIO_CLASS_RT); - test_ioprio_class_from_to_string_one("best-effort", IOPRIO_CLASS_BE); - test_ioprio_class_from_to_string_one("idle", IOPRIO_CLASS_IDLE); - test_ioprio_class_from_to_string_one("0", 0); - test_ioprio_class_from_to_string_one("1", 1); - test_ioprio_class_from_to_string_one("7", 7); - test_ioprio_class_from_to_string_one("8", 8); - test_ioprio_class_from_to_string_one("9", -EINVAL); - test_ioprio_class_from_to_string_one("-1", -EINVAL); + test_ioprio_class_from_to_string_one("none", IOPRIO_CLASS_NONE, IOPRIO_CLASS_BE); + test_ioprio_class_from_to_string_one("realtime", IOPRIO_CLASS_RT, IOPRIO_CLASS_RT); + test_ioprio_class_from_to_string_one("best-effort", IOPRIO_CLASS_BE, IOPRIO_CLASS_BE); + test_ioprio_class_from_to_string_one("idle", IOPRIO_CLASS_IDLE, IOPRIO_CLASS_IDLE); + test_ioprio_class_from_to_string_one("0", IOPRIO_CLASS_NONE, IOPRIO_CLASS_BE); + test_ioprio_class_from_to_string_one("1", 1, 1); + test_ioprio_class_from_to_string_one("7", 7, 7); + test_ioprio_class_from_to_string_one("8", 8, 8); + test_ioprio_class_from_to_string_one("9", -EINVAL, -EINVAL); + test_ioprio_class_from_to_string_one("-1", -EINVAL, -EINVAL); } static void test_setpriority_closest(void) { From 8880b2bab8b43b716553d5b2ab92d63b0e66b474 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:54:29 +0100 Subject: [PATCH 6/7] man: don't mention IOSchedulingClass=none anymore in the docs Let's not mention a redundant setting of "none". Let's instead only mention "best-effort", which is the same. Also mention the default settings properly. (Also, while we are at it, don#t document the numeric alias, that's totally redundant and harder to use, so no need to push people towards it.) --- man/systemd.exec.xml | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index aea7116e29..4f37c22b75 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -1140,10 +1140,11 @@ CapabilityBoundingSet=~CAP_B CAP_C IOSchedulingClass= - Sets the I/O scheduling class for executed processes. Takes an integer between 0 and 3 or one - of the strings , , or - . If the empty string is assigned to this option, all prior assignments to both - IOSchedulingClass= and IOSchedulingPriority= have no effect. See + Sets the I/O scheduling class for executed processes. Takes one of the strings + , or . The kernel's + default scheduling class is at a priority of 4. If the empty string is + assigned to this option, all prior assignments to both IOSchedulingClass= and + IOSchedulingPriority= have no effect. See ioprio_set2 for details. @@ -1156,7 +1157,8 @@ CapabilityBoundingSet=~CAP_B CAP_C bandwidth is made available to the unit's processes, larger values mean less bandwidth. The available priorities depend on the selected I/O scheduling class (see above). If the empty string is assigned to this option, all prior assignments to both IOSchedulingClass= and - IOSchedulingPriority= have no effect. See + IOSchedulingPriority= have no effect. For the kernel's default scheduling class + () this defaults to 4. See ioprio_set2 for details. From d9b8771108cf2955efc3852b477391017d2c599a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 15:58:50 +0100 Subject: [PATCH 7/7] test: make test-execute pass on Linux 5.15 Linux 5.15 broke kernel API: https://github.com/torvalds/linux/commit/e70344c05995a190a56bbd1a23dc2218bcc8c924 Previously setting IOPRIO_CLASS_NONE for a process would then report IOPRIO_CLASS_NONE back. But since 5.15 it reports IOPRIO_CLASS_BE instead. Since IOPRIO_CLASS_NONE is an alias for a special setting of IOPRIO_CLASS_BE this makes some sense, but it's also a kernel API breakage that our testsuite trips up on. (I made some minimal effort to inform the kernel people about this API breakage during the 5.15 rc phase, but noone was interested.) Either way let's hadle this gracefully in our test suite and accept "best-effort" too when "none" was set. (This is only triggable if the tests are run on 5.15 with full privs) --- test/test-execute/exec-ioschedulingclass-none.service | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test-execute/exec-ioschedulingclass-none.service b/test/test-execute/exec-ioschedulingclass-none.service index efe3182261..0494d45897 100644 --- a/test/test-execute/exec-ioschedulingclass-none.service +++ b/test/test-execute/exec-ioschedulingclass-none.service @@ -3,6 +3,7 @@ Description=Test for IOSchedulingClass=none [Service] -ExecStart=/bin/sh -x -c 'c=$$(LC_ALL=C ionice); test "$${c%%:*}" = "none"' +# Old kernels might report "none" here, new kernels "best-effort". +ExecStart=/bin/sh -x -c 'c=$$(LC_ALL=C ionice); test "$${c%%:*}" = "none" -o "$${c%%:*}" = "best-effort"' Type=oneshot IOSchedulingClass=none