Linux 2.4 quota (accounting?) bug ...

Linux 2.4 quota (accounting?) bug ...

[Herbert Poetzl]

Hi Honza!

a friend of mine, made me aware of the following
imbalance, which looks like a minor accounting bug 
to me, but might be a quota bug ...

fs/dquot.c : 394 vfs_quota_sync()
-----------------------------------------------------
                /* Get reference to quota so it won't be invalidated. get_dquot_ref()
                 * is enough since if dquot is locked/modified it can't be
                 * on the free list */

> 		get_dquot_ref(dquot);
 		if (dquot->dq_flags & DQ_LOCKED)
 			wait_on_dquot(dquot);
 		if (dquot_dirty(dquot))
 			sb->dq_op->sync_dquot(dquot);
>		dqput(dquot);
 		goto restart;
 	}


best,
Herbert

Re: Linux 2.4 quota (accounting?) bug ...

[Jan Kara]

  Hi,

> a friend of mine, made me aware of the following
> imbalance, which looks like a minor accounting bug 
> to me, but might be a quota bug ...
  Sorry but the code seems correct to me - we get reference to dquot by
get_dquot_ref() and than we put the reference by dqput(). dqput() is
correct because something nasty might happen in the mean time and so we
might be the last holders of the dquot. What do you think is wrong?

								Honza

> 
> fs/dquot.c : 394 vfs_quota_sync()
> -----------------------------------------------------
>                 /* Get reference to quota so it won't be invalidated. get_dquot_ref()
>                  * is enough since if dquot is locked/modified it can't be
>                  * on the free list */
> 
> > 		get_dquot_ref(dquot);
>  		if (dquot->dq_flags & DQ_LOCKED)
>  			wait_on_dquot(dquot);
>  		if (dquot_dirty(dquot))
>  			sb->dq_op->sync_dquot(dquot);
> >		dqput(dquot);
>  		goto restart;
>  	}
> 
> 
> best,
> Herbert
> 
> 
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: Linux 2.4 quota (accounting?) bug ...

[Herbert Poetzl]

On Sat, Oct 25, 2003 at 06:31:28PM +0200, Jan Kara wrote:
>   Hi,
> 
> > a friend of mine, made me aware of the following
> > imbalance, which looks like a minor accounting bug 
> > to me, but might be a quota bug ...
>   Sorry but the code seems correct to me - we get reference to dquot by
> get_dquot_ref() and than we put the reference by dqput(). dqput() is
> correct because something nasty might happen in the mean time and so we
> might be the last holders of the dquot. What do you think is wrong?

dqput() does dqstats.drops++;
which isn't correct if this should be the same as
put_dquot_ref(), but maybe I'm just irritated by 
strange statistics on some kernels showing more
drops than lookups+allocated after sync/quotaoff

best,
Herbert

> 								Honza
> 
> > 
> > fs/dquot.c : 394 vfs_quota_sync()
> > -----------------------------------------------------
> >                 /* Get reference to quota so it won't be invalidated. get_dquot_ref()
> >                  * is enough since if dquot is locked/modified it can't be
> >                  * on the free list */
> > 
> > > 		get_dquot_ref(dquot);
> >  		if (dquot->dq_flags & DQ_LOCKED)
> >  			wait_on_dquot(dquot);
> >  		if (dquot_dirty(dquot))
> >  			sb->dq_op->sync_dquot(dquot);
> > >		dqput(dquot);
> >  		goto restart;
> >  	}
> > 
> > 
> > best,
> > Herbert
> > 
> > 
> -- 
> Jan Kara <jack@suse.cz>
> SuSE CR Labs
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Linux 2.4 quota (accounting?) bug ...

[Jan Kara]

  Hi,
> On Sat, Oct 25, 2003 at 06:31:28PM +0200, Jan Kara wrote:
> >   Hi,
> > 
> > > a friend of mine, made me aware of the following
> > > imbalance, which looks like a minor accounting bug 
> > > to me, but might be a quota bug ...
> >   Sorry but the code seems correct to me - we get reference to dquot by
> > get_dquot_ref() and than we put the reference by dqput(). dqput() is
> > correct because something nasty might happen in the mean time and so we
> > might be the last holders of the dquot. What do you think is wrong?
> 
> dqput() does dqstats.drops++;
> which isn't correct if this should be the same as
> put_dquot_ref(), but maybe I'm just irritated by 
> strange statistics on some kernels showing more
> drops than lookups+allocated after sync/quotaoff
  Oh, now I see... At first I didn't understand that the problem is in
the quota statistics. I'll fix that.

							Honza

Re: Linux 2.4 quota (accounting?) bug ...

[Jan Kara]

  Hi,

> On Sat, Oct 25, 2003 at 06:31:28PM +0200, Jan Kara wrote:
> >   Hi,
> > 
> > > a friend of mine, made me aware of the following
> > > imbalance, which looks like a minor accounting bug 
> > > to me, but might be a quota bug ...
> >   Sorry but the code seems correct to me - we get reference to dquot by
> > get_dquot_ref() and than we put the reference by dqput(). dqput() is
> > correct because something nasty might happen in the mean time and so we
> > might be the last holders of the dquot. What do you think is wrong?
> 
> dqput() does dqstats.drops++;
> which isn't correct if this should be the same as
> put_dquot_ref(), but maybe I'm just irritated by 
> strange statistics on some kernels showing more
> drops than lookups+allocated after sync/quotaoff
  So I finally got to fixing the bug. The patch (also with a deletition
of unnecessary variable) is attached. Marcello please apply.

									Honza

---------------------cut here-------------------------------------------

diff -ruX ../kerndiffexclude linux-2.4.22-um/fs/dquot.c linux-2.4.22-fixstat/fs/dquot.c
--- linux-2.4.22-um/fs/dquot.c	Mon Aug 25 13:44:43 2003
+++ linux-2.4.22-fixstat/fs/dquot.c	Wed Nov  5 21:21:58 2003
@@ -392,6 +392,7 @@
 		 * is enough since if dquot is locked/modified it can't be
 		 * on the free list */
 		get_dquot_ref(dquot);
+		dqstats.lookups++;
 		if (dquot->dq_flags & DQ_LOCKED)
 			wait_on_dquot(dquot);
 		if (dquot_dirty(dquot))
@@ -622,7 +623,6 @@
 		dqput(dquot);
 		return NODQUOT;
 	}
-	++dquot->dq_referenced;
 	dqstats.lookups++;
 
 	return dquot;
@@ -642,7 +642,6 @@
 	if (dquot->dq_flags & DQ_LOCKED)
 		printk(KERN_ERR "VFS: dqduplicate(): Locked quota to be duplicated!\n");
 	get_dquot_dup_ref(dquot);
-	dquot->dq_referenced++;
 	dqstats.lookups++;
 
 	return dquot;
Only in linux-2.4.22-fixstat/include/linux: modules
diff -ruX ../kerndiffexclude linux-2.4.22-um/include/linux/quota.h linux-2.4.22-fixstat/include/linux/quota.h
--- linux-2.4.22-um/include/linux/quota.h	Mon Aug 25 13:44:44 2003
+++ linux-2.4.22-fixstat/include/linux/quota.h	Wed Nov  5 21:27:44 2003
@@ -222,8 +222,6 @@
 	loff_t dq_off;			/* Offset of dquot on disk */
 	short dq_type;			/* Type of quota */
 	short dq_flags;			/* See DQ_* */
-	unsigned long dq_referenced;	/* Number of times this dquot was 
-					   referenced during its lifetime */
 	struct mem_dqblk dq_dqb;	/* Diskquota usage */
 };