| Message ID | 20180618174433.3069-2-mbenatto@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: > Recently we found out xfs_repair were not repairing > root inode parent pointer when root inode is on short-form > and parent points to an invalid inode number (refer to: > "xfs_repair: Fix root inode's parent when it's bogus for sf > directory" on xfs-devel list). > > This test checks if xfs_repair successfully repair the > filesystem in the scenario mentioned before. > > Signed-off-by: Marco Benatto <mbenatto@redhat.com> Looks ok, I think... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/450.out | 1 + > tests/xfs/group | 1 + > 3 files changed, 55 insertions(+) > create mode 100755 tests/xfs/450 > create mode 100644 tests/xfs/450.out > > diff --git a/tests/xfs/450 b/tests/xfs/450 > new file mode 100755 > index 0000000..dc7f244 > --- /dev/null > +++ b/tests/xfs/450 > @@ -0,0 +1,53 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. > +# > +# FS QA Test 450 > +# > +# Make sure xfs_repair can repair root inode parent's pointer > +# when it contains a bogus ino when it's using shot form directory > +# > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > + > +status=1 # failure is the default! > + > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > + > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs xfs > +_supported_os Linux > +_require_scratch > + > + > +_scratch_mkfs >> /dev/null 2>&1 > + > +rootino=$(_scratch_xfs_get_metadata_field 'rootino' 'sb 0') > + > +prefix=$(_scratch_get_sfdir_prefix ${rootino} || \ > + _fail "Cannot determine sfdir prefix") > + > + > +# Corrupt root inode parent pointer > +_scratch_xfs_set_metadata_field "${prefix}.hdr.parent.i4" 0 "inode ${rootino}"\ > + >> $seqres.full > + > +_scratch_xfs_repair >> $seqres.full 2>&1 > + > +_scratch_xfs_repair -n >> $seqres.full 2>&1 > + > +if [ $? -eq 1 ] > +then > + _fail "xfs_repair failed to repair filesystem" > +fi > + > +echo "OK" > +status=0 > +exit > diff --git a/tests/xfs/450.out b/tests/xfs/450.out > new file mode 100644 > index 0000000..d86bac9 > --- /dev/null > +++ b/tests/xfs/450.out > @@ -0,0 +1 @@ > +OK > diff --git a/tests/xfs/group b/tests/xfs/group > index 932ab90..3a1db0b 100644 > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -447,3 +447,4 @@ > 447 auto mount > 448 auto quick fuzzers > 449 auto quick > +450 auto quick > -- > 2.9.5 > > -- > 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 Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: > Recently we found out xfs_repair were not repairing > root inode parent pointer when root inode is on short-form > and parent points to an invalid inode number (refer to: > "xfs_repair: Fix root inode's parent when it's bogus for sf > directory" on xfs-devel list). > > This test checks if xfs_repair successfully repair the > filesystem in the scenario mentioned before. > > Signed-off-by: Marco Benatto <mbenatto@redhat.com> > --- > tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/450.out | 1 + > tests/xfs/group | 1 + > 3 files changed, 55 insertions(+) > create mode 100755 tests/xfs/450 > create mode 100644 tests/xfs/450.out > > diff --git a/tests/xfs/450 b/tests/xfs/450 > new file mode 100755 > index 0000000..dc7f244 > --- /dev/null > +++ b/tests/xfs/450 > @@ -0,0 +1,53 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. > +# > +# FS QA Test 450 > +# > +# Make sure xfs_repair can repair root inode parent's pointer > +# when it contains a bogus ino when it's using shot form directory > +# > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > + > +status=1 # failure is the default! > + > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > + > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs xfs > +_supported_os Linux > +_require_scratch > + > + > +_scratch_mkfs >> /dev/null 2>&1 > + > +rootino=$(_scratch_xfs_get_metadata_field 'rootino' 'sb 0') > + > +prefix=$(_scratch_get_sfdir_prefix ${rootino} || \ > + _fail "Cannot determine sfdir prefix") > + > + > +# Corrupt root inode parent pointer > +_scratch_xfs_set_metadata_field "${prefix}.hdr.parent.i4" 0 "inode ${rootino}"\ > + >> $seqres.full > + > +_scratch_xfs_repair >> $seqres.full 2>&1 > + > +_scratch_xfs_repair -n >> $seqres.full 2>&1 > + > +if [ $? -eq 1 ] > +then > + _fail "xfs_repair failed to repair filesystem" > +fi Isn't this last "filesystem is not corrupt" check run by the harness via _check_scratch_fs()? (Also, "if [ .. ]; then" is the preferred fstests style) > + > +echo "OK" There's no test output, so the normal thing to do is this after setting up the test: echo "Silence is golden" We do that early on so that when something goes wrong the bad output file immediately tells you that no output was expected. > @@ -447,3 +447,4 @@ > 447 auto mount > 448 auto quick fuzzers > 449 auto quick > +450 auto quick also the metadata and repair groups should be added here. Cheers, Dave.
On Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: > Recently we found out xfs_repair were not repairing > root inode parent pointer when root inode is on short-form > and parent points to an invalid inode number (refer to: > "xfs_repair: Fix root inode's parent when it's bogus for sf > directory" on xfs-devel list). > > This test checks if xfs_repair successfully repair the > filesystem in the scenario mentioned before. > > Signed-off-by: Marco Benatto <mbenatto@redhat.com> > --- > tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/450.out | 1 + > tests/xfs/group | 1 + > 3 files changed, 55 insertions(+) > create mode 100755 tests/xfs/450 > create mode 100644 tests/xfs/450.out > > diff --git a/tests/xfs/450 b/tests/xfs/450 > new file mode 100755 > index 0000000..dc7f244 > --- /dev/null > +++ b/tests/xfs/450 > @@ -0,0 +1,53 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. > +# > +# FS QA Test 450 > +# > +# Make sure xfs_repair can repair root inode parent's pointer > +# when it contains a bogus ino when it's using shot form directory > +# > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > + > +status=1 # failure is the default! Apart from Dave's comments, there're some common definitions missing too, e.g. 'tmp' and 'here', please use './new xfs' to generate new test template. BTW, I've applied the first patch, no need to resend it in v2. 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
On 6/21/18 9:37 PM, Eryu Guan wrote: > On Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: >> Recently we found out xfs_repair were not repairing >> root inode parent pointer when root inode is on short-form >> and parent points to an invalid inode number (refer to: >> "xfs_repair: Fix root inode's parent when it's bogus for sf >> directory" on xfs-devel list). >> >> This test checks if xfs_repair successfully repair the >> filesystem in the scenario mentioned before. >> >> Signed-off-by: Marco Benatto <mbenatto@redhat.com> >> --- >> tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/xfs/450.out | 1 + >> tests/xfs/group | 1 + >> 3 files changed, 55 insertions(+) >> create mode 100755 tests/xfs/450 >> create mode 100644 tests/xfs/450.out >> >> diff --git a/tests/xfs/450 b/tests/xfs/450 >> new file mode 100755 >> index 0000000..dc7f244 >> --- /dev/null >> +++ b/tests/xfs/450 >> @@ -0,0 +1,53 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. >> +# >> +# FS QA Test 450 >> +# >> +# Make sure xfs_repair can repair root inode parent's pointer >> +# when it contains a bogus ino when it's using shot form directory >> +# >> +seq=`basename $0` >> +seqres=$RESULT_DIR/$seq >> + >> +status=1 # failure is the default! > > Apart from Dave's comments, there're some common definitions missing > too, e.g. 'tmp' and 'here', please use './new xfs' to generate new test > template. Eryu, I think I suggested that Marco could remove the "tmp" definition because it's not used in the script. Is there some reason to keep it? The script did start out with a "./new xfs" generation template. Oh, I bet some helpers depend on it... sorry, my mistake. Oops. (though maybe tmp should just get hoisted to the harness and not be required in every script, but I digress ...) Thanks, -Eric > BTW, I've applied the first patch, no need to resend it in v2. > > 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
On Thu, Jun 21, 2018 at 09:58:25PM -0500, Eric Sandeen wrote: > On 6/21/18 9:37 PM, Eryu Guan wrote: > > On Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: > >> Recently we found out xfs_repair were not repairing > >> root inode parent pointer when root inode is on short-form > >> and parent points to an invalid inode number (refer to: > >> "xfs_repair: Fix root inode's parent when it's bogus for sf > >> directory" on xfs-devel list). > >> > >> This test checks if xfs_repair successfully repair the > >> filesystem in the scenario mentioned before. > >> > >> Signed-off-by: Marco Benatto <mbenatto@redhat.com> > >> --- > >> tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> tests/xfs/450.out | 1 + > >> tests/xfs/group | 1 + > >> 3 files changed, 55 insertions(+) > >> create mode 100755 tests/xfs/450 > >> create mode 100644 tests/xfs/450.out > >> > >> diff --git a/tests/xfs/450 b/tests/xfs/450 > >> new file mode 100755 > >> index 0000000..dc7f244 > >> --- /dev/null > >> +++ b/tests/xfs/450 > >> @@ -0,0 +1,53 @@ > >> +#! /bin/bash > >> +# SPDX-License-Identifier: GPL-2.0 > >> +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. > >> +# > >> +# FS QA Test 450 > >> +# > >> +# Make sure xfs_repair can repair root inode parent's pointer > >> +# when it contains a bogus ino when it's using shot form directory > >> +# > >> +seq=`basename $0` > >> +seqres=$RESULT_DIR/$seq > >> + > >> +status=1 # failure is the default! > > > > Apart from Dave's comments, there're some common definitions missing > > too, e.g. 'tmp' and 'here', please use './new xfs' to generate new test > > template. > > Eryu, I think I suggested that Marco could remove the "tmp" definition > because it's not used in the script. Is there some reason to keep it? > The script did start out with a "./new xfs" generation template. > > Oh, I bet some helpers depend on it... sorry, my mistake. Oops. > > (though maybe tmp should just get hoisted to the harness and > not be required in every script, but I digress ...) You mean like I proposed a couple of weeks ago along with the spdx license tag updates? https://www.spinics.net/lists/fstests/msg09849.html that's next on my list of "Big cleanups for fstests To Do" list. Cheers, Dave.
Hi Eryuy et al, firstly thanks for the reviews. OK, I'll resend just the test itself (without the one which hoist the helper function for sfdir format). Given Eric's and Dave's updates, should I re-include the common definitions even those being unused within this test scope? If those are used by helpers or any other flow on xfstest I do agree with Eric and Dave about hoist it. Thanks, On Fri, Jun 22, 2018 at 12:54 AM, Dave Chinner <david@fromorbit.com> wrote: > On Thu, Jun 21, 2018 at 09:58:25PM -0500, Eric Sandeen wrote: >> On 6/21/18 9:37 PM, Eryu Guan wrote: >> > On Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: >> >> Recently we found out xfs_repair were not repairing >> >> root inode parent pointer when root inode is on short-form >> >> and parent points to an invalid inode number (refer to: >> >> "xfs_repair: Fix root inode's parent when it's bogus for sf >> >> directory" on xfs-devel list). >> >> >> >> This test checks if xfs_repair successfully repair the >> >> filesystem in the scenario mentioned before. >> >> >> >> Signed-off-by: Marco Benatto <mbenatto@redhat.com> >> >> --- >> >> tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> tests/xfs/450.out | 1 + >> >> tests/xfs/group | 1 + >> >> 3 files changed, 55 insertions(+) >> >> create mode 100755 tests/xfs/450 >> >> create mode 100644 tests/xfs/450.out >> >> >> >> diff --git a/tests/xfs/450 b/tests/xfs/450 >> >> new file mode 100755 >> >> index 0000000..dc7f244 >> >> --- /dev/null >> >> +++ b/tests/xfs/450 >> >> @@ -0,0 +1,53 @@ >> >> +#! /bin/bash >> >> +# SPDX-License-Identifier: GPL-2.0 >> >> +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. >> >> +# >> >> +# FS QA Test 450 >> >> +# >> >> +# Make sure xfs_repair can repair root inode parent's pointer >> >> +# when it contains a bogus ino when it's using shot form directory >> >> +# >> >> +seq=`basename $0` >> >> +seqres=$RESULT_DIR/$seq >> >> + >> >> +status=1 # failure is the default! >> > >> > Apart from Dave's comments, there're some common definitions missing >> > too, e.g. 'tmp' and 'here', please use './new xfs' to generate new test >> > template. >> >> Eryu, I think I suggested that Marco could remove the "tmp" definition >> because it's not used in the script. Is there some reason to keep it? >> The script did start out with a "./new xfs" generation template. >> >> Oh, I bet some helpers depend on it... sorry, my mistake. Oops. >> >> (though maybe tmp should just get hoisted to the harness and >> not be required in every script, but I digress ...) > > You mean like I proposed a couple of weeks ago along with the > spdx license tag updates? > > https://www.spinics.net/lists/fstests/msg09849.html > > that's next on my list of "Big cleanups for fstests To Do" list. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
Hello all, just FYI just sent out v2 covering Dave's and Eryu's suggestions. Thanks, On Fri, Jun 22, 2018 at 12:17 PM, Marco Benatto <mbenatto@redhat.com> wrote: > Hi Eryuy et al, > > firstly thanks for the reviews. > > OK, I'll resend just the test itself (without the one which hoist the > helper function for sfdir format). > > > Given Eric's and Dave's updates, should I re-include the common > definitions even those being unused within > this test scope? > > If those are used by helpers or any other flow on xfstest I do agree > with Eric and Dave about hoist it. > > Thanks, > > > On Fri, Jun 22, 2018 at 12:54 AM, Dave Chinner <david@fromorbit.com> wrote: >> On Thu, Jun 21, 2018 at 09:58:25PM -0500, Eric Sandeen wrote: >>> On 6/21/18 9:37 PM, Eryu Guan wrote: >>> > On Mon, Jun 18, 2018 at 02:44:33PM -0300, Marco Benatto wrote: >>> >> Recently we found out xfs_repair were not repairing >>> >> root inode parent pointer when root inode is on short-form >>> >> and parent points to an invalid inode number (refer to: >>> >> "xfs_repair: Fix root inode's parent when it's bogus for sf >>> >> directory" on xfs-devel list). >>> >> >>> >> This test checks if xfs_repair successfully repair the >>> >> filesystem in the scenario mentioned before. >>> >> >>> >> Signed-off-by: Marco Benatto <mbenatto@redhat.com> >>> >> --- >>> >> tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> >> tests/xfs/450.out | 1 + >>> >> tests/xfs/group | 1 + >>> >> 3 files changed, 55 insertions(+) >>> >> create mode 100755 tests/xfs/450 >>> >> create mode 100644 tests/xfs/450.out >>> >> >>> >> diff --git a/tests/xfs/450 b/tests/xfs/450 >>> >> new file mode 100755 >>> >> index 0000000..dc7f244 >>> >> --- /dev/null >>> >> +++ b/tests/xfs/450 >>> >> @@ -0,0 +1,53 @@ >>> >> +#! /bin/bash >>> >> +# SPDX-License-Identifier: GPL-2.0 >>> >> +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. >>> >> +# >>> >> +# FS QA Test 450 >>> >> +# >>> >> +# Make sure xfs_repair can repair root inode parent's pointer >>> >> +# when it contains a bogus ino when it's using shot form directory >>> >> +# >>> >> +seq=`basename $0` >>> >> +seqres=$RESULT_DIR/$seq >>> >> + >>> >> +status=1 # failure is the default! >>> > >>> > Apart from Dave's comments, there're some common definitions missing >>> > too, e.g. 'tmp' and 'here', please use './new xfs' to generate new test >>> > template. >>> >>> Eryu, I think I suggested that Marco could remove the "tmp" definition >>> because it's not used in the script. Is there some reason to keep it? >>> The script did start out with a "./new xfs" generation template. >>> >>> Oh, I bet some helpers depend on it... sorry, my mistake. Oops. >>> >>> (though maybe tmp should just get hoisted to the harness and >>> not be required in every script, but I digress ...) >> >> You mean like I proposed a couple of weeks ago along with the >> spdx license tag updates? >> >> https://www.spinics.net/lists/fstests/msg09849.html >> >> that's next on my list of "Big cleanups for fstests To Do" list. >> >> Cheers, >> >> Dave. >> -- >> Dave Chinner >> david@fromorbit.com > > > > -- > Marco Benatto > Senior Software Maintenance Engineer | Red Hat Brasil > T: +55 11 35246161 | M: +55 41 9 88504051 > Av. Brigadeiro Faria Lima 3900, 8° Andar. São Paulo, Brasil. RED HAT | > TRIED. TESTED. TRUSTED. Saiba porque em redhat.com
diff --git a/tests/xfs/450 b/tests/xfs/450 new file mode 100755 index 0000000..dc7f244 --- /dev/null +++ b/tests/xfs/450 @@ -0,0 +1,53 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Red Hat Inc. All Rights Reserved. +# +# FS QA Test 450 +# +# Make sure xfs_repair can repair root inode parent's pointer +# when it contains a bogus ino when it's using shot form directory +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq + +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +# Modify as appropriate. +_supported_fs xfs +_supported_os Linux +_require_scratch + + +_scratch_mkfs >> /dev/null 2>&1 + +rootino=$(_scratch_xfs_get_metadata_field 'rootino' 'sb 0') + +prefix=$(_scratch_get_sfdir_prefix ${rootino} || \ + _fail "Cannot determine sfdir prefix") + + +# Corrupt root inode parent pointer +_scratch_xfs_set_metadata_field "${prefix}.hdr.parent.i4" 0 "inode ${rootino}"\ + >> $seqres.full + +_scratch_xfs_repair >> $seqres.full 2>&1 + +_scratch_xfs_repair -n >> $seqres.full 2>&1 + +if [ $? -eq 1 ] +then + _fail "xfs_repair failed to repair filesystem" +fi + +echo "OK" +status=0 +exit diff --git a/tests/xfs/450.out b/tests/xfs/450.out new file mode 100644 index 0000000..d86bac9 --- /dev/null +++ b/tests/xfs/450.out @@ -0,0 +1 @@ +OK diff --git a/tests/xfs/group b/tests/xfs/group index 932ab90..3a1db0b 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -447,3 +447,4 @@ 447 auto mount 448 auto quick fuzzers 449 auto quick +450 auto quick
Recently we found out xfs_repair were not repairing root inode parent pointer when root inode is on short-form and parent points to an invalid inode number (refer to: "xfs_repair: Fix root inode's parent when it's bogus for sf directory" on xfs-devel list). This test checks if xfs_repair successfully repair the filesystem in the scenario mentioned before. Signed-off-by: Marco Benatto <mbenatto@redhat.com> --- tests/xfs/450 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/450.out | 1 + tests/xfs/group | 1 + 3 files changed, 55 insertions(+) create mode 100755 tests/xfs/450 create mode 100644 tests/xfs/450.out