On 9/12/16 12:13 AM, Xiao Yang wrote:
> The obsolete xfs_repair always cleared the log regardless of whether it
> is corrupted. However current xfs_repair only cleared the log when -L
> option is specified, so xfs_repair without any options failed to clear log
> on newer xfsprogs. If xfs_repair failed to clear log, xfs_repair -L option
> should be used to clear it.
>
> Signed-off-by: Xiao Yang <yangx.jy@xxxxxxxxxxxxxx>
> ---
> common/rc | 10 ++++++----
> tests/xfs/098 | 2 +-
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index 04039a4..fda108c 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1134,16 +1134,18 @@ _scratch_xfs_repair()
> $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV
> }
>
> -# Repair scratch filesystem. Returns 0 if the FS is good to go (either no
> -# errors found or errors were fixed) and nonzero otherwise; also spits out
> -# a complaint on stderr if fsck didn't tell us that the FS is good to go.
> +# Repair scratch filesystem. Return 2 if the filesystem has valuable
> +# metadata changes in log which needs to be replayed, 1 if there's
> +# corruption left to be fixed or can't find log head and tail or some
> +# other errors happened, and 0 if nothing wrong or all the corruptions
> +# were fixed.
I'm sorry to nitpick; this looks almost correct to me....
I think the problem here is really due to a bug in xfsprogs, (see patch
[PATCH] xfs_repair: exit with status 2 if log dirtiness is unknown sent
to the xfs list today), but we do need to handle binaries between 4.3.0
and if/when that fix gets applied, so catching any non-zero return value
below does make sense to me.
But the new comment above is very xfs-specific, and _repair_scratch_fs
is a generic function (it has a *) default $FSTYP case...)
And even if /xfs_repair/ returns 2, the bash function
_repair_scratch_fs() won't; $res gets overwritten by the last
call to xfs_repair, at which point there should be no dirty log.
So _repair_scratch_fs() really only returns 0 or 1, even if xfs_repair
may have a return value of 2. So the new comment is not correct.
To fix that, I would simply leave the comment unchanged.
The other remaining problem I see is this, on a CONFIG_XFS_WARN kernel:
xfs/098 12s ... 12s
_check_dmesg: something found in dmesg (see /mnt/test2/git/xfstests/results//xfs/098.dmesg)
Ran: xfs/098
Failures: xfs/098
Failed 1 of 1 tests
because the failed mount issued warnings, at least on a CONFIG_XFS_WARN build:
[249062.871158] XFS (sdb2): log mount failed
[249063.170109] XFS (sdb2): Mounting V5 Filesystem
[249063.266522] XFS (sdb2): Log inconsistent (didn't find previous header)
[249063.273135] XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line: 540
...
so we should probably have a _disable_dmesg_check; I'm not sure if it would
be better to put it in _repair_scratch_fs in the "mount failed, zap
the log" case, or to put it in test 098 directly. I think it would
be better to put it in 098, because we know we are dealing with a corrupt
log and can expect the message; putting it in _repair_scratch_fs may mask
problems on other tests that use it.
Thanks,
-Eric
> _repair_scratch_fs()
> {
> case $FSTYP in
> xfs)
> _scratch_xfs_repair "$@" 2>&1
> res=$?
> - if [ "$res" -eq 2 ]; then
> + if [ "$res" -ne 0 ]; then
> echo "xfs_repair returns $res; replay log?"
> _scratch_mount
> res=$?
> diff --git a/tests/xfs/098 b/tests/xfs/098
> index d91d617..3743e78 100755
> --- a/tests/xfs/098
> +++ b/tests/xfs/098
> @@ -93,7 +93,7 @@ echo "+ mount image"
> _scratch_mount 2>/dev/null && _fail "mount should not succeed"
>
> echo "+ repair fs"
> -_scratch_xfs_repair >> $seqres.full 2>&1
> +_repair_scratch_fs >> $seqres.full 2>&1
>
> echo "+ mount image (2)"
> _scratch_mount
>
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
