* [PATCH v2 0/5] random fixes for fstests @ 2022-06-01 6:37 Zorro Lang 2022-06-01 6:37 ` [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size Zorro Lang ` (4 more replies) 0 siblings, 5 replies; 15+ messages in thread From: Zorro Lang @ 2022-06-01 6:37 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size 2022-06-01 6:37 [PATCH v2 0/5] random fixes for fstests Zorro Lang @ 2022-06-01 6:37 ` Zorro Lang 2022-06-01 17:32 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 2/5] generic/506: call _require_quota before _qmount Zorro Lang ` (3 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Zorro Lang @ 2022-06-01 6:37 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size 2022-06-01 6:37 ` [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size Zorro Lang @ 2022-06-01 17:32 ` Darrick J. Wong 2022-06-02 5:17 ` [PATCH v3] " Zorro Lang 0 siblings, 1 reply; 15+ messages in thread From: Darrick J. Wong @ 2022-06-01 17:32 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] generic/139: require 512 bytes to be the minimum dio size 2022-06-01 17:32 ` Darrick J. Wong @ 2022-06-02 5:17 ` Zorro Lang 2022-06-02 16:40 ` Darrick J. Wong 0 siblings, 1 reply; 15+ messages in thread From: Zorro Lang @ 2022-06-02 5:17 UTC (permalink / raw) To: fstests; +Cc: djwong 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] generic/139: require 512 bytes to be the minimum dio size 2022-06-02 5:17 ` [PATCH v3] " Zorro Lang @ 2022-06-02 16:40 ` Darrick J. Wong 2022-06-02 19:13 ` Zorro Lang 0 siblings, 1 reply; 15+ messages in thread From: Darrick J. Wong @ 2022-06-02 16:40 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] generic/139: require 512 bytes to be the minimum dio size 2022-06-02 16:40 ` Darrick J. Wong @ 2022-06-02 19:13 ` Zorro Lang 0 siblings, 0 replies; 15+ messages in thread From: Zorro Lang @ 2022-06-02 19:13 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests 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 > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/5] generic/506: call _require_quota before _qmount 2022-06-01 6:37 [PATCH v2 0/5] random fixes for fstests Zorro Lang 2022-06-01 6:37 ` [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size Zorro Lang @ 2022-06-01 6:37 ` Zorro Lang 2022-06-01 17:35 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 3/5] generic/591: remove redundant output from golden image Zorro Lang ` (2 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Zorro Lang @ 2022-06-01 6:37 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/5] generic/506: call _require_quota before _qmount 2022-06-01 6:37 ` [PATCH v2 2/5] generic/506: call _require_quota before _qmount Zorro Lang @ 2022-06-01 17:35 ` Darrick J. Wong 0 siblings, 0 replies; 15+ messages in thread From: Darrick J. Wong @ 2022-06-01 17:35 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/5] generic/591: remove redundant output from golden image 2022-06-01 6:37 [PATCH v2 0/5] random fixes for fstests Zorro Lang 2022-06-01 6:37 ` [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size Zorro Lang 2022-06-01 6:37 ` [PATCH v2 2/5] generic/506: call _require_quota before _qmount Zorro Lang @ 2022-06-01 6:37 ` Zorro Lang 2022-06-01 17:38 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 4/5] generic/591: use proper sector size Zorro Lang 2022-06-01 6:37 ` [PATCH v2 5/5] gitignore: ignore missed binary files in src Zorro Lang 4 siblings, 1 reply; 15+ messages in thread From: Zorro Lang @ 2022-06-01 6:37 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/5] generic/591: remove redundant output from golden image 2022-06-01 6:37 ` [PATCH v2 3/5] generic/591: remove redundant output from golden image Zorro Lang @ 2022-06-01 17:38 ` Darrick J. Wong 0 siblings, 0 replies; 15+ messages in thread From: Darrick J. Wong @ 2022-06-01 17:38 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/5] generic/591: use proper sector size 2022-06-01 6:37 [PATCH v2 0/5] random fixes for fstests Zorro Lang ` (2 preceding siblings ...) 2022-06-01 6:37 ` [PATCH v2 3/5] generic/591: remove redundant output from golden image Zorro Lang @ 2022-06-01 6:37 ` Zorro Lang 2022-06-01 17:41 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 5/5] gitignore: ignore missed binary files in src Zorro Lang 4 siblings, 1 reply; 15+ messages in thread From: Zorro Lang @ 2022-06-01 6:37 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/5] generic/591: use proper sector size 2022-06-01 6:37 ` [PATCH v2 4/5] generic/591: use proper sector size Zorro Lang @ 2022-06-01 17:41 ` Darrick J. Wong 0 siblings, 0 replies; 15+ messages in thread From: Darrick J. Wong @ 2022-06-01 17:41 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 5/5] gitignore: ignore missed binary files in src 2022-06-01 6:37 [PATCH v2 0/5] random fixes for fstests Zorro Lang ` (3 preceding siblings ...) 2022-06-01 6:37 ` [PATCH v2 4/5] generic/591: use proper sector size Zorro Lang @ 2022-06-01 6:37 ` Zorro Lang 2022-06-01 17:42 ` Darrick J. Wong 4 siblings, 1 reply; 15+ messages in thread From: Zorro Lang @ 2022-06-01 6:37 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 5/5] gitignore: ignore missed binary files in src 2022-06-01 6:37 ` [PATCH v2 5/5] gitignore: ignore missed binary files in src Zorro Lang @ 2022-06-01 17:42 ` Darrick J. Wong 2022-06-02 5:03 ` Zorro Lang 0 siblings, 1 reply; 15+ messages in thread From: Darrick J. Wong @ 2022-06-01 17:42 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 5/5] gitignore: ignore missed binary files in src 2022-06-01 17:42 ` Darrick J. Wong @ 2022-06-02 5:03 ` Zorro Lang 0 siblings, 0 replies; 15+ messages in thread From: Zorro Lang @ 2022-06-02 5:03 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests 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 > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-06-02 19:13 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-01 6:37 [PATCH v2 0/5] random fixes for fstests Zorro Lang 2022-06-01 6:37 ` [PATCH v2 1/5] generic/139: require 512 bytes to be the minimum dio size Zorro Lang 2022-06-01 17:32 ` Darrick J. Wong 2022-06-02 5:17 ` [PATCH v3] " Zorro Lang 2022-06-02 16:40 ` Darrick J. Wong 2022-06-02 19:13 ` Zorro Lang 2022-06-01 6:37 ` [PATCH v2 2/5] generic/506: call _require_quota before _qmount Zorro Lang 2022-06-01 17:35 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 3/5] generic/591: remove redundant output from golden image Zorro Lang 2022-06-01 17:38 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 4/5] generic/591: use proper sector size Zorro Lang 2022-06-01 17:41 ` Darrick J. Wong 2022-06-01 6:37 ` [PATCH v2 5/5] gitignore: ignore missed binary files in src Zorro Lang 2022-06-01 17:42 ` Darrick J. Wong 2022-06-02 5:03 ` Zorro Lang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.