Re: [PATCH 2/2] xfs: test inobtcount upgrade

Darrick J. Wong
2021-04-03T00:29:52Z

On Wed, Mar 31, 2021 at 01:36:47PM -0400, Brian Foster wrote:
> On Tue, Mar 30, 2021 at 06:08:34PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Make sure we can actually upgrade filesystems to support inode btree
> > counters.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > tests/xfs/910 | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > tests/xfs/910.out | 23 +++++++++++
> > tests/xfs/group | 1
> > 3 files changed, 136 insertions(+)
> > create mode 100755 tests/xfs/910
> > create mode 100644 tests/xfs/910.out
> >
> >
> > diff --git a/tests/xfs/910 b/tests/xfs/910
> > new file mode 100755
> > index 00000000..5f095324
> > --- /dev/null
> > +++ b/tests/xfs/910
> > @@ -0,0 +1,112 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# Copyright (c) 2021 Oracle. All Rights Reserved.
> > +#
> > +# FS QA Test No. 910
> > +#
> > +# Check that we can upgrade a filesystem to support inobtcount and that
> > +# everything works properly after the upgrade.
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1 # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > + cd /
> > + rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs xfs
> > +_require_xfs_scratch_inobtcount
> > +_require_command "$XFS_ADMIN_PROG" "xfs_admin"
> > +_require_xfs_repair_upgrade inobtcount
> > +
> > +rm -f $seqres.full
> > +
> > +# Make sure we can't format a filesystem with inobtcount and not finobt.
> > +_scratch_mkfs -m crc=1,inobtcount=1,finobt=0 &> $seqres.full && \
> > + echo "Should not be able to format with inobtcount but not finobt."
> > +
> > +# Make sure we can't upgrade a V4 filesystem
> > +_scratch_mkfs -m crc=0,inobtcount=0,finobt=0 >> $seqres.full
> > +_scratch_xfs_admin -O inobtcount=1 2>> $seqres.full
> > +_check_scratch_xfs_features INOBTCNT
> > +
> > +# Make sure we can't upgrade a filesystem to inobtcount without finobt.
> > +_scratch_mkfs -m crc=1,inobtcount=0,finobt=0 >> $seqres.full
> > +_scratch_xfs_admin -O inobtcount=1 2>> $seqres.full
> > +_check_scratch_xfs_features INOBTCNT
> > +
> > +# Format V5 filesystem without inode btree counter support and upgrade it.
> > +# Inject failure into repair and make sure that the only path forward is
> > +# to re-run repair on the filesystem.
> > +_scratch_mkfs -m crc=1,inobtcount=0 >> $seqres.full
> > +echo "Fail partway through upgrading"
> > +XFS_REPAIR_FAIL_AFTER_PHASE=2 _scratch_xfs_repair -c inobtcount=1 2>> $seqres.full
> > +test $? -eq 137 || echo "repair should have been killed??"
> > +_check_scratch_xfs_features NEEDSREPAIR INOBTCNT
> > +_try_scratch_mount &> $tmp.mount
> > +res=$?
> > +_filter_scratch < $tmp.mount
> > +if [ $res -eq 0 ]; then
> > + echo "needsrepair should have prevented mount"
> > + _scratch_unmount
> > +fi
> > +
> > +echo "Re-run repair to finish upgrade"
> > +_scratch_xfs_repair 2>> $seqres.full
> > +_check_scratch_xfs_features NEEDSREPAIR INOBTCNT
> > +
> > +echo "Filesystem should be usable again"
> > +_try_scratch_mount &> $tmp.mount
> > +res=$?
> > +_filter_scratch < $tmp.mount
> > +if [ $res -eq 0 ]; then
> > + _scratch_unmount
> > +else
> > + echo "mount should succeed after second repair"
> > +fi
> > +_check_scratch_xfs_features NEEDSREPAIR INOBTCNT
> > +
>
> As you're probably aware I'm not a huge fan of sprinkling these kind of
> repair/mount checks throughout the feature level tests. I don't think
> it's reasonable to expect this to become a consistent pattern, so over
> time this will likely just obfuscate the purpose of some of these tests.
> That said, it seems to be harmless here so I'll just note that as my
> .02.

Ok. I think inobtcount is the only feature upgrade test that checks it.

> > +# Format V5 filesystem without inode btree counter support and upgrade it.
> > +_scratch_mkfs -m crc=1,inobtcount=0 >> $seqres.full
> > +_scratch_xfs_db -c 'version' -c 'sb 0' -c 'p' >> $seqres.full
> > +_scratch_mount >> $seqres.full
> > +
> > +echo moo > $SCRATCH_MNT/urk
> > +
> > +_scratch_unmount
> > +_check_scratch_fs
> > +
> > +# Now upgrade to inobtcount support
> > +_scratch_xfs_admin -O inobtcount=1 2>> $seqres.full
> > +_check_scratch_xfs_features INOBTCNT
> > +_check_scratch_fs
> > +_scratch_xfs_db -c 'version' -c 'sb 0' -c 'p' -c 'agi 0' -c 'p' >> $seqres.full
> > +
>
> This looks nearly the same as the previous test with the exception of
> not doing repair failure injection and instead creating a file and
> checking the resulting counters. Could we combine these two sequences
> and simultaneously run a slightly more randomized test? E.g., a logical
> sequence along the lines of:
>
> mkfs inobtcount=0
> mount
> run fsstress -n 500 or otherwise (quickly) populate w/ some randomized content
> umount
> repair w/ fail injection
> repair w/o fail injection
> check resulting counters
>
> Hm?

Yes, the two cases can be combined. Will do, thanks for the feedback!

--D

>
> Brian
>
> > +# Make sure we have nonzero counters
> > +_scratch_xfs_db -c 'agi 0' -c 'print ino_blocks fino_blocks' | \
> > + sed -e 's/= [1-9]*/= NONZERO/g'
> > +
> > +# Mount again, look at our files
> > +_scratch_mount >> $seqres.full
> > +cat $SCRATCH_MNT/urk
> > +
> > +# Make sure we can't re-add inobtcount
> > +_scratch_unmount
> > +_scratch_xfs_admin -O inobtcount=1 2>> $seqres.full
> > +_scratch_mount >> $seqres.full
> > +
> > +status=0
> > +exit
> > diff --git a/tests/xfs/910.out b/tests/xfs/910.out
> > new file mode 100644
> > index 00000000..ed78d88f
> > --- /dev/null
> > +++ b/tests/xfs/910.out
> > @@ -0,0 +1,23 @@
> > +QA output created by 910
> > +Running xfs_repair to upgrade filesystem.
> > +Inode btree count feature only supported on V5 filesystems.
> > +FEATURES: INOBTCNT:NO
> > +Running xfs_repair to upgrade filesystem.
> > +Inode btree count feature requires free inode btree.
> > +FEATURES: INOBTCNT:NO
> > +Fail partway through upgrading
> > +Adding inode btree counts to filesystem.
> > +FEATURES: NEEDSREPAIR:YES INOBTCNT:YES
> > +mount: SCRATCH_MNT: mount(2) system call failed: Structure needs cleaning.
> > +Re-run repair to finish upgrade
> > +FEATURES: NEEDSREPAIR:NO INOBTCNT:YES
> > +Filesystem should be usable again
> > +FEATURES: NEEDSREPAIR:NO INOBTCNT:YES
> > +Running xfs_repair to upgrade filesystem.
> > +Adding inode btree counts to filesystem.
> > +FEATURES: INOBTCNT:YES
> > +ino_blocks = NONZERO
> > +fino_blocks = NONZERO
> > +moo
> > +Running xfs_repair to upgrade filesystem.
> > +Filesystem already has inode btree counts.
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index 5801471b..0dc8038a 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -524,3 +524,4 @@
> > 768 auto quick repair
> > 770 auto repair
> > 773 auto quick repair
> > +910 auto quick inobtcount
> >
>