[PATCH v2 0/5] random fixes for fstests

[PATCH v2 0/5] random fixes for fstests

[Zorro Lang]

V2 did below changes:

[PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio
Improve the code change in _require_odirect()

[PATCH v2 2/5] generic/506: call _require_quota before _qmount
Nothing changed

[PATCH v2 3/5] generic/591: remove redundant output from golden image
Replace fflush(stdout) with setlinebuf(stdout). Thanks the review from Dave.

[PATCH v2 4/5] generic/591: use proper sector size
Remove bad binary files commit, and deal with them in [5/5].

[PATCH v2 5/5] gitignore: ignore missed binary files in src
Add two binary files to .gitignore file.

Removed original "[PATCH 5/5] generic/623: add overlay into the blacklist",
Amir would like to use another patch to deal with it.

=== Messages from orignal V1 ===
These 5 patches are random fixes, except patch 4/5 bases on 3/5.

 - [PATCH 1/5] generic/139: require 512 bytes to be the minimum dio size
I tried to help it support 4096 sector size, but it's not simple, especially
the golden image broken. So I'm thinking how about limit it in 512 bytes dio
testing.

 - [PATCH 2/5] generic/506: call _require_quota before _qmount
As subject

 - [PATCH 3/5] generic/591: remove redundant output from golden image
I think I found where's the issue from, but I'm not sure if it's the best
way to fix it. So welcome suggestions :)

 - [PATCH 4/5] generic/591: use proper sector size
Help splice-test to support a sector size option

 - [PATCH 5/5] generic/623: add overlay into the blacklist
I think it's simple to exclude overlay from this test directly, or we have to
check if $FSTYP=overlay, then think about how to deal with OVL_BASE_SCRATCH_DEV
things.

Thanks,
Zorro

[PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size

[Zorro Lang]

Due to generic/139 tests base on 512 bytes aligned, so skip this test
if the minimum dio write size >512. This patch also change the
common/rc::_require_dio helper, supports a minimum aligned size
argument.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 common/rc         | 17 ++++++++++++++---
 tests/generic/139 |  2 +-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index 2f31ca46..6d384eaf 100644
--- a/common/rc
+++ b/common/rc
@@ -2721,9 +2721,12 @@ _require_xfs_io_command()
 	fi
 }
 
-# check that kernel and filesystem support direct I/O
+# check that kernel and filesystem support direct I/O, and check if "$1" size
+# aligned (optional) is supported
 _require_odirect()
 {
+	local alignment=${1:+"-b $1"}
+
 	if [ $FSTYP = "ext4" ] || [ $FSTYP = "f2fs" ] ; then
 		if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
 			_notrun "$FSTYP encryption doesn't support O_DIRECT"
@@ -2735,9 +2738,17 @@ _require_odirect()
 		fi
 	fi
 	local testfile=$TEST_DIR/$$.direct
-	$XFS_IO_PROG -F -f -d -c "pwrite 0 20k" $testfile > /dev/null 2>&1
+	local opt
+	if [ -n "$1" ];then
+		opt="-b $1"
+	fi
+	$XFS_IO_PROG -F -f -d -c "pwrite $opt 0 20k" $testfile > /dev/null 2>&1
 	if [ $? -ne 0 ]; then
-		_notrun "O_DIRECT is not supported"
+		if [ -n "$alignment" ]; then
+			_notrun "O_DIRECT aligned to $1 bytes is not supported"
+		else
+			_notrun "O_DIRECT is not supported"
+		fi
 	fi
 	rm -f $testfile 2>&1 > /dev/null
 }
diff --git a/tests/generic/139 b/tests/generic/139
index 0bbc222c..3eb1519d 100755
--- a/tests/generic/139
+++ b/tests/generic/139
@@ -26,7 +26,7 @@ _cleanup()
 # real QA test starts here
 _require_test_reflink
 _require_cp_reflink
-_require_odirect
+_require_odirect 512
 
 testdir=$TEST_DIR/test-$seq
 rm -rf $testdir
-- 
2.31.1

[PATCH v2 2/5] generic/506: call _require_quota before _qmount

[Zorro Lang]

The g/506 fails on some filesystems (e.g. overlay) which doesn't
support prjquota:

  MOUNT_OPTIONS =  -o prjquota
  qmount failed

To avoid this failure, call _require_quota before doing real quota
mount.

Signed-off-by: Zorro Lang <zlang@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 tests/generic/506 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/generic/506 b/tests/generic/506
index 25a5b0f8..ec91af78 100755
--- a/tests/generic/506
+++ b/tests/generic/506
@@ -27,6 +27,7 @@ _begin_fstest shutdown auto quick metadata quota
 _supported_fs generic
 
 _require_scratch
+_require_quota
 _require_scratch_shutdown
 
 _scratch_mkfs >/dev/null 2>&1
-- 
2.31.1

[PATCH v2 3/5] generic/591: remove redundant output from golden image

[Zorro Lang]

In generic/591.out expects below output:
  concurrent reader with O_DIRECT
  concurrent reader with O_DIRECT     <=== ???
  concurrent reader without O_DIRECT
  concurrent reader without O_DIRECT  <=== ???
  sequential reader with O_DIRECT
  sequential reader without O_DIRECT

The lines marked "???" are unbelievable, due to the src/splice-test.c
only calls printf to output that message once in main function. So
Why splice-test prints that message twice sometimes? It seems related
with the "-r" option, due to the test lines without "-r" option only
print one line each time running.

A stanger thing is this "double output" issue only can be triggered by
running g/591, can't reproduce it by running splice-test manually.

By checking the code of splice-test.c, I found a "fork()" in it, and
it'll be called if the '-r' option is specified. So I suspect the
redundant output come from the child process. By the help of strace
tool, I got:

  10554 execve("/root/git/xfstests/src/splice-test", ["/root/git/xfstests/src/splice-te"..., "-r", "/mnt/test/a"], 0x7ffcabc2c0a8 /* 202 vars */) = 0
  ...
  10554 clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f937f5d5a10) = 10555
  ...
  10555 read(4, "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"..., 512) = 512
  10555 write(1, "concurrent reader with O_DIRECT\n", 32) = 32
  10555 exit_group(0)                     = ?
  10555 +++ exited with 0 +++
  10554 <... wait4 resumed>NULL, 0, NULL) = 10555
  10554 --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=10555, si_uid=0, si_status=0, si_utime=0, si_stime=1} ---
  10554 unlink("/mnt/test/a")             = 0
  10554 write(1, "concurrent reader with O_DIRECT\n", 32) = 32
  10554 exit_group(0)                     = ?
  10554 +++ exited with 0 +++

We can see the "concurrent reader with O_DIRECT\n" be printed by
parent process 10554 and child process 10555 separately.

Due to the stdout redirection that fstests does cause the stream
doesn't refer to a tty anymore, then the stdout become block
buffered, so the '\n' doesn't help to flush that printf message,
and the child print it again.

So use setlinebuf(stdout) to force it line buffered, to avoid the
confused output to be golden image. Then correct the generic/591.out

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 src/splice-test.c     | 2 ++
 tests/generic/591.out | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/splice-test.c b/src/splice-test.c
index 2f1ba2ba..dc41b0f5 100644
--- a/src/splice-test.c
+++ b/src/splice-test.c
@@ -140,6 +140,8 @@ int main(int argc, char *argv[])
 		usage(argv[0]);
 	filename = argv[optind];
 
+	/* force below printf line buffered */
+	setlinebuf(stdout);
 	printf("%s reader %s O_DIRECT\n",
 		   do_splice == do_splice1 ? "sequential" : "concurrent",
 		   (open_flags & O_DIRECT) ? "with" : "without");
diff --git a/tests/generic/591.out b/tests/generic/591.out
index d61811ee..e9fffd1d 100644
--- a/tests/generic/591.out
+++ b/tests/generic/591.out
@@ -1,7 +1,5 @@
 QA output created by 591
 concurrent reader with O_DIRECT
-concurrent reader with O_DIRECT
-concurrent reader without O_DIRECT
 concurrent reader without O_DIRECT
 sequential reader with O_DIRECT
 sequential reader without O_DIRECT
-- 
2.31.1

[PATCH v2 4/5] generic/591: use proper sector size

[Zorro Lang]

The generic/591 fails if the sector size of TEST_DEV isn't 512:

  splice-test: write: /mnt/test/a: Invalid argument

To fix this issue, this patch help src/splice-test.c to get a specify
sector size from the test case. Then let g/591 give it a proper
sector size which dio aligned.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 src/splice-test.c | 37 +++++++++++++++++++++++++------------
 tests/generic/591 |  6 ++++--
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/src/splice-test.c b/src/splice-test.c
index dc41b0f5..eb863673 100644
--- a/src/splice-test.c
+++ b/src/splice-test.c
@@ -19,19 +19,20 @@
 #include <errno.h>
 #include <malloc.h>
 
-#define SECTOR_SIZE 512
-#define BUFFER_SIZE (150 * SECTOR_SIZE)
+unsigned int sector_size;
+unsigned int buffer_size;
 
 void read_from_pipe(int fd, const char *filename, size_t size)
 {
-	char buffer[SECTOR_SIZE];
+	char *buffer;
 	size_t sz;
 	ssize_t ret;
+	buffer = malloc(buffer_size);
 
 	while (size) {
 		sz = size;
-		if (sz > sizeof buffer)
-			sz = sizeof buffer;
+		if (sz > buffer_size)
+			sz = buffer_size;
 		ret = read(fd, buffer, sz);
 		if (ret < 0)
 			err(1, "read: %s", filename);
@@ -41,6 +42,7 @@ void read_from_pipe(int fd, const char *filename, size_t size)
 		}
 		size -= sz;
 	}
+	free(buffer);
 }
 
 void do_splice1(int fd, const char *filename, size_t size)
@@ -108,7 +110,7 @@ void do_splice2(int fd, const char *filename, size_t size)
 
 void usage(const char *argv0)
 {
-	fprintf(stderr, "USAGE: %s [-rd] {filename}\n", basename(argv0));
+	fprintf(stderr, "USAGE: %s [-rd] [-s sectorsize] {filename}\n", basename(argv0));
 	exit(2);
 }
 
@@ -120,11 +122,22 @@ int main(int argc, char *argv[])
 	int opt, open_flags, fd;
 	ssize_t ret;
 
+	/*
+	 * init default sector_size and buffer_size, might be changed if the -s
+	 * option is specified
+	 */
+	sector_size = 512;
+	buffer_size = 150 * sector_size;
+
 	do_splice = do_splice1;
 	open_flags = O_CREAT | O_TRUNC | O_RDWR | O_DIRECT;
 
-	while ((opt = getopt(argc, argv, "rd")) != -1) {
+	while ((opt = getopt(argc, argv, "rds:")) != -1) {
 		switch(opt) {
+		case 's':
+			sector_size = strtol(optarg, NULL, 0);
+			buffer_size = 150 * sector_size;
+			break;
 		case 'r':
 			do_splice = do_splice2;
 			break;
@@ -146,7 +159,7 @@ int main(int argc, char *argv[])
 		   do_splice == do_splice1 ? "sequential" : "concurrent",
 		   (open_flags & O_DIRECT) ? "with" : "without");
 
-	buffer = memalign(SECTOR_SIZE, BUFFER_SIZE);
+	buffer = memalign(sector_size, buffer_size);
 	if (buffer == NULL)
 		err(1, "memalign");
 
@@ -154,11 +167,11 @@ int main(int argc, char *argv[])
 	if (fd == -1)
 		err(1, "open: %s", filename);
 
-	memset(buffer, 'x', BUFFER_SIZE);
-	ret = write(fd, buffer, BUFFER_SIZE);
+	memset(buffer, 'x', buffer_size);
+	ret = write(fd, buffer, buffer_size);
 	if (ret < 0)
 		err(1, "write: %s", filename);
-	if (ret != BUFFER_SIZE) {
+	if (ret != buffer_size) {
 		fprintf(stderr, "%s: short write\n", filename);
 		exit(1);
 	}
@@ -167,7 +180,7 @@ int main(int argc, char *argv[])
 	if (ret != 0)
 		err(1, "lseek: %s", filename);
 
-	do_splice(fd, filename, BUFFER_SIZE);
+	do_splice(fd, filename, buffer_size);
 
 	if (unlink(filename) == -1)
 		err(1, "unlink: %s", filename);
diff --git a/tests/generic/591 b/tests/generic/591
index 5efc5136..4de50e2a 100755
--- a/tests/generic/591
+++ b/tests/generic/591
@@ -24,9 +24,11 @@ _require_test
 _require_odirect
 _require_test_program "splice-test"
 
-$here/src/splice-test -r $TEST_DIR/a
+diosize=`_min_dio_alignment $TEST_DEV`
+
+$here/src/splice-test -s $diosize -r $TEST_DIR/a
 $here/src/splice-test -rd $TEST_DIR/a
-$here/src/splice-test $TEST_DIR/a
+$here/src/splice-test -s $diosize $TEST_DIR/a
 $here/src/splice-test -d $TEST_DIR/a
 
 # success, all done
-- 
2.31.1

[PATCH v2 5/5] gitignore: ignore missed binary files in src

[Zorro Lang]

Add  src/dmiperf and src/t_locks_execve into .gitignore file

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 .gitignore | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.gitignore b/.gitignore
index 88c79412..24bde45c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -77,6 +77,7 @@ tags
 /src/dirhash_collide
 /src/dirperf
 /src/dirstress
+/src/dmiperf
 /src/e4compact
 /src/ext4_resize
 /src/fault
@@ -146,6 +147,7 @@ tags
 /src/t_getcwd
 /src/t_holes
 /src/t_immutable
+/src/t_locks_execve
 /src/t_mmap_collision
 /src/t_mmap_cow_race
 /src/t_mmap_dio
-- 
2.31.1

Re: [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size

[Darrick J. Wong]

On Wed, Jun 01, 2022 at 02:37:26PM +0800, Zorro Lang wrote:
> Due to generic/139 tests base on 512 bytes aligned, so skip this test
> if the minimum dio write size >512. This patch also change the
> common/rc::_require_dio helper, supports a minimum aligned size
> argument.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  common/rc         | 17 ++++++++++++++---
>  tests/generic/139 |  2 +-
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 2f31ca46..6d384eaf 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2721,9 +2721,12 @@ _require_xfs_io_command()
>  	fi
>  }
>  
> -# check that kernel and filesystem support direct I/O
> +# check that kernel and filesystem support direct I/O, and check if "$1" size
> +# aligned (optional) is supported
>  _require_odirect()
>  {
> +	local alignment=${1:+"-b $1"}
> +
>  	if [ $FSTYP = "ext4" ] || [ $FSTYP = "f2fs" ] ; then
>  		if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
>  			_notrun "$FSTYP encryption doesn't support O_DIRECT"
> @@ -2735,9 +2738,17 @@ _require_odirect()
>  		fi
>  	fi
>  	local testfile=$TEST_DIR/$$.direct
> -	$XFS_IO_PROG -F -f -d -c "pwrite 0 20k" $testfile > /dev/null 2>&1
> +	local opt
> +	if [ -n "$1" ];then
> +		opt="-b $1"
> +	fi

Doesn't this duplicate the assignment of @alignment above?

--D

> +	$XFS_IO_PROG -F -f -d -c "pwrite $opt 0 20k" $testfile > /dev/null 2>&1
>  	if [ $? -ne 0 ]; then
> -		_notrun "O_DIRECT is not supported"
> +		if [ -n "$alignment" ]; then
> +			_notrun "O_DIRECT aligned to $1 bytes is not supported"
> +		else
> +			_notrun "O_DIRECT is not supported"
> +		fi
>  	fi
>  	rm -f $testfile 2>&1 > /dev/null
>  }
> diff --git a/tests/generic/139 b/tests/generic/139
> index 0bbc222c..3eb1519d 100755
> --- a/tests/generic/139
> +++ b/tests/generic/139
> @@ -26,7 +26,7 @@ _cleanup()
>  # real QA test starts here
>  _require_test_reflink
>  _require_cp_reflink
> -_require_odirect
> +_require_odirect 512
>  
>  testdir=$TEST_DIR/test-$seq
>  rm -rf $testdir
> -- 
> 2.31.1
> 

Re: [PATCH v2 2/5] generic/506: call _require_quota before _qmount

[Darrick J. Wong]

On Wed, Jun 01, 2022 at 02:37:27PM +0800, Zorro Lang wrote:
> The g/506 fails on some filesystems (e.g. overlay) which doesn't
> support prjquota:
> 
>   MOUNT_OPTIONS =  -o prjquota
>   qmount failed
> 
> To avoid this failure, call _require_quota before doing real quota
> mount.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/generic/506 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/generic/506 b/tests/generic/506
> index 25a5b0f8..ec91af78 100755
> --- a/tests/generic/506
> +++ b/tests/generic/506
> @@ -27,6 +27,7 @@ _begin_fstest shutdown auto quick metadata quota
>  _supported_fs generic
>  
>  _require_scratch
> +_require_quota
>  _require_scratch_shutdown
>  
>  _scratch_mkfs >/dev/null 2>&1
> -- 
> 2.31.1
> 

Re: [PATCH v2 3/5] generic/591: remove redundant output from golden image

[Darrick J. Wong]

On Wed, Jun 01, 2022 at 02:37:28PM +0800, Zorro Lang wrote:
> In generic/591.out expects below output:
>   concurrent reader with O_DIRECT
>   concurrent reader with O_DIRECT     <=== ???
>   concurrent reader without O_DIRECT
>   concurrent reader without O_DIRECT  <=== ???
>   sequential reader with O_DIRECT
>   sequential reader without O_DIRECT
> 
> The lines marked "???" are unbelievable, due to the src/splice-test.c
> only calls printf to output that message once in main function. So
> Why splice-test prints that message twice sometimes? It seems related
> with the "-r" option, due to the test lines without "-r" option only
> print one line each time running.
> 
> A stanger thing is this "double output" issue only can be triggered by
> running g/591, can't reproduce it by running splice-test manually.
> 
> By checking the code of splice-test.c, I found a "fork()" in it, and
> it'll be called if the '-r' option is specified. So I suspect the
> redundant output come from the child process. By the help of strace
> tool, I got:
> 
>   10554 execve("/root/git/xfstests/src/splice-test", ["/root/git/xfstests/src/splice-te"..., "-r", "/mnt/test/a"], 0x7ffcabc2c0a8 /* 202 vars */) = 0
>   ...
>   10554 clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f937f5d5a10) = 10555
>   ...
>   10555 read(4, "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"..., 512) = 512
>   10555 write(1, "concurrent reader with O_DIRECT\n", 32) = 32
>   10555 exit_group(0)                     = ?
>   10555 +++ exited with 0 +++
>   10554 <... wait4 resumed>NULL, 0, NULL) = 10555
>   10554 --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=10555, si_uid=0, si_status=0, si_utime=0, si_stime=1} ---
>   10554 unlink("/mnt/test/a")             = 0
>   10554 write(1, "concurrent reader with O_DIRECT\n", 32) = 32
>   10554 exit_group(0)                     = ?
>   10554 +++ exited with 0 +++
> 
> We can see the "concurrent reader with O_DIRECT\n" be printed by
> parent process 10554 and child process 10555 separately.
> 
> Due to the stdout redirection that fstests does cause the stream
> doesn't refer to a tty anymore, then the stdout become block
> buffered, so the '\n' doesn't help to flush that printf message,
> and the child print it again.
> 
> So use setlinebuf(stdout) to force it line buffered, to avoid the
> confused output to be golden image. Then correct the generic/591.out

Eww, so the printf ends up in the output buffer, which is then
duplicated in the forked child, so both parent and child emit the same
message?

Gross.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  src/splice-test.c     | 2 ++
>  tests/generic/591.out | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/splice-test.c b/src/splice-test.c
> index 2f1ba2ba..dc41b0f5 100644
> --- a/src/splice-test.c
> +++ b/src/splice-test.c
> @@ -140,6 +140,8 @@ int main(int argc, char *argv[])
>  		usage(argv[0]);
>  	filename = argv[optind];
>  
> +	/* force below printf line buffered */
> +	setlinebuf(stdout);
>  	printf("%s reader %s O_DIRECT\n",
>  		   do_splice == do_splice1 ? "sequential" : "concurrent",
>  		   (open_flags & O_DIRECT) ? "with" : "without");
> diff --git a/tests/generic/591.out b/tests/generic/591.out
> index d61811ee..e9fffd1d 100644
> --- a/tests/generic/591.out
> +++ b/tests/generic/591.out
> @@ -1,7 +1,5 @@
>  QA output created by 591
>  concurrent reader with O_DIRECT
> -concurrent reader with O_DIRECT
> -concurrent reader without O_DIRECT
>  concurrent reader without O_DIRECT
>  sequential reader with O_DIRECT
>  sequential reader without O_DIRECT
> -- 
> 2.31.1
> 

Re: [PATCH v2 4/5] generic/591: use proper sector size

[Darrick J. Wong]

On Wed, Jun 01, 2022 at 02:37:29PM +0800, Zorro Lang wrote:
> The generic/591 fails if the sector size of TEST_DEV isn't 512:
> 
>   splice-test: write: /mnt/test/a: Invalid argument
> 
> To fix this issue, this patch help src/splice-test.c to get a specify
> sector size from the test case. Then let g/591 give it a proper
> sector size which dio aligned.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  src/splice-test.c | 37 +++++++++++++++++++++++++------------
>  tests/generic/591 |  6 ++++--
>  2 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/src/splice-test.c b/src/splice-test.c
> index dc41b0f5..eb863673 100644
> --- a/src/splice-test.c
> +++ b/src/splice-test.c
> @@ -19,19 +19,20 @@
>  #include <errno.h>
>  #include <malloc.h>
>  
> -#define SECTOR_SIZE 512
> -#define BUFFER_SIZE (150 * SECTOR_SIZE)
> +unsigned int sector_size;
> +unsigned int buffer_size;
>  
>  void read_from_pipe(int fd, const char *filename, size_t size)
>  {
> -	char buffer[SECTOR_SIZE];
> +	char *buffer;
>  	size_t sz;
>  	ssize_t ret;
> +	buffer = malloc(buffer_size);
>  
>  	while (size) {
>  		sz = size;
> -		if (sz > sizeof buffer)
> -			sz = sizeof buffer;
> +		if (sz > buffer_size)
> +			sz = buffer_size;
>  		ret = read(fd, buffer, sz);
>  		if (ret < 0)
>  			err(1, "read: %s", filename);
> @@ -41,6 +42,7 @@ void read_from_pipe(int fd, const char *filename, size_t size)
>  		}
>  		size -= sz;
>  	}
> +	free(buffer);
>  }
>  
>  void do_splice1(int fd, const char *filename, size_t size)
> @@ -108,7 +110,7 @@ void do_splice2(int fd, const char *filename, size_t size)
>  
>  void usage(const char *argv0)
>  {
> -	fprintf(stderr, "USAGE: %s [-rd] {filename}\n", basename(argv0));
> +	fprintf(stderr, "USAGE: %s [-rd] [-s sectorsize] {filename}\n", basename(argv0));
>  	exit(2);
>  }
>  
> @@ -120,11 +122,22 @@ int main(int argc, char *argv[])
>  	int opt, open_flags, fd;
>  	ssize_t ret;
>  
> +	/*
> +	 * init default sector_size and buffer_size, might be changed if the -s
> +	 * option is specified
> +	 */
> +	sector_size = 512;
> +	buffer_size = 150 * sector_size;
> +
>  	do_splice = do_splice1;
>  	open_flags = O_CREAT | O_TRUNC | O_RDWR | O_DIRECT;
>  
> -	while ((opt = getopt(argc, argv, "rd")) != -1) {
> +	while ((opt = getopt(argc, argv, "rds:")) != -1) {
>  		switch(opt) {
> +		case 's':
> +			sector_size = strtol(optarg, NULL, 0);
> +			buffer_size = 150 * sector_size;
> +			break;
>  		case 'r':
>  			do_splice = do_splice2;
>  			break;
> @@ -146,7 +159,7 @@ int main(int argc, char *argv[])
>  		   do_splice == do_splice1 ? "sequential" : "concurrent",
>  		   (open_flags & O_DIRECT) ? "with" : "without");
>  
> -	buffer = memalign(SECTOR_SIZE, BUFFER_SIZE);
> +	buffer = memalign(sector_size, buffer_size);
>  	if (buffer == NULL)
>  		err(1, "memalign");
>  
> @@ -154,11 +167,11 @@ int main(int argc, char *argv[])
>  	if (fd == -1)
>  		err(1, "open: %s", filename);
>  
> -	memset(buffer, 'x', BUFFER_SIZE);
> -	ret = write(fd, buffer, BUFFER_SIZE);
> +	memset(buffer, 'x', buffer_size);
> +	ret = write(fd, buffer, buffer_size);
>  	if (ret < 0)
>  		err(1, "write: %s", filename);
> -	if (ret != BUFFER_SIZE) {
> +	if (ret != buffer_size) {
>  		fprintf(stderr, "%s: short write\n", filename);
>  		exit(1);
>  	}
> @@ -167,7 +180,7 @@ int main(int argc, char *argv[])
>  	if (ret != 0)
>  		err(1, "lseek: %s", filename);
>  
> -	do_splice(fd, filename, BUFFER_SIZE);
> +	do_splice(fd, filename, buffer_size);
>  
>  	if (unlink(filename) == -1)
>  		err(1, "unlink: %s", filename);
> diff --git a/tests/generic/591 b/tests/generic/591
> index 5efc5136..4de50e2a 100755
> --- a/tests/generic/591
> +++ b/tests/generic/591
> @@ -24,9 +24,11 @@ _require_test
>  _require_odirect
>  _require_test_program "splice-test"
>  
> -$here/src/splice-test -r $TEST_DIR/a
> +diosize=`_min_dio_alignment $TEST_DEV`
> +
> +$here/src/splice-test -s $diosize -r $TEST_DIR/a
>  $here/src/splice-test -rd $TEST_DIR/a
> -$here/src/splice-test $TEST_DIR/a
> +$here/src/splice-test -s $diosize $TEST_DIR/a
>  $here/src/splice-test -d $TEST_DIR/a
>  
>  # success, all done
> -- 
> 2.31.1
> 

Re: [PATCH v2 5/5] gitignore: ignore missed binary files in src

[Darrick J. Wong]

On Wed, Jun 01, 2022 at 02:37:30PM +0800, Zorro Lang wrote:
> Add  src/dmiperf and src/t_locks_execve into .gitignore file
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  .gitignore | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/.gitignore b/.gitignore
> index 88c79412..24bde45c 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -77,6 +77,7 @@ tags
>  /src/dirhash_collide
>  /src/dirperf
>  /src/dirstress
> +/src/dmiperf

Removed in commit c13150fe ("src/dmiperf: Remove obsolete dmiperf")

>  /src/e4compact
>  /src/ext4_resize
>  /src/fault
> @@ -146,6 +147,7 @@ tags
>  /src/t_getcwd
>  /src/t_holes
>  /src/t_immutable
> +/src/t_locks_execve

Removed in commit fd2366c6 ("fstests: remove generic/484")

--D

>  /src/t_mmap_collision
>  /src/t_mmap_cow_race
>  /src/t_mmap_dio
> -- 
> 2.31.1
> 

Re: [PATCH v2 5/5] gitignore: ignore missed binary files in src

[Zorro Lang]

On Wed, Jun 01, 2022 at 10:42:47AM -0700, Darrick J. Wong wrote:
> On Wed, Jun 01, 2022 at 02:37:30PM +0800, Zorro Lang wrote:
> > Add  src/dmiperf and src/t_locks_execve into .gitignore file
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  .gitignore | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 88c79412..24bde45c 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -77,6 +77,7 @@ tags
> >  /src/dirhash_collide
> >  /src/dirperf
> >  /src/dirstress
> > +/src/dmiperf
> 
> Removed in commit c13150fe ("src/dmiperf: Remove obsolete dmiperf")
> 
> >  /src/e4compact
> >  /src/ext4_resize
> >  /src/fault
> > @@ -146,6 +147,7 @@ tags
> >  /src/t_getcwd
> >  /src/t_holes
> >  /src/t_immutable
> > +/src/t_locks_execve
> 
> Removed in commit fd2366c6 ("fstests: remove generic/484")

Oh, sorry, so they're only left in my local directory:) Please ignore
this patch.

> 
> --D
> 
> >  /src/t_mmap_collision
> >  /src/t_mmap_cow_race
> >  /src/t_mmap_dio
> > -- 
> > 2.31.1
> > 
> 

[PATCH v3] generic/139: require 512 bytes to be the minimum dio size

[Zorro Lang]

Due to generic/139 tests base on 512 bytes aligned, so skip this test
if the minimum dio write size >512. This patch also change the
common/rc::_require_dio helper, supports a minimum aligned size
argument.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---

Thanks the review from Darrick on v2, this v3 remove some duplicate code
which I forgot.

Thanks,
Zorro

 common/rc         | 13 ++++++++++---
 tests/generic/139 |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index 2f31ca46..9823e3a1 100644
--- a/common/rc
+++ b/common/rc
@@ -2721,9 +2721,12 @@ _require_xfs_io_command()
 	fi
 }
 
-# check that kernel and filesystem support direct I/O
+# check that kernel and filesystem support direct I/O, and check if "$1" size
+# aligned (optional) is supported
 _require_odirect()
 {
+	local alignment=${1:+"-b $1"}
+
 	if [ $FSTYP = "ext4" ] || [ $FSTYP = "f2fs" ] ; then
 		if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
 			_notrun "$FSTYP encryption doesn't support O_DIRECT"
@@ -2735,9 +2738,13 @@ _require_odirect()
 		fi
 	fi
 	local testfile=$TEST_DIR/$$.direct
-	$XFS_IO_PROG -F -f -d -c "pwrite 0 20k" $testfile > /dev/null 2>&1
+	$XFS_IO_PROG -F -f -d -c "pwrite $alignment 0 20k" $testfile > /dev/null 2>&1
 	if [ $? -ne 0 ]; then
-		_notrun "O_DIRECT is not supported"
+		if [ -n "$alignment" ]; then
+			_notrun "O_DIRECT aligned to $1 bytes is not supported"
+		else
+			_notrun "O_DIRECT is not supported"
+		fi
 	fi
 	rm -f $testfile 2>&1 > /dev/null
 }
diff --git a/tests/generic/139 b/tests/generic/139
index 0bbc222c..3eb1519d 100755
--- a/tests/generic/139
+++ b/tests/generic/139
@@ -26,7 +26,7 @@ _cleanup()
 # real QA test starts here
 _require_test_reflink
 _require_cp_reflink
-_require_odirect
+_require_odirect 512
 
 testdir=$TEST_DIR/test-$seq
 rm -rf $testdir
-- 
2.31.1

Re: [PATCH v3] generic/139: require 512 bytes to be the minimum dio size

[Darrick J. Wong]

On Thu, Jun 02, 2022 at 01:17:16PM +0800, Zorro Lang wrote:
> Due to generic/139 tests base on 512 bytes aligned, so skip this test
> if the minimum dio write size >512. This patch also change the
> common/rc::_require_dio helper, supports a minimum aligned size
> argument.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
> 
> Thanks the review from Darrick on v2, this v3 remove some duplicate code
> which I forgot.
> 
> Thanks,
> Zorro
> 
>  common/rc         | 13 ++++++++++---
>  tests/generic/139 |  2 +-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 2f31ca46..9823e3a1 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2721,9 +2721,12 @@ _require_xfs_io_command()
>  	fi
>  }
>  
> -# check that kernel and filesystem support direct I/O
> +# check that kernel and filesystem support direct I/O, and check if "$1" size
> +# aligned (optional) is supported
>  _require_odirect()
>  {
> +	local alignment=${1:+"-b $1"}

This might be a nit, but you might want to do this instead:

	local blocksize=$1
	local align_args=${1:+"-b $1"}

So that there's only one "$1" to change if the arguments ever get
rearranged.  But that might never happen, and this feels nearly like
pointless navelgazing.

If you're happy with things the way they are, the logic looks ok so:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
>  	if [ $FSTYP = "ext4" ] || [ $FSTYP = "f2fs" ] ; then
>  		if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
>  			_notrun "$FSTYP encryption doesn't support O_DIRECT"
> @@ -2735,9 +2738,13 @@ _require_odirect()
>  		fi
>  	fi
>  	local testfile=$TEST_DIR/$$.direct
> -	$XFS_IO_PROG -F -f -d -c "pwrite 0 20k" $testfile > /dev/null 2>&1
> +	$XFS_IO_PROG -F -f -d -c "pwrite $alignment 0 20k" $testfile > /dev/null 2>&1
>  	if [ $? -ne 0 ]; then
> -		_notrun "O_DIRECT is not supported"
> +		if [ -n "$alignment" ]; then
> +			_notrun "O_DIRECT aligned to $1 bytes is not supported"
> +		else
> +			_notrun "O_DIRECT is not supported"
> +		fi
>  	fi
>  	rm -f $testfile 2>&1 > /dev/null
>  }
> diff --git a/tests/generic/139 b/tests/generic/139
> index 0bbc222c..3eb1519d 100755
> --- a/tests/generic/139
> +++ b/tests/generic/139
> @@ -26,7 +26,7 @@ _cleanup()
>  # real QA test starts here
>  _require_test_reflink
>  _require_cp_reflink
> -_require_odirect
> +_require_odirect 512
>  
>  testdir=$TEST_DIR/test-$seq
>  rm -rf $testdir
> -- 
> 2.31.1
> 

Re: [PATCH v3] generic/139: require 512 bytes to be the minimum dio size

[Zorro Lang]

On Thu, Jun 02, 2022 at 09:40:45AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 02, 2022 at 01:17:16PM +0800, Zorro Lang wrote:
> > Due to generic/139 tests base on 512 bytes aligned, so skip this test
> > if the minimum dio write size >512. This patch also change the
> > common/rc::_require_dio helper, supports a minimum aligned size
> > argument.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> > 
> > Thanks the review from Darrick on v2, this v3 remove some duplicate code
> > which I forgot.
> > 
> > Thanks,
> > Zorro
> > 
> >  common/rc         | 13 ++++++++++---
> >  tests/generic/139 |  2 +-
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 2f31ca46..9823e3a1 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2721,9 +2721,12 @@ _require_xfs_io_command()
> >  	fi
> >  }
> >  
> > -# check that kernel and filesystem support direct I/O
> > +# check that kernel and filesystem support direct I/O, and check if "$1" size
> > +# aligned (optional) is supported
> >  _require_odirect()
> >  {
> > +	local alignment=${1:+"-b $1"}
> 
> This might be a nit, but you might want to do this instead:
> 
> 	local blocksize=$1
> 	local align_args=${1:+"-b $1"}
> 
> So that there's only one "$1" to change if the arguments ever get
> rearranged.  But that might never happen, and this feels nearly like
> pointless navelgazing.
> 
> If you're happy with things the way they are, the logic looks ok so:

OK, I respect the meticulous attitude, will do that when I merge it :)

> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> > +
> >  	if [ $FSTYP = "ext4" ] || [ $FSTYP = "f2fs" ] ; then
> >  		if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
> >  			_notrun "$FSTYP encryption doesn't support O_DIRECT"
> > @@ -2735,9 +2738,13 @@ _require_odirect()
> >  		fi
> >  	fi
> >  	local testfile=$TEST_DIR/$$.direct
> > -	$XFS_IO_PROG -F -f -d -c "pwrite 0 20k" $testfile > /dev/null 2>&1
> > +	$XFS_IO_PROG -F -f -d -c "pwrite $alignment 0 20k" $testfile > /dev/null 2>&1
> >  	if [ $? -ne 0 ]; then
> > -		_notrun "O_DIRECT is not supported"
> > +		if [ -n "$alignment" ]; then
> > +			_notrun "O_DIRECT aligned to $1 bytes is not supported"
> > +		else
> > +			_notrun "O_DIRECT is not supported"
> > +		fi
> >  	fi
> >  	rm -f $testfile 2>&1 > /dev/null
> >  }
> > diff --git a/tests/generic/139 b/tests/generic/139
> > index 0bbc222c..3eb1519d 100755
> > --- a/tests/generic/139
> > +++ b/tests/generic/139
> > @@ -26,7 +26,7 @@ _cleanup()
> >  # real QA test starts here
> >  _require_test_reflink
> >  _require_cp_reflink
> > -_require_odirect
> > +_require_odirect 512
> >  
> >  testdir=$TEST_DIR/test-$seq
> >  rm -rf $testdir
> > -- 
> > 2.31.1
> > 
>