diff mbox series

[v4] Refactoring dio_append.c using LTP API

Message ID 20211213160437.32353-1-andrea.cervesato@suse.com
State Accepted
Headers show
Series [v4] Refactoring dio_append.c using LTP API | expand

Commit Message

Andrea Cervesato Dec. 13, 2021, 4:04 p.m. UTC
Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/io/ltp-aiodio/dio_append.c | 174 +++++++-------------
 1 file changed, 62 insertions(+), 112 deletions(-)

Comments

Petr Vorel Dec. 14, 2021, 3:04 p.m. UTC | #1
Hi Andrea,

Info for myself: this patch requires "Add io_read_eof in common.h utilities" [1]

I wonder what I'm missing:
./dio_append
tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s
dio_append.c:69: TINFO: Parent append to file
common.h:45: TBROK: open(dio_append,16449,0666) failed: EINVAL (22)
common.h:98: TINFO: child 10949 reading file
common.h:98: TINFO: child 10953 reading file
common.h:98: TINFO: child 10940 reading file
common.h:98: TINFO: child 10941 reading file
common.h:98: TINFO: child 10944 reading file
common.h:98: TINFO: child 10946 reading file
common.h:98: TINFO: child 10951 reading file
common.h:98: TINFO: child 10942 reading file
common.h:98: TINFO: child 10948 reading file
common.h:98: TINFO: child 10939 reading file
common.h:98: TINFO: child 10943 reading file
common.h:98: TINFO: child 10950 reading file
common.h:98: TINFO: child 10947 reading file
common.h:98: TINFO: child 10945 reading file
common.h:98: TINFO: child 10952 reading file
common.h:98: TINFO: child 10954 reading file

Summary:
passed   0
failed   0
broken   1
skipped  0
warnings 0

> +++ b/testcases/kernel/io/ltp-aiodio/dio_append.c
...
> +static void run(void)
> +{
> +	char *filename = "dio_append";
nit: having the same name as the binary filename is a bit confusing.
(sure will work even run as ./dio_append due use of .needs_tmpdir).

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20211213121417.21825-1-andrea.cervesato@suse.com/
Cyril Hrubis Dec. 14, 2021, 3:13 p.m. UTC | #2
Hi!
> I wonder what I'm missing:
> ./dio_append
> tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s
> dio_append.c:69: TINFO: Parent append to file
> common.h:45: TBROK: open(dio_append,16449,0666) failed: EINVAL (22)

That's an O_DIRECT open() EINVAL probably means that O_DIRECT is not
supported. Is your /tmp/ on tmpfs?
Petr Vorel Dec. 14, 2021, 4:39 p.m. UTC | #3
> Hi!
> > I wonder what I'm missing:
> > ./dio_append
> > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s
> > dio_append.c:69: TINFO: Parent append to file
> > common.h:45: TBROK: open(dio_append,16449,0666) failed: EINVAL (22)

> That's an O_DIRECT open() EINVAL probably means that O_DIRECT is not
> supported. Is your /tmp/ on tmpfs?
Yes. As we cannot use SAFE_OPEN() in io_read_eof() [1], there should be check
for errno I guess.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20211213121417.21825-1-andrea.cervesato@suse.com/
Cyril Hrubis Dec. 15, 2021, 11:35 a.m. UTC | #4
Hi!
> > > I wonder what I'm missing:
> > > ./dio_append
> > > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s
> > > dio_append.c:69: TINFO: Parent append to file
> > > common.h:45: TBROK: open(dio_append,16449,0666) failed: EINVAL (22)
> 
> > That's an O_DIRECT open() EINVAL probably means that O_DIRECT is not
> > supported. Is your /tmp/ on tmpfs?
> Yes. As we cannot use SAFE_OPEN() in io_read_eof() [1], there should be check
> for errno I guess.

Maybe it would be a bit cleaner to add a check that would attempt to
open a dummy file in the test setup and exit the O_DIRECT tests before
they attempt to fork children...
Petr Vorel Dec. 15, 2021, 11:50 a.m. UTC | #5
Hi,

...
> > > That's an O_DIRECT open() EINVAL probably means that O_DIRECT is not
> > > supported. Is your /tmp/ on tmpfs?
> > Yes. As we cannot use SAFE_OPEN() in io_read_eof() [1], there should be check
> > for errno I guess.

> Maybe it would be a bit cleaner to add a check that would attempt to
> open a dummy file in the test setup and exit the O_DIRECT tests before
> they attempt to fork children...
+1

Kind regards,
Petr
Cyril Hrubis Dec. 15, 2021, 1:11 p.m. UTC | #6
Hi!
Pushed with two minor changes, thanks.

> +/*\
> + * [Description]
>   *
> - */
> -/*
>   * dio_append - append zeroed data to a file using O_DIRECT while
>   *	a 2nd process is doing buffered reads and check if the buffer
>   *	reads always see zero.
>   */

I did rewrite this description to be a bit better.

> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +	.options = (struct tst_option[]) {
> +		{"n:", &str_numchildren, "Number of threads (default 1000)"},
> +		{"w:", &str_writesize, "Write size for each append (default 64K)"},
> +		{"c:", &str_appends, "Number of appends (default 1000)"},

And fixed these descriptions. The description for n: didn't match it at
all. For the rest I've added the switch to the string description as
well, as currently the test library is dumb and just prints the string
as it is.


It would be better to fix the library to print the swich automatically
and then fix all the tests not to include the switches in the string
description. And ideally we would pass a default value in this structure
as well...

But for now the message should include the switches otherwise it's
incomprehensible to the user.

> +		{}
> +	},
> +};
diff mbox series

Patch

diff --git a/testcases/kernel/io/ltp-aiodio/dio_append.c b/testcases/kernel/io/ltp-aiodio/dio_append.c
index b1b4dc039..3b1ddfc40 100644
--- a/testcases/kernel/io/ltp-aiodio/dio_append.c
+++ b/testcases/kernel/io/ltp-aiodio/dio_append.c
@@ -1,143 +1,93 @@ 
-
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2004 Daniel McNeil <daniel@osdl.org>
- *               2004 Open Source Development Lab
- *   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
+ *				 2004 Open Source Development Lab
+ *				 2004  Marty Ridgeway <mridge@us.ibm.com>
+ * Copyright (C) 2021 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
  */
 
-/*
- * Change History:
- *
- * 2/2004  Marty Ridgeway (mridge@us.ibm.com) Changes to adapt to LTP
+/*\
+ * [Description]
  *
- */
-/*
  * dio_append - append zeroed data to a file using O_DIRECT while
  *	a 2nd process is doing buffered reads and check if the buffer
  *	reads always see zero.
  */
 #define _GNU_SOURCE
 
-#include <stdlib.h>
-#include <sys/types.h>
-#include <signal.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <stdio.h>
-#include <unistd.h>
-#include <memory.h>
-#include <limits.h>
-
-#include "test.h"
-#define NUM_CHILDREN 8
+#include "tst_test.h"
+#include "common.h"
 
-#include "common_checkzero.h"
+static int *run_child;
 
-int read_eof(char *filename)
-{
-	int fd;
-	int i;
-	int r;
-	char buf[4096];
+static char *str_numchildren;
+static char *str_writesize;
+static char *str_appends;
 
-	while ((fd = open(filename, O_RDONLY)) < 0) {
-		sleep(1);	/* wait for file to be created */
-	}
-
-	for (i = 0; i < 1000000; i++) {
-		off_t offset;
-		char *bufoff;
-
-		offset = lseek(fd, SEEK_END, 0);
-		r = read(fd, buf, 4096);
-		if (r > 0) {
-			if ((bufoff = check_zero(buf, r))) {
-				fprintf(stderr, "non-zero read at offset %p\n",
-					offset + bufoff);
-				exit(1);
-			}
-		}
-	}
-	return 0;
-}
+static int numchildren;
+static long long writesize;
+static int appends;
 
-void dio_append(char *filename)
+static void setup(void)
 {
-	int fd;
-	void *bufptr = NULL;
-	int i;
-	int w;
+	numchildren = 16;
+	writesize = 64 * 1024;
+	appends = 1000;
 
-	fd = open(filename, O_DIRECT | O_WRONLY | O_CREAT, 0666);
+	if (tst_parse_int(str_numchildren, &numchildren, 1, INT_MAX))
+		tst_brk(TBROK, "Invalid number of children '%s'", str_numchildren);
 
-	if (fd < 0) {
-		perror("cannot create file");
-		return;
-	}
+	if (tst_parse_filesize(str_writesize, &writesize, 1, LLONG_MAX))
+		tst_brk(TBROK, "Invalid write file size '%s'", str_writesize);
 
-	TEST(posix_memalign(&bufptr, 4096, 64 * 1024));
-	if (TEST_RETURN) {
-		tst_resm(TBROK | TRERRNO, "cannot malloc aligned memory");
-		close(fd);
-		return;
-	}
+	if (tst_parse_int(str_appends, &appends, 1, INT_MAX))
+		tst_brk(TBROK, "Invalid number of appends '%s'", str_appends);
 
-	memset(bufptr, 0, 64 * 1024);
-	for (i = 0; i < 1000; i++) {
-		if ((w = write(fd, bufptr, 64 * 1024)) != 64 * 1024) {
-			fprintf(stderr, "write %d returned %d\n", i, w);
-		}
-	}
+	run_child = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 }
 
-int main(void)
+static void cleanup(void)
 {
-	char filename[PATH_MAX];
-	int pid[NUM_CHILDREN];
-	int num_children = 1;
+	SAFE_MUNMAP(run_child, sizeof(int));
+}
+
+static void run(void)
+{
+	char *filename = "dio_append";
+	int status;
 	int i;
 
-	snprintf(filename, sizeof(filename), "%s/aiodio/file",
-		 getenv("TMP") ? getenv("TMP") : "/tmp");
-
-	printf("Begin dio_append test...\n");
-
-	for (i = 0; i < num_children; i++) {
-		if ((pid[i] = fork()) == 0) {
-			/* child */
-			return read_eof(filename);
-		} else if (pid[i] < 0) {
-			/* error */
-			perror("fork error");
-			break;
-		} else {
-			/* Parent */
-			continue;
+	*run_child = 1;
+
+	for (i = 0; i < numchildren; i++) {
+		if (!SAFE_FORK()) {
+			io_read_eof(filename, run_child);
+			return;
 		}
 	}
 
-	/*
-	 * Parent appends to end of file using direct i/o
-	 */
+	tst_res(TINFO, "Parent append to file");
 
-	dio_append(filename);
+	io_append(filename, 0, O_DIRECT | O_WRONLY | O_CREAT, writesize, appends);
 
-	for (i = 0; i < num_children; i++) {
-		kill(pid[i], SIGTERM);
-	}
-	return 0;
+	if (SAFE_WAITPID(-1, &status, WNOHANG))
+		tst_res(TFAIL, "Non zero bytes read");
+	else
+		tst_res(TPASS, "All bytes read were zeroed");
+
+	*run_child = 0;
 }
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.options = (struct tst_option[]) {
+		{"n:", &str_numchildren, "Number of threads (default 1000)"},
+		{"w:", &str_writesize, "Write size for each append (default 64K)"},
+		{"c:", &str_appends, "Number of appends (default 1000)"},
+		{}
+	},
+};