Re: [PATCH 7/8] wbt: add general throttling mechanism

[Jan Kara]

On Tue 26-04-16 09:55:30, Jens Axboe wrote:
> We can hook this up to the block layer, to help throttle buffered
> writes. Or NFS can tap into it, to accomplish the same.
> 
> wbt registers a few trace points that can be used to track what is
> happening in the system:
> 
> wbt_lat: 259:0: latency 2446318
> wbt_stat: 259:0: rmean=2446318, rmin=2446318, rmax=2446318, rsamples=1,
>                wmean=518866, wmin=15522, wmax=5330353, wsamples=57
> wbt_step: 259:0: step down: step=1, window=72727272, background=8, normal=16, max=32
> 
> This shows a sync issue event (wbt_lat) that exceeded it's time. wbt_stat
> dumps the current read/write stats for that window, and wbt_step shows a
> step down event where we now scale back writes. Each trace includes the
> device, 259:0 in this case.

I have some comments below...

> +struct rq_wb {
> +	/*
> +	 * Settings that govern how we throttle
> +	 */
> +	unsigned int wb_background;		/* background writeback */
> +	unsigned int wb_normal;			/* normal writeback */
> +	unsigned int wb_max;			/* max throughput writeback */
> +	unsigned int scale_step;
> +
> +	u64 win_nsec;				/* default window size */
> +	u64 cur_win_nsec;			/* current window size */
> +
> +	unsigned int unknown_cnt;

It would be useful to have a comment here explaining that 'unknown_cnt' is
a number of consecutive periods in which we didn't have enough data to
decide about queue scaling (at least this is what I understood from the
code).

> +
> +	struct timer_list window_timer;
> +
> +	s64 sync_issue;
> +	void *sync_cookie;

So I'm somewhat wondering: What is protecting consistency of this
structure? The limits, scale_step, cur_win_nsec, unknown_cnt are updated only
from timer so those should be safe. However sync_issue & sync_cookie are
accessed from IO submission and completion path and there we need some
protection to keep those two in sync. It seems q->queue_lock should mostly
achieve those except for blk-mq submission path calling wbt_wait() which
doesn't hold queue_lock.

It seems you were aware of the possible races and the code handles them
mostly fine (although I wouldn't bet too much there is not some weird
corner case). However it would be good to comment on this somewhere and
explain what the rules for these two fields are.

> +
> +	unsigned int wc;
> +	unsigned int queue_depth;
> +
> +	unsigned long last_issue;		/* last non-throttled issue */
> +	unsigned long last_comp;		/* last non-throttled comp */
> +	unsigned long min_lat_nsec;
> +	struct backing_dev_info *bdi;
> +	struct request_queue *q;
> +	wait_queue_head_t wait;
> +	atomic_t inflight;
> +
> +	struct wb_stat_ops *stat_ops;
> +	void *ops_data;
> +};
...
> diff --git a/lib/wbt.c b/lib/wbt.c
> new file mode 100644
> index 000000000000..650da911f24f
> --- /dev/null
> +++ b/lib/wbt.c
> @@ -0,0 +1,524 @@
> +/*
> + * buffered writeback throttling. losely based on CoDel. We can't drop
> + * packets for IO scheduling, so the logic is something like this:
> + *
> + * - Monitor latencies in a defined window of time.
> + * - If the minimum latency in the above window exceeds some target, increment
> + *   scaling step and scale down queue depth by a factor of 2x. The monitoring
> + *   window is then shrunk to 100 / sqrt(scaling step + 1).
> + * - For any window where we don't have solid data on what the latencies
> + *   look like, retain status quo.
> + * - If latencies look good, decrement scaling step.

I'm wondering about two things:

1) There is a logic somewhat in this direction in blk_queue_start_tag().
   Probably it should be removed after your patches land?

2) As far as I can see in patch 8/8, you have plugged the throttling above
   the IO scheduler. When there are e.g. multiple cgroups with different IO
   limits operating, this throttling can lead to strange results (like a
   cgroup with low limit using up all available background "slots" and thus
   effectively stopping background writeback for other cgroups)? So won't
   it make more sense to plug this below the IO scheduler? Now I understand
   there may be other problems with this but I think we should put more
   though to that and provide some justification in changelogs.

> +static void calc_wb_limits(struct rq_wb *rwb)
> +{
> +	unsigned int depth;
> +
> +	if (!rwb->min_lat_nsec) {
> +		rwb->wb_max = rwb->wb_normal = rwb->wb_background = 0;
> +		return;
> +	}
> +
> +	depth = min_t(unsigned int, RWB_MAX_DEPTH, rwb->queue_depth);
> +
> +	/*
> +	 * Reduce max depth by 50%, and re-calculate normal/bg based on that
> +	 */

The comment looks a bit out of place here since we don't reduce max depth
here. We just use whatever is set in scale_step...

> +	rwb->wb_max = 1 + ((depth - 1) >> min(31U, rwb->scale_step));
> +	rwb->wb_normal = (rwb->wb_max + 1) / 2;
> +	rwb->wb_background = (rwb->wb_max + 3) / 4;
> +}
> +
> +static bool inline stat_sample_valid(struct blk_rq_stat *stat)
> +{
> +	/*
> +	 * We need at least one read sample, and a minimum of
> +	 * RWB_MIN_WRITE_SAMPLES. We require some write samples to know
> +	 * that it's writes impacting us, and not just some sole read on
> +	 * a device that is in a lower power state.
> +	 */
> +	return stat[0].nr_samples >= 1 &&
> +		stat[1].nr_samples >= RWB_MIN_WRITE_SAMPLES;
> +}
> +
> +static u64 rwb_sync_issue_lat(struct rq_wb *rwb)
> +{
> +	u64 now, issue = ACCESS_ONCE(rwb->sync_issue);
> +
> +	if (!issue || !rwb->sync_cookie)
> +		return 0;
> +
> +	now = ktime_to_ns(ktime_get());
> +	return now - issue;
> +}
> +
> +enum {
> +	LAT_OK,
> +	LAT_UNKNOWN,
> +	LAT_EXCEEDED,
> +};
> +
> +static int __latency_exceeded(struct rq_wb *rwb, struct blk_rq_stat *stat)
> +{
> +	u64 thislat;
> +
> +	/*
> +	 * If our stored sync issue exceeds the window size, or it
> +	 * exceeds our min target AND we haven't logged any entries,
> +	 * flag the latency as exceeded.
> +	 */
> +	thislat = rwb_sync_issue_lat(rwb);
> +	if (thislat > rwb->cur_win_nsec ||
> +	    (thislat > rwb->min_lat_nsec && !stat[0].nr_samples)) {
> +		trace_wbt_lat(rwb->bdi, thislat);
> +		return LAT_EXCEEDED;
> +	}

So I'm trying to wrap my head around this. If I read the code right,
rwb_sync_issue_lat() this returns time that has passed since issuing sync
request that is still running. We basically randomly pick which sync
request we track as we always start tracking a sync request when some is
issued and we are not tracking any at that moment. This is to detect the
case when latency of sync IO is very large compared to measurement window
so we would not get enough samples to make it valid?

Probably the comment could explain more of "why we do this?" than pure
"what we do".

> +
> +	if (!stat_sample_valid(stat))
> +		return LAT_UNKNOWN;
> +
> +	/*
> +	 * If the 'min' latency exceeds our target, step down.
> +	 */
> +	if (stat[0].min > rwb->min_lat_nsec) {
> +		trace_wbt_lat(rwb->bdi, stat[0].min);
> +		trace_wbt_stat(rwb->bdi, stat);
> +		return LAT_EXCEEDED;
> +	}
> +
> +	if (rwb->scale_step)
> +		trace_wbt_stat(rwb->bdi, stat);
> +
> +	return LAT_OK;
> +}
> +

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR