From 0fa0dfa04465651a18107d503f9967f84bd761d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Fri, 13 Sep 2024 19:27:13 +0200 Subject: [PATCH 1/2] core/cgroup: Apply IODevice*= directives in configured order Different device paths may resolve to same device node (lookup_block_device()), e.g. IOReadBandwidthMax=/dev/sda1 18879 IOReadBandwidthMax=/dev/sda2 18878 where both partitions resolve to /dev/sda and when these values are applied (they are associated with original paths, i.e. as if applied for different device) in the order from io_device_limits. The parsing code prepends, so they end up in reverse order wrt config file. Switch the direction so that the order of application matches the order of configuration -- i.e. semantics in all other unit file directives. Apply same change to all directives that use per-device lists. (The question whether partitions should be resolved to base device is independent.) And apply the changes equally to DBus properties write handlers. Fixes #34126 --- src/core/dbus-cgroup.c | 10 +++++----- src/core/load-fragment.c | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index 49e84b44e32..459fa6f774c 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -1424,7 +1424,7 @@ int bus_cgroup_set_property( for (type = 0; type < _CGROUP_IO_LIMIT_TYPE_MAX; type++) a->limits[type] = cgroup_io_limit_defaults[type]; - LIST_PREPEND(device_limits, c->io_device_limits, a); + LIST_APPEND(device_limits, c->io_device_limits, a); } a->limits[iol_type] = u64; @@ -1504,7 +1504,7 @@ int bus_cgroup_set_property( free(a); return -ENOMEM; } - LIST_PREPEND(device_weights, c->io_device_weights, a); + LIST_APPEND(device_weights, c->io_device_weights, a); } a->weight = weight; @@ -1578,7 +1578,7 @@ int bus_cgroup_set_property( free(a); return -ENOMEM; } - LIST_PREPEND(device_latencies, c->io_device_latencies, a); + LIST_APPEND(device_latencies, c->io_device_latencies, a); } a->target_usec = target; @@ -1659,7 +1659,7 @@ int bus_cgroup_set_property( return -ENOMEM; } - LIST_PREPEND(device_bandwidths, c->blockio_device_bandwidths, a); + LIST_APPEND(device_bandwidths, c->blockio_device_bandwidths, a); } if (read) @@ -1753,7 +1753,7 @@ int bus_cgroup_set_property( free(a); return -ENOMEM; } - LIST_PREPEND(device_weights, c->blockio_device_weights, a); + LIST_APPEND(device_weights, c->blockio_device_weights, a); } a->weight = weight; diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 7cb8648743c..8a94e06265c 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4262,7 +4262,7 @@ int config_parse_io_device_weight( w->path = TAKE_PTR(resolved); w->weight = u; - LIST_PREPEND(device_weights, c->io_device_weights, w); + LIST_APPEND(device_weights, c->io_device_weights, w); return 0; } @@ -4333,7 +4333,7 @@ int config_parse_io_device_latency( l->path = TAKE_PTR(resolved); l->target_usec = usec; - LIST_PREPEND(device_latencies, c->io_device_latencies, l); + LIST_APPEND(device_latencies, c->io_device_latencies, l); return 0; } @@ -4419,7 +4419,7 @@ int config_parse_io_limit( for (CGroupIOLimitType i = 0; i < _CGROUP_IO_LIMIT_TYPE_MAX; i++) l->limits[i] = cgroup_io_limit_defaults[i]; - LIST_PREPEND(device_limits, c->io_device_limits, l); + LIST_APPEND(device_limits, c->io_device_limits, l); } l->limits[type] = num; @@ -4500,7 +4500,7 @@ int config_parse_blockio_device_weight( w->path = TAKE_PTR(resolved); w->weight = u; - LIST_PREPEND(device_weights, c->blockio_device_weights, w); + LIST_APPEND(device_weights, c->blockio_device_weights, w); return 0; } @@ -4587,7 +4587,7 @@ int config_parse_blockio_bandwidth( b->rbps = CGROUP_LIMIT_MAX; b->wbps = CGROUP_LIMIT_MAX; - LIST_PREPEND(device_bandwidths, c->blockio_device_bandwidths, b); + LIST_APPEND(device_bandwidths, c->blockio_device_bandwidths, b); } if (read) From 321637743313f896e275fd038996b8cfb5a070b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Fri, 4 Oct 2024 20:40:51 +0200 Subject: [PATCH 2/2] test: Add test for per-device cgroup properties Reported in #34126 --- test/units/TEST-19-CGROUP.keyed-properties.sh | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100755 test/units/TEST-19-CGROUP.keyed-properties.sh diff --git a/test/units/TEST-19-CGROUP.keyed-properties.sh b/test/units/TEST-19-CGROUP.keyed-properties.sh new file mode 100755 index 00000000000..cadefe26d5f --- /dev/null +++ b/test/units/TEST-19-CGROUP.keyed-properties.sh @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -ex +set -o pipefail + +# shellcheck source=test/units/test-control.sh +. "$(dirname "$0")"/test-control.sh +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +if [[ "$(get_cgroup_hierarchy)" != unified ]]; then + echo "Skipping $0 as we're not running with the unified cgroup hierarchy" + exit 0 +fi + +testcase_iodevice_dbus () { + # Test that per-device properties are applied in configured order even for different devices (because + # they may resolve to same underlying device in the end + # Note: if device does not exist cgroup attribute write fails but systemd should still track the + # configured properties + systemd-run --unit=test0.service \ + --property="IOAccounting=yes" \ + sleep inf + + systemctl set-property test0.service \ + IOReadBandwidthMax="/dev/sda1 1M" \ + IOReadBandwidthMax="/dev/sda2 2M" \ + IOReadBandwidthMax="/dev/sda3 4M" + + local output + output=$(mktemp) + trap 'rm -f "$output"' RETURN + systemctl show -P IOReadBandwidthMax test0.service >"$output" + diff -u "$output" - </run/systemd/system/test1.service <"$output" + diff -u "$output" - <