On Wed, Aug 02, 2017 at 08:52:57AM -0700, Darrick J. Wong wrote:

On Wed, Aug 02, 2017 at 10:30:10AM -0400, Brian Foster wrote:

quoted

On Wed, Aug 02, 2017 at 04:37:42PM +0800, Eryu Guan wrote:

quoted

On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote:

quoted

From: Darrick J. Wong <redacted>

Refactor the XFS error injection helpers to use the new errortag
interface to configure error injection.  If that isn't present, fall
back either to the xfs_io/ioctl based injection or the older sysfs
knobs.  Refactor existing testcases to use the new helpers.

Signed-off-by: Darrick J. Wong <redacted>

This looks good to me overall, but I still perfer let other people
who're more familar with XFS errortag and error injection to review too.
While I do have some questions/comments :)

quoted

---
 common/inject |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 tests/xfs/141 |    5 +++--
 tests/xfs/196 |   17 ++++++-----------
 3 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/common/inject b/common/inject
index 8ecc290..9aa24de 100644
--- a/common/inject
+++ b/common/inject

@@ -35,10 +35,46 @@ _require_error_injection()
 	esac
 }
 
+# Find a given xfs mount's errortag injection knob in sysfs
+_find_xfs_mount_errortag_knob()

While The function name and comment both indicate it needs a mounted
XFS, it seems weird that the first argument is expected to be a block
device. And do we need to check if the given device is really mounted?

The xfs per-mount sysfs knobs distinguish between mounts via the
block device name.

What if we rename the helper and change its documentation as such?

# Find the errortag injection knob in sysfs for a given xfs mount's
# block device.
_find_xfs_mountdev_errortag_knob()
{
	...
}

quoted

...

quoted

quoted

 # Requires that xfs_io inject command knows about this error type
 _require_xfs_io_error_injection()
 {
 	type="$1"
+
+	# Can we find the error injection knobs via the new errortag
+	# configuration mechanism?
+	test -w "$(_find_xfs_mount_errortag_knob "${TEST_DEV}" "${type}")" && return
+

As this check goes prior to the _require_error_injection check, so I
assume this new errortag framework doesn't depend on a debug built XFS,
can I?

It depends on the sysfs knobs, and the sysfs knobs in turn depend on
XFS_DEBUG=y.

quoted

It does depend on debug mode so it probably makes sense to push this
after the _require_error_injection check. That way the DEBUG=0 message
has precedent over a message regarding lack of support for a particular
knob.

Hm?  This is what _require_xfs_io_error_injection does:

First we compute the path to the knob if it exists
(_find_xfs_mount_errortag_knob).

Then we check if that path is writable.  If it is, error injection is
enabled (which presumably means XFS_DEBUG=y) and we can continue.

If not, then we use _require_error_injection to bail out if XFS_DEBUG=n.

Ah, right. I misinterpreted that the lack of the knob would fall through
to the subsequent checks, so there's no difference with respect to the
resulting message if DEBUG=0. It still looks cleaner to me to check
debug mode first, fwiw, but not a big deal either way.

Brian

If we don't bail out, XFS_DEBUG=y and so we check if the xfs_io inject
help page knows about the error type, and bail out if it doesn't.


In the end it doesn't really matter if we look for XFS_DEBUG=y before or
after we look for the new sysfs knob.  I figured it was simple enough to
assume that if the knob is present and writable, then our preconditions
are satisfied and it's ok to proceed with the injection test.

That said, I also interpreted the "look for evidence of XFS_DEBUG=y" and
"see if xfs_io inject knows about the specific error tag" sequence as a
best effort work around for the fact that the ioctl-based injection
mechanism isn't directly discoverable at all, so instead we assume it's
safe to proceed if we can detect a debug kernel and xfs_io is new enough
to issue the request.

--D

quoted

Outside of Eryu's further comments, the rest looks good to me. This
reminds me that I still need to post the latest log tail overwrite test
that depends on this, now that the kernel bits have been reviewed...

Brian

quoted

quoted

 	_require_error_injection
 
 	# NOTE: We can't actually test error injection here because xfs

@@ -54,16 +90,34 @@ _require_xfs_io_error_injection()
 _test_inject_error()
 {
 	type="$1"
+	value="$2"
 
-	$XFS_IO_PROG -x -c "inject $type" $TEST_DIR
+	knob="$(_find_xfs_mount_errortag_knob "${TEST_DEV}" "${type}")"
+	if [ -w "${knob}" ]; then
+		test -z "${value}" && value="default"
+		echo -n "${value}" > "${knob}"
+	elif [ -z "${value}" ] || [ "${value}" = "default" ]; then
+		$XFS_IO_PROG -x -c "inject $type" $TEST_DIR
+	else
+		_notrun "Cannot inject error ${type} value ${value}."

_require_xfs_io_error_injection already made sure we can do error
jection, it looks like a bug somewhere to me if we can't inject error
here, either wanted to inject error without checking the support status
first or an implementation bug in the error injection framework in
xfstests. So "_fail" might be the right choice.

quoted

+	fi
 }
 
 # Inject an error into the scratch fs
 _scratch_inject_error()
 {
 	type="$1"
+	value="$2"
 
-	$XFS_IO_PROG -x -c "inject $type" $SCRATCH_MNT
+	knob="$(_find_xfs_mount_errortag_knob "${SCRATCH_DEV}" "${type}")"
+	if [ -w "${knob}" ]; then
+		test -z "${value}" && value="default"
+		echo -n "${value}" > "${knob}"
+	elif [ -z "${value}" ] || [ "${value}" = "default" ]; then
+		$XFS_IO_PROG -x -c "inject $type" $SCRATCH_MNT
+	else
+		_notrun "Cannot inject error ${type} value ${value}."

Same here.

Thanks,
Eryu

quoted

+	fi
 }
 
 # Unmount and remount the scratch device, dumping the log

diff --git a/tests/xfs/141 b/tests/xfs/141
index 56ff14e..f61e524 100755
--- a/tests/xfs/141
+++ b/tests/xfs/141

@@ -47,13 +47,14 @@ rm -f $seqres.full
 
 # get standard environment, filters and checks
 . ./common/rc
+. ./common/inject
 
 # real QA test starts here
 
 # Modify as appropriate.
 _supported_fs xfs
 _supported_os Linux
-_require_xfs_sysfs $(_short_dev $TEST_DEV)/log/log_badcrc_factor
+_require_xfs_io_error_injection "log_bad_crc"
 _require_scratch
 _require_command "$KILLALL_PROG" killall
 

@@ -69,7 +70,7 @@ for i in $(seq 1 5); do
 	# (increase this value to run fsstress longer).
 	factor=$((RANDOM % 100 + 1))
 	echo iteration $i log_badcrc_factor: $factor >> $seqres.full 2>&1
-	echo $factor > /sys/fs/xfs/$sdev/log/log_badcrc_factor
+	_scratch_inject_error "log_bad_crc" "$factor"
 
 	# Run fsstress until the filesystem shuts down. It will shut down
 	# automatically when error injection triggers.

diff --git a/tests/xfs/196 b/tests/xfs/196
index e9b0649..fe3f570 100755
--- a/tests/xfs/196
+++ b/tests/xfs/196

@@ -45,6 +45,7 @@ _cleanup()
 # get standard environment, filters and checks
 . ./common/rc
 . ./common/punch
+. ./common/inject
 
 # real QA test starts here
 rm -f $seqres.full

@@ -53,13 +54,7 @@ rm -f $seqres.full
 _supported_fs generic
 _supported_os Linux
 _require_scratch
-
-DROP_WRITES="drop_writes"
-# replace "drop_writes" with "fail_writes" for old kernel
-if [ -f /sys/fs/xfs/$(_short_dev $TEST_DEV)/fail_writes ];then
-	DROP_WRITES="fail_writes"
-fi
-_require_xfs_sysfs $(_short_dev $TEST_DEV)/${DROP_WRITES}
+_require_xfs_io_error_injection "drop_writes"
 
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount

@@ -72,7 +67,7 @@ bytes=$((64 * 1024))
 $XFS_IO_PROG -f -c "pwrite 0 $bytes" $file >> $seqres.full 2>&1
 
 # Enable write drops. All buffered writes are dropped from this point on.
-echo 1 > /sys/fs/xfs/$sdev/$DROP_WRITES
+_scratch_inject_error "drop_writes" 1
 
 # Write every other 4k range to split the larger delalloc extent into many more
 # smaller extents. Use pwrite because with write failures enabled, all

@@ -89,7 +84,7 @@ for i in $(seq 4096 8192 $endoff); do
 	$XFS_IO_PROG -c "pwrite $i 4k" $file >> $seqres.full 2>&1
 done
 
-echo 0 > /sys/fs/xfs/$sdev/$DROP_WRITES
+_scratch_inject_error "drop_writes" 0
 
 _scratch_cycle_mount
 $XFS_IO_PROG -c 'bmap -vp' $file | _filter_bmap

@@ -104,9 +99,9 @@ for offset in $(seq 0 100 500); do
 	$XFS_IO_PROG -fc "pwrite ${offset}m 100m" $file >> $seqres.full 2>&1
 
 	punchoffset=$((offset + 75))
-	echo 1 > /sys/fs/xfs/$sdev/$DROP_WRITES
+	_scratch_inject_error "drop_writes"
 	$XFS_IO_PROG -c "pwrite ${punchoffset}m 4k" $file >> $seqres.full 2>&1
-	echo 0 > /sys/fs/xfs/$sdev/$DROP_WRITES
+	_scratch_inject_error "drop_writes" 0
 done
 
 echo "Silence is golden."

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html