Re: [PATCH 4/7] writeback: introduce max-pause and pass-good dirty limits

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 19 Jun 2011 23:01:12 +0800
Wu Fengguang <fengguang.wu@intel.com> wrote:

> The max-pause limit helps to keep the sleep time inside
> balance_dirty_pages() within 200ms. The 200ms max sleep means per task
> rate limit of 8pages/200ms=160KB/s, which normally is enough to stop
> dirtiers from continue pushing the dirty pages high, unless there are
> a sufficient large number of slow dirtiers (ie. 500 tasks doing 160KB/s
> will still sum up to 80MB/s, reaching the write bandwidth of a slow disk).
> 
> The pass-good limit helps to let go of the good bdi's in the presence of
> a blocked bdi (ie. NFS server not responding) or slow USB disk which for
> some reason build up a large number of initial dirty pages that refuse
> to go away anytime soon.

The hard-wired numbers and hard-wired assumptions about device speeds
shouldn't be here at all.  They will be sub-optimal (and sometimes
extremely so) for all cases.  They will become wronger over time.  Or
less wrong, depending upon which way they were originally wrong.

> +		dirty_thresh = hard_dirty_limit(dirty_thresh);
> +		if (nr_dirty < dirty_thresh + dirty_thresh / DIRTY_MAXPAUSE &&
> +		    jiffies - start_time > MAX_PAUSE)
> +			break;
> +		if (nr_dirty < dirty_thresh + dirty_thresh / DIRTY_PASSGOOD &&
> +		    bdi_dirty < bdi_thresh)
> +			break;

It appears that despite their similarity, DIRTY_MAXPAUSE is a
dimensionless value whereas the units of MAX_PAUSE is jiffies.  Perhaps
more care in naming choices would clarify things like this.

The first comparison might be clearer if it used time_after().

Both statements need comments explaining what they do and *why they do
it*.