xfstests: SEEK_HOLE/SEEK_DATA advanced tests

PATCH Mark Tinguely 3 participants 6 messages Oct 16, 2012 – Oct 17, 2012 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mark Tinguely

author

2012-10-16 20:41 UTC #1 lore raw

This is the test of the advanced SEEK_HOLE and SEEK_DATA features of the lseek() function call.

Jie Liu <[email protected]> wrote the C code, I converted it to a test with his permission.

Signed-off-by Mark Tinguely <[email protected]

---
 288                     |   70 +++++++++++++++++
 288.out                 |    2 
 group                   |    1 
 src/Makefile            |    2 
 src/seek_advance_test.c |  189 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 263 insertions(+), 1 deletion(-)

288 new file +70 -0

Index: b/288
===================================================================
@@ -0,0 +1,70 @@
+#! /bin/bash
+# FS QA Test No. 288
+#
+# SEEK_DATA/SEEK_HOLE advanced feature tests.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2012 SGI.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1        # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs xfs
+_supported_os Linux
+
+dir=$TEST_DIR
+src1=$dir/advseektst1
+src2=$dir/advseektst2
+
+[ -x $here/src/seek_advance_test ] || _notrun "seek_advance_test not built"
+
+_cleanup()
+{
+	rm -f $src1 $src2
+}
+
+	echo "Silence is golden..."
+	rm -f $src1 $src2
+        write_cmd1="-c \"falloc 512k 256k\" -c \"pwrite 516k 4k\" -c \
+		    \"pwrite 800k 4k\""
+        write_cmd2="-c \"falloc 512k 256k\" -c \"pwrite 600k 16k\""
+
+        eval ${XFS_IO_PROG} -F -f "${write_cmd1}" $src1 > $seq.full 2>&1 ||
+                _fail "create test1 file failed!"
+        echo "*** create test1 file done ***" >>$seq.full
+        eval ${XFS_IO_PROG} -F -f "${write_cmd2}" $src2 >>$seq.full 2>&1 ||
+                _fail "create test2 file failed!"
+        echo "*** create test2 file done ***" >>$seq.full
+        echo >>$seq.full
+        $here/src/seek_advance_test $dir >> $seq.full || _fail "test failed"
+
+status=0
+exit
+

288.out new file +2 -0

Index: b/288.out
===================================================================
@@ -0,0 +1,2 @@
+QA output created by 288
+Silence is golden...

group +1 -0

Index: b/group
===================================================================
@@ -406,3 +406,4 @@ deprecated
 285 auto rw
 286 other
 287 auto dump quota quick
+288 other

src/Makefile +1 -1

Index: b/src/Makefile
===================================================================
@@ -18,7 +18,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getd
 	locktest unwritten_mmap bulkstat_unlink_test t_stripealign \
 	bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \
 	stale_handle pwrite_mmap_blocked fstrim t_dir_offset2 seek_sanity_test \
-	seek_copy_test
+	seek_copy_test seek_advance_test
 
 SUBDIRS =

src/seek_advance_test.c new file +189 -0

189 lines not on this page · raw message

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2012-10-17 02:33 UTC #2 lore raw

Quoted 5 lines · Mark Tinguely

This is the test of the advanced SEEK_HOLE and SEEK_DATA features of the lseek() function call.
Jie Liu <[email protected]> wrote the C code, I converted it to a test with his permission.

Jie, can you sign-off on this patch as well as it has Oracle copyright statements in it?

Quoted 1 line

Signed-off-by Mark Tinguely <[email protected]

Typo - missing closing ">"

288 line 54

Quoted 10 lines

288 new file +10 -0

+
+[ -x $here/src/seek_advance_test ] || _notrun "seek_advance_test not built"
+
+_cleanup()
+{
+	rm -f $src1 $src2
+}
+
+	echo "Silence is golden..."
+	rm -f $src1 $src2

Rather strange indenting here and for the rest of the test...

288 line 57

Quoted 3 lines

288 new file +3 -0

+        write_cmd1="-c \"falloc 512k 256k\" -c \"pwrite 516k 4k\" -c \
+		    \"pwrite 800k 4k\""
+        write_cmd2="-c \"falloc 512k 256k\" -c \"pwrite 600k 16k\""

That reminds me of line noise :/

288 line 64

Quoted 7 lines

288 new file +7 -0

+
+        eval ${XFS_IO_PROG} -F -f "${write_cmd1}" $src1 > $seq.full 2>&1 ||
+                _fail "create test1 file failed!"
+        echo "*** create test1 file done ***" >>$seq.full
+        eval ${XFS_IO_PROG} -F -f "${write_cmd2}" $src2 >>$seq.full 2>&1 ||
+                _fail "create test2 file failed!"
+        echo "*** create test2 file done ***" >>$seq.full

The way that you're executing xfs_io to create the files is, well, convoluted. It doesn't need -F anymore, either. Just run:

$XFS_IO_PROG -f -c "falloc 512k 256k" \
		-c "pwrite 516k 4k" \
		-c "pwrite 800k 4k" \
		$src1 | _filter_xfs_io

And allow the golden output match to detect failures to create the file correctly. The filter/golden output test is designed to avoid all this "did it work" detection around simple operations. Indeed, seek_advance_test checks the file exists (via the open parameters) and errors out with an error message so there is no need to check it ahead of time and specifically fail the test.

Further, with the xfs_io output in the golden output, you don't need the intermediate "echo <redundant information>" lines to determine what failed, either....

288 line 66

Quoted 2 lines

288 new file +2 -0

+        echo >>$seq.full
+        $here/src/seek_advance_test $dir >> $seq.full || _fail "test failed"

Pass in the file names, not the directory. That way you only encode the filename in one place, not have ot keep the test and .c code in step.

288.out line 2

Quoted 11 lines

288 new file +4 -0
+
+status=0
+exit
+
Index: b/288.out
===================================================================
288.out new file +2 -0
@@ -0,0 +1,2 @@
+QA output created by 288
+Silence is golden...

Not when you should be using the golden output to detect failures instead of convouted code....

group deprecated · line 409

Quoted 9 lines

Index: b/group
===================================================================
group +1 -0
@@ -406,3 +406,4 @@ deprecated
 285 auto rw
 286 other
 287 auto dump quota quick
+288 other

Why? it's a test that is quick and should always be run. If you are worried about it failing on systems that don't support SEEK_HOLE/DATA, then add a _requires_seek_hole function....

Quoted 6 lines

group +6 -0

+#include <assert.h>
+
+#ifndef SEEK_DATA
+#define SEEK_DATA	3
+#define SEEK_HOLE	4
+#endif

group

Quoted 10 lines

group +10 -0

+
+char *base_file_path;
+
+int
+verify(
+	off_t offset,
+	off_t exp_hoff,
+	off_t exp_doff,
+	off_t hoff,
+	off_t doff)

I don't really understand what the variables are supposed to mean or what is being verified here.

group

Quoted 6 lines

group +6 -0

+{
+	fprintf(stdout, "SEEK start %lu expect %ld/%ld got %ld/%ld [%s]\n",
+	offset, exp_hoff, exp_doff, hoff, doff,
+	(hoff == exp_hoff && doff == exp_doff) ? "PASS" : "FAIL");
+
+	return(hoff != exp_hoff || doff != exp_doff);

Why are you even determining pass/fail here?

This is what golden output matching is for. Dump some numbers out, and if they match the golden output the test passed. If they don't, the test fails. We can't filter this output if necessary, nor support different block size filesystems, etc. because the verification is done within the C code rather than by the filters and test harness.

Indeed, if you add SEEK_HOLE/SEEK_DATA to xfs_io to dump the next offset of that type given a start offset, then this can all be implemented in a single script using filtering golden output matching, and can easily be made to support different block sizes. This code is just going to be fragile and hard to maintain....

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Jie Liu

2012-10-17 03:37 UTC #3 lore raw

Quoted 8 lines · Dave Chinner

Quoted 5 lines · Mark Tinguely
This is the test of the advanced SEEK_HOLE and SEEK_DATA features of the lseek() function call.
Jie Liu <[email protected]> wrote the C code, I converted it to a test with his permission.
Jie, can you sign-off on this patch as well as it has Oracle copyright statements in it?

Hi Dave,

I can sign-off this patch, but it's better to keep the copyright statements with SGI for this test script since
it wrote by Mark, and Mark is already made the C test file as Oracle.

Thanks,
-Jeff

Quoted 121 lines

Quoted 1 line
Signed-off-by Mark Tinguely <[email protected]
Typo - missing closing ">"
288 line 54
Quoted 10 lines
288 new file +10 -0
+
+[ -x $here/src/seek_advance_test ] || _notrun "seek_advance_test not built"
+
+_cleanup()
+{
+	rm -f $src1 $src2
+}
+
+	echo "Silence is golden..."
+	rm -f $src1 $src2
Rather strange indenting here and for the rest of the test...
288 line 57
Quoted 3 lines
288 new file +3 -0
+        write_cmd1="-c \"falloc 512k 256k\" -c \"pwrite 516k 4k\" -c \
+		    \"pwrite 800k 4k\""
+        write_cmd2="-c \"falloc 512k 256k\" -c \"pwrite 600k 16k\""
That reminds me of line noise :/
288 line 64
Quoted 7 lines
288 new file +7 -0
+
+        eval ${XFS_IO_PROG} -F -f "${write_cmd1}" $src1 > $seq.full 2>&1 ||
+                _fail "create test1 file failed!"
+        echo "*** create test1 file done ***" >>$seq.full
+        eval ${XFS_IO_PROG} -F -f "${write_cmd2}" $src2 >>$seq.full 2>&1 ||
+                _fail "create test2 file failed!"
+        echo "*** create test2 file done ***" >>$seq.full
The way that you're executing xfs_io to create the files is, well, convoluted. It doesn't need -F anymore, either. Just run:
$XFS_IO_PROG -f -c "falloc 512k 256k" \
		-c "pwrite 516k 4k" \
		-c "pwrite 800k 4k" \
		$src1 | _filter_xfs_io
And allow the golden output match to detect failures to create the file correctly. The filter/golden output test is designed to avoid all this "did it work" detection around simple operations. Indeed, seek_advance_test checks the file exists (via the open parameters) and errors out with an error message so there is no need to check it ahead of time and specifically fail the test.
Further, with the xfs_io output in the golden output, you don't need the intermediate "echo <redundant information>" lines to determine what failed, either....
288 line 66
Quoted 2 lines
288 new file +2 -0
+        echo >>$seq.full
+        $here/src/seek_advance_test $dir >> $seq.full || _fail "test failed"
Pass in the file names, not the directory. That way you only encode the filename in one place, not have ot keep the test and .c code in step.
288.out line 2
Quoted 11 lines
288 new file +4 -0
+
+status=0
+exit
+
Index: b/288.out
===================================================================
288.out new file +2 -0
@@ -0,0 +1,2 @@
+QA output created by 288
+Silence is golden...
Not when you should be using the golden output to detect failures instead of convouted code....
group deprecated · line 409
Quoted 9 lines
Index: b/group
===================================================================
group +1 -0
@@ -406,3 +406,4 @@ deprecated
 285 auto rw
 286 other
 287 auto dump quota quick
+288 other
Why? it's a test that is quick and should always be run. If you are worried about it failing on systems that don't support SEEK_HOLE/DATA, then add a _requires_seek_hole function....
Quoted 6 lines
group +6 -0
+#include <assert.h>
+
+#ifndef SEEK_DATA
+#define SEEK_DATA	3
+#define SEEK_HOLE	4
+#endif
group
Quoted 10 lines
group +10 -0
+
+char *base_file_path;
+
+int
+verify(
+	off_t offset,
+	off_t exp_hoff,
+	off_t exp_doff,
+	off_t hoff,
+	off_t doff)
I don't really understand what the variables are supposed to mean or what is being verified here.
group
Quoted 6 lines
group +6 -0
+{
+	fprintf(stdout, "SEEK start %lu expect %ld/%ld got %ld/%ld [%s]\n",
+	offset, exp_hoff, exp_doff, hoff, doff,
+	(hoff == exp_hoff && doff == exp_doff) ? "PASS" : "FAIL");
+
+	return(hoff != exp_hoff || doff != exp_doff);
Why are you even determining pass/fail here?
This is what golden output matching is for. Dump some numbers out, and if they match the golden output the test passed. If they don't, the test fails. We can't filter this output if necessary, nor support different block size filesystems, etc. because the verification is done within the C code rather than by the filters and test harness.
Indeed, if you add SEEK_HOLE/SEEK_DATA to xfs_io to dump the next offset of that type given a start offset, then this can all be implemented in a single script using filtering golden output matching, and can easily be made to support different block sizes. This code is just going to be fragile and hard to maintain....

Quoted 3 lines

Cheers,
Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2012-10-17 07:22 UTC #4 lore raw

Quoted 7 lines · Jie Liu

Quoted 6 lines · Dave Chinner
Quoted 5 lines · Mark Tinguely
This is the test of the advanced SEEK_HOLE and SEEK_DATA features of the lseek() function call.
Jie Liu <[email protected]> wrote the C code, I converted it to a test with his permission.

Quoted 7 lines

Quoted 2 lines
Jie, can you sign-off on this patch as well as it has Oracle copyright statements in it?
Hi Dave,
I can sign-off this patch, but it's better to keep the copyright statements with SGI for this test script since
it wrote by Mark, and Mark is already made the C test file as Oracle.

The .c file has this:

Which is why the patch needs your signoff.

But if the test is redone without the .c file (i.e. SEEK_HOLE/DATA added to xfs_io) then it won't be necessary (unless you do that work :).

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Jie Liu

2012-10-17 08:32 UTC #5 lore raw

Quoted 17 lines · Dave Chinner

Quoted 7 lines · Jie Liu
Quoted 6 lines · Dave Chinner
Quoted 5 lines · Mark Tinguely
This is the test of the advanced SEEK_HOLE and SEEK_DATA features of the lseek() function call.
Jie Liu <[email protected]> wrote the C code, I converted it to a test with his permission.
Quoted 7 lines
Quoted 2 lines
Jie, can you sign-off on this patch as well as it has Oracle copyright statements in it?
Hi Dave,
I can sign-off this patch, but it's better to keep the copyright statements with SGI for this test script since
it wrote by Mark, and Mark is already made the C test file as Oracle.
The .c file has this:

Quoted 7 lines

+ * Copyright (C) 2011-2012 Oracle. All rights reserved.
Which is why the patch needs your signoff.
But if the test is redone without the .c file (i.e. SEEK_HOLE/DATA added to xfs_io) then it won't be necessary (unless you do that work :).

Ah, I see, thanks for your clarification, I'll check Mark's patch of next revised version according to your comments, and sign it.

Thanks,
-Jeff

Quoted 4 lines

Cheers,
Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Mark Tinguely

author

2012-10-17 12:51 UTC #6 lore raw

In reply to Dave Chinner

Quoted 140 lines · Dave Chinner

Quoted 5 lines · Mark Tinguely
This is the test of the advanced SEEK_HOLE and SEEK_DATA features of the lseek() function call.
Jie Liu<[email protected]> wrote the C code, I converted it to a test with his permission.
Jie, can you sign-off on this patch as well as it has Oracle copyright statements in it?
Quoted 1 line
Signed-off-by Mark Tinguely<[email protected]
Typo - missing closing ">"
288 line 54
Quoted 10 lines
288 new file +10 -0
+
+[ -x $here/src/seek_advance_test ] || _notrun "seek_advance_test not built"
+
+_cleanup()
+{
+	rm -f $src1 $src2
+}
+
+	echo "Silence is golden..."
+	rm -f $src1 $src2
Rather strange indenting here and for the rest of the test...
288 line 57
Quoted 3 lines
288 new file +3 -0
+        write_cmd1="-c \"falloc 512k 256k\" -c \"pwrite 516k 4k\" -c \
+		    \"pwrite 800k 4k\""
+        write_cmd2="-c \"falloc 512k 256k\" -c \"pwrite 600k 16k\""
That reminds me of line noise :/
Review
Quoted 7 lines
(quoted diff) +7 -0
+
+        eval ${XFS_IO_PROG} -F -f "${write_cmd1}" $src1>  $seq.full 2>&1 ||
+                _fail "create test1 file failed!"
+        echo "*** create test1 file done ***">>$seq.full
+        eval ${XFS_IO_PROG} -F -f "${write_cmd2}" $src2>>$seq.full 2>&1 ||
+                _fail "create test2 file failed!"
+        echo "*** create test2 file done ***">>$seq.full
The way that you're executing xfs_io to create the files is, well, convoluted. It doesn't need -F anymore, either. Just run:
$XFS_IO_PROG -f -c "falloc 512k 256k" \
		-c "pwrite 516k 4k" \
		-c "pwrite 800k 4k" \
		$src1 | _filter_xfs_io
And allow the golden output match to detect failures to create the file correctly. The filter/golden output test is designed to avoid all this "did it work" detection around simple operations. Indeed, seek_advance_test checks the file exists (via the open parameters) and errors out with an error message so there is no need to check it ahead of time and specifically fail the test.
Further, with the xfs_io output in the golden output, you don't need the intermediate "echo<redundant information>" lines to determine what failed, either....
Review
Quoted 2 lines
(quoted diff) +2 -0
+        echo>>$seq.full
+        $here/src/seek_advance_test $dir>>  $seq.full || _fail "test failed"
Pass in the file names, not the directory. That way you only encode the filename in one place, not have ot keep the test and .c code in step.
288.out line 2
Quoted 11 lines
288 new file +4 -0
+
+status=0
+exit
+
Index: b/288.out
===================================================================
288.out new file +2 -0
@@ -0,0 +1,2 @@
+QA output created by 288
+Silence is golden...
Not when you should be using the golden output to detect failures instead of convouted code....
group deprecated · line 409
Quoted 9 lines
Index: b/group
===================================================================
group +1 -0
@@ -406,3 +406,4 @@ deprecated
  285 auto rw
  286 other
  287 auto dump quota quick
+288 other
Why? it's a test that is quick and should always be run. If you are worried about it failing on systems that don't support SEEK_HOLE/DATA, then add a _requires_seek_hole function....
Quoted 6 lines
group +6 -0
+#include<assert.h>
+
+#ifndef SEEK_DATA
+#define SEEK_DATA	3
+#define SEEK_HOLE	4
+#endif
group
Quoted 10 lines
group +10 -0
+
+char *base_file_path;
+
+int
+verify(
+	off_t offset,
+	off_t exp_hoff,
+	off_t exp_doff,
+	off_t hoff,
+	off_t doff)
I don't really understand what the variables are supposed to mean or what is being verified here.
group
Quoted 6 lines
group +6 -0
+{
+	fprintf(stdout, "SEEK start %lu expect %ld/%ld got %ld/%ld [%s]\n",
+	offset, exp_hoff, exp_doff, hoff, doff,
+	(hoff == exp_hoff&&  doff == exp_doff) ? "PASS" : "FAIL");
+
+	return(hoff != exp_hoff || doff != exp_doff);
Why are you even determining pass/fail here?
This is what golden output matching is for. Dump some numbers out, and if they match the golden output the test passed. If they don't, the test fails. We can't filter this output if necessary, nor support different block size filesystems, etc. because the verification is done within the C code rather than by the filters and test harness.
Indeed, if you add SEEK_HOLE/SEEK_DATA to xfs_io to dump the next offset of that type given a start offset, then this can all be implemented in a single script using filtering golden output matching, and can easily be made to support different block sizes. This code is just going to be fragile and hard to maintain....

Quoted 3 lines

Cheers,
Dave.

I borrowed the test (with its test group) from an early SEEK_* test and I did not clean the test file well enough.

Adding SEEK_HOLE/SEEK_DATA to xfs_io is an excellent idea.

Thanks for the feedback.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs