quota reservation fixes for -stable?

quota reservation fixes for -stable?

[Eric Sandeen]

I think we should get the following commits to -stable:

0a5a9c725512461d19397490f3adf29931dca1f2 quota: Fix warning when a delayed write happens before quota is enabled
c469070aea5a0ada45a836937c776fd3083dae2b quota: manage reserved space when quota is not active [v2]

without these, we're just racking up the score on kerneloops:

http://www.kerneloops.org/searchweek.php?search=dquot_claim_space

(I think this is a #1 all-time high for a kerneloops count ;)

Pretty sure the 2 commits above can be backported w/o other
dependencies, but maybe Jan & Dmitry can confirm.

I dug into this a little; at least on stock .32 Fedora, it's really
only hitting on a few files, but seems to hit every time they're written:
/var/log/messages, audit.log, etc - things that likely were written
prior to quota being enabled.  Perhaps this is a Fedora initscripts
bug... 

But check this out, with a modified .32 kernel with the above 2 commints,
and a change that reports which inodes were modified:

VFS (sda12): inode(s) 1064 
VFS (sda12): Writes happened before quota was turned on thus quota information is probably inconsistent. Please run quotacheck(8).

# find /etc -inum 1064
/etc/mtab

This sounds a little chicken-and-eggy to me!

Anyway, the above 2 commits should make things -significantly- better,
so w/o any objections, let's get them to -stable.

Thanks,
-Eric

Re: quota reservation fixes for -stable?

[Jan Kara]

On Wed 31-03-10 10:36:12, Eric Sandeen wrote:
> I think we should get the following commits to -stable:
> 
> 0a5a9c725512461d19397490f3adf29931dca1f2 quota: Fix warning when a delayed write happens before quota is enabled
> c469070aea5a0ada45a836937c776fd3083dae2b quota: manage reserved space when quota is not active [v2]
  They are already queued for 2.6.33 and 2.6.32 stable trees. I sent
them to Greg last week or so.

> without these, we're just racking up the score on kerneloops:
> 
> http://www.kerneloops.org/searchweek.php?search=dquot_claim_space
> 
> (I think this is a #1 all-time high for a kerneloops count ;)
> 
> Pretty sure the 2 commits above can be backported w/o other
> dependencies, but maybe Jan & Dmitry can confirm.
> 
> I dug into this a little; at least on stock .32 Fedora, it's really
> only hitting on a few files, but seems to hit every time they're written:
> /var/log/messages, audit.log, etc - things that likely were written
> prior to quota being enabled.  Perhaps this is a Fedora initscripts
> bug... 
> 
> But check this out, with a modified .32 kernel with the above 2 commints,
> and a change that reports which inodes were modified:
> 
> VFS (sda12): inode(s) 1064 
> VFS (sda12): Writes happened before quota was turned on thus quota information is probably inconsistent. Please run quotacheck(8).
> 
> # find /etc -inum 1064
> /etc/mtab
  Ah, I'm testing quotas on non-root fs so I was wondering how come people
hit the warning so often. Now I understand :) For root filesystem on ext4
we don't have a chance to not see this warning. That being said the warning
is pretty useless for ordinary users so I guess we should just hide it
behind #ifdef __DQUOT_PARANOIA and use it for debugging purposes only.
  If noone objects soon, I'll do the change.

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

Re: quota reservation fixes for -stable?

[Eric Sandeen]

Jan Kara wrote:
> On Wed 31-03-10 10:36:12, Eric Sandeen wrote:
>> I think we should get the following commits to -stable:
>>
>> 0a5a9c725512461d19397490f3adf29931dca1f2 quota: Fix warning when a delayed write happens before quota is enabled
>> c469070aea5a0ada45a836937c776fd3083dae2b quota: manage reserved space when quota is not active [v2]
>   They are already queued for 2.6.33 and 2.6.32 stable trees. I sent
> them to Greg last week or so.

Oh, good news - sorry I missed that.

...

>> But check this out, with a modified .32 kernel with the above 2 commints,
>> and a change that reports which inodes were modified:
>>
>> VFS (sda12): inode(s) 1064 
>> VFS (sda12): Writes happened before quota was turned on thus quota information is probably inconsistent. Please run quotacheck(8).
>>
>> # find /etc -inum 1064
>> /etc/mtab
>   Ah, I'm testing quotas on non-root fs so I was wondering how come people
> hit the warning so often. Now I understand :) For root filesystem on ext4
> we don't have a chance to not see this warning. That being said the warning
> is pretty useless for ordinary users so I guess we should just hide it
> behind #ifdef __DQUOT_PARANOIA and use it for debugging purposes only.
>   If noone objects soon, I'll do the change.

Yes, I think it's better that way, otherwise we'll keep getting reports about
how they can't seem to make this warning go away.  :)

Thanks,
-Eric

> 
> 								Honza

Re: quota reservation fixes for -stable?

[Eric Sandeen]

On 04/01/2010 03:23 AM, Jan Kara wrote:

>   Ah, I'm testing quotas on non-root fs so I was wondering how come people
> hit the warning so often. Now I understand :) For root filesystem on ext4
> we don't have a chance to not see this warning. That being said the warning
> is pretty useless for ordinary users so I guess we should just hide it
> behind #ifdef __DQUOT_PARANOIA and use it for debugging purposes only.
>   If noone objects soon, I'll do the change.
> 
> 								Honza

Hm, I see this in your tree now, but isn't __DQUOT_PARANOIA defined by
default anyway?

fs/quota/dquot.c:

1da177e4	(Linus Torvalds	2005-04-16 15:20:36 -0700	83)#define
__DQUOT_PARANOIA

-Eric

Re: quota reservation fixes for -stable?

[Jan Kara]

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

On Fri 16-04-10 15:37:35, Eric Sandeen wrote:
> On 04/01/2010 03:23 AM, Jan Kara wrote:
> 
> >   Ah, I'm testing quotas on non-root fs so I was wondering how come people
> > hit the warning so often. Now I understand :) For root filesystem on ext4
> > we don't have a chance to not see this warning. That being said the warning
> > is pretty useless for ordinary users so I guess we should just hide it
> > behind #ifdef __DQUOT_PARANOIA and use it for debugging purposes only.
> >   If noone objects soon, I'll do the change.
> > 
> > 								Honza
> 
> Hm, I see this in your tree now, but isn't __DQUOT_PARANOIA defined by
> default anyway?
  Ah, good point. Thanks for spotting this. I guess the attached cleanup is
long overdue... Will push it too Linus this week and then to stable if
noone objects.

								Honza

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

[-- Attachment #2: 0001-quota-Convert-__DQUOT_PARANOIA-symbol-to-standard-co.patch --]
[-- Type: text/x-patch, Size: 3502 bytes --]

>From 320da49e9f6a5e52e944ac238a0ed75e204cce6a Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 19 Apr 2010 16:47:20 +0200
Subject: [PATCH] quota: Convert __DQUOT_PARANOIA symbol to standard config option

Make __DQUOT_PARANOIA define from the old days a standard config option
and turn it off by default.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/Kconfig |    8 ++++++++
 fs/quota/dquot.c |   16 +++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/fs/quota/Kconfig b/fs/quota/Kconfig
index dad7fb2..3e21b1e 100644
--- a/fs/quota/Kconfig
+++ b/fs/quota/Kconfig
@@ -33,6 +33,14 @@ config PRINT_QUOTA_WARNING
 	  Note that this behavior is currently deprecated and may go away in
 	  future. Please use notification via netlink socket instead.
 
+config QUOTA_DEBUG
+	bool "Additional quota sanity checks"
+	depends on QUOTA
+	default n
+	help
+	  If you say Y here, quota subsystem will perform some additional
+	  sanity checks of quota internal structures. If unsure, say N.
+
 # Generic support for tree structured quota files. Selected when needed.
 config QUOTA_TREE
 	 tristate
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 60e0e08..05c590e 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -80,8 +80,6 @@
 
 #include <asm/uaccess.h>
 
-#define __DQUOT_PARANOIA
-
 /*
  * There are three quota SMP locks. dq_list_lock protects all lists with quotas
  * and quota formats, dqstats structure containing statistics about the lists
@@ -704,7 +702,7 @@ void dqput(struct dquot *dquot)
 
 	if (!dquot)
 		return;
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 	if (!atomic_read(&dquot->dq_count)) {
 		printk("VFS: dqput: trying to free free dquot\n");
 		printk("VFS: device %s, dquot of %s %d\n",
@@ -757,7 +755,7 @@ we_slept:
 		goto we_slept;
 	}
 	atomic_dec(&dquot->dq_count);
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 	/* sanity check */
 	BUG_ON(!list_empty(&dquot->dq_free));
 #endif
@@ -854,7 +852,7 @@ we_slept:
 		dquot = NULL;
 		goto out;
 	}
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 	BUG_ON(!dquot->dq_sb);	/* Has somebody invalidated entry under us? */
 #endif
 out:
@@ -883,7 +881,7 @@ static int dqinit_needed(struct inode *inode, int type)
 static void add_dquot_ref(struct super_block *sb, int type)
 {
 	struct inode *inode, *old_inode = NULL;
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 	int reserved = 0;
 #endif
 
@@ -891,7 +889,7 @@ static void add_dquot_ref(struct super_block *sb, int type)
 	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
 		if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW))
 			continue;
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 		if (unlikely(inode_get_rsv_space(inode) > 0))
 			reserved = 1;
 #endif
@@ -916,7 +914,7 @@ static void add_dquot_ref(struct super_block *sb, int type)
 	spin_unlock(&inode_lock);
 	iput(old_inode);
 
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 	if (reserved) {
 		printk(KERN_WARNING "VFS (%s): Writes happened before quota"
 			" was turned on thus quota information is probably "
@@ -949,7 +947,7 @@ static int remove_inode_dquot_ref(struct inode *inode, int type,
 	inode->i_dquot[type] = NULL;
 	if (dquot) {
 		if (dqput_blocks(dquot)) {
-#ifdef __DQUOT_PARANOIA
+#ifdef CONFIG_QUOTA_DEBUG
 			if (atomic_read(&dquot->dq_count) != 1)
 				printk(KERN_WARNING "VFS: Adding dquot with dq_count %d to dispose list.\n", atomic_read(&dquot->dq_count));
 #endif
-- 
1.6.4.2

Re: quota reservation fixes for -stable?

[Eric Sandeen]

Jan Kara wrote:
> On Fri 16-04-10 15:37:35, Eric Sandeen wrote:
>> On 04/01/2010 03:23 AM, Jan Kara wrote:
>>
>>>   Ah, I'm testing quotas on non-root fs so I was wondering how come people
>>> hit the warning so often. Now I understand :) For root filesystem on ext4
>>> we don't have a chance to not see this warning. That being said the warning
>>> is pretty useless for ordinary users so I guess we should just hide it
>>> behind #ifdef __DQUOT_PARANOIA and use it for debugging purposes only.
>>>   If noone objects soon, I'll do the change.
>>>
>>> 								Honza
>> Hm, I see this in your tree now, but isn't __DQUOT_PARANOIA defined by
>> default anyway?
>   Ah, good point. Thanks for spotting this. I guess the attached cleanup is
> long overdue... Will push it too Linus this week and then to stable if
> noone objects.
> 
> 								Honza
> 
> 

looks great to me, thanks!

-Eric