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

Dave Chinner
2011-04-08T00:16:07Z

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.

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.
--
Dave Chinner
david@fromorbit.com