Re: [PATCH] xfstests: xfs mount option sanity test

[Ian Kent <ikent@redhat.com>]

On Fri, 2019-10-25 at 16:48 +0800, Ian Kent wrote:
> On Fri, 2019-10-25 at 16:20 +0800, Ian Kent wrote:
> > On Fri, 2019-10-25 at 16:05 +0800, Zorro Lang wrote:
> > > On Thu, Oct 24, 2019 at 12:47:17PM +0800, Ian Kent wrote:
> > > > On Tue, 2019-10-22 at 18:01 +0800, Zorro Lang wrote:
> > > > > XFS is changing to suit the new mount API, so add this case
> > > > > to
> > > > > make
> > > > > sure the changing won't bring in regression issue on xfs
> > > > > mount
> > > > > option
> > > > > parse phase, and won't change some default behaviors either.
> > > > 
> > > > I think this isn't quite right:
> > > > 
> > > > [root@f30 xfstests-dev]# diff -u /home/raven/xfstests-
> > > > dev/tests/xfs/148.out /home/raven/xfstests-
> > > > dev/results//xfs/148.out.bad
> > > > --- /home/raven/xfstests-dev/tests/xfs/148.out	2019-10-24
> > > > 09:27:27.304929313 +0800
> > > > +++ /home/raven/xfstests-dev/results//xfs/148.out.bad	2019-
> > > > 10-24 10:42:40.739436223 +0800
> > > > @@ -3,4 +3,10 @@
> > > >  ** create loop log device
> > > >  ** create loop mount point
> > > >  ** start xfs mount testing ...
> > > > +[FAILED]: mount /dev/loop0 /mnt/test/148.mnt 
> > > > +ERROR: expect there's logbufs in
> > > > rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquo
> > > > ta
> > > > ,
> > > > but not found
> > > > +[FAILED]: mount /dev/loop0 /mnt/test/148.mnt 
> > > > +ERROR: expect there's logbsize in
> > > > rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquo
> > > > ta
> > > > ,
> > > > but not found
> > > > +[FAILED]: mount /dev/loop0 /mnt/test/148.mnt 
> > > > +ERROR: expect there's logbsize in
> > > > rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquo
> > > > ta
> > > > ,
> > > > but not found
> > > >  ** end of testing
> > > > 
> > > > Above logbufs and logbsize are present in the options string
> > > > but
> > > > an error is reported.
> > > 
> > > Oh, There're two reasons to cause this failure:
> > > 1) I made a mistake at here:
> > >         if [ $rc -eq 0 ];then
> > >                 if [ "$found" != "true" ];then
> > >                         echo "[FAILED]: mount $LOOP_DEV $LOOP_MNT
> > > $opts"
> > >                         echo "ERROR: expect there's $key in
> > > $info,
> > > but not found"
> > >                         return 1
> > >                 fi
> > >         else
> > >                 if [ "$found" != "false" ];then
> > >                         echo "[FAILED]: mount $LOOP_DEV $LOOP_MNT
> > > $opts"
> > >                         echo "ERROR: expect there's not $key in
> > > $info, but found"
> > >                         return 1
> > >                 fi
> > >         fi
> > > 
> > > I should exchange the "there's" line with "there's not" line.
> > > 
> > > 2) The case expect there's not "logbufs=N" or "logbsize=N" output
> > > as
> > > default.
> > > But your system has these two options in /proc/mounts if mount
> > > without any
> > > options.
> > > 
> > > Hmm... strange, I've tested this case on rhel8 and rhel7, all
> > > passed.
> > > Where
> > > can I reproduce this error?
> > 
> > There's every chance this has changed along the way but the current
> > upstream source sets these two fields to default values at some
> > point (looks like via xfs_mountfs() from xfs_fs_fill_super() to
> > me).
> > 
> > So the super block op ->show_options() sees them as > 0 and prints
> > the option in the proc output.
> > 
> > But I'm not so familiar with xfs so the subtleties of these options
> > eludes me.
> 
> Oh, I see what's going on.
> 
> In, for example, RHEL-8 xlog_get_iclog_buffer_size() on entry we
> have:
> 
>         if (mp->m_logbufs <= 0)
>                 log->l_iclog_bufs = XLOG_MAX_ICLOGS;
>         else
>                 log->l_iclog_bufs = mp->m_logbufs;
> 
> which sets a struct xlog field but leaves the struct xfs_mount
> alone.
> 
> But upstream on entry we have:
> 
>        if (mp->m_logbufs <= 0)
>                 mp->m_logbufs = XLOG_MAX_ICLOGS;
>        if (mp->m_logbsize <= 0)
>                 mp->m_logbsize = XLOG_BIG_RECORD_BSIZE;
> 
> which sets the struct xfs_mount fields before continuing.
> 
> There's a lot more going on in xlog_get_iclog_buffer_size() so this
> is a very simple minded comparison on my part but the point is
> RHEL-8 (and I'm assuming RHEL-7 too) is different to the current
> xfs upstream.
> 
> > Dave, can you help out please wrt. current RHEL xfs or what I'm
> > missing here?
> 
> So I'd still like more info around this difference please Dave.
> Apologies for the post beer-clock intrusion, ;)

Ok, more info., first see commit 1cb51258758d7:

Author: Eric Sandeen <sandeen@sandeen.net>
Date:   Thu Aug 16 16:24:43 2007 +1000

    [XFS] choose single default logbuf count & size

and then commit 4f62282a36964:

Author: Christoph Hellwig <hch@lst.de>
Date:   Fri Jun 28 19:27:20 2019 -0700

    xfs: cleanup xlog_get_iclog_buffer_size

which is where the change occurred.

I think this last change is what's causing the difference in
behaviour.

> 
> > > Thanks,
> > > Zorro
> > > 
> > > > Ian
> > > > 
> > > > > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > > > > ---
> > > > >  tests/xfs/148     | 315
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/xfs/148.out |   6 +
> > > > >  tests/xfs/group   |   1 +
> > > > >  3 files changed, 322 insertions(+)
> > > > >  create mode 100755 tests/xfs/148
> > > > >  create mode 100644 tests/xfs/148.out
> > > > > 
> > > > > diff --git a/tests/xfs/148 b/tests/xfs/148
> > > > > new file mode 100755
> > > > > index 00000000..5c268f18
> > > > > --- /dev/null
> > > > > +++ b/tests/xfs/148
> > > > > @@ -0,0 +1,315 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (c) 2019 Red Hat, Inc. All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test 148
> > > > > +#
> > > > > +# XFS mount options sanity check, refer to 'man 5 xfs'.
> > > > > +#
> > > > > +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.*
> > > > > +	$UMOUNT_PROG $LOOP_MNT 2>/dev/null
> > > > > +	if [ -n "$LOOP_DEV" ];then
> > > > > +		_destroy_loop_device $LOOP_DEV 2>/dev/null
> > > > > +	fi
> > > > > +	if [ -n "$LOOP_SPARE_DEV" ];then
> > > > > +		_destroy_loop_device $LOOP_SPARE_DEV
> > > > > 2>/dev/null
> > > > > +	fi
> > > > > +	rm -f $LOOP_IMG
> > > > > +	rm -f $LOOP_SPARE_IMG
> > > > > +	rmdir $LOOP_MNT
> > > > > +}
> > > > > +
> > > > > +# 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
> > > > > +_supported_fs xfs
> > > > > +_supported_os Linux
> > > > > +_require_test
> > > > > +_require_loop
> > > > > +_require_xfs_io_command "falloc"
> > > > > +
> > > > > +LOOP_IMG=$TEST_DIR/$seq.dev
> > > > > +LOOP_SPARE_IMG=$TEST_DIR/$seq.logdev
> > > > > +LOOP_MNT=$TEST_DIR/$seq.mnt
> > > > > +
> > > > > +echo "** create loop device"
> > > > > +$XFS_IO_PROG -f -c "falloc 0 1g" $LOOP_IMG
> > > > > +LOOP_DEV=`_create_loop_device $LOOP_IMG`
> > > > > +
> > > > > +echo "** create loop log device"
> > > > > +$XFS_IO_PROG -f -c "falloc 0 512m" $LOOP_SPARE_IMG
> > > > > +LOOP_SPARE_DEV=`_create_loop_device $LOOP_SPARE_IMG`
> > > > > +
> > > > > +echo "** create loop mount point"
> > > > > +rmdir $LOOP_MNT 2>/dev/null
> > > > > +mkdir -p $LOOP_MNT || _fail "cannot create loopback mount
> > > > > point"
> > > > > +
> > > > > +# avoid the effection from MKFS_OPTIONS
> > > > > +MKFS_OPTIONS=""
> > > > > +do_mkfs()
> > > > > +{
> > > > > +	$MKFS_XFS_PROG -f $* $LOOP_DEV | _filter_mkfs
> > > > > > $seqres.full
> > > > > 2>$tmp.mkfs
> > > > > +	if [ "${PIPESTATUS[0]}" -ne 0 ]; then
> > > > > +		_fail "Fails on _mkfs_dev $* $LOOP_DEV"
> > > > > +	fi
> > > > > +	. $tmp.mkfs
> > > > > +}
> > > > > +
> > > > > +is_dev_mounted()
> > > > > +{
> > > > > +	findmnt --source $LOOP_DEV >/dev/null
> > > > > +	return $?
> > > > > +}
> > > > > +
> > > > > +get_mount_info()
> > > > > +{
> > > > > +	findmnt --source $LOOP_DEV -o OPTIONS -n
> > > > > +}
> > > > > +
> > > > > +force_unmount()
> > > > > +{
> > > > > +	$UMOUNT_PROG $LOOP_MNT >/dev/null 2>&1
> > > > > +}
> > > > > +
> > > > > +# _do_test <mount options> <should be mounted?> [<key
> > > > > string>
> > > > > <key
> > > > > should be found?>]
> > > > > +_do_test()
> > > > > +{
> > > > > +	local opts="$1"
> > > > > +	local mounted="$2"	# pass or fail
> > > > > +	local key="$3"
> > > > > +	local found="$4"	# true or false
> > > > > +	local rc
> > > > > +	local info
> > > > > +
> > > > > +	# mount test
> > > > > +	_mount $LOOP_DEV $LOOP_MNT $opts 2>/dev/null
> > > > > +	rc=$?
> > > > > +	if [ $rc -eq 0 ];then
> > > > > +		if [ "${mounted}" = "fail" ];then
> > > > > +			echo "[FAILED]: mount $LOOP_DEV
> > > > > $LOOP_MNT
> > > > > $opts"
> > > > > +			echo "ERROR: expect ${mounted}, but
> > > > > pass"
> > > > > +			return 1
> > > > > +		fi
> > > > > +		is_dev_mounted
> > > > > +		if [ $? -ne 0 ];then
> > > > > +			echo "[FAILED]: mount $LOOP_DEV
> > > > > $LOOP_MNT
> > > > > $opts"
> > > > > +			echo "ERROR: fs not mounted even mount
> > > > > return
> > > > > 0"
> > > > > +			return 1
> > > > > +		fi
> > > > > +	else
> > > > > +		if [ "${mount_ret}" = "pass" ];then
> > > > > +			echo "[FAILED]: mount $LOOP_DEV
> > > > > $LOOP_MNT
> > > > > $opts"
> > > > > +			echo "ERROR: expect ${mounted}, but
> > > > > fail"
> > > > > +			return 1
> > > > > +		fi
> > > > > +		is_dev_mounted
> > > > > +		if [ $? -eq 0 ];then
> > > > > +			echo "[FAILED]: mount $LOOP_DEV
> > > > > $LOOP_MNT
> > > > > $opts"
> > > > > +			echo "ERROR: fs is mounted even mount
> > > > > return
> > > > > non-zero"
> > > > > +			return 1
> > > > > +		fi
> > > > > +	fi
> > > > > +
> > > > > +	# Skip below checking if "$key" argument isn't
> > > > > specified
> > > > > +	if [ -z "$key" ];then
> > > > > +		return 0
> > > > > +	fi
> > > > > +	# Check the mount options after fs mounted.
> > > > > +	info=`get_mount_info`
> > > > > +	echo $info | grep -q "${key}"
> > > > > +	rc=$?
> > > > > +	if [ $rc -eq 0 ];then
> > > > > +		if [ "$found" != "true" ];then
> > > > > +			echo "[FAILED]: mount $LOOP_DEV
> > > > > $LOOP_MNT
> > > > > $opts"
> > > > > +			echo "ERROR: expect there's $key in
> > > > > $info, but
> > > > > not found"
> > > > > +			return 1
> > > > > +		fi
> > > > > +	else
> > > > > +		if [ "$found" != "false" ];then
> > > > > +			echo "[FAILED]: mount $LOOP_DEV
> > > > > $LOOP_MNT
> > > > > $opts"
> > > > > +			echo "ERROR: expect there's not $key in
> > > > > $info,
> > > > > but found"
> > > > > +			return 1
> > > > > +		fi
> > > > > +	fi
> > > > > +
> > > > > +	return 0
> > > > > +}
> > > > > +
> > > > > +do_test()
> > > > > +{
> > > > > +	# force unmount before testing
> > > > > +	force_unmount
> > > > > +	_do_test "$@"
> > > > > +	# force unmount after testing
> > > > > +	force_unmount
> > > > > +}
> > > > > +
> > > > > +echo "** start xfs mount testing ..."
> > > > > +# Test allocsize=size
> > > > > +# Valid values for this option are page size (typically
> > > > > 4KiB)
> > > > > through to 1GiB
> > > > > +do_mkfs
> > > > > +if [ $dbsize -ge 1024 ];then
> > > > > +	blsize="$((dbsize / 1024))k"
> > > > > +fi
> > > > > +do_test "" pass "allocsize" "false"
> > > > > +do_test "-o allocsize=$blsize" pass "allocsize=$blsize"
> > > > > "true"
> > > > > +do_test "-o allocsize=1048576k" pass "allocsize=1048576k"
> > > > > "true"
> > > > > +do_test "-o allocsize=$((dbsize / 2))" fail
> > > > > +do_test "-o allocsize=2g" fail
> > > > > +
> > > > > +# Test attr2
> > > > > +do_mkfs -m crc=1
> > > > > +do_test "" pass "attr2" "true"
> > > > > +do_test "-o attr2" pass "attr2" "true"
> > > > > +do_test "-o noattr2" fail
> > > > > +do_mkfs -m crc=0
> > > > > +do_test "" pass "attr2" "true"
> > > > > +do_test "-o attr2" pass "attr2" "true"
> > > > > +do_test "-o noattr2" pass "attr2" "false"
> > > > > +
> > > > > +# Test discard
> > > > > +do_mkfs
> > > > > +do_test "" pass "discard" "false"
> > > > > +do_test "-o discard" pass "discard" "true"
> > > > > +do_test "-o nodiscard" pass "discard" "false"
> > > > > +
> > > > > +# Test grpid|bsdgroups|nogrpid|sysvgroups
> > > > > +do_test "" pass "grpid" "false"
> > > > > +do_test "-o grpid" pass "grpid" "true"
> > > > > +do_test "-o bsdgroups" pass "grpid" "true"
> > > > > +do_test "-o nogrpid" pass "grpid" "false"
> > > > > +do_test "-o sysvgroups" pass "grpid" "false"
> > > > > +
> > > > > +# Test filestreams
> > > > > +do_test "" pass "filestreams" "false"
> > > > > +do_test "-o filestreams" pass "filestreams" "true"
> > > > > +
> > > > > +# Test ikeep
> > > > > +do_test "" pass "ikeep" "false"
> > > > > +do_test "-o ikeep" pass "ikeep" "true"
> > > > > +do_test "-o noikeep" pass "ikeep" "false"
> > > > > +
> > > > > +# Test inode32|inode64
> > > > > +do_test "" pass "inode64" "true"
> > > > > +do_test "-o inode32" pass "inode32" "true"
> > > > > +do_test "-o inode64" pass "inode64" "true"
> > > > > +
> > > > > +# Test largeio
> > > > > +do_test "" pass "largeio" "false"
> > > > > +do_test "-o largeio" pass "largeio" "true"
> > > > > +do_test "-o nolargeio" pass "largeio" "false"
> > > > > +
> > > > > +# Test logbufs=value. Valid numbers range from 2–8
> > > > > inclusive.
> > > > > +do_test "" pass "logbufs" "false"
> > > > > +do_test "-o logbufs=8" pass "logbufs=8" "true"
> > > > > +do_test "-o logbufs=2" pass "logbufs=2" "true"
> > > > > +do_test "-o logbufs=1" fail
> > > > > +###### but it gets logbufs=8 now, why? bug? #######
> > > > > +# do_test "-o logbufs=0" fail
> > > > > +do_test "-o logbufs=9" fail
> > > > > +do_test "-o logbufs=99999999999999" fail
> > > > > +
> > > > > +# Test logbsize=value.
> > > > > +do_mkfs -m crc=1 -l version=2
> > > > > +do_test "" pass "logbsize" "false"
> > > > > +do_test "-o logbsize=16384" pass "logbsize=16k" "true"
> > > > > +do_test "-o logbsize=16k" pass "logbsize=16k" "true"
> > > > > +do_test "-o logbsize=32k" pass "logbsize=32k" "true"
> > > > > +do_test "-o logbsize=64k" pass "logbsize=64k" "true"
> > > > > +do_test "-o logbsize=128k" pass "logbsize=128k" "true"
> > > > > +do_test "-o logbsize=256k" pass "logbsize=256k" "true"
> > > > > +do_test "-o logbsize=8k" fail
> > > > > +do_test "-o logbsize=512k" fail
> > > > > +####### it's invalid, but it set to default size 32k
> > > > > +# do_test "-o logbsize=0" false
> > > > > +do_mkfs -m crc=0 -l version=1
> > > > > +do_test "" pass "logbsize" "false"
> > > > > +do_test "-o logbsize=16384" pass "logbsize=16k" "true"
> > > > > +do_test "-o logbsize=16k" pass "logbsize=16k" "true"
> > > > > +do_test "-o logbsize=32k" pass "logbsize=32k" "true"
> > > > > +do_test "-o logbsize=64k" fail
> > > > > +
> > > > > +# Test logdev
> > > > > +do_mkfs
> > > > > +do_test "" pass "logdev" "false"
> > > > > +do_test "-o logdev=$LOOP_SPARE_DEV" fail
> > > > > +do_mkfs -l logdev=$LOOP_SPARE_DEV
> > > > > +do_test "-o logdev=$LOOP_SPARE_DEV" pass
> > > > > "logdev=$LOOP_SPARE_DEV"
> > > > > "true"
> > > > > +do_test "" fail
> > > > > +
> > > > > +# Test noalign
> > > > > +do_mkfs
> > > > > +do_test "" pass "noalign" "false"
> > > > > +do_test "-o noalign" pass "noalign" "true"
> > > > > +
> > > > > +# Test norecovery
> > > > > +do_test "" pass "norecovery" "false"
> > > > > +do_test "-o norecovery,ro" pass "norecovery" "true"
> > > > > +do_test "-o norecovery" fail
> > > > > +
> > > > > +# Test nouuid
> > > > > +do_test "" pass "nouuid" "false"
> > > > > +do_test "-o nouuid" pass "nouuid" "true"
> > > > > +
> > > > > +# Test noquota
> > > > > +do_test "" pass "noquota" "true"
> > > > > +do_test "-o noquota" pass "noquota" "true"
> > > > > +
> > > > > +# Test uquota/usrquota/quota/uqnoenforce/qnoenforce
> > > > > +do_test "" pass "usrquota" "false"
> > > > > +do_test "-o uquota" pass "usrquota" "true"
> > > > > +do_test "-o usrquota" pass "usrquota" "true"
> > > > > +do_test "-o quota" pass "usrquota" "true"
> > > > > +do_test "-o uqnoenforce" pass "usrquota" "true"
> > > > > +do_test "-o qnoenforce" pass "usrquota" "true"
> > > > > +
> > > > > +# Test gquota/grpquota/gqnoenforce
> > > > > +do_test "" pass "grpquota" "false"
> > > > > +do_test "-o gquota" pass "grpquota" "true"
> > > > > +do_test "-o grpquota" pass "grpquota" "true"
> > > > > +do_test "-o gqnoenforce" pass "gqnoenforce" "true"
> > > > > +
> > > > > +# Test pquota/prjquota/pqnoenforce
> > > > > +do_test "" pass "prjquota" "false"
> > > > > +do_test "-o pquota" pass "prjquota" "true"
> > > > > +do_test "-o prjquota" pass "prjquota" "true"
> > > > > +do_test "-o pqnoenforce" pass "pqnoenforce" "true"
> > > > > +
> > > > > +# Test sunit=value and swidth=value
> > > > > +do_mkfs -d sunit=128,swidth=128
> > > > > +do_test "-o sunit=8,swidth=8" pass "sunit=8,swidth=8" "true"
> > > > > +do_test "-o sunit=8,swidth=64" pass "sunit=8,swidth=64"
> > > > > "true"
> > > > > +do_test "-o sunit=128,swidth=128" pass
> > > > > "sunit=128,swidth=128"
> > > > > "true"
> > > > > +do_test "-o sunit=256,swidth=256" pass
> > > > > "sunit=256,swidth=256"
> > > > > "true"
> > > > > +do_test "-o sunit=2,swidth=2" fail
> > > > > +
> > > > > +# Test swalloc
> > > > > +do_mkfs
> > > > > +do_test "" pass "swalloc" "false"
> > > > > +do_test "-o swalloc" pass "swalloc" "true"
> > > > > +
> > > > > +# Test wsync
> > > > > +do_test "" pass "wsync" "false"
> > > > > +do_test "-o wsync" pass "wsync" "true"
> > > > > +
> > > > > +echo "** end of testing"
> > > > > +# success, all done
> > > > > +status=0
> > > > > +exit
> > > > > diff --git a/tests/xfs/148.out b/tests/xfs/148.out
> > > > > new file mode 100644
> > > > > index 00000000..a71d9231
> > > > > --- /dev/null
> > > > > +++ b/tests/xfs/148.out
> > > > > @@ -0,0 +1,6 @@
> > > > > +QA output created by 148
> > > > > +** create loop device
> > > > > +** create loop log device
> > > > > +** create loop mount point
> > > > > +** start xfs mount testing ...
> > > > > +** end of testing
> > > > > diff --git a/tests/xfs/group b/tests/xfs/group
> > > > > index f4ebcd8c..019aebad 100644
> > > > > --- a/tests/xfs/group
> > > > > +++ b/tests/xfs/group
> > > > > @@ -145,6 +145,7 @@
> > > > >  145 dmapi
> > > > >  146 dmapi
> > > > >  147 dmapi
> > > > > +148 auto quick mount
> > > > >  150 dmapi
> > > > >  151 dmapi
> > > > >  152 dmapi