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

[Richard Palethorpe <rpalethorpe@suse.de>]

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