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...