[PATCH v3.3 0/2] xfstests: test the nfs/cifs/btrfs/xfs reflink/dedupe ioctls

[PATCH v3.3 0/2] xfstests: test the nfs/cifs/btrfs/xfs reflink/dedupe ioctls

[Darrick J. Wong]

Hi all,

This is a small patch set against the reflink/dedupe test cases in
xfstests.  The first patch is a rewrite of the tools to find the
lowest vacant ID number and to move a test case.  These two programs
are useful for staging a lot of new tests at a high number and moving
them to lower numbers when the maintainer wants to accept the new
tests.

The second patch updates the golden output for the test that examines
the results of feeding bad inputs to the two ioctls.  The new error
values are based on a discussion of how to react to bad file types on
the mailing lists and the ongoing work to hoist the ioctls to the VFS
level.

If you're going to start using this mess, you probably ought to just
pull from my github trees for kernel[1], xfsprogs[2], xfstests[3], and
xfs-docs[4].  They should just work with the btrfs that's in 4.3...
and somewhat buggily with the 4.3 XFS patched with [1].

(You don't need to pull the kernel or xfs-docs git trees if you're not
working on XFS reflink.  The relevant xfs_io support will be in
xfsprogs 4.3 and the tests were pulled into the xfstests-dev repo last
week.)

Comments and questions are, as always, welcome.

--D

[1] https://github.com/djwong/linux/tree/for-dave
[2] https://github.com/djwong/xfsprogs/tree/for-dave
[3] https://github.com/djwong/xfstests/tree/for-dave
[4] https://github.com/djwong/xfs-documentation/tree/for-dave

[PATCH 1/2] test-scripts: test migration scripts

[Darrick J. Wong]

Add two scripts: "nextid" finds the next available test ID number in a
group, and "mvtest" relocates a test, fixes the golden output, and
moves the group entry for that test.

v2: sorting group files should preserve group order; nextid should use
the same algorithm as new; move both tools to tools/.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tools/mvtest     |   55 +++++++++++++++++++++++++++
 tools/nextid     |    1 
 tools/sort-group |  112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100755 tools/mvtest
 create mode 120000 tools/nextid
 create mode 100755 tools/sort-group


diff --git a/tools/mvtest b/tools/mvtest
new file mode 100755
index 0000000..af601d6
--- /dev/null
+++ b/tools/mvtest
@@ -0,0 +1,55 @@
+#!/bin/sh
+
+# Renumber a test
+dir="$(dirname "$0")"
+
+if [ -z "$1" ] || [ "$1" = "--help" ]; then
+	echo "Usage: $0 path_to_test new_path_to_test"
+	exit 1
+fi
+
+src="$1"
+dest="$2"
+
+die() {
+	echo "$@"
+	exit 1
+}
+
+append() {
+	out="$1"
+	shift
+	echo "$@" >> "${out}"
+}
+
+test "${src}" != "${dest}" || die "Test \"${src}\" is the same as dest."
+test -e "tests/${src}" || die "Test \"${src}\" does not exist."
+test ! -e "tests/${dest}" || die "Test \"${src}\" already exists."
+
+sid="$(basename "${src}")"
+did="$(basename "${dest}")"
+
+sgroup="$(basename "$(dirname "tests/${src}")")"
+dgroup="$(basename "$(dirname "tests/${dest}")")"
+
+sgroupfile="tests/${sgroup}/group"
+dgroupfile="tests/${sgroup}/group"
+
+git mv "tests/${src}" "tests/${dest}"
+git mv "tests/${src}.out" "tests/${dest}.out"
+sed -e "s/^# FS QA Test No. ${sid}$/# FS QA Test No. ${did}/g" -i "tests/${dest}"
+sed -e "s/^QA output created by ${sid}$/QA output created by ${did}/g" -i "tests/${dest}.out"
+sed -e "s/test-${sid}/test-${did}/g" -i "tests/${dest}.out"
+
+grpline="$(grep "^${sid} " "${sgroupfile}")"
+newgrpline="$(echo "${grpline}" | sed -e "s/^${sid} /${did} /g")"
+
+sed -e "/^${sid}.*$/d" -i "${sgroupfile}"
+cp "${dgroupfile}" "${dgroupfile}.new"
+append "${dgroupfile}.new" "${newgrpline}"
+"${dir}/sort-group.py" "${dgroupfile}.new"
+mv "${dgroupfile}.new" "${dgroupfile}"
+
+echo "Moved \"${src}\" to \"${dest}\"."
+
+exit 0
diff --git a/tools/nextid b/tools/nextid
new file mode 120000
index 0000000..5c31d60
--- /dev/null
+++ b/tools/nextid
@@ -0,0 +1 @@
+sort-group
\ No newline at end of file
diff --git a/tools/sort-group b/tools/sort-group
new file mode 100755
index 0000000..84944ed
--- /dev/null
+++ b/tools/sort-group
@@ -0,0 +1,112 @@
+#!/usr/bin/env python
+import sys
+
+# Sort a group list, carefully preserving comments.
+
+def xfstest_key(key):
+	'''Extract the numeric part of a test name if possible.'''
+	k = 0
+
+	assert type(key) == str
+
+	# No test number at all...
+	if not key[0].isdigit():
+		return key
+
+	# ...otherwise extract as much number as we can.
+	for digit in key:
+		if digit.isdigit():
+			k = k * 10 + int(digit)
+		else:
+			return k
+	return k
+
+def read_group(fd):
+	'''Read the group list, carefully attaching comments to the next test.'''
+	tests = {}
+	comments = None
+
+	for line in fd:
+		sline = line.strip()
+		tokens = sline.split()
+		if len(tokens) == 0 or tokens[0] == '#':
+			if comments == None:
+				comments = []
+			comments.append(sline)
+		else:
+			tests[tokens[0]] = (comments, tokens[1:])
+			comments = None
+	return tests
+
+def sort_keys(keys):
+	'''Separate keys into integer and non-integer tests.'''
+	int_keys = []
+	int_xkeys = []
+	str_keys = []
+
+	# Sort keys into integer(ish) tests and other
+	for key in keys:
+		xkey = xfstest_key(key)
+		if type(xkey) == int:
+			int_keys.append(key)
+			int_xkeys.append(xkey)
+		else:
+			str_keys.append(key)
+	return (int_keys, int_xkeys, str_keys)
+
+def write_sorted(tests, fd):
+	def dump_xkey(xkey):
+		(comments, tokens) = tests[key]
+		if comments:
+			for c in comments:
+				fd.write('%s\n' % c)
+		fd.write('%s %s\n' % (key, ' '.join(tokens)))
+	'''Print tests (and comments) in number order.'''
+
+	(int_keys, ignored, str_keys) = sort_keys(tests.keys())
+	for key in sorted(int_keys, key = xfstest_key):
+		dump_xkey(key)
+	for key in sorted(str_keys):
+		dump_xkey(key)
+
+def sort_main():
+	if '--help' in sys.argv[1:]:
+		print('Usage: %s groupfiles' % sys.argv[0])
+		sys.exit(0)
+
+	for arg in sys.argv[1:]:
+		with open(arg, 'r+') as fd:
+			x = read_group(fd)
+			fd.seek(0, 0)
+			write_sorted(x, fd)
+
+def nextid_main():
+	if '--help' in sys.argv[1:]:
+		print('Usage: %s group[/startid] ' % sys.argv[0])
+		sys.exit(0)
+
+	if len(sys.argv) != 2:
+		print('Specify exactly one group name.')
+		sys.exit(1)
+
+	c = sys.argv[1].split('/')
+	if len(c) > 1:
+		startid = int(c[1])
+	else:
+		startid = 1
+	group = c[0]
+
+	with open('tests/%s/group' % group, 'r') as fd:
+		x = read_group(fd)
+		xkeys = {int(x) for x in sort_keys(x.keys())[1]}
+
+		xid = startid
+		while xid in xkeys:
+			xid += 1
+		print('%s/%d' % (group, xid))
+
+if __name__ == '__main__':
+	if 'nextid' in sys.argv[0]:
+		nextid_main()
+	else:
+		sort_main()

[PATCH 2/2] generic/15[78]: fix error messages in the golden output

[Darrick J. Wong]

Fix the error messages in the golden output for generic/15[78], which
examine the responses to invalid inputs as returned by the
clone/clone_range/extent_same ioctls.  Also fix a filtering omission.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/generic/157     |   12 ++++++++----
 tests/generic/157.out |    2 +-
 tests/generic/158     |   10 +++++++---
 tests/generic/158.out |    8 ++++----
 4 files changed, 20 insertions(+), 12 deletions(-)


diff --git a/tests/generic/157 b/tests/generic/157
index a43fb0d..f3de285 100755
--- a/tests/generic/157
+++ b/tests/generic/157
@@ -75,10 +75,14 @@ mkdir "$TESTDIR1/dir1"
 seq 1 $((2 * BLKSZ / 250)) | while read f; do
 	touch "$TESTDIR1/dir1/$f"
 done
-mknod "$TESTDIR1/dev1" b 8 0
+mknod "$TESTDIR1/dev1" c 1 3
 mkfifo "$TESTDIR1/fifo1"
 sync
 
+_filter_enotty() {
+	sed -e 's/Inappropriate ioctl for device/Operation not supported/g'
+}
+
 echo "Try cross-device reflink"
 _reflink_range "$TESTDIR1/file1" 0 "$TESTDIR2/file1" 0 $BLKSZ
 
@@ -98,13 +102,13 @@ echo "Try to reflink a device"
 _reflink_range "$TESTDIR1/dev1" 0 "$TESTDIR1/file2" 0 $BLKSZ
 
 echo "Try to reflink to a dir"
-_reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/dir1" 0 $BLKSZ
+_reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/dir1" 0 $BLKSZ 2>&1 | _filter_test_dir
 
 echo "Try to reflink to a device"
-_reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/dev1" 0 $BLKSZ
+_reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/dev1" 0 $BLKSZ 2>&1 | _filter_enotty
 
 echo "Try to reflink to a fifo"
-_reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/fifo1" 0 $BLKSZ -n
+_reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/fifo1" 0 $BLKSZ -n 2>&1 | _filter_enotty
 
 echo "Try to reflink an append-only file"
 _reflink_range "$TESTDIR1/file1" 0 "$TESTDIR1/file3" 0 $BLKSZ -a
diff --git a/tests/generic/157.out b/tests/generic/157.out
index 177e7f8..3a690ca 100644
--- a/tests/generic/157.out
+++ b/tests/generic/157.out
@@ -14,7 +14,7 @@ XFS_IOC_CLONE_RANGE: Is a directory
 Try to reflink a device
 XFS_IOC_CLONE_RANGE: Invalid argument
 Try to reflink to a dir
-/mnt/test-157/dir1: Is a directory
+TEST_DIR/test-157/dir1: Is a directory
 Try to reflink to a device
 XFS_IOC_CLONE_RANGE: Operation not supported
 Try to reflink to a fifo
diff --git a/tests/generic/158 b/tests/generic/158
index a499b21..db5d05c 100755
--- a/tests/generic/158
+++ b/tests/generic/158
@@ -76,10 +76,14 @@ mkdir "$TESTDIR1/dir1"
 seq 1 $((2 * BLKSZ / 250)) | while read f; do
 	touch "$TESTDIR1/dir1/$f"
 done
-mknod "$TESTDIR1/dev1" b 8 0
+mknod "$TESTDIR1/dev1" c 1 3
 mkfifo "$TESTDIR1/fifo1"
 sync
 
+_filter_enotty() {
+	sed -e 's/Inappropriate ioctl for device/Invalid argument/g'
+}
+
 echo "Try cross-device dedupe"
 _dedupe_range "$TESTDIR1/file1" 0 "$TESTDIR2/file1" 0 $BLKSZ
 
@@ -96,10 +100,10 @@ echo "Try to dedupe a dir"
 _dedupe_range "$TESTDIR1/dir1" 0 "$TESTDIR1/file2" 0 $BLKSZ
 
 echo "Try to dedupe a device"
-_dedupe_range "$TESTDIR1/dev1" 0 "$TESTDIR1/file2" 0 $BLKSZ
+_dedupe_range "$TESTDIR1/dev1" 0 "$TESTDIR1/file2" 0 $BLKSZ 2>&1 | _filter_enotty
 
 echo "Try to dedupe to a dir"
-_dedupe_range "$TESTDIR1/file1" 0 "$TESTDIR1/dir1" 0 $BLKSZ
+_dedupe_range "$TESTDIR1/file1" 0 "$TESTDIR1/dir1" 0 $BLKSZ 2>&1 | _filter_test_dir
 
 echo "Try to dedupe to a device"
 _dedupe_range "$TESTDIR1/file1" 0 "$TESTDIR1/dev1" 0 $BLKSZ
diff --git a/tests/generic/158.out b/tests/generic/158.out
index 36a3f1f..1210429 100644
--- a/tests/generic/158.out
+++ b/tests/generic/158.out
@@ -12,13 +12,13 @@ dedupe: Invalid argument
 Try to dedupe a dir
 XFS_IOC_FILE_EXTENT_SAME: Is a directory
 Try to dedupe a device
-XFS_IOC_FILE_EXTENT_SAME: Permission denied
+XFS_IOC_FILE_EXTENT_SAME: Invalid argument
 Try to dedupe to a dir
-/mnt/test-158/dir1: Is a directory
+TEST_DIR/test-158/dir1: Is a directory
 Try to dedupe to a device
-dedupe: Permission denied
+dedupe: Operation not supported
 Try to dedupe to a fifo
-dedupe: Permission denied
+dedupe: Operation not supported
 Try to dedupe an append-only file
 Dedupe two files
 Check scratch fs

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

Re: [PATCH 2/2] generic/15[78]: fix error messages in the golden output

[Christoph Hellwig]

> --- a/tests/generic/158.out
> +++ b/tests/generic/158.out
>  Try to dedupe a device
> -XFS_IOC_FILE_EXTENT_SAME: Permission denied
> +XFS_IOC_FILE_EXTENT_SAME: Invalid argument
>  Try to dedupe to a dir
> -/mnt/test-158/dir1: Is a directory
> +TEST_DIR/test-158/dir1: Is a directory
>  Try to dedupe to a device
> -dedupe: Permission denied
> +dedupe: Operation not supported
>  Try to dedupe to a fifo
> -dedupe: Permission denied
> +dedupe: Operation not supported

Shouldn't these be Invalid argument just like the
to a device case above or the clone case?

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] generic/15[78]: fix error messages in the golden output

[Christoph Hellwig]

> Shouldn't these be Invalid argument just like the
> to a device case above or the clone case?

E.g. something like this on top of your patch:


diff --git a/tests/generic/158.out b/tests/generic/158.out
index 1210429..dff3692 100644
--- a/tests/generic/158.out
+++ b/tests/generic/158.out
@@ -16,9 +16,9 @@ XFS_IOC_FILE_EXTENT_SAME: Invalid argument
 Try to dedupe to a dir
 TEST_DIR/test-158/dir1: Is a directory
 Try to dedupe to a device
-dedupe: Operation not supported
+dedupe: Invalid argument
 Try to dedupe to a fifo
-dedupe: Operation not supported
+dedupe: Invalid argument
 Try to dedupe an append-only file
 Dedupe two files
 Check scratch fs

Re: [PATCH 2/2] generic/15[78]: fix error messages in the golden output

[Darrick J. Wong]

On Sat, Nov 21, 2015 at 10:06:44AM -0800, Christoph Hellwig wrote:
> > --- a/tests/generic/158.out
> > +++ b/tests/generic/158.out
> >  Try to dedupe a device
> > -XFS_IOC_FILE_EXTENT_SAME: Permission denied
> > +XFS_IOC_FILE_EXTENT_SAME: Invalid argument
> >  Try to dedupe to a dir
> > -/mnt/test-158/dir1: Is a directory
> > +TEST_DIR/test-158/dir1: Is a directory
> >  Try to dedupe to a device
> > -dedupe: Permission denied
> > +dedupe: Operation not supported
> >  Try to dedupe to a fifo
> > -dedupe: Permission denied
> > +dedupe: Operation not supported
> 
> Shouldn't these be Invalid argument just like the
> to a device case above or the clone case?

I was trying to mirror the behavior of reflink, which spits out EOPNOTSUPP when
the destination isn't a regular file and EINVAL when the source isn't a regular
file.

--D

> 
> Otherwise looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] generic/15[78]: fix error messages in the golden output

[Christoph Hellwig]

On Mon, Nov 23, 2015 at 01:25:33PM -0800, Darrick J. Wong wrote:
> > Shouldn't these be Invalid argument just like the
> > to a device case above or the clone case?
> 
> I was trying to mirror the behavior of reflink, which spits out
> EOPNOTSUPP when the destination isn't a regular file and EINVAL
> when the source isn't a regular file.

clone is called on the destination and takes the source from the
ioctl argument.  dedupe is called on the source and then opens the
destinations, so they're not really comparable.  Btrfs currently
returns EACCES for non-dir, non-regular destinations which look
wrong and I think EINVAL for a mismatch between source and destination
types would make most sense.  I've also prepared a btrfs patch for
this and clone, but I'd like to have consensus on the exact error
first.