Re: [PATCH 2/9 RESEND] generic: test quota handling on remount ro failure

[Brian Foster <bfoster@redhat.com>]

On Wed, Apr 01, 2015 at 02:06:30PM +0800, Eryu Guan wrote:
> On Wed, Mar 25, 2015 at 03:32:27PM -0400, Brian Foster wrote:
> > On Fri, Mar 20, 2015 at 06:16:51PM +0800, Eryu Guan wrote:
> > > Remount ro should not turn qouta off unconditionally, even remount ro
> > > failed, also kernel should not oops on the next succeeded remount ro.
> > > 
> > > Signed-off-by: Eryu Guan <eguan@redhat.com>
> > > ---
> > >  tests/generic/072     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/072.out |  3 +++
> > >  tests/generic/group   |  1 +
> > >  3 files changed, 73 insertions(+)
> > >  create mode 100755 tests/generic/072
> > >  create mode 100644 tests/generic/072.out
> > > 
> > > diff --git a/tests/generic/072 b/tests/generic/072
> > > new file mode 100755
> > > index 0000000..ef40822
> > > --- /dev/null
> > > +++ b/tests/generic/072
> > > @@ -0,0 +1,69 @@
> > > +#! /bin/bash
> > > +# FS QA Test No. 072
> > > +#
> > > +# Test quota handling on remount ro failure
> > > +#
> > > +#-----------------------------------------------------------------------
> > > +# Copyright (c) 2015 Red Hat Inc.  All Rights Reserved.
> > > +#
> > > +# This program is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU General Public License as
> > > +# published by the Free Software Foundation.
> > > +#
> > > +# This program is distributed in the hope that it would be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program; if not, write the Free Software Foundation,
> > > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > > +#-----------------------------------------------------------------------
> > > +#
> > > +
> > > +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
> > > +. ./common/quota
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs generic
> > > +_supported_os Linux
> > > +_require_test
> > > +_require_scratch
> > > +_require_quota
> > > +
> > > +rm -f $seqres.full
> > > +
> > > +_scratch_mkfs >>$seqres.full 2>&1
> > > +_scratch_mount "-o usrquota,grpquota"
> > > +
> > > +quotacheck -ug $SCRATCH_MNT >/dev/null 2>&1
> > > +quotaon $SCRATCH_MNT >/dev/null 2>&1
> > > +
> > 
> > What's the purpose of the above two commands? The quotacheck seems to
> > fail (masked by the redirect to /dev/null) and shouldn't quota be on by
> > virtue of the mount options?
> 
> extN needs quotacheck and quotaon first to enable quota, xfs doesn't
> need these commands and as you said quotacheck also fails on xfs. So
> output of these commands are redirected to /dev/null
> 

Ok, comment please. :) We should point out that these commands are not
necessary for all filesystems and could even fail, and therefore the
output is thrown away and exit status is not important. We print the
quota status at some point below, and ultimately that's all we care
about.

> > 
> > > +# first remount ro with a bad option, a failed remount ro should not disable quota
> > > +_scratch_mount "-o remount,ro,nosuchopt" >>$seqres.full 2>&1
> > > +quotaon -p $SCRATCH_MNT | _filter_scratch
> > > +# second remount should succeed, no oops or hang expected
> > > +_scratch_mount "-o remount,ro"
> > > +
> > 
> > FWIW, when I '-o remount,ro,nosuchopt' on XFS the remount ro actually
> > succeeds and the bad option is dropped. Does that differ from other fs
> > behavior? If not, I wonder whether the second remount,ro is necessary
> > here as well.
> 
> The first remount (with unsupported option) fails on extN.
> 
> Maybe this test should be in shared, and only support extN?
> 

Perhaps the test is less interesting for xfs, but I don't think it
necessarily precludes running it there. That behavior could change in
the future. It's just that the test is ambiguous and could lead to
confusion if somebody happens to look at this sometime down the road and
doesn't have that context from author.

We should have a comment for the first remount that explains the remount
may or may not return failure depending on fs, but regardless, quota
should remain enabled. We should probably have '|| _fail "failed to
remount ro"' for the final remount to indicate that this should always
succeed (and not rely on whether _scratch_mount fails the test as a side
effect).

Brian

> Thanks,
> Eryu
> > 
> > Brian
> > 
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/072.out b/tests/generic/072.out
> > > new file mode 100644
> > > index 0000000..ff0e180
> > > --- /dev/null
> > > +++ b/tests/generic/072.out
> > > @@ -0,0 +1,3 @@
> > > +QA output created by 072
> > > +group quota on SCRATCH_MNT (SCRATCH_DEV) is on
> > > +user quota on SCRATCH_MNT (SCRATCH_DEV) is on
> > > diff --git a/tests/generic/group b/tests/generic/group
> > > index 03ad7b8..c0210d2 100644
> > > --- a/tests/generic/group
> > > +++ b/tests/generic/group
> > > @@ -74,6 +74,7 @@
> > >  069 rw udf auto quick
> > >  070 attr udf auto quick stress
> > >  071 auto quick prealloc
> > > +072 auto quick quota
> > >  074 rw udf auto
> > >  075 rw udf auto quick
> > >  076 metadata rw udf auto quick stress
> > > -- 
> > > 2.1.0
> > > 
> > > --
> > > 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
> > --
> > 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
> --
> 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