missing inode_add_bytes in dquot_alloc_space

missing inode_add_bytes in dquot_alloc_space

[Dave Kleikamp]

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

Jan,
I don't think it was intentional, but in dquot_alloc_space, if
IS_NOQUOTA(inode) is true, then i_blocks doesn't get updated.  This
patch would fix it to do what I believe it should.  Is this right, or am
I missing something?
-- 
David Kleikamp
IBM Linux Technology Center

[-- Attachment #2: dquot.patch --]
[-- Type: text/plain, Size: 333 bytes --]

===== fs/dquot.c 1.70 vs edited =====
--- 1.70/fs/dquot.c	Mon Dec 29 15:37:59 2003
+++ edited/fs/dquot.c	Wed Jan 14 09:56:58 2004
@@ -890,6 +890,7 @@
 
 	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
 	if (IS_NOQUOTA(inode)) {
+		inode_add_bytes(inode, number);
 		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
 		return QUOTA_OK;
 	}

Re: missing inode_add_bytes in dquot_alloc_space

[Jan Kara]

  Hi,

> I don't think it was intentional, but in dquot_alloc_space, if
> IS_NOQUOTA(inode) is true, then i_blocks doesn't get updated.  This
> patch would fix it to do what I believe it should.  Is this right, or am
> I missing something?
  Good spotting. Thanks. Actually the same problem is at other functions
(dquot_free_space(), etc.). I'll fix it completely in the afternoon.

						Thanks for spotting
								Honza

Re: missing inode_add_bytes in dquot_alloc_space

[Jan Kara]

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

  Hi,

> I don't think it was intentional, but in dquot_alloc_space, if
> IS_NOQUOTA(inode) is true, then i_blocks doesn't get updated.  This
> patch would fix it to do what I believe it should.  Is this right, or am
> I missing something?
  Here's the patch which should fix all the functions which had
problems. Patch applies against 2.6.1 well too. I'll also send it so
Linus..

								Honza

[-- Attachment #2: quota-2.6.0-1-noqfix.diff --]
[-- Type: text/plain, Size: 1739 bytes --]

diff -ruX ../kerndiffexclude linux-2.6.0-test11-um/fs/dquot.c linux-2.6.0-test11-um-1-noqfix/fs/dquot.c
--- linux-2.6.0/fs/dquot.c	Mon Dec  1 12:31:28 2003
+++ linux-2.6.0-1-noqfix/fs/dquot.c	Fri Jan 16 10:10:25 2004
@@ -884,11 +884,9 @@
 		warntype[cnt] = NOWARN;
 
 	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
-	if (IS_NOQUOTA(inode)) {
-		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
-		return QUOTA_OK;
-	}
 	spin_lock(&dq_data_lock);
+	if (IS_NOQUOTA(inode))
+		goto add_bytes;
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		if (inode->i_dquot[cnt] == NODQUOT)
 			continue;
@@ -900,6 +898,7 @@
 			continue;
 		dquot_incr_space(inode->i_dquot[cnt], number);
 	}
+add_bytes:
 	inode_add_bytes(inode, number);
 	ret = QUOTA_OK;
 warn_put_all:
@@ -953,16 +952,15 @@
 	unsigned int cnt;
 
 	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
-	if (IS_NOQUOTA(inode)) {
-		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
-		return;
-	}
 	spin_lock(&dq_data_lock);
+	if (IS_NOQUOTA(inode))
+		goto sub_bytes;
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		if (inode->i_dquot[cnt] == NODQUOT)
 			continue;
 		dquot_decr_space(inode->i_dquot[cnt], number);
 	}
+sub_bytes:
 	inode_sub_bytes(inode, number);
 	spin_unlock(&dq_data_lock);
 	up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
@@ -1010,8 +1008,10 @@
 		warntype[cnt] = NOWARN;
 	}
 	down_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
-	if (IS_NOQUOTA(inode))	/* File without quota accounting? */
-		goto warn_put_all;
+	if (IS_NOQUOTA(inode)) {	/* File without quota accounting? */
+		up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
+		return QUOTA_OK;
+	}
 	/* First build the transfer_to list - here we can block on reading of dquots... */
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		switch (cnt) {

Re: missing inode_add_bytes in dquot_alloc_space

[Christoph Hellwig]

On Fri, Jan 16, 2004 at 12:20:28PM +0100, Jan Kara wrote:
>  	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> -	if (IS_NOQUOTA(inode)) {
> -		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> -		return QUOTA_OK;
> -	}
>  	spin_lock(&dq_data_lock);
> +	if (IS_NOQUOTA(inode))
> +		goto add_bytes;

This means we take a global lock now even for non-quota operations?
Doesn't sound like a really good idea..

Re: missing inode_add_bytes in dquot_alloc_space

[Jan Kara]

> On Fri, Jan 16, 2004 at 12:20:28PM +0100, Jan Kara wrote:
> >  	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> > -	if (IS_NOQUOTA(inode)) {
> > -		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> > -		return QUOTA_OK;
> > -	}
> >  	spin_lock(&dq_data_lock);
> > +	if (IS_NOQUOTA(inode))
> > +		goto add_bytes;
> 
> This means we take a global lock now even for non-quota operations?
> Doesn't sound like a really good idea..
  This is only the case of quota files themselves - other cases (like
when quota is not compiled/not turned on) are solved by macros in
quotaops.h used by filesystems.

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

Re: missing inode_add_bytes in dquot_alloc_space

[Dave Kleikamp]

On Fri, 2004-01-16 at 06:19, Christoph Hellwig wrote:
> On Fri, Jan 16, 2004 at 12:20:28PM +0100, Jan Kara wrote:
> >  	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> > -	if (IS_NOQUOTA(inode)) {
> > -		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> > -		return QUOTA_OK;
> > -	}
> >  	spin_lock(&dq_data_lock);
> > +	if (IS_NOQUOTA(inode))
> > +		goto add_bytes;
> 
> This means we take a global lock now even for non-quota operations?
> Doesn't sound like a really good idea..

I'm not sure if it's right or wrong, but it's consistent with
quotaops.h:

static __inline__ int DQUOT_ALLOC_SPACE_NODIRTY(struct inode *inode, qsize_t nr)
{
	if (sb_any_quota_enabled(inode->i_sb)) {
		/* Used space is updated in alloc_space() */
		if (inode->i_sb->dq_op->alloc_space(inode, nr, 0) == NO_QUOTA)
			return 1;
	}
	else {
		spin_lock(&dq_data_lock);
		inode_add_bytes(inode, nr);
		spin_unlock(&dq_data_lock);
	}
	return 0;
}

-- 
David Kleikamp
IBM Linux Technology Center

Re: missing inode_add_bytes in dquot_alloc_space

[Christoph Hellwig]

On Fri, Jan 16, 2004 at 02:33:58PM +0100, Jan Kara wrote:
>   This is only the case of quota files themselves - other cases (like
> when quota is not compiled/not turned on) are solved by macros in
> quotaops.h used by filesystems.

Ah, okay - sorry for the noise.