Re: [PATCH 4/9] xfs: introduce background inode reclaim work

[Alex Elder <aelder@sgi.com>]

On Fri, 2011-04-08 at 10:19 +1000, Dave Chinner wrote:
> On Thu, Apr 07, 2011 at 04:16:22PM -0500, Alex Elder wrote:
> > On Thu, 2011-04-07 at 11:57 +1000, Dave Chinner wrote:
> > > +	if (!(mp->m_super->s_flags & MS_ACTIVE))
> > > +		return;
> > > +
> > > +	rcu_read_lock();
> > > +	if (radix_tree_tagged(&mp->m_perag_tree, XFS_ICI_RECLAIM_TAG)) {
> > > +		queue_delayed_work(xfs_syncd_wq, &mp->m_reclaim_work,
> > > +			msecs_to_jiffies(xfs_syncd_centisecs / 6 * 10));
> > 
> > Probably better to do the multiply before the divide here.
> > (But whatever... it's heuristic.)
> 
> I always tend to divide before multiply to prevent the multiple from
> overflowing before the divide is done. In this case the granularity
> of xfs_syncd_centisecs is sufficient that the rounding error of the
> divide is meaningless. ie. 30s = 3000.

Funny, I normally do the same, but in this case
I was thinking about getting better accuracy.
The numbers involved (msecs / jiffies and the
number of msec) are not likely to be even close
to sqrt(INT32_MAX) (let alone sqrt(UINT64_MAX)).
Putting the calculation inside the parenthesis
is much better though, and I did notice that
change.

					-Alex

> FWIW, I changed this from:
> 
> 	xfs_syncd_centisecs / 6 * msecs_to_jiffies(10)
> 
> because msecs_to_jiffies() has larger rounding problems. e.g. @
> CONFIG_HZ=250, msecs_to_jiffies(10) = 3 which is actually 12ms. That
> is, we want to sleep for 5s at a time, and the two different
> calculations give:
> 
> New:
> 	msecs_to_jiffies(3000 / 6 * 10) = 5000 / 4 jiffies
> 					= 1250 jiffies
> 					= 5s
> Old:
> 	3000 / 6 * msecs_to_jiffies(10) = 500 * 3 jiffies
> 					= 1500 jiffies
> 					= 6s
> 
> This 20% rounding error is the reason we've recently noticed xfssyncd
> running every 36s rather than every 30s....
> 
> Cheers,
> 
> Dave.



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs