[LTP] [PATCH v1] Create dio_read.c test

[LTP] [PATCH v1] Create dio_read.c test

[Andrea Cervesato via ltp]

dio_read.c has been created to replace ltp-diorh.c

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 runtest/ltp-aiodio.part4                   |  10 +-
 testcases/kernel/io/ltp-aiodio/.gitignore  |   2 +-
 testcases/kernel/io/ltp-aiodio/dio_read.c  | 162 +++++++++++++++++++
 testcases/kernel/io/ltp-aiodio/ltp-diorh.c | 176 ---------------------
 4 files changed, 168 insertions(+), 182 deletions(-)
 create mode 100644 testcases/kernel/io/ltp-aiodio/dio_read.c
 delete mode 100644 testcases/kernel/io/ltp-aiodio/ltp-diorh.c

diff --git a/runtest/ltp-aiodio.part4 b/runtest/ltp-aiodio.part4
index ef1cfdac6..88a1b492b 100644
--- a/runtest/ltp-aiodio.part4
+++ b/runtest/ltp-aiodio.part4
@@ -61,8 +61,8 @@ DIT001 dio_truncate
 DIT002 dio_truncate
 #Running read_checkzero
 #gread_checkzero
-#Running ltp-diorh
-DOR000 ltp-diorh $TMPDIR/aiodio.$$/file2
-DOR001 ltp-diorh $TMPDIR/aiodio.$$/file3
-DOR002 ltp-diorh $TMPDIR/aiodio.$$/file4
-DOR003 ltp-diorh $TMPDIR/aiodio.$$/file5
+#Running dio_read
+DOR000 dio_read
+DOR001 dio_read
+DOR002 dio_read
+DOR003 dio_read
diff --git a/testcases/kernel/io/ltp-aiodio/.gitignore b/testcases/kernel/io/ltp-aiodio/.gitignore
index 8da8e946b..f5f20d57e 100644
--- a/testcases/kernel/io/ltp-aiodio/.gitignore
+++ b/testcases/kernel/io/ltp-aiodio/.gitignore
@@ -5,6 +5,6 @@
 /dio_append
 /dio_sparse
 /dio_truncate
+/dio_read
 /dirty
-/ltp-diorh
 /read_checkzero
diff --git a/testcases/kernel/io/ltp-aiodio/dio_read.c b/testcases/kernel/io/ltp-aiodio/dio_read.c
new file mode 100644
index 000000000..aa75329b2
--- /dev/null
+++ b/testcases/kernel/io/ltp-aiodio/dio_read.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *   Copyright (C) 2021 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Create a file using buffered writes while other processes are doing
+ * O_DIRECT reads and check if the buffer reads always see zero.
+ */
+
+#define _GNU_SOURCE
+
+#include <sys/wait.h>
+#include "tst_test.h"
+#include "common.h"
+
+static char *str_numchildren;
+static char *str_writesize;
+static char *str_readsize;
+static char *str_filesize;
+
+static int numchildren;
+static long long readsize;
+static long long writesize;
+static long long filesize;
+static long long alignment;
+static char *iobuf;
+
+static void do_buffered_writes(int fd, char *bufptr, long long fsize, long long wsize, int pattern)
+{
+	long long offset;
+	long long w;
+
+	memset(bufptr, pattern, wsize);
+
+	tst_res(TINFO, "child %i reading file", getpid());
+	for (offset = 0; offset + wsize <= fsize; offset += wsize) {
+		w = pwrite(fd, bufptr, wsize, offset);
+		if (w < 0)
+			tst_brk(TBROK, "pwrite: %s", tst_strerrno(-w));
+		if (w != wsize)
+			tst_brk(TBROK, "pwrite: wrote %lld bytes out of %lld", w, wsize);
+
+		fsync(fd);
+	}
+}
+
+static int do_direct_reads(char *filename, char *bufptr, long long fsize, long long rsize)
+{
+	int fd;
+	long long offset;
+	long long w;
+	int fail = 0;
+
+	while ((fd = open(filename, O_RDONLY | O_DIRECT, 0666)) < 0)
+		usleep(100);
+
+	for (offset = 0; offset + rsize < fsize; offset += rsize) {
+		char *bufoff;
+
+		w = pread(fd, bufptr, rsize, offset);
+		if (w < 0)
+			tst_brk(TBROK, "pread: %s", tst_strerrno(-w));
+		if (w != rsize)
+			tst_brk(TBROK, "pread: read %lld bytes out of %lld", w, rsize);
+
+		bufoff = check_zero(bufptr, rsize);
+		if (bufoff) {
+			fail = 1;
+			break;
+		}
+	}
+
+	SAFE_CLOSE(fd);
+	return fail;
+}
+
+static void setup(void)
+{
+	struct stat sb;
+	long long buffsize;
+
+	numchildren = 100;
+	readsize = 32 * 1024 * 1024;
+	writesize = 32 * 1024 * 1024;
+	filesize = 128 * 1024 * 1024;
+	alignment = 512;
+
+	if (tst_parse_filesize(str_filesize, &filesize, 1, LLONG_MAX))
+		tst_brk(TBROK, "Invalid file size '%s'", str_filesize);
+
+	if (tst_parse_int(str_numchildren, &numchildren, 1, INT_MAX))
+		tst_brk(TBROK, "Invalid number of children '%s'", str_numchildren);
+
+	if (tst_parse_filesize(str_writesize, &writesize, 1, filesize))
+		tst_brk(TBROK, "Invalid write blocks size '%s'", str_writesize);
+
+	if (tst_parse_filesize(str_readsize, &readsize, 1, filesize))
+		tst_brk(TBROK, "Invalid read blocks size '%s'", str_readsize);
+
+	SAFE_STAT(".", &sb);
+	alignment = sb.st_blksize;
+
+	buffsize = readsize;
+	if (writesize > readsize)
+		buffsize = writesize;
+
+	iobuf = SAFE_MEMALIGN(alignment, buffsize);
+}
+
+static void run(void)
+{
+	char *filename = "file.bin";
+	int pid[numchildren];
+	int fd;
+	int i;
+	int fail;
+
+	fd = SAFE_OPEN(filename, O_CREAT | O_TRUNC | O_RDWR, 0666);
+	SAFE_FTRUNCATE(fd, filesize);
+
+	for (i = 0; i < numchildren; i++) {
+		pid[i] = SAFE_FORK();
+		if (!pid[i]) {
+			do_buffered_writes(fd, iobuf, filesize, writesize, 0);
+			return;
+		}
+	}
+
+	fail = do_direct_reads(filename, iobuf, filesize, readsize);
+	for (i = 0; i < numchildren; i++)
+		wait4(pid[i], NULL, WNOHANG, 0);
+
+	/* Fill the file with a known pattern so that the blocks
+	 * on disk can be detected if they become exposed. */
+	do_buffered_writes(fd, iobuf, filesize, writesize, 1);
+	fsync(fd);
+
+	SAFE_FTRUNCATE(fd, 0);
+	fsync(fd);
+
+	if (fail)
+		tst_res(TFAIL, "Non zero bytes read");
+	else
+		tst_res(TPASS, "All bytes read were zeroed");
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.options = (struct tst_option[]) {
+		{"n:", &str_numchildren, "Number of threads (default 100)"},
+		{"w:", &str_writesize, "Size of writing blocks (default 32M)"},
+		{"r:", &str_readsize, "Size of reading blocks (default 32M)"},
+		{"s:", &str_filesize, "File size (default 128M)"},
+		{}
+	},
+};
diff --git a/testcases/kernel/io/ltp-aiodio/ltp-diorh.c b/testcases/kernel/io/ltp-aiodio/ltp-diorh.c
deleted file mode 100644
index 3bdf62388..000000000
--- a/testcases/kernel/io/ltp-aiodio/ltp-diorh.c
+++ /dev/null
@@ -1,176 +0,0 @@
-/*
- *   Copyright (C) 2003,2004 Red Hat, Inc.  All rights reserved.
- *
- *   The contents of this file may be used under the terms of the GNU
- *   General Public License version 2 (the "GPL")
- *
- *   Author: Stephen C. Tweedie <sct@redhat.com>
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- *
- * Module: .c
- */
-
-/*
- * Change History:
- *
- * 2/2004  Marty Ridgeway (mridge@us.ibm.com) Changes to adapt to LTP
- *
- */
-
-#define _XOPEN_SOURCE 600
-#define _GNU_SOURCE
-#define MAX_ITERATIONS 250
-
-#include <unistd.h>
-#include <stdlib.h>
-#include <stdio.h>
-#include <string.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <sys/mman.h>
-#include <sys/wait.h>
-
-#define BIGSIZE 128*1024*1024
-#define READSIZE 32*1024*1024
-#define WRITESIZE 32*1024*1024
-
-int pagesize;
-char *iobuf;
-int pass = 0;
-
-void assert(const char *what, int assertion)
-{
-	if (assertion)
-		return;
-	perror(what);
-	exit(1);
-}
-
-void do_buffered_writes(int fd, int pattern)
-{
-	int rc;
-	int offset;
-
-	memset(iobuf, pattern, WRITESIZE);
-	for (offset = 0; offset + WRITESIZE <= BIGSIZE; offset += WRITESIZE) {
-		rc = pwrite(fd, iobuf, WRITESIZE, offset);
-		assert("pwrite", rc >= 0);
-		if (rc != WRITESIZE) {
-			fprintf(stderr, "Pass %d: short write (%d out of %d)\n",
-				pass, rc, WRITESIZE);
-			exit(1);
-		}
-		fsync(fd);
-	}
-}
-
-int do_direct_reads(char *filename)
-{
-	int fd;
-	int offset;
-	int rc, i;
-	int *p;
-
-	fd = open(filename, O_DIRECT | O_RDONLY, 0);
-	assert("open", fd >= 0);
-
-	for (offset = 0; offset + READSIZE <= BIGSIZE; offset += READSIZE) {
-		rc = pread(fd, iobuf, READSIZE, offset);
-		assert("pread", rc >= 0);
-		if (rc != READSIZE) {
-			fprintf(stderr, "Pass: %d short read (%d out of %d)\n",
-				pass, rc, READSIZE);
-			exit(1);
-		}
-		for (i = 0, p = (int *)iobuf; i < READSIZE; i += 4) {
-			if (*p) {
-				fprintf(stderr,
-					"Pass: %d Found data (%08x) at offset %d+%d\n",
-					pass, *p, offset, i);
-				close(fd);
-				return 1;
-			}
-			p++;
-		}
-	}
-	close(fd);
-	return 0;
-}
-
-int main(int argc, char *argv[])
-{
-	char *filename;
-	int fd;
-	int pid;
-	int err;
-	int bufsize;
-
-	if (argc != 2) {
-		fprintf(stderr, "Needs a filename as an argument.\n");
-		exit(1);
-	}
-
-	filename = argv[1];
-
-	pagesize = getpagesize();
-	bufsize = READSIZE;
-	if (WRITESIZE > READSIZE)
-		bufsize = WRITESIZE;
-	err = posix_memalign((void **)&iobuf, pagesize, bufsize);
-	if (err) {
-		fprintf(stderr, "Error allocating %d aligned bytes.\n",
-			bufsize);
-		exit(1);
-	}
-
-	fd = open(filename, O_CREAT | O_TRUNC | O_RDWR, 0666);
-	assert("open", fd >= 0);
-
-	do {
-
-		assert("ftruncate", ftruncate(fd, BIGSIZE) == 0);
-		fsync(fd);
-
-		pid = fork();
-		assert("fork", pid >= 0);
-
-		if (!pid) {
-			do_buffered_writes(fd, 0);
-			exit(0);
-		}
-
-		err = do_direct_reads(filename);
-
-		wait4(pid, NULL, WNOHANG, 0);
-
-		if (err)
-			break;
-
-		/* Fill the file with a known pattern so that the blocks
-		 * on disk can be detected if they become exposed. */
-		do_buffered_writes(fd, 1);
-		fsync(fd);
-
-		assert("ftruncate", ftruncate(fd, 0) == 0);
-		fsync(fd);
-	} while (pass++ < MAX_ITERATIONS);
-
-	if (!err) {
-		fprintf(stdout, "ltp-diorh: Completed %d iterations OK \n",
-			pass);
-	}
-
-	return err;
-}
-- 
2.34.1


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

Re: [LTP] [PATCH v1] Create dio_read.c test

[Cyril Hrubis]

Hi!
> --- /dev/null
> +++ b/testcases/kernel/io/ltp-aiodio/dio_read.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *   Copyright (C) 2021 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Create a file using buffered writes while other processes are doing
> + * O_DIRECT reads and check if the buffer reads always see zero.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <sys/wait.h>
> +#include "tst_test.h"
> +#include "common.h"
> +
> +static char *str_numchildren;
> +static char *str_writesize;
> +static char *str_readsize;
> +static char *str_filesize;
> +
> +static int numchildren;
> +static long long readsize;
> +static long long writesize;
> +static long long filesize;
> +static long long alignment;

Can we please initialize these variables here instead of the setup?

Also there is no need to initialize alignment since it's initialized
unconditionally.

> +static char *iobuf;
> +
> +static void do_buffered_writes(int fd, char *bufptr, long long fsize, long long wsize, int pattern)
> +{
> +	long long offset;
> +	long long w;
> +
> +	memset(bufptr, pattern, wsize);
> +
> +	tst_res(TINFO, "child %i reading file", getpid());
> +	for (offset = 0; offset + wsize <= fsize; offset += wsize) {
> +		w = pwrite(fd, bufptr, wsize, offset);
> +		if (w < 0)
> +			tst_brk(TBROK, "pwrite: %s", tst_strerrno(-w));
> +		if (w != wsize)
> +			tst_brk(TBROK, "pwrite: wrote %lld bytes out of %lld", w, wsize);
> +
> +		fsync(fd);
> +	}
> +}

The biggest problem I do see here is that unlike the other test we have
rewritten the children are not synchronized with parent here.

It mostly works for the defaults because we set the write size to be
equal to the read size. But for instance if we set write size 10x
smaller than the read size the parent that reads the data would finish
much sooner than the children and the children would be writing while
nobody is reading. And the other way around, if the reads are small the
writes would finish early.

I guess that the best strategy would be to change the children to loop
until the parent is reading so that we do not waste any time writing
when noone is reading and the other way around.

> +static int do_direct_reads(char *filename, char *bufptr, long long fsize, long long rsize)
> +{
> +	int fd;
> +	long long offset;
> +	long long w;
> +	int fail = 0;
> +
> +	while ((fd = open(filename, O_RDONLY | O_DIRECT, 0666)) < 0)
> +		usleep(100);
> +
> +	for (offset = 0; offset + rsize < fsize; offset += rsize) {
> +		char *bufoff;
> +
> +		w = pread(fd, bufptr, rsize, offset);
> +		if (w < 0)
> +			tst_brk(TBROK, "pread: %s", tst_strerrno(-w));
> +		if (w != rsize)
> +			tst_brk(TBROK, "pread: read %lld bytes out of %lld", w, rsize);
> +
> +		bufoff = check_zero(bufptr, rsize);
> +		if (bufoff) {
> +			fail = 1;
> +			break;
> +		}
> +	}
> +
> +	SAFE_CLOSE(fd);
> +	return fail;
> +}
> +
> +static void setup(void)
> +{
> +	struct stat sb;
> +	long long buffsize;
> +
> +	numchildren = 100;
> +	readsize = 32 * 1024 * 1024;
> +	writesize = 32 * 1024 * 1024;
> +	filesize = 128 * 1024 * 1024;
> +	alignment = 512;
> +
> +	if (tst_parse_filesize(str_filesize, &filesize, 1, LLONG_MAX))
> +		tst_brk(TBROK, "Invalid file size '%s'", str_filesize);
> +
> +	if (tst_parse_int(str_numchildren, &numchildren, 1, INT_MAX))
> +		tst_brk(TBROK, "Invalid number of children '%s'", str_numchildren);
> +
> +	if (tst_parse_filesize(str_writesize, &writesize, 1, filesize))
> +		tst_brk(TBROK, "Invalid write blocks size '%s'", str_writesize);
> +
> +	if (tst_parse_filesize(str_readsize, &readsize, 1, filesize))
> +		tst_brk(TBROK, "Invalid read blocks size '%s'", str_readsize);
> +
> +	SAFE_STAT(".", &sb);
> +	alignment = sb.st_blksize;
> +
> +	buffsize = readsize;
> +	if (writesize > readsize)
> +		buffsize = writesize;
> +
> +	iobuf = SAFE_MEMALIGN(alignment, buffsize);
> +}
> +
> +static void run(void)
> +{
> +	char *filename = "file.bin";
> +	int pid[numchildren];
> +	int fd;
> +	int i;
> +	int fail;
> +
> +	fd = SAFE_OPEN(filename, O_CREAT | O_TRUNC | O_RDWR, 0666);
> +	SAFE_FTRUNCATE(fd, filesize);
> +
> +	for (i = 0; i < numchildren; i++) {
> +		pid[i] = SAFE_FORK();
> +		if (!pid[i]) {
> +			do_buffered_writes(fd, iobuf, filesize, writesize, 0);
> +			return;
> +		}
> +	}
> +
> +	fail = do_direct_reads(filename, iobuf, filesize, readsize);
> +	for (i = 0; i < numchildren; i++)
> +		wait4(pid[i], NULL, WNOHANG, 0);

Just use SAFE_WAIT() and get rid of the pid[] array. As long as we know
how many children we want to wait for we don't have to store the pids...

> +	/* Fill the file with a known pattern so that the blocks
> +	 * on disk can be detected if they become exposed. */
> +	do_buffered_writes(fd, iobuf, filesize, writesize, 1);
> +	fsync(fd);
> +	SAFE_FTRUNCATE(fd, 0);
> +	fsync(fd);

Actually this part has to be done before the test, not after it. So this
should be just after SAFE_OPEN().

Also this should use SAFE_FSYNC().

> +	if (fail)
> +		tst_res(TFAIL, "Non zero bytes read");
> +	else
> +		tst_res(TPASS, "All bytes read were zeroed");

Also either we move the SAFE_OPEN() into the test setup() and add
SAFE_CLOSE() to a test cleanup() or we have to close the fd here.

Remmeber the run() may be called repeatedly with either of -i and -I and
without being closed this function will leak file descriptor and with
large enough number of iterations it will hit ulimit.

> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +	.options = (struct tst_option[]) {
> +		{"n:", &str_numchildren, "Number of threads (default 100)"},
> +		{"w:", &str_writesize, "Size of writing blocks (default 32M)"},
> +		{"r:", &str_readsize, "Size of reading blocks (default 32M)"},
> +		{"s:", &str_filesize, "File size (default 128M)"},
> +		{}
> +	},
> +};

...

> -	fd = open(filename, O_CREAT | O_TRUNC | O_RDWR, 0666);
> -	assert("open", fd >= 0);
> -
> -	do {
> -
> -		assert("ftruncate", ftruncate(fd, BIGSIZE) == 0);
> -		fsync(fd);
> -
> -		pid = fork();
> -		assert("fork", pid >= 0);
> -
> -		if (!pid) {
> -			do_buffered_writes(fd, 0);
> -			exit(0);
> -		}
> -
> -		err = do_direct_reads(filename);
> -
> -		wait4(pid, NULL, WNOHANG, 0);
> -
> -		if (err)
> -			break;
> -
> -		/* Fill the file with a known pattern so that the blocks
> -		 * on disk can be detected if they become exposed. */
> -		do_buffered_writes(fd, 1);
> -		fsync(fd);
> -
> -		assert("ftruncate", ftruncate(fd, 0) == 0);
> -		fsync(fd);
> -	} while (pass++ < MAX_ITERATIONS);

Note that the old test did 250 iterations and took for about a minute or
two on my machine. Since we start 100 children writing single iteration
takes much longer but still we can do about 10 iterations and have
roughtly the same runtime. So when you are changing the runtest file can
you please add a few different variants where the number of children and
number of iterations would be combined so that they result in similar
runtime?

For me two different entries would look like:

dio_read -n 1 -i 250
dio_read -n 100 -i 10

-- 
Cyril Hrubis
chrubis@suse.cz

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