Re: [LTP] [PATCH v2] Refactoring aiodio_sparse.c test using LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
Mostly the same comments apply here as well.

> +static void read_sparse(const char *filename, int filesize)
> +{
> +	char buff[4096];
> +	int fd;
> +	int i;
> +	int r;
> +
> +	while ((fd = open(filename, O_RDONLY, 0666)) < 0)
> +		usleep(100);
> +
> +	tst_res(TINFO, "child %i reading file", getpid());
> +
> +	SAFE_LSEEK(fd, SEEK_SET, 0);
> +	while (*run_child) {
> +		off_t offset = 0;
> +		char *bufoff;

The seek() should really be here.

> +		for (i = 0; i < filesize + 1; i += sizeof(buff)) {
> +			r = SAFE_READ(0, fd, buff, sizeof(buff));
> +			if (r > 0) {
> +				bufoff = check_zero(buff, r);
> +				if (bufoff) {
> +					tst_res(TINFO, "non-zero read at offset %zu",
> +						offset + (bufoff - buff));
> +					break;
> +				}
> +				offset += r;
> +			}
> +		}
> +	}
>  
> -#include "common_sparse.h"
> +	SAFE_CLOSE(fd);
> +}

And there is really no point in having more copies of this function.

> -/*
> - * do async DIO writes to a sparse file
> - */
> -int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
> +static void aiodio_sparse(char *filename, int writesize, int filesize, int num_aio)
>  {
> +	int fd;
>  	int i, w;
>  	struct iocb **iocbs;
>  	off_t offset;
> @@ -72,16 +82,15 @@ int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
>  	if ((num_aio * writesize) > filesize)
>  		num_aio = filesize / writesize;
>  
> +	fd = SAFE_OPEN(filename, O_DIRECT | O_WRONLY | O_CREAT, 0666);
> +	SAFE_FTRUNCATE(fd, filesize);
> +
>  	memset(&myctx, 0, sizeof(myctx));
>  	io_queue_init(num_aio, &myctx);
>  
> -	iocbs = malloc(sizeof(struct iocb *) * num_aio);
> -	for (i = 0; i < num_aio; i++) {
> -		if ((iocbs[i] = malloc(sizeof(struct iocb))) == 0) {
> -			tst_resm(TBROK | TERRNO, "malloc()");
> -			return 1;
> -		}
> -	}
> +	iocbs = SAFE_MALLOC(sizeof(struct iocb *) * num_aio);
> +	for (i = 0; i < num_aio; i++)
> +		iocbs[i] = SAFE_MALLOC(sizeof(struct iocb));

Can't we just allocate array of struct iocb and use that instead of the
indirection here?

	struct iocb *iocbs;


	iocbs = SAFE_MALLOC(sizeof(struct iocb) * num_aio);

Then all the difference in the code below is that iocbs[i]->foo becomes
iocbs[i].foo and iocbs[i] becomes &iocbs[i].


>  	/*
>  	 * allocate the iocbs array and iocbs with buffers
> @@ -90,11 +99,7 @@ int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
>  	for (i = 0; i < num_aio; i++) {
>  		void *bufptr;
>  
> -		TEST(posix_memalign(&bufptr, align, writesize));
> -		if (TEST_RETURN) {
> -			tst_resm(TBROK | TRERRNO, "cannot allocate aligned memory");
> -			return 1;
> -		}
> +		bufptr = SAFE_MEMALIGN(getpagesize(), writesize);
>  		memset(bufptr, 0, writesize);
>  		io_prep_pwrite(iocbs[i], fd, bufptr, writesize, offset);
>  		offset += writesize;
> @@ -103,13 +108,9 @@ int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
>  	/*
>  	 * start the 1st num_aio write requests
>  	 */
> -	if ((w = io_submit(myctx, num_aio, iocbs)) < 0) {
> -		tst_resm(TBROK, "io_submit() returned %i", w);
> -		return 1;
> -	}
> -
> -	if (debug)
> -		tst_resm(TINFO, "io_submit() returned %d", 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 AIO until done.
> @@ -120,21 +121,9 @@ int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
>  		int n;
>  		struct iocb *iocbp;
>  
> -		if (debug)
> -			tst_resm(TINFO,
> -				 "aiodio_sparse: offset %p filesize %d inflight %d",
> -				 &offset, filesize, aio_inflight);
> -
> -		if ((n = io_getevents(myctx, 1, 1, &event, 0)) != 1) {
> -			if (-n != EINTR)
> -				tst_resm(TBROK, "io_getevents() returned %d",
> -					 n);
> -			break;
> -		}
> -
> -		if (debug)
> -			tst_resm(TINFO,
> -				 "aiodio_sparse: io_getevent() returned %d", n);
> +		n = io_getevents(myctx, 1, 1, &event, 0);
> +		if (n != 1 && -n != EINTR)
> +			tst_brk(TBROK, "io_getevents: %s", tst_strerrno(-n));
>  
>  		aio_inflight--;
>  
> @@ -142,30 +131,18 @@ int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
>  		 * check if write succeeded.
>  		 */
>  		iocbp = (struct iocb *)event.obj;
> -		if (event.res2 != 0 || event.res != iocbp->u.c.nbytes) {
> -			tst_resm(TBROK,
> -				 "AIO write offset %lld expected %ld got %ld",
> -				 iocbp->u.c.offset, iocbp->u.c.nbytes,
> -				 event.res);
> -			break;
> -		}
> -
> -		if (debug)
> -			tst_resm(TINFO,
> -				 "aiodio_sparse: io_getevent() res %ld res2 %ld",
> -				 event.res, event.res2);
> +		if (event.res2 != 0 || event.res != iocbp->u.c.nbytes)
> +			tst_brk(TBROK,
> +					"AIO write offset %lld expected %ld got %ld",
> +					iocbp->u.c.offset, iocbp->u.c.nbytes,
> +					event.res);

This is a bit strangely formatted, there is no point to start the string
on newline here.


>  		/* start next write */
>  		io_prep_pwrite(iocbp, fd, iocbp->u.c.buf, writesize, offset);
>  		offset += writesize;
> -		if ((w = io_submit(myctx, 1, &iocbp)) < 0) {
> -			tst_resm(TBROK, "io_submit failed at offset %ld",
> -				 offset);
> -			break;
> -		}
> -
> -		if (debug)
> -			tst_resm(TINFO, "io_submit() return %d", w);
> +		w = io_submit(myctx, 1, &iocbp);
> +		if (w < 0)
> +			tst_brk(TBROK, "io_submit: %s", tst_strerrno(-w));
>  
>  		aio_inflight++;
>  	}
> @@ -177,161 +154,67 @@ int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
>  		int n;
>  		struct iocb *iocbp;
>  
> -		if ((n = io_getevents(myctx, 1, 1, &event, 0)) != 1) {
> -			tst_resm(TBROK, "io_getevents failed");
> -			break;
> -		}
> +		n = io_getevents(myctx, 1, 1, &event, 0);
> +		if (n != 1)
> +			tst_brk(TBROK, "io_getevents failed");
> +
>  		aio_inflight--;
> +
>  		/*
>  		 * check if write succeeded.
>  		 */
>  		iocbp = (struct iocb *)event.obj;
> -		if (event.res2 != 0 || event.res != iocbp->u.c.nbytes) {
> -			tst_resm(TBROK,
> -				 "AIO write offset %lld expected %ld got %ld",
> -				 iocbp->u.c.offset, iocbp->u.c.nbytes,
> -				 event.res);
> -		}
> +		if (event.res2 != 0 || event.res != iocbp->u.c.nbytes)
> +			tst_brk(TBROK,
> +					"AIO write offset %lld expected %ld got %ld",
> +					iocbp->u.c.offset, iocbp->u.c.nbytes,
> +					event.res);

Here as well.

Also there is some code repetition, it would probably make the code
clearner if we added a check_result() function to check the event and
maybe submit_write() that would do the io_prep_write(), io_submit() and
adjust the offset.

>  	}
> +}
>  
> -	return 0;
> +static void setup(void)
> +{
> +	run_child = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>  }
>  
> -static void usage(void)
> +static void cleanup(void)
>  {
> -	fprintf(stderr, "usage: dio_sparse [-n children] [-s filesize]"
> -		" [-w writesize]\n");
> -	exit(1);
> +	SAFE_MUNMAP(run_child, sizeof(int));
>  }
>  
> -int main(int argc, char **argv)
> +static void run(void)
>  {
> -	char *filename = "aiodio_sparse";
> -	int pid[NUM_CHILDREN];
> -	int num_children = 1;
> +	int status;
>  	int i;
> -	long alignment = 512;
> -	int writesize = 65536;
> -	int filesize = 100 * 1024 * 1024;
> -	int num_aio = 16;
> -	int children_errors = 0;
> -	int c;
> -	int ret;
> -
> -	while ((c = getopt(argc, argv, "dw:n:a:s:i:")) != -1) {
> -		char *endp;
> -		switch (c) {
> -		case 'd':
> -			debug++;
> -			break;
> -		case 'i':
> -			num_aio = atoi(optarg);
> -			break;
> -		case 'a':
> -			alignment = strtol(optarg, &endp, 0);
> -			alignment = (int)scale_by_kmg((long long)alignment,
> -						      *endp);
> -			break;
> -		case 'w':
> -			writesize = strtol(optarg, &endp, 0);
> -			writesize =
> -			    (int)scale_by_kmg((long long)writesize, *endp);
> -			break;
> -		case 's':
> -			filesize = strtol(optarg, &endp, 0);
> -			filesize =
> -			    (int)scale_by_kmg((long long)filesize, *endp);
> -			break;
> -		case 'n':
> -			num_children = atoi(optarg);
> -			if (num_children > NUM_CHILDREN) {
> -				fprintf(stderr,
> -					"number of children limited to %d\n",
> -					NUM_CHILDREN);
> -				num_children = NUM_CHILDREN;
> -			}
> -			break;
> -		case '?':
> -			usage();
> -			break;
> -		}
> -	}

And here as well, these should be kept as command line test parameters.

> -	setup();
> -	tst_resm(TINFO, "Dirtying free blocks");
> -	dirty_freeblocks(filesize);

Where did this go? I guess that we should put it into the test setup.

> -	fd = SAFE_OPEN(cleanup, filename,
> -		O_DIRECT | O_WRONLY | O_CREAT | O_EXCL, 0600);
> -	SAFE_FTRUNCATE(cleanup, fd, filesize);
> -
> -	tst_resm(TINFO, "Starting I/O tests");
> -	signal(SIGTERM, SIG_DFL);
> -	for (i = 0; i < num_children; i++) {
> -		switch (pid[i] = fork()) {
> -		case 0:
> -			SAFE_CLOSE(NULL, fd);
> -			read_sparse(filename, filesize);
> -			break;
> -		case -1:
> -			while (i-- > 0)
> -				kill(pid[i], SIGTERM);
> -
> -			tst_brkm(TBROK | TERRNO, cleanup, "fork()");
> -		default:
> -			continue;
> -		}
> -	}
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -
> -	ret = aiodio_sparse(fd, alignment, writesize, filesize, num_aio);
> -
> -	tst_resm(TINFO, "Killing childrens(s)");
> -
> -	for (i = 0; i < num_children; i++)
> -		kill(pid[i], SIGTERM);
> +	*run_child = 1;
>  
> -	for (i = 0; i < num_children; i++) {
> -		int status;
> -		pid_t p;
> -
> -		p = waitpid(pid[i], &status, 0);
> -		if (p < 0) {
> -			tst_resm(TBROK | TERRNO, "waitpid()");
> -		} else {
> -			if (WIFEXITED(status) && WEXITSTATUS(status) == 10)
> -				children_errors++;
> +	for (i = 0; i < NUM_CHILDREN; i++) {
> +		if (!SAFE_FORK()) {
> +			read_sparse(FILE_NAME, FILE_SIZE);
> +			return;
>  		}
>  	}
>  
> -	if (children_errors)
> -		tst_resm(TFAIL, "%i children(s) exited abnormally",
> -			 children_errors);
> +	tst_res(TINFO, "Parent create a sparse file");
>  
> -	if (!children_errors && !ret)
> -		tst_resm(TPASS, "Test passed");
> +	aiodio_sparse(FILE_NAME, WRITE_SIZE, FILE_SIZE, NUM_AIO);
>  
> -	cleanup();
> -	tst_exit();
> -}
> +	if (SAFE_WAITPID(-1, &status, WNOHANG))
> +		tst_res(TFAIL, "Non zero bytes read");
> +	else
> +		tst_res(TPASS, "All bytes read were zeroed");
>  
> -static void setup(void)
> -{
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	tst_tmpdir();
> -}
> -
> -static void cleanup(void)
> -{
> -	if (fd > 0 && close(fd))
> -		tst_resm(TWARN | TERRNO, "Failed to close file");
> -
> -	tst_rmdir();
> +	*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)
> -{
> -	tst_brkm(TCONF, NULL, "test requires libaio and it's development packages");
> -}
> +TST_TEST_TCONF("test requires libaio and its development packages");
>  #endif
> -- 
> 2.34.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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