Re: [PATCH] xfs: force background CIL push under sustained load

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 23, 2010 at 01:53:55PM -0500, Alex Elder wrote:
> On Thu, 2010-09-23 at 12:27 +1000, Dave Chinner wrote:
> > + * than half the log" rule that recovery requires us to keep.
> > + *
> > + * Further, we need to make sure the background CIL push is efficient, which
> > + * means we need to give the background push a chance to commit without
> > + * blocking all the current transaction commits. Hence we need some space
> > + * between the threshold and the 25% limit to allow background pushes to be
> > + * tried, but not enforced. To make this simple and fast to calculate, set
> > + * the background push threshold to 1/8th (12.5%) the size of the log, and then start
> > + * enforcing the background push at 50% above this. i.e. at 3/16th or 18.75% of
> > + * the log size. This should keep us well under the limits of the AIL pushing
> > + * threshold, yet give us plenty of space for aggregation on large logs.
> >   */
> 
> I think the above explanation is pretty good but I don't know that it's
> as clear or concise as it could be.  I don't claim this is better but
> I'll take a shot (I don't like offering criticism without suggesting
> an alternative).
> 
>  * With dynamic reservations, we can basically make up arbitrary
>  * limits for the checkpoint size so long as they don't violate any
>  * other size rules.  Recovery imposes a rule that no transaction
>  * exceed half the log, so we are limited by that.  Furthermore, the
>  * log transaction reservation subsystem tries to keep 25% of the
>  * log free, so we should keep below that limit or we risk not being
>  * able to get the space we need.
>  *
>  * In order to keep background CIL push efficient, we will set a
>  * lower threshold at which background pushing is attempted without
>  * blocking current transaction commits.  A separate, higher bound
>  * defines when CIL pushes are forced in order to ensure we stay
>  * within our transaction size limits.

Yes, makes sense. I'll rework it along these lines.

> > -
> > -#define XLOG_CIL_SPACE_LIMIT(log)	\
> > -	(min((log->l_logsize >> 2), (8 * 1024 * 1024)))
> > +#define XLOG_CIL_SPACE_LIMIT(log)	(log->l_logsize >> 3)
> > +#define XLOG_CIL_HARD_SPACE_LIMIT(log)	(3 * (log->l_logsize >> 4))
> 
> Maybe "LIMIT" isn't quite the right name for these two.
> (But I have no better suggestion.)

Threshold is really the only other word that matches, but I think
limit is better here as it conveys a sense that it is something we
don't really want to cross...

> I don't really care much about this, but I'll take this
> opportunity for a small rant.
> 
> The difference in calculation cost/speed between "x >> 3" and
> "x / 8" is vanishingly small.

That is architecture dependent, but in most cases these days the
compiler will optimise a divide-by-power-of−2-constant into a shift
operation anyway. I'm pretty sure that optimisation is done on even
on x86 as a shift is a single cycle operation while an integer
divide still takes several cycles and consumes more power.

> I think it is meaningful to use
> a shift in places where a power-of-two is mandated, but in places
> like this it suggests there is a constraint that simply doesn't
> exist.  So for example, you could have chosen (log->logsize / 10)
> as the "try pushing" value, and (log->logsize / 4 - 1) as the
> "must push" value.

It's more the fact that XFS uses power-of-2 logic (i.e shifts)
everywhere. I just tend to be consistent with what is already there.
In this case, the AIL push thresholds are calculated using shifts:

	free_threshold = MAX(free_threshold, (log->l_logBBsize >> 2));

and so when you compare that to the XLOG_CIL_SPACE_LIMIT()
definitions, it is immediately clear that the CIL limits are smaller
than the AIL push threshold...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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