From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from cn.fujitsu.com ([59.151.112.132]:40546 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751135AbcIMGMf (ORCPT ); Tue, 13 Sep 2016 02:12:35 -0400 Message-ID: <57D798C5.2010207@cn.fujitsu.com> Date: Tue, 13 Sep 2016 14:12:21 +0800 From: Xiao Yang MIME-Version: 1.0 Subject: Re: [PATCH v3] xfs/098: fix xfs_repair on newer xfsprogs References: <20160909122831.GD12847@dhcp12-143.nay.redhat.com> <1473657237-13148-1-git-send-email-yangx.jy@cn.fujitsu.com> <8daf0b97-0614-6dba-4477-f52c4c7290e1@sandeen.net> In-Reply-To: <8daf0b97-0614-6dba-4477-f52c4c7290e1@sandeen.net> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: fstests-owner@vger.kernel.org To: Eric Sandeen Cc: fstests@vger.kernel.org, zlang@redhat.com, darrick.wong@oracle.com List-ID: On 2016/09/12 20:59, Eric Sandeen wrote: > 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 >> --- >> 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. > > Hi Eric Thanks for your suggestion, i agree with you. I will 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 > > OK, you are right. I got the same warnings on a CONFIG_XFS_WARN build, so i will add _disable_dmesg_check in 098 as you said. Thanks, Xiao Yang >> _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 >> > > . >