[PATCH] quota: add per-inode reservaton space sanity checks.

[PATCH] quota: add per-inode reservaton space sanity checks.

[Dmitry Monakhov]

We already has per-dquot sanity checks, but with per-inode checks
quota leakage investigation becomes much easier.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/quota/dquot.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index e0b870f..4db57b7 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1428,6 +1428,8 @@ EXPORT_SYMBOL(inode_add_rsv_space);
 void inode_claim_rsv_space(struct inode *inode, qsize_t number)
 {
 	spin_lock(&inode->i_lock);
+	if (*inode_reserved_space(inode) < number)
+		WARN_ON_ONCE(1);
 	*inode_reserved_space(inode) -= number;
 	__inode_add_bytes(inode, number);
 	spin_unlock(&inode->i_lock);
@@ -1437,6 +1439,8 @@ EXPORT_SYMBOL(inode_claim_rsv_space);
 void inode_sub_rsv_space(struct inode *inode, qsize_t number)
 {
 	spin_lock(&inode->i_lock);
+	if (*inode_reserved_space(inode) < number)
+		WARN_ON_ONCE(1);
 	*inode_reserved_space(inode) -= number;
 	spin_unlock(&inode->i_lock);
 }
-- 
1.6.6.1

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[Jan Kara]

On Tue 30-03-10 18:25:28, Dmitry Monakhov wrote:
> We already has per-dquot sanity checks, but with per-inode checks
> quota leakage investigation becomes much easier.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/quota/dquot.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index e0b870f..4db57b7 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1428,6 +1428,8 @@ EXPORT_SYMBOL(inode_add_rsv_space);
>  void inode_claim_rsv_space(struct inode *inode, qsize_t number)
>  {
>  	spin_lock(&inode->i_lock);
> +	if (*inode_reserved_space(inode) < number)
> +		WARN_ON_ONCE(1);
  Maybe just: WARN_ON_ONCE(*inode_reserved_space(inode) < number)

>  	*inode_reserved_space(inode) -= number;
>  	__inode_add_bytes(inode, number);
>  	spin_unlock(&inode->i_lock);
> @@ -1437,6 +1439,8 @@ EXPORT_SYMBOL(inode_claim_rsv_space);
>  void inode_sub_rsv_space(struct inode *inode, qsize_t number)
>  {
>  	spin_lock(&inode->i_lock);
> +	if (*inode_reserved_space(inode) < number)
> +		WARN_ON_ONCE(1);
  And here as well...

>  	*inode_reserved_space(inode) -= number;
>  	spin_unlock(&inode->i_lock);
>  }
  But otherwise I agree...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[dmonakhov]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

Jan Kara <jack@suse.cz> writes:

> On Tue 30-03-10 18:25:28, Dmitry Monakhov wrote:
>> We already has per-dquot sanity checks, but with per-inode checks
>> quota leakage investigation becomes much easier.
>> 
>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>> ---
>>  fs/quota/dquot.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>> 
>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
>> index e0b870f..4db57b7 100644
>> --- a/fs/quota/dquot.c
>> +++ b/fs/quota/dquot.c
>> @@ -1428,6 +1428,8 @@ EXPORT_SYMBOL(inode_add_rsv_space);
>>  void inode_claim_rsv_space(struct inode *inode, qsize_t number)
>>  {
>>  	spin_lock(&inode->i_lock);
>> +	if (*inode_reserved_space(inode) < number)
>> +		WARN_ON_ONCE(1);
>   Maybe just: WARN_ON_ONCE(*inode_reserved_space(inode) < number)
As you wish.

[-- Attachment #2: 0001-quota-add-per-inode-reservaton-space-sanity-checks.patch --]
[-- Type: text/plain, Size: 1222 bytes --]

>From 610afddec4ae4e33d2481284d2c3463439284833 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Tue, 30 Mar 2010 20:08:12 +0400
Subject: [PATCH] quota: add per-inode reservaton space sanity checks.

We already has per-dquot sanity checks, but with per-inode checks
quota leakage investigation becomes much easier.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/quota/dquot.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index e0b870f..f1c50e4 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1428,6 +1428,7 @@ EXPORT_SYMBOL(inode_add_rsv_space);
 void inode_claim_rsv_space(struct inode *inode, qsize_t number)
 {
 	spin_lock(&inode->i_lock);
+	WARN_ON_ONCE(*inode_reserved_space(inode) < number);
 	*inode_reserved_space(inode) -= number;
 	__inode_add_bytes(inode, number);
 	spin_unlock(&inode->i_lock);
@@ -1437,6 +1438,7 @@ EXPORT_SYMBOL(inode_claim_rsv_space);
 void inode_sub_rsv_space(struct inode *inode, qsize_t number)
 {
 	spin_lock(&inode->i_lock);
+	WARN_ON_ONCE(*inode_reserved_space(inode) < number);
 	*inode_reserved_space(inode) -= number;
 	spin_unlock(&inode->i_lock);
 }
-- 
1.6.6.1

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

dmonakhov@openvz.org writes:

> Jan Kara <jack@suse.cz> writes:
>
>> On Tue 30-03-10 18:25:28, Dmitry Monakhov wrote:
>>> We already has per-dquot sanity checks, but with per-inode checks
>>> quota leakage investigation becomes much easier.
>>> 
>>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>>> ---
>>>  fs/quota/dquot.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
>>> index e0b870f..4db57b7 100644
>>> --- a/fs/quota/dquot.c
>>> +++ b/fs/quota/dquot.c
>>> @@ -1428,6 +1428,8 @@ EXPORT_SYMBOL(inode_add_rsv_space);
>>>  void inode_claim_rsv_space(struct inode *inode, qsize_t number)
>>>  {
>>>  	spin_lock(&inode->i_lock);
>>> +	if (*inode_reserved_space(inode) < number)
>>> +		WARN_ON_ONCE(1);
>>   Maybe just: WARN_ON_ONCE(*inode_reserved_space(inode) < number)
Ohh. While writing testcase for automatic-quota consistency check.
I've failed to reliably detect condition where quota goes inconsistent.
And it appears to be not what easy. We have hided real problems too deeply.
The only reliable way is to grep dmesg for hard-coded "fs/quota/dquot.c"
string which produced by WARN_ON_ONCE.

After some thoughts (sometimes it happens with me too)
i'm think what it is reasonable rewrite all quota error messages logic.
Make it similar to ext4
quota_error(...) {
   printk("QUOTA error (dev:%s)", ...);
   handle_quota_error(sb) /* let sb to know what quota is corrupted */
}
quota_warn(...) {
   printk("QUOTA warn (dev:%s)", ...);
}
In fact it is very important to let filesystem to now that it's quota
is corrupted(quota == fs-metadata). It may set fsck_needed flag on
super block for later correction.
So please ignore this patch for now. I'll prepare another one which
redesign error conditions handling logic.

BTW: I've attached my testcase. I hope it will be useful for you.
It able to catch quota inconsistency caused by incorrect symlink
handling, but it is not reliable for writepage/fallocate bug in ext4.


[-- Attachment #2: 0001-xfstests-add-quota-stress-test.patch --]
[-- Type: text/plain, Size: 5857 bytes --]

>From fa70a77403f21a871decc0c61d665a82ae492f7c Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Tue, 30 Mar 2010 20:59:20 +0400
Subject: [PATCH] xfstests: add quota stress test

Run fsstress on filesystem with quota enabled, then recheckquota
and comare with old value.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 500      |  179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 500.full |    4 ++
 500.out  |    5 ++
 group    |    1 +
 4 files changed, 189 insertions(+), 0 deletions(-)
 create mode 100755 500
 create mode 100644 500.full
 create mode 100644 500.out

diff --git a/500 b/500
new file mode 100755
index 0000000..47999d6
--- /dev/null
+++ b/500
@@ -0,0 +1,179 @@
+#! /bin/bash
+# FS QA Test No. 500
+#
+# Quota accounting stress test
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2010 Dmitry Monakhov.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
+owner=dmonakhov@openvz.org
+
+seq=`basename $0`
+echo "QA output created by $seq"
+killall="/usr/bin/killall"
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+quota_supported=0
+
+_cleanup()
+{
+    rm -f $tmp.*
+}
+
+require_quota()
+{
+    case $FSTYP in
+	ext2|reiserfs|jfs)
+	    export MOUNT_OPTIONS="$MOUNT_OPTIONS -o usrquota,grpquota"
+	    quota_supported=1
+	    ;;
+	ext3|ext4)
+	    export MOUNT_OPTIONS="$MOUNT_OPTIONS -o jqfmt=vfsv0,usrjquota=aquota.user,grpjquota=aquota.group"
+	    quota_supported=1
+	    ;;
+	*)
+	    quota_supported=0
+	    ;;
+	esac
+}
+
+setup_quota()
+{
+    mountpoint=$1
+    case $FSTYP in
+	xfs)
+	    ;;
+	ext*|reiserfs|jfs)
+	    quotaoff $mountpoint &>/dev/null
+	    quotacheck -c -u -g $mountpoint
+	    quotaon $mountpoint
+	    ;;
+	*)
+	    _fail "Don't know how to turn on quota on $FSTYP"
+	    ;;
+    esac
+}
+
+check_quota()
+{
+    mountpoint=$1
+    case $FSTYP in
+	ext*|jfs|reiserfs)
+	    NAME=`mktemp -d $tmp_dir/XXXX`
+	    mkdir -p $NAME/orig $NAME/chk
+	    sync;
+	    repquota -n -u $mountpoint | sort > ${NAME}/orig/user
+	    repquota -n -g $mountpoint | sort > ${NAME}/orig/group
+
+	    # All writers was killed and fs was synced.
+	    # Now quota may be safely disabled for quota recalculation
+	    quotaoff $mountpoint
+	    quotacheck -c -u -g $mountpoint
+	    quotaon $mountpoint
+
+	    repquota -n -u $mountpoint | sort > ${NAME}/chk/user
+	    repquota -n -g $mountpoint | sort > ${NAME}/chk/group
+
+	    #remove this 
+	    cp -r ${NAME} /tmp/DIFF
+	    echo "Quota check " >> $seq.full
+	    diff -upr ${NAME}/{orig,chk} >> $seq.full
+	    local quotacheck_status=$?
+
+	    if [ $quotacheck_status -ne 0 ]; then
+		_fail " Quota check failed, see quota diff."
+	    fi
+	    ;;
+	*)
+	    _fail "Don't know how to check quota on $FSTYP"
+	    ;;
+    esac
+}
+
+workout()
+{
+    # Disable bash job controll, to prevent message about killed task.
+    set +m
+
+    #Timing parameters
+    nr_sec=15
+    kill_tries=10
+    echo Running fsstress. | tee -a $seq.full
+
+####################################################
+##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
+##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
+######################################################
+    $FSSTRESS_PROG \
+	-d $SCRATCH_MNT/fsstress \
+	-p 100 -n 9999999 -s $seed > /dev/null 2>&1 &
+    sleep $((nr_sec - kill_tries))
+
+    for ((i = 0; i < $kill_tries; i++))
+    do
+	killall -r -q -TERM fsstress 2> /dev/null
+	sleep 1
+    done
+
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This following sequance seed it is takes 5 sec to reproduce quota
+# inconsistency bug in ext4
+seed=1270493518
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+require_quota
+[ -x $killall ] || _notrun "$killall executable not found"
+if [ $quota_supported -ne 1 ] ;then 
+    _notrun "Don't know how to turn on quota on $FSTYP"
+fi
+
+rm -f $seq.full
+
+umount $TEST_DEV >/dev/null 2>&1
+umount $SCRATCH_DEV >/dev/null 2>&1
+echo "*** MKFS ***"                         >>$seq.full
+echo ""                                     >>$seq.full
+_scratch_mkfs    >/dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount   >/dev/null 2>&1 || _fail "mount failed"
+setup_quota $SCRATCH_MNT
+
+mkdir -p $SCRATCH_MNT/fsstress
+workout
+
+echo Checking quota
+check_quota $SCRATCH_MNT
+umount $SCRATCH_MNT
+
+echo 
+echo Checking filesystem
+_check_scratch_fs
+_scratch_mount
+status=$?
+exit
diff --git a/500.full b/500.full
new file mode 100644
index 0000000..8f41b34
--- /dev/null
+++ b/500.full
@@ -0,0 +1,4 @@
+*** MKFS ***
+
+Running fsstress.
+Quota check 
diff --git a/500.out b/500.out
new file mode 100644
index 0000000..b4614fc
--- /dev/null
+++ b/500.out
@@ -0,0 +1,5 @@
+QA output created by 500
+Running fsstress.
+Checking quota
+
+Checking filesystem
diff --git a/group b/group
index 8d4a83a..3164c37 100644
--- a/group
+++ b/group
@@ -339,3 +339,4 @@ deprecated
 223 auto quick
 224 auto
 225 auto quick
+500 auto quota
\ No newline at end of file
-- 
1.6.6

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[Dave Chinner]

On Wed, Mar 31, 2010 at 09:20:17AM +0400, Dmitry Monakhov wrote:
> BTW: I've attached my testcase. I hope it will be useful for you.
> It able to catch quota inconsistency caused by incorrect symlink
> handling, but it is not reliable for writepage/fallocate bug in ext4.
> 

> From fa70a77403f21a871decc0c61d665a82ae492f7c Mon Sep 17 00:00:00 2001
> From: Dmitry Monakhov <dmonakhov@openvz.org>
> Date: Tue, 30 Mar 2010 20:59:20 +0400
> Subject: [PATCH] xfstests: add quota stress test
> 
> Run fsstress on filesystem with quota enabled, then recheckquota
> and comare with old value.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  500      |  179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  500.full |    4 ++
>  500.out  |    5 ++
>  group    |    1 +
>  4 files changed, 189 insertions(+), 0 deletions(-)
>  create mode 100755 500
>  create mode 100644 500.full
>  create mode 100644 500.out
> 
> diff --git a/500 b/500
> new file mode 100755
> index 0000000..47999d6
> --- /dev/null
> +++ b/500
> @@ -0,0 +1,179 @@
> +#! /bin/bash
> +# FS QA Test No. 500
> +#
> +# Quota accounting stress test
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2010 Dmitry Monakhov.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#
> +#-----------------------------------------------------------------------
> +#
> +# creator
> +owner=dmonakhov@openvz.org
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +killall="/usr/bin/killall"
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +quota_supported=0
> +
> +_cleanup()
> +{
> +    rm -f $tmp.*
> +}
> +
> +require_quota()
> +{
> +    case $FSTYP in
> +	ext2|reiserfs|jfs)
> +	    export MOUNT_OPTIONS="$MOUNT_OPTIONS -o usrquota,grpquota"

XFS uses this format, too.

> +	    quota_supported=1
> +	    ;;
> +	ext3|ext4)
> +	    export MOUNT_OPTIONS="$MOUNT_OPTIONS -o jqfmt=vfsv0,usrjquota=aquota.user,grpjquota=aquota.group"
> +	    quota_supported=1
> +	    ;;
> +	*)
> +	    quota_supported=0
> +	    ;;
> +	esac
> +}

There's already a "_require_quota()" function in common.quota that
checks if the filesystem being tested supports quotas and that the
quota tools are installed. Can you add these checks to that
function?

_require_quota also calls _notrun directly, so no need for the
quota_supported variable, either.

Also, can you use 8 space tabs for indenting?

> +setup_quota()
> +{
> +    mountpoint=$1

This is only ever called for $SCRATCH_MNT, so just hardcoding it
is fine. It makes it щomewhat easier to add XFS support - a
recalculation requires turning quotas off, then unmounting and
remounting with quotas enabled to trigger a recalculation.

> +    case $FSTYP in
> +	xfs)
		quotaoff $SCRATCH_MNT &>/dev/null
		umount $SCRATCH_MNT
		_scratch_mount
> +	    ;;
> +	ext*|reiserfs|jfs)
> +	    quotaoff $mountpoint &>/dev/null
> +	    quotacheck -c -u -g $mountpoint
> +	    quotaon $mountpoint
> +	    ;;
> +	*)
> +	    _fail "Don't know how to turn on quota on $FSTYP"
> +	    ;;
> +    esac
> +}

FWIW, test 219 does almost exactly the same as this setup_quota
function, so maybe this shoul dbe made common...


> +
> +check_quota()
> +{
> +    mountpoint=$1
> +    case $FSTYP in
> +	ext*|jfs|reiserfs)
> +	    NAME=`mktemp -d $tmp_dir/XXXX`
> +	    mkdir -p $NAME/orig $NAME/chk
> +	    sync;
> +	    repquota -n -u $mountpoint | sort > ${NAME}/orig/user
> +	    repquota -n -g $mountpoint | sort > ${NAME}/orig/group
> +
> +	    # All writers was killed and fs was synced.
> +	    # Now quota may be safely disabled for quota recalculation
> +	    quotaoff $mountpoint
> +	    quotacheck -c -u -g $mountpoint
> +	    quotaon $mountpoint

And that is a call to setup_quota(), which then means it could
easily be extended to support XFS as well. With the above change for
XFS, it runs and passes this test....

> +
> +	    repquota -n -u $mountpoint | sort > ${NAME}/chk/user
> +	    repquota -n -g $mountpoint | sort > ${NAME}/chk/group
> +
> +	    #remove this 
> +	    cp -r ${NAME} /tmp/DIFF
> +	    echo "Quota check " >> $seq.full
> +	    diff -upr ${NAME}/{orig,chk} >> $seq.full
> +	    local quotacheck_status=$?
> +
> +	    if [ $quotacheck_status -ne 0 ]; then
> +		_fail " Quota check failed, see quota diff."

"in $seq.full"

Also, do you need a local variable just to check the exit status?

> +	    fi
> +	    ;;
> +	*)
> +	    _fail "Don't know how to check quota on $FSTYP"
> +	    ;;
> +    esac
> +}
> +
> +workout()
> +{
> +    # Disable bash job controll, to prevent message about killed task.
> +    set +m
> +
> +    #Timing parameters
> +    nr_sec=15
> +    kill_tries=10
> +    echo Running fsstress. | tee -a $seq.full
> +
> +####################################################
> +##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
> +##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
> +######################################################

You can probably just kill those.

> +    $FSSTRESS_PROG \
> +	-d $SCRATCH_MNT/fsstress \
> +	-p 100 -n 9999999 -s $seed > /dev/null 2>&1 &
> +    sleep $((nr_sec - kill_tries))
> +
> +    for ((i = 0; i < $kill_tries; i++))
> +    do
> +	killall -r -q -TERM fsstress 2> /dev/null
> +	sleep 1
> +    done
> +
> +}
> +
> +trap "_cleanup ; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +# This following sequance seed it is takes 5 sec to reproduce quota
> +# inconsistency bug in ext4
> +seed=1270493518
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +require_quota
> +[ -x $killall ] || _notrun "$killall executable not found"
> +if [ $quota_supported -ne 1 ] ;then 
> +    _notrun "Don't know how to turn on quota on $FSTYP"
> +fi
> +
> +rm -f $seq.full
> +
> +umount $TEST_DEV >/dev/null 2>&1
> +umount $SCRATCH_DEV >/dev/null 2>&1
> +echo "*** MKFS ***"                         >>$seq.full
> +echo ""                                     >>$seq.full
> +_scratch_mkfs    >/dev/null 2>&1 || _fail "mkfs failed"
> +_scratch_mount   >/dev/null 2>&1 || _fail "mount failed"
> +setup_quota $SCRATCH_MNT
> +
> +mkdir -p $SCRATCH_MNT/fsstress
> +workout
> +
> +echo Checking quota
> +check_quota $SCRATCH_MNT
> +umount $SCRATCH_MNT
> +
> +echo 
> +echo Checking filesystem
> +_check_scratch_fs
> +_scratch_mount
> +status=$?
> +exit
> diff --git a/500.full b/500.full
> new file mode 100644
> index 0000000..8f41b34
> --- /dev/null
> +++ b/500.full
> @@ -0,0 +1,4 @@
> +*** MKFS ***
> +
> +Running fsstress.
> +Quota check 
> diff --git a/500.out b/500.out
> new file mode 100644
> index 0000000..b4614fc
> --- /dev/null
> +++ b/500.out
> @@ -0,0 +1,5 @@
> +QA output created by 500
> +Running fsstress.
> +Checking quota
> +
> +Checking filesystem
> diff --git a/group b/group
> index 8d4a83a..3164c37 100644
> --- a/group
> +++ b/group
> @@ -339,3 +339,4 @@ deprecated
>  223 auto quick
>  224 auto
>  225 auto quick
> +500 auto quota

It's a quick test, too, so you may as well add it to that group.

Also, can you redo this as test 226 so it can be pushed into the
testsuite?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[dmonakhov]

Dave Chinner <david@fromorbit.com> writes:

> On Wed, Mar 31, 2010 at 09:20:17AM +0400, Dmitry Monakhov wrote:
>> BTW: I've attached my testcase. I hope it will be useful for you.
>> It able to catch quota inconsistency caused by incorrect symlink
>> handling, but it is not reliable for writepage/fallocate bug in ext4.
>> 
>
>> From fa70a77403f21a871decc0c61d665a82ae492f7c Mon Sep 17 00:00:00 2001
>> From: Dmitry Monakhov <dmonakhov@openvz.org>
>> Date: Tue, 30 Mar 2010 20:59:20 +0400
>> Subject: [PATCH] xfstests: add quota stress test
>> 
>> Run fsstress on filesystem with quota enabled, then recheckquota
>> and comare with old value.
>> 
>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>> ---
>>  500      |  179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  500.full |    4 ++
>>  500.out  |    5 ++
>>  group    |    1 +
>>  4 files changed, 189 insertions(+), 0 deletions(-)
>>  create mode 100755 500
>>  create mode 100644 500.full
>>  create mode 100644 500.out
>> 
>> diff --git a/500 b/500
>> new file mode 100755
>> index 0000000..47999d6
>> --- /dev/null
>> +++ b/500
>> @@ -0,0 +1,179 @@
>> +#! /bin/bash
>> +# FS QA Test No. 500
>> +#
>> +# Quota accounting stress test
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2010 Dmitry Monakhov.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#
>> +#-----------------------------------------------------------------------
>> +#
>> +# creator
>> +owner=dmonakhov@openvz.org
>> +
>> +seq=`basename $0`
>> +echo "QA output created by $seq"
>> +killall="/usr/bin/killall"
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +quota_supported=0
>> +
>> +_cleanup()
>> +{
>> +    rm -f $tmp.*
>> +}
>> +
>> +require_quota()
>> +{
>> +    case $FSTYP in
>> +	ext2|reiserfs|jfs)
>> +	    export MOUNT_OPTIONS="$MOUNT_OPTIONS -o usrquota,grpquota"
>
> XFS uses this format, too.
>
>> +	    quota_supported=1
>> +	    ;;
>> +	ext3|ext4)
>> +	    export MOUNT_OPTIONS="$MOUNT_OPTIONS -o jqfmt=vfsv0,usrjquota=aquota.user,grpjquota=aquota.group"
>> +	    quota_supported=1
>> +	    ;;
>> +	*)
>> +	    quota_supported=0
>> +	    ;;
>> +	esac
>> +}
>
> There's already a "_require_quota()" function in common.quota that
Yep. overlooked this one.
> checks if the filesystem being tested supports quotas and that the
> quota tools are installed. Can you add these checks to that
> function?
>
> _require_quota also calls _notrun directly, so no need for the
> quota_supported variable, either.
>
> Also, can you use 8 space tabs for indenting?
Ok, will redo accruing to all your comments. To make the testcase more
useful i want to perform grep dmesg. But currently  this technique
is not used in xfs-testcase. How can i do it in a convenient way?
>
>> +setup_quota()
>> +{
>> +    mountpoint=$1
>
> This is only ever called for $SCRATCH_MNT, so just hardcoding it
> is fine. It makes it щomewhat easier to add XFS support - a
> recalculation requires turning quotas off, then unmounting and
> remounting with quotas enabled to trigger a recalculation.
>
>> +    case $FSTYP in
>> +	xfs)
> 		quotaoff $SCRATCH_MNT &>/dev/null
> 		umount $SCRATCH_MNT
> 		_scratch_mount
>> +	    ;;
>> +	ext*|reiserfs|jfs)
>> +	    quotaoff $mountpoint &>/dev/null
>> +	    quotacheck -c -u -g $mountpoint
>> +	    quotaon $mountpoint
>> +	    ;;
>> +	*)
>> +	    _fail "Don't know how to turn on quota on $FSTYP"
>> +	    ;;
>> +    esac
>> +}
>
> FWIW, test 219 does almost exactly the same as this setup_quota
> function, so maybe this shoul dbe made common...
>
>
>> +
>> +check_quota()
>> +{
>> +    mountpoint=$1
>> +    case $FSTYP in
>> +	ext*|jfs|reiserfs)
>> +	    NAME=`mktemp -d $tmp_dir/XXXX`
>> +	    mkdir -p $NAME/orig $NAME/chk
>> +	    sync;
>> +	    repquota -n -u $mountpoint | sort > ${NAME}/orig/user
>> +	    repquota -n -g $mountpoint | sort > ${NAME}/orig/group
>> +
>> +	    # All writers was killed and fs was synced.
>> +	    # Now quota may be safely disabled for quota recalculation
>> +	    quotaoff $mountpoint
>> +	    quotacheck -c -u -g $mountpoint
>> +	    quotaon $mountpoint
>
> And that is a call to setup_quota(), which then means it could
> easily be extended to support XFS as well. With the above change for
> XFS, it runs and passes this test....
>
>> +
>> +	    repquota -n -u $mountpoint | sort > ${NAME}/chk/user
>> +	    repquota -n -g $mountpoint | sort > ${NAME}/chk/group
>> +
>> +	    #remove this 
>> +	    cp -r ${NAME} /tmp/DIFF
>> +	    echo "Quota check " >> $seq.full
>> +	    diff -upr ${NAME}/{orig,chk} >> $seq.full
>> +	    local quotacheck_status=$?
>> +
>> +	    if [ $quotacheck_status -ne 0 ]; then
>> +		_fail " Quota check failed, see quota diff."
>
> "in $seq.full"
>
> Also, do you need a local variable just to check the exit status?
>
>> +	    fi
>> +	    ;;
>> +	*)
>> +	    _fail "Don't know how to check quota on $FSTYP"
>> +	    ;;
>> +    esac
>> +}
>> +
>> +workout()
>> +{
>> +    # Disable bash job controll, to prevent message about killed task.
>> +    set +m
>> +
>> +    #Timing parameters
>> +    nr_sec=15
>> +    kill_tries=10
>> +    echo Running fsstress. | tee -a $seq.full
>> +
>> +####################################################
>> +##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
>> +##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
>> +######################################################
>
> You can probably just kill those.
>
>> +    $FSSTRESS_PROG \
>> +	-d $SCRATCH_MNT/fsstress \
>> +	-p 100 -n 9999999 -s $seed > /dev/null 2>&1 &
>> +    sleep $((nr_sec - kill_tries))
>> +
>> +    for ((i = 0; i < $kill_tries; i++))
>> +    do
>> +	killall -r -q -TERM fsstress 2> /dev/null
>> +	sleep 1
>> +    done
>> +
>> +}
>> +
>> +trap "_cleanup ; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +
>> +# This following sequance seed it is takes 5 sec to reproduce quota
>> +# inconsistency bug in ext4
>> +seed=1270493518
>> +
>> +# real QA test starts here
>> +_supported_fs generic
>> +_supported_os Linux
>> +_require_scratch
>> +require_quota
>> +[ -x $killall ] || _notrun "$killall executable not found"
>> +if [ $quota_supported -ne 1 ] ;then 
>> +    _notrun "Don't know how to turn on quota on $FSTYP"
>> +fi
>> +
>> +rm -f $seq.full
>> +
>> +umount $TEST_DEV >/dev/null 2>&1
>> +umount $SCRATCH_DEV >/dev/null 2>&1
>> +echo "*** MKFS ***"                         >>$seq.full
>> +echo ""                                     >>$seq.full
>> +_scratch_mkfs    >/dev/null 2>&1 || _fail "mkfs failed"
>> +_scratch_mount   >/dev/null 2>&1 || _fail "mount failed"
>> +setup_quota $SCRATCH_MNT
>> +
>> +mkdir -p $SCRATCH_MNT/fsstress
>> +workout
>> +
>> +echo Checking quota
>> +check_quota $SCRATCH_MNT
>> +umount $SCRATCH_MNT
>> +
>> +echo 
>> +echo Checking filesystem
>> +_check_scratch_fs
>> +_scratch_mount
>> +status=$?
>> +exit
>> diff --git a/500.full b/500.full
>> new file mode 100644
>> index 0000000..8f41b34
>> --- /dev/null
>> +++ b/500.full
>> @@ -0,0 +1,4 @@
>> +*** MKFS ***
>> +
>> +Running fsstress.
>> +Quota check 
>> diff --git a/500.out b/500.out
>> new file mode 100644
>> index 0000000..b4614fc
>> --- /dev/null
>> +++ b/500.out
>> @@ -0,0 +1,5 @@
>> +QA output created by 500
>> +Running fsstress.
>> +Checking quota
>> +
>> +Checking filesystem
>> diff --git a/group b/group
>> index 8d4a83a..3164c37 100644
>> --- a/group
>> +++ b/group
>> @@ -339,3 +339,4 @@ deprecated
>>  223 auto quick
>>  224 auto
>>  225 auto quick
>> +500 auto quota
>
> It's a quick test, too, so you may as well add it to that group.
>
> Also, can you redo this as test 226 so it can be pushed into the
> testsuite?
>
> Cheers,
>
> Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[Jan Kara]

On Wed 31-03-10 09:20:17, Dmitry Monakhov wrote:
> dmonakhov@openvz.org writes:
> 
> > Jan Kara <jack@suse.cz> writes:
> >
> >> On Tue 30-03-10 18:25:28, Dmitry Monakhov wrote:
> >>> We already has per-dquot sanity checks, but with per-inode checks
> >>> quota leakage investigation becomes much easier.
> >>> 
> >>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> >>> ---
> >>>  fs/quota/dquot.c |    4 ++++
> >>>  1 files changed, 4 insertions(+), 0 deletions(-)
> >>> 
> >>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> >>> index e0b870f..4db57b7 100644
> >>> --- a/fs/quota/dquot.c
> >>> +++ b/fs/quota/dquot.c
> >>> @@ -1428,6 +1428,8 @@ EXPORT_SYMBOL(inode_add_rsv_space);
> >>>  void inode_claim_rsv_space(struct inode *inode, qsize_t number)
> >>>  {
> >>>  	spin_lock(&inode->i_lock);
> >>> +	if (*inode_reserved_space(inode) < number)
> >>> +		WARN_ON_ONCE(1);
> >>   Maybe just: WARN_ON_ONCE(*inode_reserved_space(inode) < number)
> Ohh. While writing testcase for automatic-quota consistency check.
> I've failed to reliably detect condition where quota goes inconsistent.
> And it appears to be not what easy. We have hided real problems too deeply.
> The only reliable way is to grep dmesg for hard-coded "fs/quota/dquot.c"
> string which produced by WARN_ON_ONCE.
> 
> After some thoughts (sometimes it happens with me too)
> i'm think what it is reasonable rewrite all quota error messages logic.
> Make it similar to ext4
> quota_error(...) {
>    printk("QUOTA error (dev:%s)", ...);
>    handle_quota_error(sb) /* let sb to know what quota is corrupted */
> }
> quota_warn(...) {
>    printk("QUOTA warn (dev:%s)", ...);
> }
> In fact it is very important to let filesystem to now that it's quota
> is corrupted(quota == fs-metadata). It may set fsck_needed flag on
> super block for later correction.
  Yes, it looks like a good idea.

> So please ignore this patch for now. I'll prepare another one which
> redesign error conditions handling logic.
  OK.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: add per-inode reservaton space sanity checks.

[Dave Chinner]

On Wed, Mar 31, 2010 at 11:17:39AM +0400, dmonakhov@openvz.org wrote:
> Dave Chinner <david@fromorbit.com> writes:
> > On Wed, Mar 31, 2010 at 09:20:17AM +0400, Dmitry Monakhov wrote:
> >> BTW: I've attached my testcase. I hope it will be useful for you.
> >> It able to catch quota inconsistency caused by incorrect symlink
> >> handling, but it is not reliable for writepage/fallocate bug in ext4.
....
> > There's already a "_require_quota()" function in common.quota that
> Yep. overlooked this one.
> > checks if the filesystem being tested supports quotas and that the
> > quota tools are installed. Can you add these checks to that
> > function?
> >
> > _require_quota also calls _notrun directly, so no need for the
> > quota_supported variable, either.
> >
> > Also, can you use 8 space tabs for indenting?
> Ok, will redo accruing to all your comments. To make the testcase more
> useful i want to perform grep dmesg. But currently  this technique
> is not used in xfs-testcase.

It hasn't been used because in the past kernel output is has not been
needed to report a test success or fail. If the test fails, and
there's pertinent infomration in the kernel log, then normally the
developer grabs that him/herself after the failure.

> How can i do it in a convenient way?

What information do you want to grab from the kernel log?  If you
make the test linux platform specific (IIRC you already have), then
you could probably just run dmesg and sed/awk/grep/perl the output
to get what you want.  Lots of tests take output from something and
then filter it down like this to get the required, anonymised
information to match against the golden output...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com