All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 1/2] API/cgroup: Add io controller
@ 2022-03-15 13:41 Richard Palethorpe via ltp
  2022-03-15 13:41 ` [LTP] [PATCH 2/2] cgroups: Add first IO controller test Richard Palethorpe via ltp
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Palethorpe via ltp @ 2022-03-15 13:41 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 lib/tst_cgroup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 8cca0654d..62e1c00db 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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [LTP] [PATCH 2/2] cgroups: Add first IO controller test
  2022-03-15 13:41 [LTP] [PATCH 1/2] API/cgroup: Add io controller Richard Palethorpe via ltp
@ 2022-03-15 13:41 ` Richard Palethorpe via ltp
  2022-03-23 17:56   ` Cyril Hrubis
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Palethorpe via ltp @ 2022-03-15 13:41 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

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      | 162 ++++++++++++++++++
 4 files changed, 172 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..706e67c89
--- /dev/null
+++ b/testcases/kernel/controllers/io/io_control01.c
@@ -0,0 +1,162 @@
+// 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 <mntent.h>
+
+#include "tst_test.h"
+
+static unsigned int dev_major, dev_minor;
+
+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));
+	unsigned long st_rbytes = 0, st_wbytes = 0, st_rios = 0, st_wios = 0;
+
+	SAFE_CG_READ(tst_cg, "io.stat", buf, BUFSIZ - 1);
+	line = strtok_r(buf, "\n", &buf_ptr);
+	while (line) {
+		unsigned int mjr, mnr;
+		unsigned long dbytes, dios;
+
+		const int convs =
+			sscanf(line,
+			       "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
+			       &mjr, &mnr, &st_rbytes, &st_wbytes, &st_rios, &st_wios,
+			       &dbytes, &dios);
+
+		if (convs < 2)
+			continue;
+
+		tst_res(TINFO, "Found %u:%u in io.stat", dev_major, dev_minor);
+
+		if (mjr == dev_major || 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) {
+		unsigned int mjr, mnr;
+		unsigned long rbytes, wbytes, rios, wios, dbytes, dios;
+
+		const int convs =
+			sscanf(line,
+			       "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
+			       &mjr, &mnr, &rbytes, &wbytes, &rios, &wios,
+			       &dbytes, &dios);
+
+		if (convs < 8)
+			break;
+
+		if (mjr != dev_major || 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(rbytes > st_rbytes, "(rbytes=%lu) > (st_rbytes=%lu)", rbytes, st_rbytes);
+		TST_EXP_EXPR(wbytes > st_wbytes, "(wbytes=%lu) > (st_wbytes=%lu)", wbytes, st_wbytes);
+		TST_EXP_EXPR(rios > st_rios, "(rios=%lu) > (st_rios=%lu)", rios, st_rios);
+		TST_EXP_EXPR(wios > st_wios, "(wios=%lu) > (st_wios=%lu)", wios, st_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 mntent *mnt;
+	FILE *mntf = setmntent("/proc/self/mounts", "r");
+	struct stat st;
+
+	strcpy(path + strlen(path), "/mnt");
+
+	if (!mntf) {
+		tst_brk(TBROK | TERRNO, "Can't open /proc/self/mounts");
+		return;
+	}
+
+	mnt = getmntent(mntf);
+	if (!mnt) {
+		tst_brk(TBROK | TERRNO, "Can't read mounts or no mounts?");
+		return;
+	}
+
+	do {
+		if (strcmp(mnt->mnt_dir, path))
+			continue;
+
+		SAFE_STAT(mnt->mnt_fsname, &st);
+		dev_major = major(st.st_rdev);
+		dev_minor = minor(st.st_rdev);
+
+		return;
+
+	} while ((mnt = getmntent(mntf)));
+
+	tst_brk(TBROK, "Could not find mount device");
+}
+
+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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [LTP] [PATCH 2/2] cgroups: Add first IO controller test
  2022-03-15 13:41 ` [LTP] [PATCH 2/2] cgroups: Add first IO controller test Richard Palethorpe via ltp
@ 2022-03-23 17:56   ` Cyril Hrubis
  2022-03-28 14:40     ` Richard Palethorpe
  0 siblings, 1 reply; 4+ messages in thread
From: Cyril Hrubis @ 2022-03-23 17:56 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: ltp

Hi!
> 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      | 162 ++++++++++++++++++
>  4 files changed, 172 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..706e67c89
> --- /dev/null
> +++ b/testcases/kernel/controllers/io/io_control01.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* [Description]

This is not the correct docparse hearder.

> + * 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 <mntent.h>
> +
> +#include "tst_test.h"
> +
> +static unsigned int dev_major, dev_minor;
> +
> +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));
> +	unsigned long st_rbytes = 0, st_wbytes = 0, st_rios = 0, st_wios = 0;
> +
> +	SAFE_CG_READ(tst_cg, "io.stat", buf, BUFSIZ - 1);
> +	line = strtok_r(buf, "\n", &buf_ptr);
> +	while (line) {
> +		unsigned int mjr, mnr;
> +		unsigned long dbytes, dios;
> +
> +		const int convs =
> +			sscanf(line,
> +			       "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
> +			       &mjr, &mnr, &st_rbytes, &st_wbytes, &st_rios, &st_wios,
> +			       &dbytes, &dios);
> +
> +		if (convs < 2)
> +			continue;
> +
> +		tst_res(TINFO, "Found %u:%u in io.stat", dev_major, dev_minor);
> +
> +		if (mjr == dev_major || 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) {
> +		unsigned int mjr, mnr;
> +		unsigned long rbytes, wbytes, rios, wios, dbytes, dios;
> +
> +		const int convs =
> +			sscanf(line,
> +			       "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
> +			       &mjr, &mnr, &rbytes, &wbytes, &rios, &wios,
> +			       &dbytes, &dios);
> +
> +		if (convs < 8)
> +			break;
> +
> +		if (mjr != dev_major || 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(rbytes > st_rbytes, "(rbytes=%lu) > (st_rbytes=%lu)", rbytes, st_rbytes);
> +		TST_EXP_EXPR(wbytes > st_wbytes, "(wbytes=%lu) > (st_wbytes=%lu)", wbytes, st_wbytes);
> +		TST_EXP_EXPR(rios > st_rios, "(rios=%lu) > (st_rios=%lu)", rios, st_rios);
> +		TST_EXP_EXPR(wios > st_wios, "(wios=%lu) > (st_wios=%lu)", wios, st_wios);

So we only test here that the counters are updated, that sounds fine for
a simple test.

Do you plan to try anything for io.max? Maybe something as basic as
running two concurent processes with very different limits and checking
that the more limited process transfer less bytes per unit of time?

> +		goto out;
> +	}

We do have two very similar copies of this parsing code, maybe we should
put that into a function, and pack the parameters into a structure o
avoid copy&paste like this. e.g.

struct iostats {
	unsigned long st_rbytes;
	unsigned long st_wbytes;
	unsigned long st_rios;
	unsigned long st_wios;
};

static int read_iostats(const char *stats,
                        unsigned int dev_min, unsigned int dev_maj,
			struct iostats *iostats);


> +	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 mntent *mnt;
> +	FILE *mntf = setmntent("/proc/self/mounts", "r");
> +	struct stat st;
> +
> +	strcpy(path + strlen(path), "/mnt");
> +
> +	if (!mntf) {
> +		tst_brk(TBROK | TERRNO, "Can't open /proc/self/mounts");
> +		return;
> +	}
> +
> +	mnt = getmntent(mntf);
> +	if (!mnt) {
> +		tst_brk(TBROK | TERRNO, "Can't read mounts or no mounts?");
> +		return;
> +	}
> +
> +	do {
> +		if (strcmp(mnt->mnt_dir, path))
> +			continue;
> +
> +		SAFE_STAT(mnt->mnt_fsname, &st);
> +		dev_major = major(st.st_rdev);
> +		dev_minor = minor(st.st_rdev);
> +
> +		return;
> +
> +	} while ((mnt = getmntent(mntf)));

I guess that this should probably go to the test library. We already
have tst_find_backding_dev() in there which is doing something a bit
similar. Looking at the code what we do here is to translate a
mountpoint into a device so it may be something as:

int tst_find_dev_by_mntpoint()

> +	tst_brk(TBROK, "Could not find mount device");
> +}
> +
> +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

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [LTP] [PATCH 2/2] cgroups: Add first IO controller test
  2022-03-23 17:56   ` Cyril Hrubis
@ 2022-03-28 14:40     ` Richard Palethorpe
  0 siblings, 0 replies; 4+ messages in thread
From: Richard Palethorpe @ 2022-03-28 14:40 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: ltp

Hello,

>> +
>> +		tst_res(TPASS, "Found %u:%u in io.stat", dev_major, dev_minor);
>> +		TST_EXP_EXPR(rbytes > st_rbytes, "(rbytes=%lu) > (st_rbytes=%lu)", rbytes, st_rbytes);
>> +		TST_EXP_EXPR(wbytes > st_wbytes, "(wbytes=%lu) > (st_wbytes=%lu)", wbytes, st_wbytes);
>> +		TST_EXP_EXPR(rios > st_rios, "(rios=%lu) > (st_rios=%lu)", rios, st_rios);
>> +		TST_EXP_EXPR(wios > st_wios, "(wios=%lu) > (st_wios=%lu)", wios, st_wios);
>
> So we only test here that the counters are updated, that sounds fine for
> a simple test.
>
> Do you plan to try anything for io.max? Maybe something as basic as
> running two concurent processes with very different limits and checking
> that the more limited process transfer less bytes per unit of time?

Yes, although maybe not immediately. In the case of io.max this should
not even require comparing concurrent processes as the limits are simply the
absolute number of bytes or iops performed per second by a particular
group. So we can just set them to a very low value and see that reading
or writing takes longer than when unlimited.

io.cost.qos is more complicated because limits are a proportion of the
overall sibling activity.

>
>> +		goto out;
>> +	}
>
> We do have two very similar copies of this parsing code, maybe we should
> put that into a function, and pack the parameters into a structure o
> avoid copy&paste like this. e.g.
>
> struct iostats {
> 	unsigned long st_rbytes;
> 	unsigned long st_wbytes;
> 	unsigned long st_rios;
> 	unsigned long st_wios;
> };
>
> static int read_iostats(const char *stats,
>                         unsigned int dev_min, unsigned int dev_maj,
> 			struct iostats *iostats);
>

+1

>
>> +	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 mntent *mnt;
>> +	FILE *mntf = setmntent("/proc/self/mounts", "r");
>> +	struct stat st;
>> +
>> +	strcpy(path + strlen(path), "/mnt");
>> +
>> +	if (!mntf) {
>> +		tst_brk(TBROK | TERRNO, "Can't open /proc/self/mounts");
>> +		return;
>> +	}
>> +
>> +	mnt = getmntent(mntf);
>> +	if (!mnt) {
>> +		tst_brk(TBROK | TERRNO, "Can't read mounts or no mounts?");
>> +		return;
>> +	}
>> +
>> +	do {
>> +		if (strcmp(mnt->mnt_dir, path))
>> +			continue;
>> +
>> +		SAFE_STAT(mnt->mnt_fsname, &st);
>> +		dev_major = major(st.st_rdev);
>> +		dev_minor = minor(st.st_rdev);
>> +
>> +		return;
>> +
>> +	} while ((mnt = getmntent(mntf)));
>
> I guess that this should probably go to the test library. We already
> have tst_find_backding_dev() in there which is doing something a bit
> similar. Looking at the code what we do here is to translate a
> mountpoint into a device so it may be something as:
>
> int tst_find_dev_by_mntpoint()

OK, I wasn't sure if to replace tst_find_backing_dev() which doesn't do
what the name suggests to me. The actual device it returns will be some
anonymous virtual block device in many cases. If it is btrfs, then the
"backing device" is probably a real device not an anonymous device
created for the subpartion. OTOH maybe this is what was intended.

>
>> +	tst_brk(TBROK, "Could not find mount device");
>> +}
>> +
>> +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


-- 
Thank you,
Richard.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-03-28 15:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-15 13:41 [LTP] [PATCH 1/2] API/cgroup: Add io controller Richard Palethorpe via ltp
2022-03-15 13:41 ` [LTP] [PATCH 2/2] cgroups: Add first IO controller test Richard Palethorpe via ltp
2022-03-23 17:56   ` Cyril Hrubis
2022-03-28 14:40     ` Richard Palethorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.