public inbox for [email protected]
 help / color / mirror / Atom feed
From: Brian Foster <[email protected]>
To: Zorro Lang <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH v4 5/5] fsx: add IO_URING test
Date: Tue, 8 Sep 2020 14:36:25 -0400	[thread overview]
Message-ID: <20200908183625.GE737175@bfoster> (raw)
In-Reply-To: <[email protected]>

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 <[email protected]>
> ---

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 <[email protected]>

> +		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
> 


  reply	other threads:[~2020-09-08 18:36 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-06 17:55 [PATCH v4 0/5] fsstress,fsx: add io_uring test and do some fix Zorro Lang
2020-09-06 17:55 ` [PATCH v4 1/5] fsstress: add IO_URING read and write operations Zorro Lang
2020-09-08 18:34   ` Brian Foster
2020-09-06 17:55 ` [PATCH v4 2/5] fsstress: reduce the number of events when io_setup Zorro Lang
2020-09-08 18:34   ` Brian Foster
2020-09-06 17:55 ` [PATCH v4 3/5] fsstress: fix memory leak in do_aio_rw Zorro Lang
2020-09-08 18:35   ` Brian Foster
2020-09-06 17:55 ` [PATCH v4 4/5] fsx: introduce fsx_rw to combine aio_rw with general read and write Zorro Lang
2020-09-08 18:35   ` Brian Foster
2020-09-06 17:55 ` [PATCH v4 5/5] fsx: add IO_URING test Zorro Lang
2020-09-08 18:36   ` Brian Foster [this message]
2020-09-09  3:58     ` Zorro Lang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200908183625.GE737175@bfoster \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox