From 5cfe9715f5faf81244f7f7eeae35d68ad2d85816 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 11:18:42 +0900 Subject: [PATCH 1/9] udev/iocost: set default target in parse_config() And make the failure in parsing config critical. --- src/udev/iocost/iocost.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 54b50b4a8d..2ded9ab9f7 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -25,7 +25,9 @@ static int parse_config(void) { static const ConfigTableItem items[] = { { "IOCost", "TargetSolution", config_parse_string, 0, &arg_target_solution }, }; - return config_parse( + int r; + + r = config_parse( NULL, "/etc/udev/iocost.conf", NULL, @@ -35,6 +37,17 @@ static int parse_config(void) { CONFIG_PARSE_WARN, NULL, NULL); + if (r < 0) + return r; + + if (!arg_target_solution) { + arg_target_solution = strdup("naive"); + if (!arg_target_solution) + return log_oom(); + } + + log_debug("Target solution: %s", arg_target_solution); + return 0; } static int help(void) { @@ -318,15 +331,9 @@ static int run(int argc, char *argv[]) { if (r <= 0) return r; - (void) parse_config(); - - if (!arg_target_solution) { - arg_target_solution = strdup("naive"); - if (!arg_target_solution) - return log_oom(); - } - - log_debug("Target solution: %s.", arg_target_solution); + r = parse_config(); + if (r < 0) + return r; return iocost_main(argc, argv); } From 878f3a4f0949bba4a2e1d0084e8d45cae125fe66 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 11:19:53 +0900 Subject: [PATCH 2/9] udev/iocost: arg_target_solution is always non-NULL --- src/udev/iocost/iocost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 2ded9ab9f7..15afdacd31 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -128,7 +128,7 @@ static int choose_solution(char **solutions, const char **ret_name) { return log_error_errno( SYNTHETIC_ERRNO(EINVAL), "IOCOST_SOLUTIONS exists in hwdb but is empty."); - if (arg_target_solution && strv_find(solutions, arg_target_solution)) { + if (strv_contains(solutions, arg_target_solution)) { *ret_name = arg_target_solution; log_debug("Selected solution based on target solution: %s", *ret_name); } else { From dcb379619ff4a3ecdeae5bc7ff466ed22efe3768 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 11:36:28 +0900 Subject: [PATCH 3/9] udev/iocost: drop unnecessary initializations --- src/udev/iocost/iocost.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 15afdacd31..83b43ec109 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -147,7 +147,7 @@ static int query_named_solution( _cleanup_strv_free_ char **solutions = NULL; _cleanup_free_ char *upper_name = NULL, *qos_key = NULL, *model_key = NULL; - const char *qos = NULL, *model = NULL; + const char *qos, *model; int r; assert(ret_qos); @@ -204,7 +204,7 @@ static int query_named_solution( static int apply_solution_for_path(const char *path, const char *name) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; _cleanup_free_ char *qos = NULL, *model = NULL; - const char *qos_params = NULL, *model_params = NULL; + const char *qos_params, *model_params; dev_t devnum; int r; @@ -250,8 +250,7 @@ static int apply_solution_for_path(const char *path, const char *name) { static int query_solutions_for_path(const char *path) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; _cleanup_strv_free_ char **solutions = NULL; - const char *default_solution = NULL; - const char *model_name = NULL; + const char *default_solution, *model_name; int r; r = sd_device_new_from_path(&device, path); @@ -289,7 +288,7 @@ static int query_solutions_for_path(const char *path) { arg_target_solution, default_solution); STRV_FOREACH(s, solutions) { - const char *model = NULL, *qos = NULL; + const char *model, *qos; r = query_named_solution(device, *s, &model, &qos); if (r < 0 || !model || !qos) From 9c271f4509b9d2837469a924c22737bbbb09ffaa Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 11:34:26 +0900 Subject: [PATCH 4/9] udev/iocost: merge get_known_solutions() and choose_solution() As these are always called sequentially. No functional change, just refactoring. --- src/udev/iocost/iocost.c | 60 +++++++++++++++------------------------- 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 83b43ec109..801751a398 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -101,41 +101,37 @@ static int parse_argv(int argc, char *argv[]) { return 1; } -static int get_known_solutions(sd_device *device, char ***ret_solutions) { +static int get_known_solutions(sd_device *device, int log_level, char ***ret_solutions, const char **ret_selected) { _cleanup_free_ char **s = NULL; - const char *value; + const char *value, *found; int r; assert(ret_solutions); + assert(ret_selected); r = sd_device_get_property_value(device, "IOCOST_SOLUTIONS", &value); + if (r == -ENOENT) + return log_device_full_errno(device, log_level, r, "No iocost solution found for device."); if (r < 0) - return r; + return log_device_error_errno(device, r, "Failed to query solutions from device: %m"); s = strv_split(value, WHITESPACE); if (!s) - return -ENOMEM; + return log_oom(); + if (strv_isempty(s)) + return log_device_error_errno(device, SYNTHETIC_ERRNO(EINVAL), + "IOCOST_SOLUTIONS exists in hwdb but is empty."); - *ret_solutions = TAKE_PTR(s); - - return 0; -} - -static int choose_solution(char **solutions, const char **ret_name) { - assert(ret_name); - - if (strv_isempty(solutions)) - return log_error_errno( - SYNTHETIC_ERRNO(EINVAL), "IOCOST_SOLUTIONS exists in hwdb but is empty."); - - if (strv_contains(solutions, arg_target_solution)) { - *ret_name = arg_target_solution; - log_debug("Selected solution based on target solution: %s", *ret_name); + found = strv_find(s, arg_target_solution); + if (found) { + *ret_selected = found; + log_device_debug(device, "Selected solution based on target solution: %s", *ret_selected); } else { - *ret_name = solutions[0]; - log_debug("Selected first available solution: %s", *ret_name); + *ret_selected = s[0]; + log_device_debug(device, "Selected first available solution: %s", *ret_selected); } + *ret_solutions = TAKE_PTR(s); return 0; } @@ -157,13 +153,7 @@ static int query_named_solution( * in the IOCOST_SOLUTIONS key or the one specified by the TargetSolution setting. */ if (!name) { - r = get_known_solutions(device, &solutions); - if (r == -ENOENT) - return log_device_debug_errno(device, r, "No entry found for device, skipping iocost logic."); - if (r < 0) - return log_device_error_errno(device, r, "Failed to query solutions from device: %m"); - - r = choose_solution(solutions, &name); + r = get_known_solutions(device, LOG_DEBUG, &solutions, &name); if (r < 0) return r; } @@ -250,7 +240,7 @@ static int apply_solution_for_path(const char *path, const char *name) { static int query_solutions_for_path(const char *path) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; _cleanup_strv_free_ char **solutions = NULL; - const char *default_solution, *model_name; + const char *selected_solution, *model_name; int r; r = sd_device_new_from_path(&device, path); @@ -269,15 +259,9 @@ static int query_solutions_for_path(const char *path) { if (r < 0) return log_device_error_errno(device, r, "Model name for device %s is unknown", path); - r = get_known_solutions(device, &solutions); - if (r == -ENOENT) { - log_device_info(device, "Attribute IOCOST_SOLUTIONS missing, model not found in hwdb."); + r = get_known_solutions(device, LOG_INFO, &solutions, &selected_solution); + if (r == -ENOENT) return 0; - } - if (r < 0) - return log_device_error_errno(device, r, "Couldn't access IOCOST_SOLUTIONS for device %s, model name %s on hwdb: %m\n", path, model_name); - - r = choose_solution(solutions, &default_solution); if (r < 0) return r; @@ -285,7 +269,7 @@ static int query_solutions_for_path(const char *path) { "Preferred solution: %s\n" "Solution that would be applied: %s", path, model_name, - arg_target_solution, default_solution); + arg_target_solution, selected_solution); STRV_FOREACH(s, solutions) { const char *model, *qos; From 934613bb88054cd5e548bd77dfad6cea73b618ad Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 11:56:23 +0900 Subject: [PATCH 5/9] udev/iocost: call get_known_solutions() in apply_solution_for_path() Then, the solution name can be logged. --- src/udev/iocost/iocost.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 801751a398..9737cf6af7 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -141,23 +141,14 @@ static int query_named_solution( const char **ret_model, const char **ret_qos) { - _cleanup_strv_free_ char **solutions = NULL; _cleanup_free_ char *upper_name = NULL, *qos_key = NULL, *model_key = NULL; const char *qos, *model; int r; + assert(name); assert(ret_qos); assert(ret_model); - /* If NULL is passed we query the default solution, which is the first one listed - * in the IOCOST_SOLUTIONS key or the one specified by the TargetSolution setting. - */ - if (!name) { - r = get_known_solutions(device, LOG_DEBUG, &solutions, &name); - if (r < 0) - return r; - } - upper_name = strdup(name); if (!upper_name) return log_oom(); @@ -193,6 +184,7 @@ static int query_named_solution( static int apply_solution_for_path(const char *path, const char *name) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; + _cleanup_strv_free_ char **solutions = NULL; _cleanup_free_ char *qos = NULL, *model = NULL; const char *qos_params, *model_params; dev_t devnum; @@ -202,16 +194,24 @@ static int apply_solution_for_path(const char *path, const char *name) { if (r < 0) return log_error_errno(r, "Error looking up device: %m"); + r = sd_device_get_devnum(device, &devnum); + if (r < 0) + return log_device_error_errno(device, r, "Error getting devnum: %m"); + + if (!name) { + r = get_known_solutions(device, LOG_DEBUG, &solutions, &name); + if (r == -ENOENT) + return 0; + if (r < 0) + return r; + } + r = query_named_solution(device, name, &model_params, &qos_params); if (r == -ENOENT) return 0; if (r < 0) return r; - r = sd_device_get_devnum(device, &devnum); - if (r < 0) - return log_device_error_errno(device, r, "Error getting devnum: %m"); - if (asprintf(&qos, DEVNUM_FORMAT_STR " enable=1 ctrl=user %s", DEVNUM_FORMAT_VAL(devnum), qos_params) < 0) return log_oom(); @@ -219,8 +219,9 @@ static int apply_solution_for_path(const char *path, const char *name) { return log_oom(); log_debug("Applying iocost parameters to %s using solution '%s'\n" - "\tio.cost.qos: %s\n" - "\tio.cost.model: %s\n", path, name ?: "default", qos, model); + "\tio.cost.qos: %s\n" + "\tio.cost.model: %s\n", + path, name, qos, model); r = cg_set_attribute("io", NULL, "io.cost.qos", qos); if (r < 0) { From b2fccd07293da8cabdbfea9638ac235a9e076757 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 12:17:18 +0900 Subject: [PATCH 6/9] udev/iocost: query_named_solution() provides non-NULL model and qos on success --- src/udev/iocost/iocost.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 9737cf6af7..804d11ca17 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -275,8 +275,7 @@ static int query_solutions_for_path(const char *path) { STRV_FOREACH(s, solutions) { const char *model, *qos; - r = query_named_solution(device, *s, &model, &qos); - if (r < 0 || !model || !qos) + if (query_named_solution(device, *s, &model, &qos) < 0) continue; log_info("%s: io.cost.qos: %s\n" From ede5e271b1900c5cc85e330a5da2c657282d7910 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 12:26:02 +0900 Subject: [PATCH 7/9] udev/iocost: fix log message --- src/udev/iocost/iocost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 804d11ca17..fc8223e2b6 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -168,7 +168,7 @@ static int query_named_solution( if (r == -ENOENT) return log_device_debug_errno(device, r, "No value found for key %s, skipping iocost logic.", qos_key); if (r < 0) - return log_device_error_errno(device, r, "Failed to obtain model for iocost solution from device: %m"); + return log_device_error_errno(device, r, "Failed to obtain QoS for iocost solution from device: %m"); r = sd_device_get_property_value(device, model_key, &model); if (r == -ENOENT) From c413ae18b542cbd76c59d61deae49c404bd23de8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 12:05:36 +0900 Subject: [PATCH 8/9] udev/iocost: use ID_MODEL_FROM_DATABASE if exists To make the rule consistent with 'iocost query'. --- rules.d/90-iocost.rules | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rules.d/90-iocost.rules b/rules.d/90-iocost.rules index 50f778a0ae..6c31cae56b 100644 --- a/rules.d/90-iocost.rules +++ b/rules.d/90-iocost.rules @@ -13,7 +13,11 @@ ENV{DEVTYPE}=="partition", GOTO="iocost_end" ACTION=="remove", GOTO="iocost_end" -ENV{ID_MODEL}!="", IMPORT{builtin}="hwdb 'block::name:$env{ID_MODEL}:fwrev:$env{ID_REVISION}:'" +ENV{.MODEL}="" +ENV{ID_MODEL}!="", ENV{.MODEL}="$env{ID_MODEL}" +ENV{ID_MODEL_FROM_DATABASE}!="", ENV{.MODEL}="$env{ID_MODEL_FROM_DATABASE}" + +ENV{.MODEL}!="", IMPORT{builtin}="hwdb 'block::name:$env{.MODEL}:fwrev:$env{ID_REVISION}:'" ENV{IOCOST_SOLUTIONS}!="", RUN+="iocost apply $env{DEVNAME}" From fc73d971822afaea29bdaeec054e90a4885d0cce Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 12:18:40 +0900 Subject: [PATCH 9/9] udev/iocost: invert DEVTYPE match No functional change, just refactoring. Addresses https://github.com/systemd/systemd/pull/23325#discussion_r1171006967. --- rules.d/90-iocost.rules | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rules.d/90-iocost.rules b/rules.d/90-iocost.rules index 6c31cae56b..34311ded6d 100644 --- a/rules.d/90-iocost.rules +++ b/rules.d/90-iocost.rules @@ -8,9 +8,7 @@ # (at your option) any later version. SUBSYSTEM!="block", GOTO="iocost_end" - -ENV{DEVTYPE}=="partition", GOTO="iocost_end" - +ENV{DEVTYPE}!="disk", GOTO="iocost_end" ACTION=="remove", GOTO="iocost_end" ENV{.MODEL}=""