[LTP] [PATCH 1/3] diotest: use user specified bufsize correctly

[LTP] [PATCH 1/3] diotest: use user specified bufsize correctly

[Eryu Guan]

In diotestN, option b is used to specify the buffer size used in the
tests, but some tests are overriding this user specified size with
the default value (BUFSIZE).

So always use 'bufsize' once it's initialized by user specified
value.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 testcases/kernel/io/direct_io/diotest2.c |  9 +++++----
 testcases/kernel/io/direct_io/diotest3.c |  1 -
 testcases/kernel/io/direct_io/diotest4.c | 10 +++++-----
 testcases/kernel/io/direct_io/diotest5.c |  2 +-
 testcases/kernel/io/direct_io/diotest6.c |  6 +++---
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/testcases/kernel/io/direct_io/diotest2.c b/testcases/kernel/io/direct_io/diotest2.c
index 15ae932..c32dbeb 100644
--- a/testcases/kernel/io/direct_io/diotest2.c
+++ b/testcases/kernel/io/direct_io/diotest2.c
@@ -69,6 +69,10 @@ int TST_TOTAL = 3;		/* Total number of test conditions */
 #define WRITE_DIRECT 2
 #define RDWR_DIRECT 3
 
+int fd1 = -1;
+char filename[LEN];
+int bufsize = BUFSIZE;
+
 /*
  * runtest: write the data to the file. Read the data from the file and compare.
  *	For each iteration, write data starting at offse+iter*bufsize
@@ -78,7 +82,7 @@ int runtest(int fd_r, int fd_w, int iter, off64_t offset, int action)
 {
 	char *buf1;
 	char *buf2;
-	int i, bufsize = BUFSIZE;
+	int i;
 
 	/* Allocate for buffers */
 	if ((buf1 = valloc(bufsize)) == 0) {
@@ -129,15 +133,12 @@ void prg_usage()
 	exit(1);
 }
 
-int fd1 = -1;
-char filename[LEN];
 static void setup(void);
 static void cleanup(void);
 
 int main(int argc, char *argv[])
 {
 	int iter = 100;		/* Iterations. Default 100 */
-	int bufsize = BUFSIZE;	/* Buffer size. Default 4k */
 	off64_t offset = 0;	/* Offset. Default 0 */
 	int i, action, fd_r, fd_w;
 	int fail_count = 0, total = 0, failed = 0;
diff --git a/testcases/kernel/io/direct_io/diotest3.c b/testcases/kernel/io/direct_io/diotest3.c
index 5e204e7..efbc79d 100644
--- a/testcases/kernel/io/direct_io/diotest3.c
+++ b/testcases/kernel/io/direct_io/diotest3.c
@@ -96,7 +96,6 @@ int runtest(int fd_r, int fd_w, int childnum, int action)
 	char *buf1;
 	char *buf2;
 	off_t seekoff;
-	int bufsize = BUFSIZE;
 	int i;
 
 	/* Allocate for buffers */
diff --git a/testcases/kernel/io/direct_io/diotest4.c b/testcases/kernel/io/direct_io/diotest4.c
index e9a7ac4..d8a3a42 100644
--- a/testcases/kernel/io/direct_io/diotest4.c
+++ b/testcases/kernel/io/direct_io/diotest4.c
@@ -224,19 +224,19 @@ int main(int argc, char *argv[])
 		tst_brkm(TBROK, cleanup, "open failed for %s: %s",
 			 filename, strerror(errno));
 	}
-	if ((buf0 = valloc(BUFSIZE)) == NULL) {
+	if ((buf0 = valloc(bufsize)) == NULL) {
 		tst_brkm(TBROK, cleanup, "valloc() buf0 failed: %s",
 			 strerror(errno));
 	}
 	for (i = 1; i < fblocks; i++) {
-		fillbuf(buf0, BUFSIZE, (char)i);
-		if (write(fd, buf0, BUFSIZE) < 0) {
+		fillbuf(buf0, bufsize, (char)i);
+		if (write(fd, buf0, bufsize) < 0) {
 			tst_brkm(TBROK, cleanup, "write failed for %s: %s",
 				 filename, strerror(errno));
 		}
 	}
 	close(fd);
-	if ((buf2 = valloc(BUFSIZE)) == NULL) {
+	if ((buf2 = valloc(bufsize)) == NULL) {
 		tst_brkm(TBROK, cleanup, "valloc() buf2 failed: %s",
 			 strerror(errno));
 	}
@@ -284,7 +284,7 @@ int main(int argc, char *argv[])
 	total++;
 
 	/* Test-4: Read beyond the file size */
-	offset = BUFSIZE * (fblocks + 10);
+	offset = bufsize * (fblocks + 10);
 	count = bufsize;
 	if (lseek(fd, offset, SEEK_SET) < 0) {
 		tst_resm(TFAIL, "lseek failed: %s", strerror(errno));
diff --git a/testcases/kernel/io/direct_io/diotest5.c b/testcases/kernel/io/direct_io/diotest5.c
index 2c3774a..dc4ecd1 100644
--- a/testcases/kernel/io/direct_io/diotest5.c
+++ b/testcases/kernel/io/direct_io/diotest5.c
@@ -84,7 +84,7 @@ static int fd1 = -1;
 */
 int runtest(int fd_r, int fd_w, int iter, off64_t offset, int action)
 {
-	int i, bufsize = BUFSIZE;
+	int i;
 	struct iovec *iov1, *iov2, *iovp;
 
 	/* Allocate for buffers and data pointers */
diff --git a/testcases/kernel/io/direct_io/diotest6.c b/testcases/kernel/io/direct_io/diotest6.c
index 42e1d34..5594e04 100644
--- a/testcases/kernel/io/direct_io/diotest6.c
+++ b/testcases/kernel/io/direct_io/diotest6.c
@@ -85,11 +85,11 @@ static void prg_usage(void)
 int runtest(int fd_r, int fd_w, int childnum, int action)
 {
 	off64_t seekoff;
-	int i, bufsize = BUFSIZE;
+	int i;
 	char *buf1, *buf2;
 
-	buf1 = valloc(BUFSIZE);
-	buf2 = valloc(BUFSIZE);
+	buf1 = valloc(bufsize);
+	buf2 = valloc(bufsize);
 
 	if (!buf1 || !buf2) {
 		tst_resm(TBROK | TERRNO, "valloc() failed");
-- 
2.9.3

[LTP] [PATCH 2/3] diotest6: test readv/writev not read/write

[Eryu Guan]

According to the comments in diotest6, it's supposed to test
readv/writev syscalls not read/write, which are tested by diotest3.

Update the case to test readv/writev.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 testcases/kernel/io/direct_io/diotest6.c | 77 +++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/testcases/kernel/io/direct_io/diotest6.c b/testcases/kernel/io/direct_io/diotest6.c
index 5594e04..92dd232 100644
--- a/testcases/kernel/io/direct_io/diotest6.c
+++ b/testcases/kernel/io/direct_io/diotest6.c
@@ -85,60 +85,87 @@ static void prg_usage(void)
 int runtest(int fd_r, int fd_w, int childnum, int action)
 {
 	off64_t seekoff;
-	int i;
-	char *buf1, *buf2;
+	int i, ret = -1;
+	struct iovec *iov_r, *iov_w, *iovp;
 
-	buf1 = valloc(bufsize);
-	buf2 = valloc(bufsize);
-
-	if (!buf1 || !buf2) {
-		tst_resm(TBROK | TERRNO, "valloc() failed");
-		free(buf1);
-		free(buf2);
+	/* allocate read/write io vectors */
+	iov_r = calloc(nvector, sizeof(*iov_r));
+	if (!iov_r) {
+		tst_resm(TBROK | TERRNO, "calloc failed for iov_r array");
+		return -1;
+	}
+	iov_w = calloc(nvector, sizeof(*iov_w));
+	if (!iov_w) {
+		tst_resm(TBROK | TERRNO, "calloc failed for iov_w array");
+		free(iov_r);
 		return -1;
 	}
 
-	/* Allocate for buffers and data pointers */
-	seekoff = offset + bufsize * childnum;
+	/* allocate buffers and setup read/write io vectors */
+	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++) {
+		iovp->iov_base = valloc(bufsize);
+		if (!iovp->iov_base) {
+			tst_resm(TBROK | TERRNO, "valloc error iov_r[%d]", i);
+			goto free_out;
+		}
+		iovp->iov_len = bufsize;
+	}
+	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++) {
+		iovp->iov_base = valloc(bufsize);
+		if (!iovp->iov_base) {
+			tst_resm(TBROK | TERRNO, "valloc error iov_w[%d]", i);
+			goto free_out;
+		}
+		iovp->iov_len = bufsize;
+	}
 
 	/* seek, write, read and verify */
+	seekoff = offset + bufsize * childnum * nvector;
 	for (i = 0; i < iter; i++) {
-		fillbuf(buf1, bufsize, childnum+i);
+		vfillbuf(iov_w, nvector, childnum+i);
 
 		if (lseek(fd_w, seekoff, SEEK_SET) < 0) {
 			tst_resm(TFAIL, "lseek before write failed: %s",
 				 strerror(errno));
-			return (-1);
+			goto free_out;
 		}
-		if (write(fd_w, buf1, bufsize) < bufsize) {
-			tst_resm(TFAIL, "write failed: %s", strerror(errno));
-			return (-1);
+		if (writev(fd_w, iov_w, nvector) < 0) {
+			tst_resm(TFAIL, "writev failed: %s", strerror(errno));
+			goto free_out;
 		}
 		if (action == READ_DIRECT) {
 			/* Make sure data is on to disk before read */
 			if (fsync(fd_w) < 0) {
 				tst_resm(TFAIL, "fsync failed: %s",
 					 strerror(errno));
-				return (-1);
+				goto free_out;
 			}
 		}
 		if (lseek(fd_r, seekoff, SEEK_SET) < 0) {
 			tst_resm(TFAIL, "lseek before read failed: %s",
 				 strerror(errno));
-			return (-1);
+			goto free_out;
 		}
-		int ret;
-		if ((ret = read(fd_r, buf2, bufsize)) < bufsize) {
-			tst_resm(TFAIL, "read failed: %s", strerror(errno));
-			return (-1);
+		if (readv(fd_r, iov_r, nvector) < 0) {
+			tst_resm(TFAIL, "readv failed: %s", strerror(errno));
+			goto free_out;
 		}
-		if (bufcmp(buf1, buf2, bufsize) != 0) {
+		if (vbufcmp(iov_w, iov_r, nvector) != 0) {
 			tst_resm(TFAIL, "comparsion failed. Child=%d offset=%d",
 				 childnum, (int)seekoff);
-			return (-1);
+			goto free_out;
 		}
 	}
-	return 0;
+	ret = 0;
+
+free_out:
+	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++)
+		free(iovp->iov_base);
+	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++)
+		free(iovp->iov_base);
+	free(iov_r);
+	free(iov_w);
+	return ret;
 }
 
 /*
-- 
2.9.3

[LTP] [PATCH 2/3] diotest6: test readv/writev not read/write

[Cyril Hrubis]

Hi!
> -	if (!buf1 || !buf2) {
> -		tst_resm(TBROK | TERRNO, "valloc() failed");
> -		free(buf1);
> -		free(buf2);
> +	/* allocate read/write io vectors */
> +	iov_r = calloc(nvector, sizeof(*iov_r));
> +	if (!iov_r) {
> +		tst_resm(TBROK | TERRNO, "calloc failed for iov_r array");
> +		return -1;
> +	}
> +	iov_w = calloc(nvector, sizeof(*iov_w));
> +	if (!iov_w) {
> +		tst_resm(TBROK | TERRNO, "calloc failed for iov_w array");
> +		free(iov_r);
>  		return -1;
>  	}

Doing free(NULL) is no-op. So we can simplify this code by allocating
both arrays at once and then checking if !iow_r || !iow_w just like we
did with the normal buffers.

> -	/* Allocate for buffers and data pointers */
> -	seekoff = offset + bufsize * childnum;
> +	/* allocate buffers and setup read/write io vectors */
> +	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++) {
> +		iovp->iov_base = valloc(bufsize);
> +		if (!iovp->iov_base) {
> +			tst_resm(TBROK | TERRNO, "valloc error iov_r[%d]", i);
> +			goto free_out;
> +		}
> +		iovp->iov_len = bufsize;
> +	}

Uh, why do we increment both iovp and i here?

When we can simply do:

for (i = 0; i < nvector; i++) {
	iovp[i].iov_base = valloc(bufsize);
	...
}

> +	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++) {
> +		iovp->iov_base = valloc(bufsize);
> +		if (!iovp->iov_base) {
> +			tst_resm(TBROK | TERRNO, "valloc error iov_w[%d]", i);
> +			goto free_out;
> +		}
> +		iovp->iov_len = bufsize;
> +	}

Here as well.

>  	/* seek, write, read and verify */
> +	seekoff = offset + bufsize * childnum * nvector;
>  	for (i = 0; i < iter; i++) {
> -		fillbuf(buf1, bufsize, childnum+i);
> +		vfillbuf(iov_w, nvector, childnum+i);
>  
>  		if (lseek(fd_w, seekoff, SEEK_SET) < 0) {
>  			tst_resm(TFAIL, "lseek before write failed: %s",
>  				 strerror(errno));
> -			return (-1);
> +			goto free_out;
>  		}
> -		if (write(fd_w, buf1, bufsize) < bufsize) {
> -			tst_resm(TFAIL, "write failed: %s", strerror(errno));
> -			return (-1);
> +		if (writev(fd_w, iov_w, nvector) < 0) {
> +			tst_resm(TFAIL, "writev failed: %s", strerror(errno));
> +			goto free_out;
>  		}

We have to check here that the whole vector was written as well.

>  		if (action == READ_DIRECT) {
>  			/* Make sure data is on to disk before read */
>  			if (fsync(fd_w) < 0) {
>  				tst_resm(TFAIL, "fsync failed: %s",
>  					 strerror(errno));
> -				return (-1);
> +				goto free_out;
>  			}
>  		}
>  		if (lseek(fd_r, seekoff, SEEK_SET) < 0) {
>  			tst_resm(TFAIL, "lseek before read failed: %s",
>  				 strerror(errno));
> -			return (-1);
> +			goto free_out;
>  		}
> -		int ret;
> -		if ((ret = read(fd_r, buf2, bufsize)) < bufsize) {
> -			tst_resm(TFAIL, "read failed: %s", strerror(errno));
> -			return (-1);
> +		if (readv(fd_r, iov_r, nvector) < 0) {
> +			tst_resm(TFAIL, "readv failed: %s", strerror(errno));
> +			goto free_out;
>  		}

Here as well.

> -		if (bufcmp(buf1, buf2, bufsize) != 0) {
> +		if (vbufcmp(iov_w, iov_r, nvector) != 0) {
>  			tst_resm(TFAIL, "comparsion failed. Child=%d offset=%d",
>  				 childnum, (int)seekoff);
> -			return (-1);
> +			goto free_out;
>  		}
>  	}
> -	return 0;
> +	ret = 0;
> +
> +free_out:

I would call this label err: or just out: but that is minor.

> +	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++)
> +		free(iovp->iov_base);
> +	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++)
> +		free(iovp->iov_base);
> +	free(iov_r);
> +	free(iov_w);
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.9.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/3] diotest6: test readv/writev not read/write

[Eryu Guan]

On Wed, Mar 15, 2017 at 11:31:12AM +0100, Cyril Hrubis wrote:
> Hi!
> > -	if (!buf1 || !buf2) {
> > -		tst_resm(TBROK | TERRNO, "valloc() failed");
> > -		free(buf1);
> > -		free(buf2);
> > +	/* allocate read/write io vectors */
> > +	iov_r = calloc(nvector, sizeof(*iov_r));
> > +	if (!iov_r) {
> > +		tst_resm(TBROK | TERRNO, "calloc failed for iov_r array");
> > +		return -1;
> > +	}
> > +	iov_w = calloc(nvector, sizeof(*iov_w));
> > +	if (!iov_w) {
> > +		tst_resm(TBROK | TERRNO, "calloc failed for iov_w array");
> > +		free(iov_r);
> >  		return -1;
> >  	}
> 
> Doing free(NULL) is no-op. So we can simplify this code by allocating
> both arrays at once and then checking if !iow_r || !iow_w just like we
> did with the normal buffers.

OK.

> 
> > -	/* Allocate for buffers and data pointers */
> > -	seekoff = offset + bufsize * childnum;
> > +	/* allocate buffers and setup read/write io vectors */
> > +	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++) {
> > +		iovp->iov_base = valloc(bufsize);
> > +		if (!iovp->iov_base) {
> > +			tst_resm(TBROK | TERRNO, "valloc error iov_r[%d]", i);
> > +			goto free_out;
> > +		}
> > +		iovp->iov_len = bufsize;
> > +	}
> 
> Uh, why do we increment both iovp and i here?
> 
> When we can simply do:
> 
> for (i = 0; i < nvector; i++) {
> 	iovp[i].iov_base = valloc(bufsize);
> 	...
> }

You're right, no need to increase both iovp and i. I "copied" this
pattern from diotest5.c, and "i" isn't even used in similar loop there.

> 
> > +	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++) {
> > +		iovp->iov_base = valloc(bufsize);
> > +		if (!iovp->iov_base) {
> > +			tst_resm(TBROK | TERRNO, "valloc error iov_w[%d]", i);
> > +			goto free_out;
> > +		}
> > +		iovp->iov_len = bufsize;
> > +	}
> 
> Here as well.
> 
> >  	/* seek, write, read and verify */
> > +	seekoff = offset + bufsize * childnum * nvector;
> >  	for (i = 0; i < iter; i++) {
> > -		fillbuf(buf1, bufsize, childnum+i);
> > +		vfillbuf(iov_w, nvector, childnum+i);
> >  
> >  		if (lseek(fd_w, seekoff, SEEK_SET) < 0) {
> >  			tst_resm(TFAIL, "lseek before write failed: %s",
> >  				 strerror(errno));
> > -			return (-1);
> > +			goto free_out;
> >  		}
> > -		if (write(fd_w, buf1, bufsize) < bufsize) {
> > -			tst_resm(TFAIL, "write failed: %s", strerror(errno));
> > -			return (-1);
> > +		if (writev(fd_w, iov_w, nvector) < 0) {
> > +			tst_resm(TFAIL, "writev failed: %s", strerror(errno));
> > +			goto free_out;
> >  		}
> 
> We have to check here that the whole vector was written as well.

Sure, I noticed that there're other diotests break out if write[v]
didn't write bufsize data.

> 
> >  		if (action == READ_DIRECT) {
> >  			/* Make sure data is on to disk before read */
> >  			if (fsync(fd_w) < 0) {
> >  				tst_resm(TFAIL, "fsync failed: %s",
> >  					 strerror(errno));
> > -				return (-1);
> > +				goto free_out;
> >  			}
> >  		}
> >  		if (lseek(fd_r, seekoff, SEEK_SET) < 0) {
> >  			tst_resm(TFAIL, "lseek before read failed: %s",
> >  				 strerror(errno));
> > -			return (-1);
> > +			goto free_out;
> >  		}
> > -		int ret;
> > -		if ((ret = read(fd_r, buf2, bufsize)) < bufsize) {
> > -			tst_resm(TFAIL, "read failed: %s", strerror(errno));
> > -			return (-1);
> > +		if (readv(fd_r, iov_r, nvector) < 0) {
> > +			tst_resm(TFAIL, "readv failed: %s", strerror(errno));
> > +			goto free_out;
> >  		}
> 
> Here as well.
> 
> > -		if (bufcmp(buf1, buf2, bufsize) != 0) {
> > +		if (vbufcmp(iov_w, iov_r, nvector) != 0) {
> >  			tst_resm(TFAIL, "comparsion failed. Child=%d offset=%d",
> >  				 childnum, (int)seekoff);
> > -			return (-1);
> > +			goto free_out;
> >  		}
> >  	}
> > -	return 0;
> > +	ret = 0;
> > +
> > +free_out:
> 
> I would call this label err: or just out: but that is minor.

OK, I'll rename it to "err".

Thanks for the review!

Eryu

> 
> > +	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++)
> > +		free(iovp->iov_base);
> > +	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++)
> > +		free(iovp->iov_base);
> > +	free(iov_r);
> > +	free(iov_w);
> > +	return ret;
> >  }
> >  
> >  /*
> > -- 
> > 2.9.3
> > 
> > 
> > -- 
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH 3/3] runtest/dio: enable dio29/30

[Eryu Guan]

Not sure why dio29 and dio30 are commented out, I found a bug in
4.10 kernel by running them, see this patch

https://patchwork.kernel.org/patch/9598823/

So let's enable them by default.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 runtest/dio | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/runtest/dio b/runtest/dio
index 97820e9..a26e001 100644
--- a/runtest/dio
+++ b/runtest/dio
@@ -40,8 +40,8 @@ dio27 diotest6 -b 8192 -o 1024000 -i 1000 -v 100
 dio28 diotest6 -b 8192 -o 1024000 -i 1000 -v 200
 
 ### Run the tests with more children
-#dio29 diotest3 -b 65536 -n 100 -i 1000 -o 1024000
-#dio30 diotest6 -b 65536 -n 100 -i 1000 -o 1024000
+dio29 diotest3 -b 65536 -n 100 -i 1000 -o 1024000
+dio30 diotest6 -b 65536 -n 100 -i 1000 -o 1024000
 #
 # RAW DEVICE TEST SECTION
 #   DEV1 and DEV2 should be exported prior to execution or
-- 
2.9.3

[LTP] [PATCH 3/3] runtest/dio: enable dio29/30

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/3] diotest: use user specified bufsize correctly

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz