[v2] xfstests, generic: add project quota attribute tests

Commit Message

Wang Shilong July 6, 2016, 6:22 a.m. UTC

From: Wang Shilong <[e-mail]>

lsattr/chattr support both of ext4 and xfs, add
a test to cover both of them.

 1. ioctl with project quota.
 2. project inherit attribute.
 3. Link accross project should fail
 4. change project ignores quota

Signed-off-by: Wang Shilong <[e-mail]>
---
 tests/generic/362     | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/362.out |   8 ++++
 tests/generic/group   |   1 +
 3 files changed, 139 insertions(+)
 create mode 100755 tests/generic/362
 create mode 100644 tests/generic/362.out

Comments

Eric Sandeen July 6, 2016, 4:10 p.m. UTC | #1

On 7/6/16 1:22 AM, Wang Shilong wrote:
> From: Wang Shilong <[e-mail]>
> 
> lsattr/chattr support both of ext4 and xfs, add
> a test to cover both of them.

Thanks for making this generic; some comments below.
 
>  1. ioctl with project quota.
>  2. project inherit attribute.
>  3. Link accross project should fail

s/accross/across/ FWIW :)

>  4. change project ignores quota
> 
> Signed-off-by: Wang Shilong <[e-mail]>
> ---
>  tests/generic/362     | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/362.out |   8 ++++
>  tests/generic/group   |   1 +
>  3 files changed, 139 insertions(+)
>  create mode 100755 tests/generic/362
>  create mode 100644 tests/generic/362.out
> 
> diff --git a/tests/generic/362 b/tests/generic/362
> new file mode 100755
> index 0000000..f763bc5
> --- /dev/null
> +++ b/tests/generic/362
> @@ -0,0 +1,130 @@
> +#! /bin/bash
> +# FS QA Test No. 030
> +#
> +# Test Project quota attr function
> +#
> +#-----------------------------------------------------------------------
> +# Copyright 2016 (C) Wang Shilong <[e-mail]>
> +#
> +# 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
> +#
> +#-----------------------------------------------------------------------
> +#
> +
> +seqfull=$0
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +    rm -f $tmp.*
> +}
> +
> +trap "_cleanup ; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +. ./common/quota
> +
> +# real QA test starts here
> +_supported_fs ext4 xfs
> +_supported_os Linux
> +
> +_require_scratch
> +_require_chattr
> +_require_test_lsattr
> +_require_quota

We need a _require test to see if the generic quota tools
support project quota, or this will fail due to lack of
userspace support:

+setquota: invalid option -- 'P'
+setquota: Usage:
+  setquota [-u|-g] [-rm] [-F quotaformat] <user|group>
...

same for chattr & lsattr:

+Usage: chattr [-RVf] [-+=AacDdeijsSu] [-v version] files...
...
+lsattr: invalid option -- 'p'

If any of these don't work, the test should _notrun with
a short explanation about the requirement; see more below.

> +
> +rm -f $seqres.full
> +MKFSOPTIONS=""
> +MOUNTOPTIONS=""
> +
> +function setup_quota_options()
> +{
> +    case $FSTYP in
> +    xfs)
> +	#quotaon rely on this.
> +	MOUNTOPTIONS="-o pquota"
> +	;;
> +    ext4)
> +	#project quota is disabled in default.
> +	MKFSOPTIONS="-O quota,project"
> +	;;

Ok, that explains that mystery, I wasn't sure how to enable
project quota on ext4.

(but I'm curious, why doesn't ext4 have a pquota mount option to
match its grpquota and usrquota mount options?  Seems like strange
asymmetry)

But this will also _require a check to be sure mke2fs understands
the "-O project" option, and _notrun if it doesn't.

I think this could all be wrapped up in a "_require_vfs_project_quota"
test, which tests:

1) linux-quota userspace support
2) e2fsprogs userspace support
3) kernel support for the filesystem being tested.

(if the filesystem doesn't support it in kernelspace, we get stuff like
+mount: wrong fs type, bad option, bad superblock on /dev/sdb2,
+       missing codepage or helper program, or other error
+       In some cases useful info is found in syslog - try
+       dmesg | tail  or so
+
+chattr: Inappropriate ioctl for device while setting project on /mnt/scratch/dir
+lsattr: Inappropriate ioctl for device While reading project on /mnt/scratch/dir/foo)

> +    *)
> +        ;;
> +    esac
> +
> +}
> +
> +function set_inherit_attribute()

I don't think this is really needed, either xfs_io or chattr
should work, assuming it's new enough.

Just using chattr should suffice for all filesystems; the test
uses lsattr directly later, so using chattr directly as well makes
more sense to me.

> +{
> +    case $FSTYP in
> +    xfs)
> +	$XFS_IO_PROG -r -c 'chattr +P' $*

(why "-r" ?)

> +	;;
> +    ext4)
> +	chattr +P $*
> +	;;
> +    *)
> +        ;;
> +    esac
> +}
> +
> +setup_quota_options
> +
> +echo "+ create scratch fs"
> +_scratch_mkfs $MKFSOPTIONS > /dev/null 2>&1
> +
> +echo "+ mount fs image"
> +_scratch_mount $MOUNTOPTIONS
> +
> +function attr_test()
> +{
> +	#default project without inherit
> +	mkdir $SCRATCH_MNT/dir
> +	chattr -p 123456 $SCRATCH_MNT/dir | _filter_scratch | _filter_spaces
> +	lsattr -p $SCRATCH_MNT/dir | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +	touch $SCRATCH_MNT/dir/foo
> +	lsattr -p $SCRATCH_MNT/dir/foo | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +
> +	#test project inherit with inherit attribute
> +	set_inherit_attribute $SCRATCH_MNT/dir
> +	touch $SCRATCH_MNT/dir/foo1
> +	lsattr -p $SCRATCH_MNT/dir/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +
> +	#Link accross project should fail

s/accross/across :)

> +	mkdir $SCRATCH_MNT/dir1
> +	set_inherit_attribute $SCRATCH_MNT/dir1
> +	chattr -p 654321 $SCRATCH_MNT/dir1 | _filter_scratch | _filter_spaces
> +	ln $SCRATCH_MNT/dir/foo1 $SCRATCH_MNT/dir1/foo1 2>&1 \
> +		| _filter_scratch | _filter_spaces

ln output seems to have changed at some point, so there needs to be some filtering
or replacement on the ln error:

    -ln: failed to create hard link 'SCRATCH_MNT/dir1/foo1' => 'SCRATCH_MNT/dir/foo1': Invalid cross-device link
    +ln: creating hard link 'SCRATCH_MNT/dir1/foo1' => 'SCRATCH_MNT/dir/foo1': Invalid cross-device link

> +
> +	#mv accross different projects should work

s/accross/across :)

> +	mv $SCRATCH_MNT/dir/foo1 $SCRATCH_MNT/dir1/foo1
> +	lsattr -p $SCRATCH_MNT/dir1/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +
> +	#change project ignores quota
> +	quotaon $SCRATCH_MNT >/dev/null 2>&1
> +	setquota -P 654321 0 0 0 1 $SCRATCH_MNT/
> +	chattr -p 123456 $SCRATCH_MNT/dir1/foo1 | _filter_scratch | _filter_spaces
> +	lsattr -p $SCRATCH_MNT/dir1/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +}
> +attr_test
> diff --git a/tests/generic/362.out b/tests/generic/362.out
> new file mode 100644
> index 0000000..31db991
> --- /dev/null
> +++ b/tests/generic/362.out
> @@ -0,0 +1,8 @@
> +QA output created by 362
> ++ create scratch fs
> ++ mount fs image
> +0 SCRATCH_MNT/dir/foo
> +123456 SCRATCH_MNT/dir/foo1
> +ln: failed to create hard link 'SCRATCH_MNT/dir1/foo1' => 'SCRATCH_MNT/dir/foo1': Invalid cross-device link
> +654321 SCRATCH_MNT/dir1/foo1
> +123456 SCRATCH_MNT/dir1/foo1
> diff --git a/tests/generic/group b/tests/generic/group
> index 7491282..e834762 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -364,3 +364,4 @@
>  359 auto quick clone
>  360 auto quick metadata
>  361 auto quick
> +362 auto quick
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner July 6, 2016, 11:35 p.m. UTC | #2

On Wed, Jul 06, 2016 at 03:22:51PM +0900, Wang Shilong wrote:
> From: Wang Shilong <[e-mail]>
> 
> lsattr/chattr support both of ext4 and xfs, add
> a test to cover both of them.
> 
>  1. ioctl with project quota.
>  2. project inherit attribute.
>  3. Link accross project should fail
>  4. change project ignores quota
> 
> Signed-off-by: Wang Shilong <[e-mail]>

FWIW, this test already points out a few problems with the ext4
project quota implementation....

> +#
> +# Test Project quota attr function
> +#
> +#-----------------------------------------------------------------------
> +# Copyright 2016 (C) Wang Shilong <[e-mail]>

Ambiguous copyright statement. Is it your personal copyright, or
should it be DDN, the company you work for? If it's a personal
copyright, please use your personal email address, not the email
address supllied by the company you work for. If it's DDN that
owns the copyright, then please put DDN's name here along with your
DDN email address.


> +# real QA test starts here
> +_supported_fs ext4 xfs
> +_supported_os Linux

Generic tests shouldn't need a _supported_fs line - the filesystems
it runs on should be selected by the _requires*  functions below.

> +_require_scratch
> +_require_chattr
> +_require_test_lsattr
> +_require_quota

needs  _require_prjquota, and that function needs to be modified to
detect for both XFS and ext4 support.

> +
> +rm -f $seqres.full
> +MKFSOPTIONS=""
> +MOUNTOPTIONS=""

This doesn't seem appropriate. We really want to test project quotas
under all the configurations that come in from the environment. e.g.
for different block sizes.

> +function setup_quota_options()
> +{
> +    case $FSTYP in
> +    xfs)
> +	#quotaon rely on this.
> +	MOUNTOPTIONS="-o pquota"
> +	;;
> +    ext4)
> +	#project quota is disabled in default.
> +	MKFSOPTIONS="-O quota,project"
> +	;;
> +    *)
> +        ;;
> +    esac
> +
> +}

Oh, I see now. Really, ext4 needs to support the pquota mount
option, just like all the other filesystem quotas are configured.
Please fix that ext4 kernel bug, not hack around it in the test
harnesses.

> +function set_inherit_attribute()
> +{
> +    case $FSTYP in
> +    xfs)
> +	$XFS_IO_PROG -r -c 'chattr +P' $*
> +	;;
> +    ext4)
> +	chattr +P $*
> +	;;
> +    *)
> +        ;;
> +    esac
> +}

Don't the two programs use exactly the same ioctl interface to set
the project attribute?  If not, then that's a deficiency in the ext4
project quota implementation that needs fixing. If there's some
other reason for xfs_io not working on ext4, then that needs fixing.
Again, don't work around implementation problems in the test harness.

> +setup_quota_options
> +
> +echo "+ create scratch fs"
> +_scratch_mkfs $MKFSOPTIONS > /dev/null 2>&1
> +
> +echo "+ mount fs image"
> +_scratch_mount $MOUNTOPTIONS

Once everything is done via mount option, then we can use the
standard _qmount_option function for setting the quota type, and
_qmount for mounting with quotas enabled according to
_qmount_option.


> +function attr_test()
> +{
> +	#default project without inherit
> +	mkdir $SCRATCH_MNT/dir
> +	chattr -p 123456 $SCRATCH_MNT/dir | _filter_scratch | _filter_spaces
> +	lsattr -p $SCRATCH_MNT/dir | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +	touch $SCRATCH_MNT/dir/foo
> +	lsattr -p $SCRATCH_MNT/dir/foo | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +
> +	#test project inherit with inherit attribute
> +	set_inherit_attribute $SCRATCH_MNT/dir
> +	touch $SCRATCH_MNT/dir/foo1
> +	lsattr -p $SCRATCH_MNT/dir/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +
> +	#Link accross project should fail
> +	mkdir $SCRATCH_MNT/dir1
> +	set_inherit_attribute $SCRATCH_MNT/dir1
> +	chattr -p 654321 $SCRATCH_MNT/dir1 | _filter_scratch | _filter_spaces
> +	ln $SCRATCH_MNT/dir/foo1 $SCRATCH_MNT/dir1/foo1 2>&1 \
> +		| _filter_scratch | _filter_spaces
> +
> +	#mv accross different projects should work
> +	mv $SCRATCH_MNT/dir/foo1 $SCRATCH_MNT/dir1/foo1
> +	lsattr -p $SCRATCH_MNT/dir1/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +
> +	#change project ignores quota
> +	quotaon $SCRATCH_MNT >/dev/null 2>&1
> +	setquota -P 654321 0 0 0 1 $SCRATCH_MNT/
> +	chattr -p 123456 $SCRATCH_MNT/dir1/foo1 | _filter_scratch | _filter_spaces
> +	lsattr -p $SCRATCH_MNT/dir1/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
> +}
> +attr_test

We don't need this encapsulated in a function.

Also, there is nothing to set status=0 before exit, so this test
will always fail due to the exit code being 1.

> diff --git a/tests/generic/362.out b/tests/generic/362.out
> new file mode 100644
> index 0000000..31db991
> --- /dev/null
> +++ b/tests/generic/362.out
> @@ -0,0 +1,8 @@
> +QA output created by 362
> ++ create scratch fs
> ++ mount fs image
> +0 SCRATCH_MNT/dir/foo
> +123456 SCRATCH_MNT/dir/foo1
> +ln: failed to create hard link 'SCRATCH_MNT/dir1/foo1' => 'SCRATCH_MNT/dir/foo1': Invalid cross-device link
> +654321 SCRATCH_MNT/dir1/foo1
> +123456 SCRATCH_MNT/dir1/foo1
> diff --git a/tests/generic/group b/tests/generic/group
> index 7491282..e834762 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -364,3 +364,4 @@
>  359 auto quick clone
>  360 auto quick metadata
>  361 auto quick
> +362 auto quick

and quota.

Cheers,

Dave.

Eric Sandeen July 7, 2016, 2:47 a.m. UTC | #3

On 7/6/16 6:35 PM, Dave Chinner wrote:

...

>> +_require_scratch
>> +_require_chattr
>> +_require_test_lsattr
>> +_require_quota
> 
> needs  _require_prjquota, and that function needs to be modified to
> detect for both XFS and ext4 support.

I think that if there is desire to test both xfs and non-xfs userspace
with project quota, then we need to differentiate between "e2fsprogs
and linux-quota and the kernel all support it" and "xfsprogs and
the kernel both support it" don't we?

IOWs if the test uses setquota/repquota, chattr, mkfs, and fsck to
work with project quota, then that's a different set of requirements
from a test using xfs_io, xfs_quota, etc.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner July 8, 2016, 12:51 a.m. UTC | #4

On Wed, Jul 06, 2016 at 09:47:28PM -0500, Eric Sandeen wrote:
> On 7/6/16 6:35 PM, Dave Chinner wrote:
> 
> ...
> 
> >> +_require_scratch
> >> +_require_chattr
> >> +_require_test_lsattr
> >> +_require_quota
> > 
> > needs  _require_prjquota, and that function needs to be modified to
> > detect for both XFS and ext4 support.
> 
> I think that if there is desire to test both xfs and non-xfs userspace
> with project quota, then we need to differentiate between "e2fsprogs
> and linux-quota and the kernel all support it" and "xfsprogs and
> the kernel both support it" don't we?

Well, it should be just "linux-quota and kernel". ext4 needs to
have the same mount option behaviour for project quota as it does
for all other types of quota, not be dependent on mkfs....

> IOWs if the test uses setquota/repquota, chattr, mkfs, and fsck to
> work with project quota, then that's a different set of requirements
> from a test using xfs_io, xfs_quota, etc.

_require_linux_prjquota
_require_xfs_prjquota

But that said, both ext4 and xfs need to work for both
configurations, and they should all be using the common xfstests
quota infrastructure....

Cheers,

Dave.

Theodore Ts'o July 8, 2016, 2:46 a.m. UTC | #5

On Fri, Jul 08, 2016 at 10:51:27AM +1000, Dave Chinner wrote:
> On Wed, Jul 06, 2016 at 09:47:28PM -0500, Eric Sandeen wrote:
> > On 7/6/16 6:35 PM, Dave Chinner wrote:
> > 
> > ...
> > 
> > >> +_require_scratch
> > >> +_require_chattr
> > >> +_require_test_lsattr
> > >> +_require_quota
> > > 
> > > needs  _require_prjquota, and that function needs to be modified to
> > > detect for both XFS and ext4 support.
> > 
> > I think that if there is desire to test both xfs and non-xfs userspace
> > with project quota, then we need to differentiate between "e2fsprogs
> > and linux-quota and the kernel all support it" and "xfsprogs and
> > the kernel both support it" don't we?
> 
> Well, it should be just "linux-quota and kernel". ext4 needs to
> have the same mount option behaviour for project quota as it does
> for all other types of quota, not be dependent on mkfs....

Project quota for ext4 is an optional thing, and if nothing else, we
need to have a separate feature flag for legacy file systems that were
created before we started supporting project quota.  So if you want to
support project quota you *will* need to have a version of e2fsck that
understands project quota, and a version of mke2fs that knows how to
request that project quota be enabled, etc., etc.

So while it might be *nice* if ext4 could support project quota
without being dependent on having a specific version of mke2fs and
e2fsck installed, it's just simply not possible....

> > IOWs if the test uses setquota/repquota, chattr, mkfs, and fsck to
> > work with project quota, then that's a different set of requirements
> > from a test using xfs_io, xfs_quota, etc.
> 
> _require_linux_prjquota
> _require_xfs_prjquota
> 
> But that said, both ext4 and xfs need to work for both
> configurations, and they should all be using the common xfstests
> quota infrastructure....

Agreed, but we want xfstests to be able to support systems where
linux-quota (aka quotatools) and/or e2fsprogs and/or the kernel
haven't been upgraded to support project quota, don't we?  If for no
other reason than to be kind to the poor souls who have to support
RHEL 6.  :-)

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Sandeen July 8, 2016, 3:19 a.m. UTC | #6

On 7/7/16 9:46 PM, Theodore Ts'o wrote:
> On Fri, Jul 08, 2016 at 10:51:27AM +1000, Dave Chinner wrote:
>> On Wed, Jul 06, 2016 at 09:47:28PM -0500, Eric Sandeen wrote:
>>> On 7/6/16 6:35 PM, Dave Chinner wrote:
>>>
>>> ...
>>>
>>>>> +_require_scratch
>>>>> +_require_chattr
>>>>> +_require_test_lsattr
>>>>> +_require_quota
>>>>
>>>> needs  _require_prjquota, and that function needs to be modified to
>>>> detect for both XFS and ext4 support.
>>>
>>> I think that if there is desire to test both xfs and non-xfs userspace
>>> with project quota, then we need to differentiate between "e2fsprogs
>>> and linux-quota and the kernel all support it" and "xfsprogs and
>>> the kernel both support it" don't we?
>>
>> Well, it should be just "linux-quota and kernel". ext4 needs to
>> have the same mount option behaviour for project quota as it does
>> for all other types of quota, not be dependent on mkfs....
> 
> Project quota for ext4 is an optional thing, and if nothing else, we
> need to have a separate feature flag for legacy file systems that were
> created before we started supporting project quota.  So if you want to
> support project quota you *will* need to have a version of e2fsck that
> understands project quota, and a version of mke2fs that knows how to
> request that project quota be enabled, etc., etc.
> 
> So while it might be *nice* if ext4 could support project quota
> without being dependent on having a specific version of mke2fs and
> e2fsck installed, it's just simply not possible....
> 
>>> IOWs if the test uses setquota/repquota, chattr, mkfs, and fsck to
>>> work with project quota, then that's a different set of requirements
>>> from a test using xfs_io, xfs_quota, etc.
>>
>> _require_linux_prjquota
>> _require_xfs_prjquota
>>
>> But that said, both ext4 and xfs need to work for both
>> configurations, and they should all be using the common xfstests
>> quota infrastructure....
> 
> Agreed, but we want xfstests to be able to support systems where
> linux-quota (aka quotatools) and/or e2fsprogs and/or the kernel
> haven't been upgraded to support project quota, don't we?  If for no
> other reason than to be kind to the poor souls who have to support
> RHEL 6.  :-)

It's unlikely that ext4 project quota will find its way to RHEL6. ;)

But the point I keep trying to make - and failing, apparently - 
is that we will / should have two sets of tests for userspace
functionality at least; one using standard quota tools, and one
using xfs_quota.  Both should test the same kernel paths, but
if we want to know that userspace is working we need to test both.

And to test one or the other, we need to know that *it* supports
project quota before proceeding.

I don't know how we got to the point where we have 2 parallel
quota infrastructures, it's an unfortunate mess IMHO.  :(

But if we want to test xfs_quota tests on ext4, we still
need to know that e2fsprogs is pquota-capable.

If we want to test standard quota tools on ext4, we need to know
that *those* binaries are capable, as well as e2fsprogs.  etc...

-Eric

> 						- Ted
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner July 8, 2016, 4:57 a.m. UTC | #7

On Thu, Jul 07, 2016 at 10:19:27PM -0500, Eric Sandeen wrote:
> 
> 
> On 7/7/16 9:46 PM, Theodore Ts'o wrote:
> > On Fri, Jul 08, 2016 at 10:51:27AM +1000, Dave Chinner wrote:
> >> On Wed, Jul 06, 2016 at 09:47:28PM -0500, Eric Sandeen wrote:
> >>> On 7/6/16 6:35 PM, Dave Chinner wrote:
> >>>
> >>> ...
> >>>
> >>>>> +_require_scratch
> >>>>> +_require_chattr
> >>>>> +_require_test_lsattr
> >>>>> +_require_quota
> >>>>
> >>>> needs  _require_prjquota, and that function needs to be modified to
> >>>> detect for both XFS and ext4 support.
> >>>
> >>> I think that if there is desire to test both xfs and non-xfs userspace
> >>> with project quota, then we need to differentiate between "e2fsprogs
> >>> and linux-quota and the kernel all support it" and "xfsprogs and
> >>> the kernel both support it" don't we?
> >>
> >> Well, it should be just "linux-quota and kernel". ext4 needs to
> >> have the same mount option behaviour for project quota as it does
> >> for all other types of quota, not be dependent on mkfs....
> > 
> > Project quota for ext4 is an optional thing, and if nothing else, we
> > need to have a separate feature flag for legacy file systems that were
> > created before we started supporting project quota.

No shit, Sherlock.

But we don't need the feature bit set by mkfs to support project
quota - it only needs to be set when the project quota file is
created by quotacheck. Or by the first mount -o pquota operation
after quotacheck has created the quotafile.

Either way, project quota can be dynamically turned on and be
protected against legacy filesystems, and it still can't be turned
on without a userspace or kernel that understands project quotas
being installed.

> > So if you want to
> > support project quota you *will* need to have a version of e2fsck that
> > understands project quota, and a version of mke2fs that knows how to
> > request that project quota be enabled, etc., etc.
> > So while it might be *nice* if ext4 could support project quota
> > without being dependent on having a specific version of mke2fs and
> > e2fsck installed, it's just simply not possible....

We can test for that in _requires_linux_prjquota() and _notrun the
test is it isn't present. We do this to test for all optional
features in different filesystems - how is doing this for
ext4 project quota support in any way difficult or so special it's
not possible to implement such checks?

> >>> IOWs if the test uses setquota/repquota, chattr, mkfs, and fsck to
> >>> work with project quota, then that's a different set of requirements
> >>> from a test using xfs_io, xfs_quota, etc.
> >>
> >> _require_linux_prjquota
> >> _require_xfs_prjquota
> >>
> >> But that said, both ext4 and xfs need to work for both
> >> configurations, and they should all be using the common xfstests
> >> quota infrastructure....
> > 
> > Agreed, but we want xfstests to be able to support systems where
> > linux-quota (aka quotatools) and/or e2fsprogs and/or the kernel
> > haven't been upgraded to support project quota, don't we?  If for no
> > other reason than to be kind to the poor souls who have to support
> > RHEL 6.  :-)
> 
> It's unlikely that ext4 project quota will find its way to RHEL6. ;)
> 
> But the point I keep trying to make - and failing, apparently - 
> is that we will / should have two sets of tests for userspace
> functionality at least; one using standard quota tools, and one
> using xfs_quota.  Both should test the same kernel paths, but
> if we want to know that userspace is working we need to test both.

That's what I've been trying to say.

i.e.  the only difference between two project quota tests should be
the binaries run to set project quota flags, limits and get reports.
Otherwise the tests should be the same, similar to how we already
abstract quota setup and mounting, or like we abstract the fallocate
vs XFS preallocation/punch/zero commands in xfs_io in
_test_generic_punch().

i.e. test code is common between two tests, only the setup is
different. And, most importantly, they should give identical
accounting results.

Cheers,

Dave.

Theodore Ts'o July 8, 2016, 5:02 a.m. UTC | #8

On Thu, Jul 07, 2016 at 10:19:27PM -0500, Eric Sandeen wrote:
> 
> But the point I keep trying to make - and failing, apparently - 
> is that we will / should have two sets of tests for userspace
> functionality at least; one using standard quota tools, and one
> using xfs_quota.  Both should test the same kernel paths, but
> if we want to know that userspace is working we need to test both.

I agree completely that we should test both userspace stacks.  Where I
disagree is whether this has anything to do with using the usrquota
and grpquota mount options, and which ext4 mkfs options are used to
set up quota or project quota support.  That's a completely orthogonal
issue.

> I don't know how we got to the point where we have 2 parallel
> quota infrastructures, it's an unfortunate mess IMHO.  :(

Actually, I've been staring at the quotatools source code and it's
even more complicated than that.  There are newer quotactl
subcommands, such as Q_XGETQSTAT and Q_XGETQSTATV, which currently
quotatools only tries using if it thinks the quota format (which in
this sense seems to be system API, not the actual quota file format
--- these two concepts seem to have been overloaded at some point) is
"xfs".

Currently quotatools only assumes the "xfs" quota format should be
used for "xfs" and "gfs" --- but it works for other file systems,
including ext4 as well.  As a result, there's certain information,
such as whether ext4 is doing limits enforcement as well as quota
tracking, which is *not* being exposed to the user.  I suspect one of
the reasons for this is the tests in quotatools for which kernel
interfaces are present are fairly primitive, and in fact there are
some comments in quotasys.c which makes references to behaviours of
certain specific Red Hat kernel versions to decide which interfaces
are available.  :-(

And if we just did the simple thing to enable use of the "new" (aka
"xfs", although this is ***massive*** misnomer) quota format in
quotatools, it would break if the latest quota tools were ever
compiled on older Enterprise Linux systems.

Ugh.  I suspect one of the reasons why things have gotten into such a
mess is that quota is mostly an enterprise feature, and most community
distros and community users/kernel developers don't really use it.  As
a result, no one has bothered to put in clean ways of doing interface
versioning, as I suspect a lot of work happens right before the
Enterprise Linux cutoff date, and there isn't much incentive to clean
things up afterwards.

> But if we want to test xfs_quota tests on ext4, we still
> need to know that e2fsprogs is pquota-capable.
> 
> If we want to test standard quota tools on ext4, we need to know
> that *those* binaries are capable, as well as e2fsprogs.  etc...

Completely agree.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kara July 11, 2016, 4:15 p.m. UTC | #9

On Fri 08-07-16 01:02:28, Ted Tso wrote:
> On Thu, Jul 07, 2016 at 10:19:27PM -0500, Eric Sandeen wrote:
> > I don't know how we got to the point where we have 2 parallel
> > quota infrastructures, it's an unfortunate mess IMHO.  :(
> 
> Actually, I've been staring at the quotatools source code and it's
> even more complicated than that.  There are newer quotactl
> subcommands, such as Q_XGETQSTAT and Q_XGETQSTATV, which currently
> quotatools only tries using if it thinks the quota format (which in
> this sense seems to be system API, not the actual quota file format
> --- these two concepts seem to have been overloaded at some point) is
> "xfs".

quota-tools currently assume that certain quotactl(8) calls are fine only
for certain quota formats. And this has been long true - since its
beginning, XFS and VFS quota stacks were pretty independent including
supported quotactl calls. Only recently (about year and half ago) I have
added necessary plumbing so that "XFS quotactls" and "VFS quotactls" can be
used independently of the underlying filesystem. The distinction still
remained in quota-tools because there was no real need in unifying them and
we still have to keep knowing about the distinction so that quota-tools can
work with older kernels.

> Currently quotatools only assumes the "xfs" quota format should be
> used for "xfs" and "gfs" --- but it works for other file systems,
> including ext4 as well.  As a result, there's certain information,
> such as whether ext4 is doing limits enforcement as well as quota
> tracking, which is *not* being exposed to the user.  I suspect one of
> the reasons for this is the tests in quotatools for which kernel
> interfaces are present are fairly primitive, and in fact there are
> some comments in quotasys.c which makes references to behaviours of
> certain specific Red Hat kernel versions to decide which interfaces
> are available.  :-(

No, the reason is that until recently there was no kernel interface
to convey this information to userspace for VFS based quotas and nobody
complained :). If there is some functionality you miss in quota-tools, then
I can have a look at implementing it. E.g. reporting whether only tracking
or also enforcement is enabled is relatively simple to add.
 
> And if we just did the simple thing to enable use of the "new" (aka
> "xfs", although this is ***massive*** misnomer) quota format in
> quotatools, it would break if the latest quota tools were ever
> compiled on older Enterprise Linux systems.

What would you like to achieve with this? There is 'QF_META' format which
is different from 'QF_XFS' format basically only in the set of quotactls
used. As I said above it might be nice to separate kernel-api from the
underlying-quota-format but in reality these two were bound together in
older kernels so they are not really independent.

								Honza

Theodore Ts'o July 11, 2016, 5:12 p.m. UTC | #10

On Mon, Jul 11, 2016 at 06:15:56PM +0200, Jan Kara wrote:
> 
> What would you like to achieve with this? There is 'QF_META' format which
> is different from 'QF_XFS' format basically only in the set of quotactls
> used. As I said above it might be nice to separate kernel-api from the
> underlying-quota-format but in reality these two were bound together in
> older kernels so they are not really independent.

The main reason why I noticed is with the new (err, "latest") ext4
quota (enabled via mke2fs -t ext4 -O quota) implementation, we enable
quota tracking at mount time.  (This may be true with journalled quota
as well, actually).  But we don't actually enable quota *enforcement*
until quotaon is given.

The problem is that quotaon -p prints the status of whether or not
quota *tracking* is enabled, and with the new ext4 quota, quota
tracking is *always* enabled.  So quota -p doesn't report anything
useful for new ext4 quota systems, and when I started to look at how
to change things, that's when I noticed that we weren't using the new
quotactl commands with ext4 even though they worked, and that the new
quotactl implementation had more functionality than the older ones.

BTW, I've seriously been thinking about changing the default so that
if you use mke2fs -O quota, quota enforcement is also enabled by
default at mount time, and we use a mount option to disable quota
enforcement.  If we then added a way of selectively enabling and
disabling quota enforcement via quota-tools, then we would be bringing
behaivour of how ext4 quota works to like how xfs treats quota.  The
question I have is how to do this in a way that isn't surprising for
people who are used to the old behaviour --- but mke2fs -O quota is
still relatively new, so maybe we could get away with it without
having to add more superblock flags.

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kara July 12, 2016, 10:59 a.m. UTC | #11

On Mon 11-07-16 13:12:42, Ted Tso wrote:
> On Mon, Jul 11, 2016 at 06:15:56PM +0200, Jan Kara wrote:
> > 
> > What would you like to achieve with this? There is 'QF_META' format which
> > is different from 'QF_XFS' format basically only in the set of quotactls
> > used. As I said above it might be nice to separate kernel-api from the
> > underlying-quota-format but in reality these two were bound together in
> > older kernels so they are not really independent.
> 
> The main reason why I noticed is with the new (err, "latest") ext4
> quota (enabled via mke2fs -t ext4 -O quota) implementation, we enable
> quota tracking at mount time.  (This may be true with journalled quota
> as well, actually).  But we don't actually enable quota *enforcement*
> until quotaon is given.

Yes.

> The problem is that quotaon -p prints the status of whether or not
> quota *tracking* is enabled, and with the new ext4 quota, quota
> tracking is *always* enabled.  So quota -p doesn't report anything
> useful for new ext4 quota systems, and when I started to look at how
> to change things, that's when I noticed that we weren't using the new
> quotactl commands with ext4 even though they worked, and that the new
> quotactl implementation had more functionality than the older ones.

OK. But with XFS you'd notice that quotaon -p also returns 'on' whenever
the accounting is turned on. So ext4 and xfs behave in the same way.
Arguably it would be more useful if quotaon -p reported 'off', 'accounting',
'enforcement'. Maybe I'll do that.

> BTW, I've seriously been thinking about changing the default so that
> if you use mke2fs -O quota, quota enforcement is also enabled by
> default at mount time, and we use a mount option to disable quota
> enforcement.  If we then added a way of selectively enabling and
> disabling quota enforcement via quota-tools, then we would be bringing
> behaivour of how ext4 quota works to like how xfs treats quota.  The
> question I have is how to do this in a way that isn't surprising for
> people who are used to the old behaviour --- but mke2fs -O quota is
> still relatively new, so maybe we could get away with it without
> having to add more superblock flags.

So currently if you use quotaon(8) it will turn on/off only enforcement in
ext4 (accounting is always on when the feature is enabled) - don't get
confused with 'quotaon -p' output - that tests something different.

Speaking of automatic enabling of quota enforcement: I wanted to keep the
old behavior where no enforcement happens until you run quotaon(8) which is
how things traditionally worked for ext2/3/4. That's why things default to
having enforcement off. If we want to make things more consistent with XFS,
one option I can see is that when e.g. 'usrquota' mount option is set, then
user quota enforcement will be turned on. That is essentially how XFS
works (including the mount option name). What do you think?

								Honza

Jan Kara July 12, 2016, 2:32 p.m. UTC | #12

On Tue 12-07-16 12:59:08, Jan Kara wrote:
> On Mon 11-07-16 13:12:42, Ted Tso wrote:
> > On Mon, Jul 11, 2016 at 06:15:56PM +0200, Jan Kara wrote:
> > > 
> > > What would you like to achieve with this? There is 'QF_META' format which
> > > is different from 'QF_XFS' format basically only in the set of quotactls
> > > used. As I said above it might be nice to separate kernel-api from the
> > > underlying-quota-format but in reality these two were bound together in
> > > older kernels so they are not really independent.
> > 
> > The main reason why I noticed is with the new (err, "latest") ext4
> > quota (enabled via mke2fs -t ext4 -O quota) implementation, we enable
> > quota tracking at mount time.  (This may be true with journalled quota
> > as well, actually).  But we don't actually enable quota *enforcement*
> > until quotaon is given.
> 
> Yes.
> 
> > The problem is that quotaon -p prints the status of whether or not
> > quota *tracking* is enabled, and with the new ext4 quota, quota
> > tracking is *always* enabled.  So quota -p doesn't report anything
> > useful for new ext4 quota systems, and when I started to look at how
> > to change things, that's when I noticed that we weren't using the new
> > quotactl commands with ext4 even though they worked, and that the new
> > quotactl implementation had more functionality than the older ones.
> 
> OK. But with XFS you'd notice that quotaon -p also returns 'on' whenever
> the accounting is turned on. So ext4 and xfs behave in the same way.
> Arguably it would be more useful if quotaon -p reported 'off', 'accounting',
> 'enforcement'. Maybe I'll do that.

This is now done and push out to quota-tools repository. When XGETSTAT
quotactl is available, 'quotaon -pva' will report whether the quota is
enabled only for accounting or also enforced.

								Honza

Theodore Ts'o July 12, 2016, 4:15 p.m. UTC | #13

On Tue, Jul 12, 2016 at 12:59:08PM +0200, Jan Kara wrote:
> OK. But with XFS you'd notice that quotaon -p also returns 'on' whenever
> the accounting is turned on. So ext4 and xfs behave in the same way.
> Arguably it would be more useful if quotaon -p reported 'off', 'accounting',
> 'enforcement'. Maybe I'll do that.

That would be good, since at the moment to determine whether or not
quota enforcement is enabled or not.

> Speaking of automatic enabling of quota enforcement: I wanted to keep the
> old behavior where no enforcement happens until you run quotaon(8) which is
> how things traditionally worked for ext2/3/4. That's why things default to
> having enforcement off. If we want to make things more consistent with XFS,
> one option I can see is that when e.g. 'usrquota' mount option is set, then
> user quota enforcement will be turned on. That is essentially how XFS
> works (including the mount option name). What do you think?

That seems reasonable.  The only concern is that it might be confusing
for people who are using older, legacy quota setups, but given that
usrquota would cause enforcing plus accounting to be enabled, it
shouldn't cause a problem.

I think aligning with XFS so that the user experience for quota should
be as identical as possible regardless of which file system is being
used makes a lot of sense, especially since you've been adding a bunch
of the plumbing for quotactl to make this possible.

On the kernel side this means that teaching ext4 so that if the
usrquota monut option is specified, quota enforcing will be enabled.
We should make the necessary changes in kernel and possily quota-tools
so that quotaoff can also disable quota enforcing (just like with
XFS).

Does that sound like a plan?

          				- Ted
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/tests/generic/362 b/tests/generic/362
new file mode 100755
index 0000000..f763bc5
--- /dev/null
+++ b/tests/generic/362
@@ -0,0 +1,130 @@ 
+#! /bin/bash
+# FS QA Test No. 030
+#
+# Test Project quota attr function
+#
+#-----------------------------------------------------------------------
+# Copyright 2016 (C) Wang Shilong <[e-mail]>
+#
+# 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
+#
+#-----------------------------------------------------------------------
+#
+
+seqfull=$0
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    rm -f $tmp.*
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+. ./common/quota
+
+# real QA test starts here
+_supported_fs ext4 xfs
+_supported_os Linux
+
+_require_scratch
+_require_chattr
+_require_test_lsattr
+_require_quota
+
+rm -f $seqres.full
+MKFSOPTIONS=""
+MOUNTOPTIONS=""
+
+function setup_quota_options()
+{
+    case $FSTYP in
+    xfs)
+	#quotaon rely on this.
+	MOUNTOPTIONS="-o pquota"
+	;;
+    ext4)
+	#project quota is disabled in default.
+	MKFSOPTIONS="-O quota,project"
+	;;
+    *)
+        ;;
+    esac
+
+}
+
+function set_inherit_attribute()
+{
+    case $FSTYP in
+    xfs)
+	$XFS_IO_PROG -r -c 'chattr +P' $*
+	;;
+    ext4)
+	chattr +P $*
+	;;
+    *)
+        ;;
+    esac
+}
+
+setup_quota_options
+
+echo "+ create scratch fs"
+_scratch_mkfs $MKFSOPTIONS > /dev/null 2>&1
+
+echo "+ mount fs image"
+_scratch_mount $MOUNTOPTIONS
+
+function attr_test()
+{
+	#default project without inherit
+	mkdir $SCRATCH_MNT/dir
+	chattr -p 123456 $SCRATCH_MNT/dir | _filter_scratch | _filter_spaces
+	lsattr -p $SCRATCH_MNT/dir | _filter_scratch | $AWK_PROG '{print $1" "$3}'
+	touch $SCRATCH_MNT/dir/foo
+	lsattr -p $SCRATCH_MNT/dir/foo | _filter_scratch | $AWK_PROG '{print $1" "$3}'
+
+	#test project inherit with inherit attribute
+	set_inherit_attribute $SCRATCH_MNT/dir
+	touch $SCRATCH_MNT/dir/foo1
+	lsattr -p $SCRATCH_MNT/dir/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
+
+	#Link accross project should fail
+	mkdir $SCRATCH_MNT/dir1
+	set_inherit_attribute $SCRATCH_MNT/dir1
+	chattr -p 654321 $SCRATCH_MNT/dir1 | _filter_scratch | _filter_spaces
+	ln $SCRATCH_MNT/dir/foo1 $SCRATCH_MNT/dir1/foo1 2>&1 \
+		| _filter_scratch | _filter_spaces
+
+	#mv accross different projects should work
+	mv $SCRATCH_MNT/dir/foo1 $SCRATCH_MNT/dir1/foo1
+	lsattr -p $SCRATCH_MNT/dir1/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
+
+	#change project ignores quota
+	quotaon $SCRATCH_MNT >/dev/null 2>&1
+	setquota -P 654321 0 0 0 1 $SCRATCH_MNT/
+	chattr -p 123456 $SCRATCH_MNT/dir1/foo1 | _filter_scratch | _filter_spaces
+	lsattr -p $SCRATCH_MNT/dir1/foo1 | _filter_scratch | $AWK_PROG '{print $1" "$3}'
+}
+attr_test
diff --git a/tests/generic/362.out b/tests/generic/362.out
new file mode 100644
index 0000000..31db991
--- /dev/null
+++ b/tests/generic/362.out
@@ -0,0 +1,8 @@ 
+QA output created by 362
++ create scratch fs
++ mount fs image
+0 SCRATCH_MNT/dir/foo
+123456 SCRATCH_MNT/dir/foo1
+ln: failed to create hard link 'SCRATCH_MNT/dir1/foo1' => 'SCRATCH_MNT/dir/foo1': Invalid cross-device link
+654321 SCRATCH_MNT/dir1/foo1
+123456 SCRATCH_MNT/dir1/foo1
diff --git a/tests/generic/group b/tests/generic/group
index 7491282..e834762 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -364,3 +364,4 @@ 
 359 auto quick clone
 360 auto quick metadata
 361 auto quick
+362 auto quick