| Message ID | 150067468569.30639.11717903404151243816.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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 <darrick.wong@oracle.com> 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 :) > --- > 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? > +{ > + dev="$1" > + knob="$2" > + shortdev="$(_short_dev "${dev}")" > + tagfile="/sys/fs/xfs/${shortdev}/errortag/${knob}" > + > + # Some of the new sysfs errortag knobs were previously available via > + # another sysfs path. > + case "${knob}" in > + "log_bad_crc") > + if [ ! -w "${tagfile}" ]; then > + tagfile="/sys/fs/xfs/${shortdev}/log/log_badcrc_factor" > + fi > + ;; > + "drop_writes") > + if [ ! -w "${tagfile}" ]; then > + tagfile="/sys/fs/xfs/${shortdev}/drop_writes" > + fi > + if [ ! -w "${tagfile}" ]; then > + tagfile="/sys/fs/xfs/${shortdev}/fail_writes" > + fi > + ;; > + *) > + ;; > + esac > + > + echo "${tagfile}" > +} > + > # 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? > _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. > + 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 > + 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
On Wed, Aug 02, 2017 at 04:37:42PM +0800, Eryu Guan wrote: > On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > 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 <darrick.wong@oracle.com> > > 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 :) > > > --- > > 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. ... > > # 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 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. 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 > > _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. > > > + 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 > > > + 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 fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 02, 2017 at 10:30:10AM -0400, Brian Foster wrote: > On Wed, Aug 02, 2017 at 04:37:42PM +0800, Eryu Guan wrote: > > On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > 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 <darrick.wong@oracle.com> > > > > 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 :) > > > > > --- > > > 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() { ... } > ... > > > # 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. > 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. 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 > 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 > > > > _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. > > > > > + 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 > > > > > + 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 fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: > > On Wed, Aug 02, 2017 at 04:37:42PM +0800, Eryu Guan wrote: > > > On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > > > 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 <darrick.wong@oracle.com> > > > > > > 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 :) > > > > > > > --- > > > > 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() > { > ... > } > > > ... > > > > # 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. > > > 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 > > > 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 > > > > > > _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. > > > > > > > + 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 > > > > > > > + 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 -- 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
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: > > On Wed, Aug 02, 2017 at 04:37:42PM +0800, Eryu Guan wrote: > > > On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > > > 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 <darrick.wong@oracle.com> > > > > > > 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 :) > > > > > > > --- > > > > 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() This looks better to me, thanks! > { > ... > } > > > ... > > > > # 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. > > > 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. > > 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. Agreed, the order doesn't really matter here, I'm fine with either way. Thanks, Eryu -- 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
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() +{ + dev="$1" + knob="$2" + shortdev="$(_short_dev "${dev}")" + tagfile="/sys/fs/xfs/${shortdev}/errortag/${knob}" + + # Some of the new sysfs errortag knobs were previously available via + # another sysfs path. + case "${knob}" in + "log_bad_crc") + if [ ! -w "${tagfile}" ]; then + tagfile="/sys/fs/xfs/${shortdev}/log/log_badcrc_factor" + fi + ;; + "drop_writes") + if [ ! -w "${tagfile}" ]; then + tagfile="/sys/fs/xfs/${shortdev}/drop_writes" + fi + if [ ! -w "${tagfile}" ]; then + tagfile="/sys/fs/xfs/${shortdev}/fail_writes" + fi + ;; + *) + ;; + esac + + echo "${tagfile}" +} + # 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 + _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}." + 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}." + 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."