[v1] Refactoring aiodio_append.c using LTP API

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 .../kernel/io/ltp-aiodio/aiodio_append.c      | 192 +++++++++---------
 1 file changed, 95 insertions(+), 97 deletions(-)

Hi Andrea,

...
> +static void run(void)
> +{
> +	char *filename = "file";
You decided to remove file as first parameter.
That's fine ($TMPDIR/aiodio.$$/file2 as parameter used $$ to get unique
directory, but new API uses unique directory anyway), but then you should update
runtest/ltp-aiodio.part4 - remove $TMPDIR/aiodio.$$/file2.
nit: I'd just define that name with preprocessor #define.

> +	int filesize = FILE_SIZE;
> +	int num_children = NUM_CHILDREN;
> +	int status;
>  	int i;
> -	char *filename = argv[1];

> -	printf("Starting aio/dio append test...\n");
> +	*run_child = 1;

>  	for (i = 0; i < num_children; i++) {
nit: I'd use directly NUM_CHILDREN
> -		if ((pid[i] = fork()) == 0) {
> -			/* child */
> -			return read_eof(filename);
> -		} else if (pid[i] < 0) {
> -			/* error */
> -			perror("fork error");
> -			break;
> -		} else {
> -			/* Parent */
> -			continue;
> +		if (!SAFE_FORK()) {
> +			read_eof(filename, filesize);
... and here FILE_SIZE
> +			return;
>  		}
>  	}
...

> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +};
>  #else
> -int main(void)
> +static void run(void)
>  {
> -	tst_brkm(TCONF, NULL, "test requires libaio and it's development packages");
> +	tst_res(TCONF, "test requires libaio and it's development packages");
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +};
>  #endif

New API has shorter variant when need to quit due missing dependencies:
TST_TEST_TCONF(), e.g.:

#else
	TST_TEST_TCONF("test requires libaio and it's development packages");
#endif

Kind regards,
Petr

Hi!
> -int read_eof(char *filename)
> +static void read_eof(const char *filename, size_t bs)
>  {
>  	int fd;
> -	int i;
>  	int r;
> -	char buf[4096];
> +	char *bufptr;
>  
> -	while ((fd = open(filename, O_RDONLY)) < 0) {
> -		sleep(1);	/* wait for file to be created */
> -	}
> +	while ((fd = open(filename, O_RDONLY, 0666)) < 0)
> +		usleep(100);
> +
> +	bufptr = SAFE_MEMALIGN(getpagesize(), bs);
>  
> -	for (i = 0; i < 1000000; i++) {
> +	tst_res(TINFO, "child %i reading file", getpid());
> +	while (*run_child) {
>  		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);
> +		offset = SAFE_LSEEK(fd, 0, SEEK_END);
> +		do {
> +			r = SAFE_READ(0, fd, bufptr, bs);
> +			if (r > 0) {
> +				bufoff = check_zero(bufptr, r);
> +				if (bufoff) {
> +					tst_res(TINFO, "non-zero read at offset %zu",
> +						offset + (bufoff - bufptr));
> +					free(bufptr);
> +					SAFE_CLOSE(fd);
> +					return;
> +				}
> +				offset += r;
>  			}
> -		}
> +		} while (r > 0);
>  	}
> -	return 0;
> -}
>  
> -#define NUM_AIO 16
> -#define AIO_SIZE 64*1024
> +	free(bufptr);
> +	SAFE_CLOSE(fd);
> +}

This looks like another version of the function that reads a file in a
loop until told to stop. Can we please have a single instance of that in
a header?

>  /*
>   * append to the end of a file using AIO DIRECT.
>   */
> -void aiodio_append(char *filename)
> +static void aiodio_append(char *filename, int bcount)
>  {
>  	int fd;
>  	void *bufptr;
> @@ -91,21 +84,13 @@ void aiodio_append(char *filename)
>  	struct io_event event;
>  	struct timespec timeout;
>  
> -	fd = open(filename, O_DIRECT | O_WRONLY | O_CREAT, 0666);
> -	if (fd < 0) {
> -		perror("cannot create file");
> -		return;
> -	}
> +	fd = SAFE_OPEN(filename, O_DIRECT | O_WRONLY | O_CREAT, 0666);
>  
>  	memset(&myctx, 0, sizeof(myctx));
>  	io_queue_init(NUM_AIO, &myctx);
>  
>  	for (i = 0; i < NUM_AIO; i++) {
> -		TEST(posix_memalign(&bufptr, 4096, AIO_SIZE));
> -		if (TEST_RETURN) {
> -			tst_resm(TBROK | TRERRNO, "cannot malloc aligned memory");
> -			return;
> -		}
> +		bufptr = SAFE_MEMALIGN(getpagesize(), AIO_SIZE);
>  		memset(bufptr, 0, AIO_SIZE);
>  		io_prep_pwrite(&iocb_array[i], fd, bufptr, AIO_SIZE, offset);
>  		iocbs[i] = &iocb_array[i];
> @@ -115,14 +100,14 @@ void aiodio_append(char *filename)
>  	/*
>  	 * Start the 1st NUM_AIO requests
>  	 */
> -	if ((w = io_submit(myctx, NUM_AIO, iocbs)) < 0) {
> -		fprintf(stderr, "io_submit write returned %d\n", w);
> -	}
> +	w = io_submit(myctx, NUM_AIO, iocbs);
> +	if (w < 0)
> +		tst_brk(TBROK, "io_submit: %s", tst_strerrno(-w));
>  
>  	/*
>  	 * As AIO requests finish, keep issuing more AIOs.
>  	 */
> -	for (; i < 1000; i++) {
> +	for (; i < bcount; i++) {
>  		int n = 0;
>  		struct iocb *iocbp;
>  
> @@ -131,56 +116,69 @@ void aiodio_append(char *filename)
>  			iocbp = (struct iocb *)event.obj;
>  
>  			if (n > 0) {
> -				io_prep_pwrite(iocbp, fd, iocbp->u.c.buf,
> -					       AIO_SIZE, offset);
> +				io_prep_pwrite(iocbp, fd, iocbp->u.c.buf, AIO_SIZE, offset);
>  				offset += AIO_SIZE;
> -				if ((w = io_submit(myctx, 1, &iocbp)) < 0) {
> -					fprintf(stderr,
> -						"write %d returned %d\n", i, w);
> -				}
> +				w = io_submit(myctx, 1, &iocbp);
> +				if (w < 0)
> +					tst_brk(TBROK, "io_submit: %s", tst_strerrno(-w));
>  			}
>  		}
>  	}
>  }
>  
> -int main(int argc, char **argv)
> +static void setup(void)
> +{
> +	run_child = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +}
> +
> +static void cleanup(void)
>  {
> -	int pid[NUM_CHILDREN];
> -	int num_children = 1;
> +	SAFE_MUNMAP(run_child, sizeof(int));
> +}
> +
> +static void run(void)
> +{
> +	char *filename = "file";
> +	int filesize = FILE_SIZE;
> +	int num_children = NUM_CHILDREN;
> +	int status;
>  	int i;
> -	char *filename = argv[1];
>  
> -	printf("Starting aio/dio append test...\n");
> +	*run_child = 1;
>  
>  	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;
> +		if (!SAFE_FORK()) {
> +			read_eof(filename, filesize);
> +			return;
>  		}
>  	}
>  
> -	/*
> -	 * Parent appends to end of file using direct i/o
> -	 */
> +	tst_res(TINFO, "Parent append to file");
>  
> -	aiodio_append(filename);
> +	aiodio_append(filename, 1000);
>  
> -	for (i = 0; i < num_children; i++) {
> -		kill(pid[i], SIGTERM);
> -	}
> +	if (SAFE_WAITPID(-1, &status, WNOHANG))
> +		tst_res(TFAIL, "Non zero bytes read");
> +	else
> +		tst_res(TPASS, "All bytes read were zeroed");
>  
> -	return 0;
> +	*run_child = 0;
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +};
>  #else
> -int main(void)
> +static void run(void)
>  {
> -	tst_brkm(TCONF, NULL, "test requires libaio and it's development packages");
> +	tst_res(TCONF, "test requires libaio and it's development packages");
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +};

We do have a shorthand macro for this:

TST_TEST_TCONF("test requires libaio and it's development packages");

>  #endif
> -- 
> 2.34.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp