Re: [PATCH v4 5/5] fsx: add IO_URING test

[Brian Foster <bfoster@redhat.com>]

On Mon, Sep 07, 2020 at 01:55:13AM +0800, Zorro Lang wrote:
> New IO_URING test for fsx, use -U option to enable IO_URING test.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---

Just a couple nits...

>  ltp/fsx.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 131 insertions(+), 3 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 92f506ba..e7f23d15 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
...
> @@ -2429,6 +2436,113 @@ aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
...
> +int
> +uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +{
> +	struct io_uring_sqe     *sqe;
> +	struct io_uring_cqe     *cqe;
> +	struct iovec            iovec;
> +	int ret;
> +	int res, res2 = 0;
> +	char *p = buf;
> +	unsigned l = len;
> +	unsigned o = offset;

It looks a little odd that some variable names are aligned with tabs
while others use a single space.. I'm not sure we care one way or
another for xfstests, but perhaps use one approach or the other..?

> +
> +	/*
> +	 * Due to io_uring tries non-blocking IOs (especially read), that
> +	 * always cause 'normal' short reading. To avoid this short read
> +	 * fail, try to loop read/write (escpecilly read) data.
> +	 */
> +	while (l > 0) {
> +		sqe = io_uring_get_sqe(&ring);
> +		if (!sqe) {
> +			fprintf(stderr, "uring_rw: io_uring_get_sqe failed: %s\n",
> +					strerror(errno));
> +			return -1;
> +		}
> +
> +		iovec.iov_base = p;
> +		iovec.iov_len = l;
> +		if (rw == READ) {
> +			io_uring_prep_readv(sqe, fd, &iovec, 1, o);
> +		} else {
> +			io_uring_prep_writev(sqe, fd, &iovec, 1, o);
> +		}
> +
> +		ret = io_uring_submit_and_wait(&ring, 1);
> +		if (ret != 1) {
> +			fprintf(stderr, "errcode=%d\n", -ret);
> +			fprintf(stderr, "uring %s: io_uring_submit failed: %s\n",
> +					rw == READ ? "read":"write", strerror(-ret));
> +			goto uring_error;
> +		}
> +
> +		ret = io_uring_wait_cqe(&ring, &cqe);
> +		if (ret != 0) {
> +			fprintf(stderr, "errcode=%d\n", -ret);
> +			fprintf(stderr, "uring %s: io_uring_wait_cqe failed: %s\n",
> +					rw == READ ? "read":"write", strerror(-ret));
> +			goto uring_error;
> +		}
> +
> +		res = cqe->res;

If we assign ret here instead of res, it looks like we could optimize
away res entirely.

Nits and my limited experience with uring aside, the patch otherwise
LGTM. Are you planning any new tests to take advantage of this,
particularly since it looks like -A (-U disabled) is the default? In any
event, a quick test run of fsstress/fsx doesn't seem to explode:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +		io_uring_cqe_seen(&ring, cqe);
> +
> +		if (res > 0) {
> +			o += res;
> +			l -= res;
> +			p += res;
> +			res2 += res;
> +		} else if (res < 0) {
> +			ret = res;
> +			fprintf(stderr, "errcode=%d\n", -ret);
> +			fprintf(stderr, "uring %s: io_uring failed: %s\n",
> +					rw == READ ? "read":"write", strerror(-ret));
> +			goto uring_error;
> +		} else {
> +			fprintf(stderr, "uring %s bad io length: %d instead of %u\n",
> +					rw == READ ? "read":"write", res2, len);
> +			break;
> +		}
> +	}
> +	return res2;
> +
> + uring_error:
> +	/*
> +	 * The caller expects error return in traditional libc
> +	 * convention, i.e. -1 and the errno set to error.
> +	 */
> +	errno = -ret;
> +	return -1;
> +}
> +#else
> +int
> +uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +{
> +	fprintf(stderr, "io_rw: need IO_URING support!\n");
> +	exit(111);
> +}
> +#endif
> +
>  int
>  fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  {
> @@ -2436,6 +2550,8 @@ fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  
>  	if (aio) {
>  		ret = aio_rw(rw, fd, buf, len, offset);
> +	} else if (uring) {
> +		ret = uring_rw(rw, fd, buf, len, offset);
>  	} else {
>  		if (rw == READ)
>  			ret = read(fd, buf, len);
> @@ -2498,7 +2614,7 @@ main(int argc, char **argv)
>  	setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
>  
>  	while ((ch = getopt_long(argc, argv,
> -				 "b:c:dfg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:WXZ",
> +				 "b:c:dfg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:UWXZ",
>  				 longopts, NULL)) != EOF)
>  		switch (ch) {
>  		case 'b':
> @@ -2606,6 +2722,9 @@ main(int argc, char **argv)
>  		case 'A':
>  			aio = 1;
>  			break;
> +		case 'U':
> +			uring = 1;
> +			break;
>  		case 'D':
>  			debugstart = getnum(optarg, &endp);
>  			if (debugstart < 1)
> @@ -2696,6 +2815,11 @@ main(int argc, char **argv)
>  	if (argc != 1)
>  		usage();
>  
> +	if (aio && uring) {
> +		fprintf(stderr, "-A and -U shouldn't be used together\n");
> +		usage();
> +	}
> +
>  	if (integrity && !dirpath) {
>  		fprintf(stderr, "option -i <logdev> requires -P <dirpath>\n");
>  		usage();
> @@ -2786,6 +2910,10 @@ main(int argc, char **argv)
>  	if (aio) 
>  		aio_setup();
>  #endif
> +#ifdef URING
> +	if (uring)
> +		uring_setup();
> +#endif
>  
>  	if (!(o_flags & O_TRUNC)) {
>  		off_t ret;
> -- 
> 2.20.1
>