xfstests: SEEK_HOLE/SEEK_DATA advanced tests

xfstests: SEEK_HOLE/SEEK_DATA advanced tests

[Mark Tinguely]

[-- Attachment #1: xfstests-advanced-seek_hole-seek_data.patch --]
[-- Type: text/plain, Size: 9261 bytes --]

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

Jie Liu <jeff.liu@oracle.com> wrote the C code, I converted it to
a test with his permission.

Signed-off-by: Mark Tinguely <tinguely@sgi.com
---
 288                     |   70 +++++++++++++++++
 288.out                 |    2 
 group                   |    1 
 src/Makefile            |    2 
 src/seek_advance_test.c |  189 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 263 insertions(+), 1 deletion(-)

Index: b/288
===================================================================
--- /dev/null
+++ 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
+owner=tinguely@sgi.com
+
+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
+
Index: b/288.out
===================================================================
--- /dev/null
+++ b/288.out
@@ -0,0 +1,2 @@
+QA output created by 288
+Silence is golden...
Index: b/group
===================================================================
--- a/group
+++ b/group
@@ -406,3 +406,4 @@ deprecated
 285 auto rw
 286 other
 287 auto dump quota quick
+288 other
Index: b/src/Makefile
===================================================================
--- a/src/Makefile
+++ 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 =
 
Index: b/src/seek_advance_test.c
===================================================================
--- /dev/null
+++ b/src/seek_advance_test.c
@@ -0,0 +1,189 @@
+/*
+ * Copyright (C) 2011-2012 Oracle. 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 v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will 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 to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+ /* Test the lseek() SEEK_DATA/SEEK_HOLE unwritten extent support
+  * found in XFS starting in Linux 3.7.
+  */
+
+#define _XOPEN_SOURCE 500
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/vfs.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <assert.h>
+
+#ifndef SEEK_DATA
+#define SEEK_DATA	3
+#define SEEK_HOLE	4
+#endif
+
+char *base_file_path;
+
+int
+verify(
+	off_t offset,
+	off_t exp_hoff,
+	off_t exp_doff,
+	off_t hoff,
+	off_t doff)
+{
+	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);
+}
+
+int
+test01(void)
+{
+	int fd, ret=0;
+	char filename[32];
+	off_t hpos, dpos;
+
+	snprintf(filename, sizeof(filename), "%s/advseektst1", base_file_path);
+	fd = open(filename, O_RDWR, 0644);
+	if (fd < 0) {
+		fprintf(stderr, " ERROR %d: Failed to open file '%s'\n",
+			errno, filename);
+		return(1);
+	}
+
+	hpos = lseek(fd, 512*1024, SEEK_HOLE);
+	dpos = lseek(fd, 512*1024, SEEK_DATA);
+	ret += verify(512*1024, 512*1024, 516*1024, hpos, dpos);
+
+	/* Wrote 4k past 516k, the hole located at 520k */
+	hpos = lseek(fd, 516*1024, SEEK_HOLE);
+	dpos = lseek(fd, 516*1024, SEEK_DATA);
+	ret += verify(516*1024, 520*1024, 516*1024, hpos, dpos);
+
+	hpos = lseek(fd, 520*1024, SEEK_HOLE);
+	dpos = lseek(fd, 520*1024, SEEK_DATA);
+	ret += verify(520*1024, 520*1024, 800*1024, hpos, dpos);
+
+	hpos = lseek(fd, 800*1024, SEEK_HOLE);
+	dpos = lseek(fd, 800*1024, SEEK_DATA);
+	/* Wrote 4k past 800k, the hole located at 804k */
+	ret += verify(800*1024, 804*1024, 800*1024, hpos, dpos);
+
+	/* A few cases for byte precious verfication */
+	hpos = lseek(fd, 512*1024 - 1, SEEK_HOLE);
+	dpos = lseek(fd, 512*1024 - 1, SEEK_DATA);
+	ret += verify(512*1024-1, 512*1024-1, 516*1024, hpos, dpos);
+
+	/* 516k landed in hole, wrote 4k past 516k */
+	hpos = lseek(fd, 516*1024 - 1, SEEK_HOLE);
+	dpos = lseek(fd, 516*1024 - 1, SEEK_DATA);
+	ret += verify(516*1024-1, 516*1024-1, 516*1024, hpos, dpos);
+
+	hpos = lseek(fd, 520*1024 - 1, SEEK_HOLE);
+	dpos = lseek(fd, 520*1024 - 1, SEEK_DATA);
+	ret += verify(520*1024-1, 520*1024, 520*1024-1, hpos, dpos);
+
+	/* Wrote 4k past 800k, 800k-1 is hole */
+	hpos = lseek(fd, 800*1024-1, SEEK_HOLE);
+	dpos = lseek(fd, 800*1024-1, SEEK_DATA);
+	ret += verify(800*1024-1, 800*1024-1, 800*1024, hpos, dpos);
+	close(fd);
+	return(ret);
+}
+
+int
+test02(void)
+{
+	int fd, ret=0;
+	char filename[32];
+	off_t hpos, dpos;
+
+	snprintf(filename, sizeof(filename), "%s/advseektst2", base_file_path);
+	fd = open(filename, O_RDWR, 0644);
+	if (fd < 0) {
+		fprintf(stderr, " ERROR %d: Failed to open file '%s'\n",
+			errno, filename);
+		return(1);
+	}
+
+	hpos = lseek(fd, 511*1024, SEEK_HOLE);
+	dpos = lseek(fd, 511*1024, SEEK_DATA);
+	ret += verify(511*1014, 511*1024, 600*1024, hpos, dpos);
+
+	/* Wrote 16k past 600, the hole locate at 616k */
+	hpos = lseek(fd, 600*1024, SEEK_HOLE);
+	dpos = lseek(fd, 600*1024, SEEK_DATA);
+	ret += verify(600*1024, 616*1024, 600*1024, hpos, dpos);
+
+	hpos = lseek(fd, 604*1024, SEEK_HOLE);
+	dpos = lseek(fd, 604*1024, SEEK_DATA);
+	ret += verify(604*1024, 616*1024, 604*1024, hpos, dpos);
+
+	hpos = lseek(fd, 608*1024, SEEK_HOLE);
+	dpos = lseek(fd, 608*1024, SEEK_DATA);
+	ret += verify(608*1024, 616*1024, 608*1024, hpos, dpos);
+
+	hpos = lseek(fd, 609*1024, SEEK_HOLE);
+	dpos = lseek(fd, 609*1024, SEEK_DATA);
+	ret += verify(609*1024, 616*1024, 609*1024, hpos, dpos);
+
+	hpos = lseek(fd, 612*1024, SEEK_HOLE);
+	dpos = lseek(fd, 612*1024, SEEK_DATA);
+	ret += verify(612*1024, 616*1024, 612*1024, hpos, dpos);
+
+	hpos = lseek(fd, 616*1024, SEEK_HOLE);
+	dpos = lseek(fd, 616*1024, SEEK_DATA);
+	ret += verify(616*1024, 616*1024, -1, hpos, dpos);
+
+	hpos = lseek(fd, 616*1024 - 1, SEEK_HOLE);
+	dpos = lseek(fd, 616*1024 - 1, SEEK_DATA);
+	ret += verify(616*1024-1, 616*1024, 616*1024-1, hpos, dpos);
+
+	hpos = lseek(fd, 616*1024 + 1, SEEK_HOLE);
+	dpos = lseek(fd, 616*1024 + 1, SEEK_DATA);
+	ret += verify(616*1024+1, 616*1024+1, -1, hpos, dpos);
+	close(fd);
+	return(ret);
+}
+
+int
+main(
+	int argc,
+	char **argv)
+{
+	int ret = 0;
+
+	if (argc != 2) {
+	fprintf(stdout, "Usage: %s base_file_path\n", argv[0]);
+	exit(1);
+	}
+
+	base_file_path = (char *)strdup(argv[1]);
+	if (!base_file_path)
+	return ret;
+
+	ret = test01();
+	printf("\n");
+	ret += test02();
+
+	free(base_file_path);
+	exit(ret);
+}


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: xfstests: SEEK_HOLE/SEEK_DATA advanced tests

[Dave Chinner]

On Tue, Oct 16, 2012 at 03:42:41PM -0500, Mark Tinguely wrote:
> This is the test of the advanced SEEK_HOLE and SEEK_DATA features
> of the lseek() function call.
> 
> Jie Liu <jeff.liu@oracle.com> 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?

> Signed-off-by: Mark Tinguely <tinguely@sgi.com

Typo - missing closing ">"

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

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

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

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

> +
> +status=0
> +exit
> +
> Index: b/288.out
> ===================================================================
> --- /dev/null
> +++ b/288.out
> @@ -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....

> Index: b/group
> ===================================================================
> --- a/group
> +++ b/group
> @@ -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....

> +#include <assert.h>
> +
> +#ifndef SEEK_DATA
> +#define SEEK_DATA	3
> +#define SEEK_HOLE	4
> +#endif
> +
> +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.

> +{
> +	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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: xfstests: SEEK_HOLE/SEEK_DATA advanced tests

[Jie Liu]

On 10/17/12 10:34, Dave Chinner wrote:
> On Tue, Oct 16, 2012 at 03:42:41PM -0500, Mark Tinguely wrote:
>> This is the test of the advanced SEEK_HOLE and SEEK_DATA features
>> of the lseek() function call.
>>
>> Jie Liu <jeff.liu@oracle.com> 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
>> Signed-off-by: Mark Tinguely <tinguely@sgi.com
> Typo - missing closing ">"
>
>> +
>> +[ -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...
>
>> +        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 :/
>
>> +
>> +        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....
>
>> +        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.
>
>> +
>> +status=0
>> +exit
>> +
>> Index: b/288.out
>> ===================================================================
>> --- /dev/null
>> +++ b/288.out
>> @@ -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....
>
>> Index: b/group
>> ===================================================================
>> --- a/group
>> +++ b/group
>> @@ -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....
>
>> +#include <assert.h>
>> +
>> +#ifndef SEEK_DATA
>> +#define SEEK_DATA	3
>> +#define SEEK_HOLE	4
>> +#endif
>> +
>> +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.
>
>> +{
>> +	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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: xfstests: SEEK_HOLE/SEEK_DATA advanced tests

[Dave Chinner]

On Wed, Oct 17, 2012 at 11:38:53AM +0800, Jie Liu wrote:
> On 10/17/12 10:34, Dave Chinner wrote:
> > On Tue, Oct 16, 2012 at 03:42:41PM -0500, Mark Tinguely wrote:
> >> This is the test of the advanced SEEK_HOLE and SEEK_DATA features
> >> of the lseek() function call.
> >>
> >> Jie Liu <jeff.liu@oracle.com> 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.

The .c file has this:

+ * 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 :).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: xfstests: SEEK_HOLE/SEEK_DATA advanced tests

[Jie Liu]

On 10/17/12 15:24, Dave Chinner wrote:
> On Wed, Oct 17, 2012 at 11:38:53AM +0800, Jie Liu wrote:
>> On 10/17/12 10:34, Dave Chinner wrote:
>>> On Tue, Oct 16, 2012 at 03:42:41PM -0500, Mark Tinguely wrote:
>>>> This is the test of the advanced SEEK_HOLE and SEEK_DATA features
>>>> of the lseek() function call.
>>>>
>>>> Jie Liu <jeff.liu@oracle.com> 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.
> The .c file has this:
>
> + * 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
>
> Cheers,
>
> Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: xfstests: SEEK_HOLE/SEEK_DATA advanced tests

[Mark Tinguely]

On 10/16/12 21:34, Dave Chinner wrote:
> On Tue, Oct 16, 2012 at 03:42:41PM -0500, Mark Tinguely wrote:
>> This is the test of the advanced SEEK_HOLE and SEEK_DATA features
>> of the lseek() function call.
>>
>> Jie Liu<jeff.liu@oracle.com>  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?
>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com
>
> Typo - missing closing ">"
>
>> +
>> +[ -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...
>
>> +        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 :/
>
>> +
>> +        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....
>
>> +        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.
>
>> +
>> +status=0
>> +exit
>> +
>> Index: b/288.out
>> ===================================================================
>> --- /dev/null
>> +++ b/288.out
>> @@ -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....
>
>> Index: b/group
>> ===================================================================
>> --- a/group
>> +++ b/group
>> @@ -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....
>
>> +#include<assert.h>
>> +
>> +#ifndef SEEK_DATA
>> +#define SEEK_DATA	3
>> +#define SEEK_HOLE	4
>> +#endif
>> +
>> +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.
>
>> +{
>> +	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.

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs