From 771903608c7317defee99ec0c8dc107471c41d71 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Fri, 2 Feb 2024 13:57:16 -0800
Subject: [PATCH] libct/cg: write unified resources line by line

It has been pointed out that some controllers can not accept multiple
lines of output at once. In particular, io.max can only set one device
at a time.

Practically, the only multi-line resource values we can get come from
unified.* -- let's write those line by line.

Add a test case.

Reported-by: Tao Shen <shentaoskyking@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/cgroups/file.go      | 34 +++++++++++++++++++++++++++++++
 libcontainer/cgroups/file_test.go | 22 ++++++++++++++++++++
 libcontainer/cgroups/fs2/fs2.go   |  2 +-
 tests/integration/cgroups.bats    | 32 +++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/libcontainer/cgroups/file.go b/libcontainer/cgroups/file.go
index 16aae5a3b..78c5bcf0d 100644
--- a/libcontainer/cgroups/file.go
+++ b/libcontainer/cgroups/file.go
@@ -57,6 +57,40 @@ func WriteFile(dir, file, data string) error {
 	return nil
 }
 
+// WriteFileByLine is the same as WriteFile, except if data contains newlines,
+// it is written line by line.
+func WriteFileByLine(dir, file, data string) error {
+	i := strings.Index(data, "\n")
+	if i == -1 {
+		return WriteFile(dir, file, data)
+	}
+
+	fd, err := OpenFile(dir, file, unix.O_WRONLY)
+	if err != nil {
+		return err
+	}
+	defer fd.Close()
+	start := 0
+	for {
+		var line string
+		if i == -1 {
+			line = data[start:]
+		} else {
+			line = data[start : start+i+1]
+		}
+		_, err := fd.WriteString(line)
+		if err != nil {
+			return fmt.Errorf("failed to write %q: %w", line, err)
+		}
+		if i == -1 {
+			break
+		}
+		start += i + 1
+		i = strings.Index(data[start:], "\n")
+	}
+	return nil
+}
+
 const (
 	cgroupfsDir    = "/sys/fs/cgroup"
 	cgroupfsPrefix = cgroupfsDir + "/"
diff --git a/libcontainer/cgroups/file_test.go b/libcontainer/cgroups/file_test.go
index 6236bb9ec..5ce438380 100644
--- a/libcontainer/cgroups/file_test.go
+++ b/libcontainer/cgroups/file_test.go
@@ -73,3 +73,25 @@ func TestOpenat2(t *testing.T) {
 		fd.Close()
 	}
 }
+
+func BenchmarkWriteFile(b *testing.B) {
+	TestMode = true
+	defer func() { TestMode = false }()
+
+	dir := b.TempDir()
+	tc := []string{
+		"one",
+		"one\ntwo\nthree",
+		"10:200 foo=bar boo=far\n300:1200 something=other\ndefault 45000\n",
+		"\n\n\n\n\n\n\n\n",
+	}
+
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, val := range tc {
+			if err := WriteFileByLine(dir, "file", val); err != nil {
+				b.Fatal(err)
+			}
+		}
+	}
+}
diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go
index 0a579b23e..87c89e8a1 100644
--- a/libcontainer/cgroups/fs2/fs2.go
+++ b/libcontainer/cgroups/fs2/fs2.go
@@ -238,7 +238,7 @@ func (m *Manager) setUnified(res map[string]string) error {
 		if strings.Contains(k, "/") {
 			return fmt.Errorf("unified resource %q must be a file name (no slashes)", k)
 		}
-		if err := cgroups.WriteFile(m.dirPath, k, v); err != nil {
+		if err := cgroups.WriteFileByLine(m.dirPath, k, v); err != nil {
 			// Check for both EPERM and ENOENT since O_CREAT is used by WriteFile.
 			if errors.Is(err, os.ErrPermission) || errors.Is(err, os.ErrNotExist) {
 				// Check if a controller is available,
diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats
index d5ca1325d..3a74eb262 100644
--- a/tests/integration/cgroups.bats
+++ b/tests/integration/cgroups.bats
@@ -187,6 +187,38 @@ function setup() {
 	[[ "$weights" == *"$major:$minor 444"* ]]
 }
 
+@test "runc run (per-device multiple iops via unified)" {
+	requires root cgroups_v2
+
+	dd if=/dev/zero of=backing1.img bs=4096 count=1
+	dev1=$(losetup --find --show backing1.img) || skip "unable to create a loop device"
+
+	# Second device.
+	dd if=/dev/zero of=backing2.img bs=4096 count=1
+	dev2=$(losetup --find --show backing2.img) || skip "unable to create a loop device"
+
+	set_cgroups_path
+
+	IFS=$' \t:' read -r major1 minor1 <<<"$(lsblk -nd -o MAJ:MIN "$dev1")"
+	IFS=$' \t:' read -r major2 minor2 <<<"$(lsblk -nd -o MAJ:MIN "$dev2")"
+	update_config '	  .linux.devices += [
+				{path: "'"$dev1"'", type: "b", major: '"$major1"', minor: '"$minor1"'},
+				{path: "'"$dev2"'", type: "b", major: '"$major2"', minor: '"$minor2"'}
+			   ]
+			| .linux.resources.unified |=
+				{"io.max": "'"$major1"':'"$minor1"' riops=333 wiops=444\n'"$major2"':'"$minor2"' riops=555 wiops=666\n"}'
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_dev_weight
+	[ "$status" -eq 0 ]
+
+	# The loop devices are no longer needed.
+	losetup -d "$dev1"
+	losetup -d "$dev2"
+
+	weights=$(get_cgroup_value "io.max")
+	grep "^$major1:$minor1 .* riops=333 wiops=444$" <<<"$weights"
+	grep "^$major2:$minor2 .* riops=555 wiops=666$" <<<"$weights"
+}
+
 @test "runc run (cpu.idle)" {
 	requires cgroups_cpu_idle
 	[ $EUID -ne 0 ] && requires rootless_cgroup