* [PATCH v2] xfstests, generic: add project quota attribute tests @ 2016-07-06 6:22 Wang Shilong 2016-07-06 16:10 ` Eric Sandeen 2016-07-06 23:35 ` Dave Chinner 0 siblings, 2 replies; 18+ messages in thread From: Wang Shilong @ 2016-07-06 6:22 UTC (permalink / raw) To: fstests; +Cc: linux-ext4, tytso, sihara, lixi, Wang Shilong From: Wang Shilong <wshilong@ddn.com> 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 <wshilong@ddn.com> --- 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 <wshilong@ddn.com> +# +# 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 -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-06 6:22 [PATCH v2] xfstests, generic: add project quota attribute tests Wang Shilong @ 2016-07-06 16:10 ` Eric Sandeen 2016-07-06 23:35 ` Dave Chinner 1 sibling, 0 replies; 18+ messages in thread From: Eric Sandeen @ 2016-07-06 16:10 UTC (permalink / raw) To: Wang Shilong, fstests; +Cc: linux-ext4, tytso, sihara, lixi, Wang Shilong On 7/6/16 1:22 AM, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > 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 <wshilong@ddn.com> > --- > 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 <wshilong@ddn.com> > +# > +# 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-06 6:22 [PATCH v2] xfstests, generic: add project quota attribute tests Wang Shilong 2016-07-06 16:10 ` Eric Sandeen @ 2016-07-06 23:35 ` Dave Chinner 2016-07-07 2:47 ` Eric Sandeen 1 sibling, 1 reply; 18+ messages in thread From: Dave Chinner @ 2016-07-06 23:35 UTC (permalink / raw) To: Wang Shilong; +Cc: fstests, linux-ext4, tytso, sihara, lixi, Wang Shilong On Wed, Jul 06, 2016 at 03:22:51PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > 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 <wshilong@ddn.com> 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 <wshilong@ddn.com> 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-06 23:35 ` Dave Chinner @ 2016-07-07 2:47 ` Eric Sandeen 2016-07-08 0:51 ` Dave Chinner 0 siblings, 1 reply; 18+ messages in thread From: Eric Sandeen @ 2016-07-07 2:47 UTC (permalink / raw) To: Dave Chinner, Wang Shilong Cc: fstests, linux-ext4, tytso, sihara, lixi, Wang Shilong 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-07 2:47 ` Eric Sandeen @ 2016-07-08 0:51 ` Dave Chinner 2016-07-08 2:46 ` Theodore Ts'o 0 siblings, 1 reply; 18+ messages in thread From: Dave Chinner @ 2016-07-08 0:51 UTC (permalink / raw) To: Eric Sandeen Cc: Wang Shilong, fstests, linux-ext4, tytso, sihara, lixi, Wang Shilong 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-08 0:51 ` Dave Chinner @ 2016-07-08 2:46 ` Theodore Ts'o 2016-07-08 3:19 ` Eric Sandeen 0 siblings, 1 reply; 18+ messages in thread From: Theodore Ts'o @ 2016-07-08 2:46 UTC (permalink / raw) To: Dave Chinner Cc: Eric Sandeen, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-08 2:46 ` Theodore Ts'o @ 2016-07-08 3:19 ` Eric Sandeen 2016-07-08 4:57 ` Dave Chinner 2016-07-08 5:02 ` Theodore Ts'o 0 siblings, 2 replies; 18+ messages in thread From: Eric Sandeen @ 2016-07-08 3:19 UTC (permalink / raw) To: Theodore Ts'o, Dave Chinner Cc: Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-08 3:19 ` Eric Sandeen @ 2016-07-08 4:57 ` Dave Chinner 2016-07-08 5:02 ` Theodore Ts'o 1 sibling, 0 replies; 18+ messages in thread From: Dave Chinner @ 2016-07-08 4:57 UTC (permalink / raw) To: Eric Sandeen Cc: Theodore Ts'o, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-08 3:19 ` Eric Sandeen 2016-07-08 4:57 ` Dave Chinner @ 2016-07-08 5:02 ` Theodore Ts'o 2016-07-11 16:15 ` Jan Kara 1 sibling, 1 reply; 18+ messages in thread From: Theodore Ts'o @ 2016-07-08 5:02 UTC (permalink / raw) To: Eric Sandeen Cc: Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-08 5:02 ` Theodore Ts'o @ 2016-07-11 16:15 ` Jan Kara 2016-07-11 17:12 ` Theodore Ts'o 0 siblings, 1 reply; 18+ messages in thread From: Jan Kara @ 2016-07-11 16:15 UTC (permalink / raw) To: Theodore Ts'o Cc: Eric Sandeen, Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-11 16:15 ` Jan Kara @ 2016-07-11 17:12 ` Theodore Ts'o 2016-07-12 10:59 ` Jan Kara 0 siblings, 1 reply; 18+ messages in thread From: Theodore Ts'o @ 2016-07-11 17:12 UTC (permalink / raw) To: Jan Kara Cc: Eric Sandeen, Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-11 17:12 ` Theodore Ts'o @ 2016-07-12 10:59 ` Jan Kara 2016-07-12 14:32 ` Jan Kara 2016-07-12 16:15 ` Theodore Ts'o 0 siblings, 2 replies; 18+ messages in thread From: Jan Kara @ 2016-07-12 10:59 UTC (permalink / raw) To: Theodore Ts'o Cc: Jan Kara, Eric Sandeen, Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-12 10:59 ` Jan Kara @ 2016-07-12 14:32 ` Jan Kara 2016-07-12 16:15 ` Theodore Ts'o 1 sibling, 0 replies; 18+ messages in thread From: Jan Kara @ 2016-07-12 14:32 UTC (permalink / raw) To: Theodore Ts'o Cc: Jan Kara, Eric Sandeen, Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-12 10:59 ` Jan Kara 2016-07-12 14:32 ` Jan Kara @ 2016-07-12 16:15 ` Theodore Ts'o 2016-07-14 13:13 ` Jan Kara 1 sibling, 1 reply; 18+ messages in thread From: Theodore Ts'o @ 2016-07-12 16:15 UTC (permalink / raw) To: Jan Kara Cc: Eric Sandeen, Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-12 16:15 ` Theodore Ts'o @ 2016-07-14 13:13 ` Jan Kara 2016-07-15 1:15 ` Wang Shilong 0 siblings, 1 reply; 18+ messages in thread From: Jan Kara @ 2016-07-14 13:13 UTC (permalink / raw) To: Theodore Ts'o Cc: Jan Kara, Eric Sandeen, Dave Chinner, Wang Shilong, fstests, linux-ext4, sihara, lixi, Wang Shilong [-- Attachment #1: Type: text/plain, Size: 2675 bytes --] On Tue 12-07-16 12:15:47, Ted Tso wrote: > 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. This is now done. > > 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. Yes, my thinking about this was that when you transfer from a legacy quota setup to the latest one with hidden quota files, you have to somewhat update your mindset anyway (e.g. quota information is now created using tune2fs / mke2fs and checked using e2fsck instead of quotacheck(8)). So the fact that you won't need quotaon(8) is only a small addition to that. > 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? Yes. quotaoff will already disable only enforcement for ext4 with hidden quota files so there's no need to change anything there. We just need to add kernel support for enabling quota enforcement based on mount option. Attached patch should do that - I still need to test whether it doesn't break anything so don't apply it yet, just have a look whether it looks sane to you. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR [-- Attachment #2: 0001-ext4-Enable-quota-enforcement-based-on-mount-options.patch --] [-- Type: text/x-patch, Size: 5071 bytes --] >From 80653a01d0a7ec10598ad5d1eee55612d5a355a5 Mon Sep 17 00:00:00 2001 From: Jan Kara <jack@suse.cz> Date: Thu, 14 Jul 2016 15:08:24 +0200 Subject: [PATCH] ext4: Enable quota enforcement based on mount options When quota information is stored in quota files, we enable only quota accounting on mount and enforcement is enabled only in response to Q_QUOTAON quotactl. To make ext4 behavior consistent with XFS, we add a possibility to enable quota enforcement on mount by specifying corresponding quota mount option (usrquota, grpquota, prjquota). Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/ext4.h | 12 +++++++++--- fs/ext4/super.c | 34 ++++++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index b84aa1ca480a..224cb832e2dc 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -1129,9 +1129,15 @@ struct ext4_inode_info { #define EXT4_MOUNT_POSIX_ACL 0x08000 /* POSIX Access Control Lists */ #define EXT4_MOUNT_NO_AUTO_DA_ALLOC 0x10000 /* No auto delalloc mapping */ #define EXT4_MOUNT_BARRIER 0x20000 /* Use block barriers */ -#define EXT4_MOUNT_QUOTA 0x80000 /* Some quota option set */ -#define EXT4_MOUNT_USRQUOTA 0x100000 /* "old" user quota */ -#define EXT4_MOUNT_GRPQUOTA 0x200000 /* "old" group quota */ +#define EXT4_MOUNT_QUOTA 0x40000 /* Some quota option set */ +#define EXT4_MOUNT_USRQUOTA 0x80000 /* "old" user quota, + * enable enforcement for hidden + * quota files */ +#define EXT4_MOUNT_GRPQUOTA 0x100000 /* "old" group quota, enable + * enforcement for hidden quota + * files */ +#define EXT4_MOUNT_PRJQUOTA 0x200000 /* Enable project quota + * enforcement */ #define EXT4_MOUNT_DIOREAD_NOLOCK 0x400000 /* Enable support for dio read nolocking */ #define EXT4_MOUNT_JOURNAL_CHECKSUM 0x800000 /* Journal checksums */ #define EXT4_MOUNT_JOURNAL_ASYNC_COMMIT 0x1000000 /* Journal Async Commit */ diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 3822a5aedc61..a03232634cd5 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1187,7 +1187,7 @@ enum { Opt_usrjquota, Opt_grpjquota, Opt_offusrjquota, Opt_offgrpjquota, Opt_jqfmt_vfsold, Opt_jqfmt_vfsv0, Opt_jqfmt_vfsv1, Opt_quota, Opt_noquota, Opt_barrier, Opt_nobarrier, Opt_err, - Opt_usrquota, Opt_grpquota, Opt_i_version, Opt_dax, + Opt_usrquota, Opt_grpquota, Opt_prjquota, Opt_i_version, Opt_dax, Opt_stripe, Opt_delalloc, Opt_nodelalloc, Opt_mblk_io_submit, Opt_lazytime, Opt_nolazytime, Opt_nomblk_io_submit, Opt_block_validity, Opt_noblock_validity, @@ -1247,6 +1247,7 @@ static const match_table_t tokens = { {Opt_noquota, "noquota"}, {Opt_quota, "quota"}, {Opt_usrquota, "usrquota"}, + {Opt_prjquota, "prjquota"}, {Opt_barrier, "barrier=%u"}, {Opt_barrier, "barrier"}, {Opt_nobarrier, "nobarrier"}, @@ -1466,8 +1467,11 @@ static const struct mount_opts { MOPT_SET | MOPT_Q}, {Opt_grpquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_GRPQUOTA, MOPT_SET | MOPT_Q}, + {Opt_prjquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_PRJQUOTA, + MOPT_SET | MOPT_Q}, {Opt_noquota, (EXT4_MOUNT_QUOTA | EXT4_MOUNT_USRQUOTA | - EXT4_MOUNT_GRPQUOTA), MOPT_CLEAR | MOPT_Q}, + EXT4_MOUNT_GRPQUOTA | EXT4_MOUNT_PRJQUOTA), + MOPT_CLEAR | MOPT_Q}, {Opt_usrjquota, 0, MOPT_Q}, {Opt_grpjquota, 0, MOPT_Q}, {Opt_offusrjquota, 0, MOPT_Q}, @@ -1756,13 +1760,17 @@ static int parse_options(char *options, struct super_block *sb, return 0; } #ifdef CONFIG_QUOTA - if (ext4_has_feature_quota(sb) && - (test_opt(sb, USRQUOTA) || test_opt(sb, GRPQUOTA))) { - ext4_msg(sb, KERN_INFO, "Quota feature enabled, usrquota and grpquota " - "mount options ignored."); - clear_opt(sb, USRQUOTA); - clear_opt(sb, GRPQUOTA); - } else if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) { + /* + * We do the test below only for project quotas. 'usrquota' and + * 'grpquota' mount options are allowed even without quota feature + * to support legacy quotas in quota files. + */ + if (test_opt(sb, PRJQUOTA) && !ext4_has_feature_project(sb)) { + ext4_msg(sb, KERN_ERR, "Project quota feature not enabled. " + "Cannot enable project quota enforcement."); + return 0; + } + if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) { if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA]) clear_opt(sb, USRQUOTA); @@ -5129,12 +5137,18 @@ static int ext4_enable_quotas(struct super_block *sb) le32_to_cpu(EXT4_SB(sb)->s_es->s_grp_quota_inum), le32_to_cpu(EXT4_SB(sb)->s_es->s_prj_quota_inum) }; + bool quota_mopt[EXT4_MAXQUOTAS] = { + test_opt(sb, USRQUOTA), + test_opt(sb, GRPQUOTA), + test_opt(sb, PRJQUOTA), + }; sb_dqopt(sb)->flags |= DQUOT_QUOTA_SYS_FILE; for (type = 0; type < EXT4_MAXQUOTAS; type++) { if (qf_inums[type]) { err = ext4_quota_enable(sb, type, QFMT_VFS_V1, - DQUOT_USAGE_ENABLED); + DQUOT_USAGE_ENABLED | + (quota_mopt[type] ? DQUOT_LIMITS_ENABLED : 0)); if (err) { ext4_warning(sb, "Failed to enable quota tracking " -- 2.6.6 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-14 13:13 ` Jan Kara @ 2016-07-15 1:15 ` Wang Shilong 2016-07-18 10:20 ` Jan Kara 0 siblings, 1 reply; 18+ messages in thread From: Wang Shilong @ 2016-07-15 1:15 UTC (permalink / raw) To: Jan Kara Cc: Theodore Ts'o, Eric Sandeen, Dave Chinner, fstests, Ext4 Developers List, Shuichi Ihara, Li Xi, Wang Shilong On Thu, Jul 14, 2016 at 10:13 PM, Jan Kara <jack@suse.cz> wrote: > On Tue 12-07-16 12:15:47, Ted Tso wrote: >> Does that sound like a plan? > > Yes. quotaoff will already disable only enforcement for ext4 with hidden > quota files so there's no need to change anything there. We just need to > add kernel support for enabling quota enforcement based on mount option. > Attached patch should do that - I still need to test whether it doesn't > break anything so don't apply it yet, just have a look whether it looks > sane to you. I sent a similar patch weeks ago. http://marc.info/?l=linux-ext4&m=146787897927183&w=2 and also quota tools to support it: http://marc.info/?l=linux-ext4&m=146787899027184&w=2 > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-15 1:15 ` Wang Shilong @ 2016-07-18 10:20 ` Jan Kara 2016-07-18 12:45 ` Wang Shilong 0 siblings, 1 reply; 18+ messages in thread From: Jan Kara @ 2016-07-18 10:20 UTC (permalink / raw) To: Wang Shilong Cc: Jan Kara, Theodore Ts'o, Eric Sandeen, Dave Chinner, fstests, Ext4 Developers List, Shuichi Ihara, Li Xi, Wang Shilong On Fri 15-07-16 10:15:31, Wang Shilong wrote: > On Thu, Jul 14, 2016 at 10:13 PM, Jan Kara <jack@suse.cz> wrote: > > On Tue 12-07-16 12:15:47, Ted Tso wrote: > > >> Does that sound like a plan? > > > > Yes. quotaoff will already disable only enforcement for ext4 with hidden > > quota files so there's no need to change anything there. We just need to > > add kernel support for enabling quota enforcement based on mount option. > > Attached patch should do that - I still need to test whether it doesn't > > break anything so don't apply it yet, just have a look whether it looks > > sane to you. > > I sent a similar patch weeks ago. > http://marc.info/?l=linux-ext4&m=146787897927183&w=2 Your patch was actually quite different. It did implement prjquota option but the option did something else than in my patch... In your patch you implemented prjquota option to match the behavior of original quota in normal files. In my patch I've implemented behavior of prjquota option to behave similarly as in XFS - i.e. enable enforcement of project quotas when project quota feature is enabled. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfstests, generic: add project quota attribute tests 2016-07-18 10:20 ` Jan Kara @ 2016-07-18 12:45 ` Wang Shilong 0 siblings, 0 replies; 18+ messages in thread From: Wang Shilong @ 2016-07-18 12:45 UTC (permalink / raw) To: Jan Kara Cc: Theodore Ts'o, Eric Sandeen, Dave Chinner, fstests, Ext4 Developers List, Shuichi Ihara, Li Xi, Wang Shilong On Mon, Jul 18, 2016 at 6:20 PM, Jan Kara <jack@suse.cz> wrote: > On Fri 15-07-16 10:15:31, Wang Shilong wrote: >> On Thu, Jul 14, 2016 at 10:13 PM, Jan Kara <jack@suse.cz> wrote: >> > On Tue 12-07-16 12:15:47, Ted Tso wrote: >> >> >> Does that sound like a plan? >> > >> > Yes. quotaoff will already disable only enforcement for ext4 with hidden >> > quota files so there's no need to change anything there. We just need to >> > add kernel support for enabling quota enforcement based on mount option. >> > Attached patch should do that - I still need to test whether it doesn't >> > break anything so don't apply it yet, just have a look whether it looks >> > sane to you. >> >> I sent a similar patch weeks ago. >> http://marc.info/?l=linux-ext4&m=146787897927183&w=2 > > Your patch was actually quite different. It did implement prjquota option > but the option did something else than in my patch... In your patch you > implemented prjquota option to match the behavior of original quota in > normal files. In my patch I've implemented behavior of prjquota option to > behave similarly as in XFS - i.e. enable enforcement of project quotas when > project quota feature is enabled. I see, ok, that makes sense for me. Thank you, Shilong > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-07-18 12:45 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-06 6:22 [PATCH v2] xfstests, generic: add project quota attribute tests Wang Shilong 2016-07-06 16:10 ` Eric Sandeen 2016-07-06 23:35 ` Dave Chinner 2016-07-07 2:47 ` Eric Sandeen 2016-07-08 0:51 ` Dave Chinner 2016-07-08 2:46 ` Theodore Ts'o 2016-07-08 3:19 ` Eric Sandeen 2016-07-08 4:57 ` Dave Chinner 2016-07-08 5:02 ` Theodore Ts'o 2016-07-11 16:15 ` Jan Kara 2016-07-11 17:12 ` Theodore Ts'o 2016-07-12 10:59 ` Jan Kara 2016-07-12 14:32 ` Jan Kara 2016-07-12 16:15 ` Theodore Ts'o 2016-07-14 13:13 ` Jan Kara 2016-07-15 1:15 ` Wang Shilong 2016-07-18 10:20 ` Jan Kara 2016-07-18 12:45 ` Wang Shilong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.