Re: [RFC PATCH] check: try to fix the test device if it gets corrupted

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, Jul 15, 2017 at 09:30:58PM -0400, Theodore Ts'o wrote:
> On Mon, Mar 27, 2017 at 04:51:03PM +0800, Eryu Guan wrote:
> > 
> > Sorry I lost this thread, I thought I've replied but apparently I didn't..
> > 
> > I agreed with both of you and Darrick, I think we can try to repair the
> > corrupted test fs, and if repair succeeds we can continue the test, and
> > stop running the whole test if repair fails.
> 
> Sorry for the delay in getting back to this.  Things got busy and this
> got dropped on my end.
> 
> I've fixed the whitespace nits that you pointed out and am using _log_err.
> 
> > I think we should try to fix other filesystems too?
> 
> Hmm...  yeah.  The main reason why I hadn't was because xfs has
> _scratch_xfs_repair and _scratch_xfs_check, which are very similar.
> But _check_xfs_test_fs looks *very* different from _scratch_xfs_check,
> and I'm not sure why.

_check_xfs_filesystem is a helper that does all the checking that we can
do to an xfs filesystem (online fsck, the old xfs_check, and the newer
xfs_repair).  AFAICT that's what all new xfstest code should be calling.
xfs_check is obsolete.

_check_xfs_test_fs calls _check_xfs_filesystem and then, weirdly, calls
a nonexistent tool calld xfs_repair_ipaths to .... I assume fix all the
problems that can crop with the Irix XFS parent pointer implementation.
None of that exists on Linux and Irix is long dead, so I assume the
"check for ipath consistency" can go away entirely.

_scratch_xfs_check calls xfs_check directly, so I think it should get
replaced with _check_scratch_fs, which calls _check_xfs_filesystem.

<keep scrolling>

> 
> So I've created a _repair_xfs_test_fs which is modelled after the
> simpler _scratch_xfs_repair function, but I'm not 100% sure that is
> correct.
> 
> Anyways, WDYT?
> 
> 						- Ted
> 
> From 96a13cc22878ee5c016a606d76f8e9a6bd84eb20 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Wed, 1 Mar 2017 19:54:08 -0500
> Subject: [PATCH] check: try to fix the test device if it gets corrupted
> 
> If the test device gets corrupted all subsequent tests will fail.  To
> prevent this from causing all subsequent tests to be useless, try
> repair the file system on TEST_DEV if possible.  We don't need to do
> this with the scratch device since that file system gets recreated
> each time anyway.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  check      |  7 ++++++-
>  common/rc  | 41 +++++++++++++++++++++++++++++++++++++++++
>  common/xfs | 12 ++++++++++++
>  3 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/check b/check
> index f8db3cd6..d89d2e91 100755
> --- a/check
> +++ b/check
> @@ -476,7 +476,12 @@ _summary()
>  _check_filesystems()
>  {
>  	if [ -f ${RESULT_DIR}/require_test ]; then
> -		_check_test_fs || err=true
> +		if ! _check_test_fs ; then
> +			err=true
> +			echo "Trying to repair broken TEST_DEV file system"
> +			_repair_test_fs
> +			_test_mount
> +		fi
>  		rm -f ${RESULT_DIR}/require_test*
>  	fi
>  	if [ -f ${RESULT_DIR}/require_scratch ]; then
> diff --git a/common/rc b/common/rc
> index 328b6b07..d37a1611 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1201,6 +1201,47 @@ _repair_scratch_fs()
>      esac
>  }
>  
> +_repair_test_fs()
> +{
> +	case $FSTYP in
> +	xfs)
> +		_repair_xfs_test_fs "$@" >$tmp.repair 2>&1
> +		res=$?
> +		if [ "$res" -ne 0 ]; then
> +			echo "xfs_repair returns $res; replay log?" >>$tmp.repair
> +			_test_mount
> +			res=$?
> +			if [ $res -gt 0 ]; then
> +				echo "mount returns $res; zap log?" >>$tmp.repair
> +				_xfs_repair_test_fs -L >>$tmp.repair 2>&1
> +				echo "log zap returns $?" >> $tmp.repair
> +			else
> +				umount "$TEST_DEV"
> +			fi
> +			_xfs_repair_test_fs "$@" >>$tmp.repair 2>&1
> +			res=$?
> +		fi

Structurally this all looks ok, but it's a little weird that we have
_scratch_xfs_repair for the scratch device (object-verb) but
_repair_test_fs (verb-object) for the test device.

--D

> +		;;
> +	*)
> +		# Let's hope fsck -y suffices...
> +		fsck -t $FSTYP -fy $TEST_DEV >$tmp.repair 2>&1
> +		res=$?
> +		if test "$res" -lt 4 ; then
> +			res=0
> +		fi
> +		;;
> +	esac
> +	if [ $res -ne 0 ]; then
> +		_log_err "_repair_test_fs: failed, err=$res"
> +		echo "*** fsck.$FSTYP output ***"	>>$seqres.full
> +		cat $tmp.repair				>>$seqres.full
> +		echo "*** end fsck.$FSTYP output"	>>$seqres.full
> +
> +	fi
> +	rm -f $tmp.repair
> +	return $res
> +}
> +
>  _get_pids_by_name()
>  {
>      if [ $# -ne 1 ]
> diff --git a/common/xfs b/common/xfs
> index a1ee3847..c8f4e46b 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -443,6 +443,18 @@ _check_xfs_test_fs()
>  	fi
>  }
>  
> +# modeled after _scratch_xfs_repair
> +_repair_xfs_test_fs()
> +{
> +	TEST_OPTIONS=""
> +	[ "$USE_EXTERNAL" = yes -a ! -z "$TEST_LOGDEV" ] && \
> +		TEST_OPTIONS="-l$TEST_LOGDEV"
> +	[ "$USE_EXTERNAL" = yes -a ! -z "$TEST_RTDEV" ] && \
> +		TEST_OPTIONS=$TEST_OPTIONS" -r$TEST_RTDEV"
> +	[ "$LARGE_TEST_DEV" = yes ] && TEST_OPTIONS=$TEST_OPTIONS" -t"
> +	$XFS_REPAIR_PROG $TEST_OPTIONS $* $TEST_DEV
> +}
> +
>  _require_xfs_test_rmapbt()
>  {
>  	_require_test
> -- 
> 2.11.0.rc0.7.gbe5a750
> 
> --
> 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