[LTP] [PATCH v2 1/3] API/cgroup: Add io controller

[LTP] [PATCH v2 1/3] API/cgroup: Add io controller

[Richard Palethorpe via ltp]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V2:
* Fix doc format
* abstract reading io.stat
* abstract stating the underlying device

 lib/tst_cgroup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 57940ba09..feb5b3d07 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -83,6 +83,7 @@ enum cgroup_ctrl_indx {
 	CTRL_MEMORY = 1,
 	CTRL_CPU,
 	CTRL_CPUSET,
+	CTRL_IO,
 };
 #define CTRLS_MAX CTRL_CPUSET
 
@@ -191,6 +192,11 @@ static const struct cgroup_file cpuset_ctrl_files[] = {
 	{ }
 };
 
+static const struct cgroup_file io_ctrl_files[] = {
+	{ "io.stat", NULL, CTRL_IO },
+	{ }
+};
+
 /* Lookup tree for item names. */
 static struct cgroup_ctrl controllers[] = {
 	[0] = { "cgroup", cgroup_ctrl_files, 0, NULL, 0 },
@@ -203,6 +209,9 @@ static struct cgroup_ctrl controllers[] = {
 	[CTRL_CPUSET] = {
 		"cpuset", cpuset_ctrl_files, CTRL_CPUSET, NULL, 0
 	},
+	[CTRL_IO] = {
+		"io", io_ctrl_files, CTRL_IO, NULL, 0
+	},
 	{ }
 };
 
-- 
2.35.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 2/3] API/device: Add func to stat the actual dev mounted to a path

[Richard Palethorpe via ltp]

BTRFS appears to create "anonymous" block devices for each sub
partition. Stating an inode on this FS (and others) just returns the
anon dev number. Instead we have to find the mounted device in
/proc/self/mounts and stat its special file in /dev/.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_device.h |  6 ++++++
 lib/tst_device.c     | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/tst_device.h b/include/tst_device.h
index 95ccd453e..977427f1c 100644
--- a/include/tst_device.h
+++ b/include/tst_device.h
@@ -8,6 +8,7 @@
 
 #include <unistd.h>
 #include <stdint.h>
+#include <sys/stat.h>
 
 struct tst_device {
 	const char *dev;
@@ -112,6 +113,11 @@ void tst_purge_dir(const char *path);
  */
 void tst_find_backing_dev(const char *path, char *dev);
 
+/*
+ * Stat the device mounted on a given path.
+ */
+void tst_stat_mount_dev(const char *const mnt_path, struct stat *const st);
+
 /*
  * Returns the size of a physical device block size for the specific path
  * @path   Path to find the block size
diff --git a/lib/tst_device.c b/lib/tst_device.c
index 1ef667fa0..d296f9118 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -25,6 +25,7 @@
 #include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <sys/mount.h>
+#include <mntent.h>
 #include <errno.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -548,6 +549,40 @@ void tst_find_backing_dev(const char *path, char *dev)
 		tst_brkm(TCONF, NULL, "dev(%s) isn't a block dev", dev);
 }
 
+void tst_stat_mount_dev(const char *const mnt_path, struct stat *const st)
+{
+	struct mntent *mnt;
+	FILE *mntf = setmntent("/proc/self/mounts", "r");
+
+	if (!mntf) {
+		tst_brkm(TBROK | TERRNO, NULL, "Can't open /proc/self/mounts");
+		return;
+	}
+
+	mnt = getmntent(mntf);
+	if (!mnt) {
+		tst_brkm(TBROK | TERRNO, NULL, "Can't read mounts or no mounts?");
+		return;
+	}
+
+	do {
+		if (strcmp(mnt->mnt_dir, mnt_path)) {
+			mnt = getmntent(mntf);
+			continue;
+		}
+
+		if (stat(mnt->mnt_fsname, st)) {
+			tst_brkm(TBROK | TERRNO, NULL,
+				 "Can't stat '%s', mounted at '%s'",
+				 mnt->mnt_fsname, mnt_path);
+		}
+
+		return;
+	} while (mnt);
+
+	tst_brkm(TBROK, NULL, "Could not find mount device");
+}
+
 int tst_dev_block_size(const char *path)
 {
 	int fd;
-- 
2.35.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 2/3] API/device: Add func to stat the actual dev mounted to a path

[Petr Vorel]

Hi Richie,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 3/3] cgroups: Add first IO controller test

[Richard Palethorpe via ltp]

In V1 there is the blkio controller. This was renamed to just io on
V2. The interface and functionality is significantly
different. Presently there do not appear to be any tests for the V2
controller.

Note that one can not simply stat a file on BTRFS to find the actual
block device the filesystem is using. Nor can you read
/proc/self/mountinfo. BTRFS seems to generate "anonymous"
devices (e.g. 0:27) and this is what is reported by stat. These
however are invisible to the IO controller.

So instead we have to look in /proc/mounts for the device path then
stat the special (/dev/<device>) file to get the actual major and
minor device number.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 runtest/controllers                           |   3 +
 testcases/kernel/controllers/io/.gitignore    |   1 +
 testcases/kernel/controllers/io/Makefile      |   6 +
 .../kernel/controllers/io/io_control01.c      | 155 ++++++++++++++++++
 4 files changed, 165 insertions(+)
 create mode 100644 testcases/kernel/controllers/io/.gitignore
 create mode 100644 testcases/kernel/controllers/io/Makefile
 create mode 100644 testcases/kernel/controllers/io/io_control01.c

diff --git a/runtest/controllers b/runtest/controllers
index 3108a2561..22d482050 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -360,6 +360,9 @@ cpuset_regression_test cpuset_regression_test.sh
 
 cgroup_xattr	cgroup_xattr
 
+# V2 IO controller (was blkio)
+io_control01 io_control01
+
 pids_1_1 pids.sh 1 1 0
 pids_1_2 pids.sh 1 2 0
 pids_1_10 pids.sh 1 10 0
diff --git a/testcases/kernel/controllers/io/.gitignore b/testcases/kernel/controllers/io/.gitignore
new file mode 100644
index 000000000..d626fa80d
--- /dev/null
+++ b/testcases/kernel/controllers/io/.gitignore
@@ -0,0 +1 @@
+io_control01
diff --git a/testcases/kernel/controllers/io/Makefile b/testcases/kernel/controllers/io/Makefile
new file mode 100644
index 000000000..5ea7d67db
--- /dev/null
+++ b/testcases/kernel/controllers/io/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/controllers/io/io_control01.c b/testcases/kernel/controllers/io/io_control01.c
new file mode 100644
index 000000000..a6bf01639
--- /dev/null
+++ b/testcases/kernel/controllers/io/io_control01.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+/*\
+ *
+ * [Description]
+ *
+ * Perform some I/O on a file and check if at least some of it is
+ * recorded by the I/O controller.
+ *
+ * The exact amount of I/O performed is dependent on the file system,
+ * page cache, scheduler and block driver. We call sync and drop the
+ * file's page cache to force reading and writing. We also write
+ * random data to try to prevent compression.
+ *
+ * The pagecache is a particular issue for reading. If the call to
+ * fadvise is ignored then the data may only be read from the
+ * cache. So that no I/O requests are made.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/sysmacros.h>
+
+#include "tst_test.h"
+
+struct io_stats {
+	unsigned int mjr;
+	unsigned int mnr;
+	unsigned long rbytes;
+	unsigned long wbytes;
+	unsigned long rios;
+	unsigned long wios;
+	unsigned long dbytes;
+	unsigned long dios;
+};
+
+static unsigned int dev_major, dev_minor;
+
+static int read_io_stats(const char *const line, struct io_stats *const stat)
+{
+	return sscanf(line,
+		      "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
+		      &stat->mjr, &stat->mnr,
+		      &stat->rbytes, &stat->wbytes, &stat->rios, &stat->wios,
+		      &stat->dbytes, &stat->dios);
+}
+
+static void run(void)
+{
+	int i, fd;
+	char *line, *buf_ptr;
+	const size_t pgsz = SAFE_SYSCONF(_SC_PAGESIZE);
+	char *buf = SAFE_MALLOC(MAX((size_t)BUFSIZ, pgsz));
+	struct io_stats start;
+
+	SAFE_CG_READ(tst_cg, "io.stat", buf, BUFSIZ - 1);
+	line = strtok_r(buf, "\n", &buf_ptr);
+	while (line) {
+		const int convs = read_io_stats(line, &start);
+
+		if (convs < 2)
+			continue;
+
+		tst_res(TINFO, "Found %u:%u in io.stat", dev_major, dev_minor);
+
+		if (start.mjr == dev_major || start.mnr == dev_minor)
+			break;
+
+		line = strtok_r(NULL, "\n", &buf_ptr);
+	}
+
+	SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid());
+
+	fd = SAFE_OPEN("/dev/urandom", O_RDONLY, 0600);
+	SAFE_READ(1, fd, buf, pgsz);
+	SAFE_CLOSE(fd);
+
+	fd = SAFE_OPEN("mnt/dat", O_WRONLY | O_CREAT, 0600);
+
+	for (i = 0; i < 4; i++) {
+		SAFE_WRITE(1, fd, buf, pgsz);
+		SAFE_FSYNC(fd);
+		TST_EXP_PASS_SILENT(posix_fadvise(fd, pgsz * i, pgsz, POSIX_FADV_DONTNEED));
+	}
+
+	SAFE_CLOSE(fd);
+	fd = SAFE_OPEN("mnt/dat", O_RDONLY, 0600);
+
+	for (i = 0; i < 4; i++)
+		SAFE_READ(1, fd, buf, pgsz);
+
+	tst_res(TPASS, "Did some IO in the IO controller");
+
+	SAFE_CG_READ(tst_cg, "io.stat", buf, BUFSIZ - 1);
+	line = strtok_r(buf, "\n", &buf_ptr);
+	while (line) {
+		struct io_stats end;
+		const int convs = read_io_stats(line, &end);
+
+		if (convs < 8)
+			break;
+
+		if (end.mjr != dev_major || end.mnr != dev_minor) {
+			line = strtok_r(NULL, "\n", &buf_ptr);
+			continue;
+		}
+
+		tst_res(TPASS, "Found %u:%u in io.stat", dev_major, dev_minor);
+		TST_EXP_EXPR(end.rbytes > start.rbytes,
+			     "(rbytes=%lu) > (st_rbytes=%lu)",
+			     end.rbytes, start.rbytes);
+		TST_EXP_EXPR(end.wbytes > start.wbytes,
+			     "(wbytes=%lu) > (st_wbytes=%lu)",
+			     end.wbytes, start.wbytes);
+		TST_EXP_EXPR(end.rios > start.rios,
+			     "(rios=%lu) > (st_rios=%lu)",
+			     end.rios, start.rios);
+		TST_EXP_EXPR(end.wios > start.wios,
+			     "(wios=%lu) > (st_wios=%lu)",
+			     end.wios, start.wios);
+
+		goto out;
+	}
+
+	tst_res(TINFO, "io.stat:\n%s", buf);
+	tst_res(TFAIL, "Did not find %u:%u in io.stat", dev_major, dev_minor);
+out:
+	free(buf);
+	SAFE_CLOSE(fd);
+	SAFE_UNLINK("mnt/dat");
+}
+
+static void setup(void)
+{
+	char buf[PATH_MAX] = { 0 };
+	char *path = SAFE_GETCWD(buf, PATH_MAX - sizeof("mnt") - 1);
+	struct stat st;
+
+	strcpy(path + strlen(path), "/mnt");
+
+	tst_stat_mount_dev(path, &st);
+	dev_major = major(st.st_rdev);
+	dev_minor = minor(st.st_rdev);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.needs_device = 1,
+	.mntpoint = "mnt",
+	.mount_device = 1,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *const[]){ "ntfs", "tmpfs", NULL },
+	.needs_cgroup_ver = TST_CG_V2,
+	.needs_cgroup_ctrls = (const char *const[]){ "io", NULL },
+};
-- 
2.35.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 3/3] cgroups: Add first IO controller test

[Petr Vorel]

Hi Richie,

> +++ b/testcases/kernel/controllers/io/io_control01.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
nit: not sure if it was deliberate not adding it, but you may want to add your/SUSE copyright.

> +/*\
> + *
> + * [Description]
> + *
> + * Perform some I/O on a file and check if at least some of it is
> + * recorded by the I/O controller.
> + *
> + * The exact amount of I/O performed is dependent on the file system,
> + * page cache, scheduler and block driver. We call sync and drop the
> + * file's page cache to force reading and writing. We also write
> + * random data to try to prevent compression.
> + *
> + * The pagecache is a particular issue for reading. If the call to
> + * fadvise is ignored then the data may only be read from the
> + * cache. So that no I/O requests are made.
> + */
> +
...
> +static int read_io_stats(const char *const line, struct io_stats *const stat)
> +{
> +	return sscanf(line,
> +		      "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
> +		      &stat->mjr, &stat->mnr,
> +		      &stat->rbytes, &stat->wbytes, &stat->rios, &stat->wios,
> +		      &stat->dbytes, &stat->dios);
> +}
checkpatch.pl false positive:
io_control01.c:40: WARNING: unchecked sscanf return value
Obviously perl parsing has some limitations as we check read_io_stats() return
value.

...
> +static void setup(void)
> +{
> +	char buf[PATH_MAX] = { 0 };
> +	char *path = SAFE_GETCWD(buf, PATH_MAX - sizeof("mnt") - 1);
> +	struct stat st;
> +
> +	strcpy(path + strlen(path), "/mnt");
> +
> +	tst_stat_mount_dev(path, &st);
> +	dev_major = major(st.st_rdev);
> +	dev_minor = minor(st.st_rdev);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_device = 1,
nit: testcases/kernel/controllers/io/io_control01.c: useless tag: needs_device

Kind regards,
Petr

> +	.mntpoint = "mnt",
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const[]){ "ntfs", "tmpfs", NULL },
> +	.needs_cgroup_ver = TST_CG_V2,
> +	.needs_cgroup_ctrls = (const char *const[]){ "io", NULL },
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 3/3] cgroups: Add first IO controller test

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
>> +++ b/testcases/kernel/controllers/io/io_control01.c
>> @@ -0,0 +1,155 @@
>> +// SPDX-License-Identifier: GPL-2.0
> nit: not sure if it was deliberate not adding it, but you may want to
> add your/SUSE copyright.
>
>> +/*\
>> + *
>> + * [Description]
>> + *
>> + * Perform some I/O on a file and check if at least some of it is
>> + * recorded by the I/O controller.
>> + *
>> + * The exact amount of I/O performed is dependent on the file system,
>> + * page cache, scheduler and block driver. We call sync and drop the
>> + * file's page cache to force reading and writing. We also write
>> + * random data to try to prevent compression.
>> + *
>> + * The pagecache is a particular issue for reading. If the call to
>> + * fadvise is ignored then the data may only be read from the
>> + * cache. So that no I/O requests are made.
>> + */
>> +
> ...
>> +static int read_io_stats(const char *const line, struct io_stats *const stat)
>> +{
>> +	return sscanf(line,
>> +		      "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
>> +		      &stat->mjr, &stat->mnr,
>> +		      &stat->rbytes, &stat->wbytes, &stat->rios, &stat->wios,
>> +		      &stat->dbytes, &stat->dios);
>> +}
> checkpatch.pl false positive:
> io_control01.c:40: WARNING: unchecked sscanf return value
> Obviously perl parsing has some limitations as we check read_io_stats() return
> value.

I'm not sure what to do about this other than switch to a macro which is
a bit silly.

IMO sscanf should have the warn_unused_result attribute and this should
be inherited by read_io_stats. All of which is better handled by the
compiler.

>
> ...
>> +static void setup(void)
>> +{
>> +	char buf[PATH_MAX] = { 0 };
>> +	char *path = SAFE_GETCWD(buf, PATH_MAX - sizeof("mnt") - 1);
>> +	struct stat st;
>> +
>> +	strcpy(path + strlen(path), "/mnt");
>> +
>> +	tst_stat_mount_dev(path, &st);
>> +	dev_major = major(st.st_rdev);
>> +	dev_minor = minor(st.st_rdev);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.setup = setup,
>> +	.needs_device = 1,
> nit: testcases/kernel/controllers/io/io_control01.c: useless tag: needs_device

Pushed with fixes (including needs_device).
-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 3/3] cgroups: Add first IO controller test

[Petr Vorel]

Hi Richie,

LGTM, just please remove needs_device as not needed.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +static void setup(void)
> +{
> +	char buf[PATH_MAX] = { 0 };
> +	char *path = SAFE_GETCWD(buf, PATH_MAX - sizeof("mnt") - 1);
> +	struct stat st;
> +
> +	strcpy(path + strlen(path), "/mnt");
> +
> +	tst_stat_mount_dev(path, &st);
> +	dev_major = major(st.st_rdev);
> +	dev_minor = minor(st.st_rdev);
If we ever support test setup run inside do_setup() (i.e. setup function which
would be run just once - before for_each_variant(), for_each_filesystem() and
fork_testrun()), this test would have use case for it (I suppose more tests
which use .all_filesystems). Because tst_stat_mount_dev() could be run just for
first filesystem. But that's not related to the testcase.

Kind regards,
Petr

> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_device = 1,
> +	.mntpoint = "mnt",
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const[]){ "ntfs", "tmpfs", NULL },
> +	.needs_cgroup_ver = TST_CG_V2,
> +	.needs_cgroup_ctrls = (const char *const[]){ "io", NULL },
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/3] API/cgroup: Add io controller

[Petr Vorel]

Hi Richie,

Reviewed-by: Petr Vorel <pvorel@suse.cz>
obviously correct.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp