On 6/29/18 2:43 PM, Liu Bo wrote:

On Fri, Jun 29, 2018 at 02:26:07PM -0600, Jens Axboe wrote:

quoted

On 6/29/18 2:23 PM, Liu Bo wrote:

quoted

On Fri, Jun 29, 2018 at 02:00:01PM -0600, Jens Axboe wrote:

quoted

On 6/20/18 9:07 PM, Liu Bo wrote:

quoted

When a new tg is created, tg->bio_cnt_ret_time is 0, so if the first
IO going thru this tg turns out to be a bad one, we fail to record it
in tg->bad_bio_cnt as

if (jiffies > bio_cnt_ret_time) {
	tg->bad_bio_cnt /= 2;
}

Shouldn't we rather ensure that ->bio_cnt_ret_time is initialized to
jiffies?

Indeed, it's what the patch does, i.e. initialize tg->bio_cnt_reset_time to
jiffies on the first use.

You do it on the first use, on the hot path, presumable. My suggestion
was to do it when tg is instantiated instead. From a quick look, that
would appear to be in throtl_pd_alloc().

Doing it when tg is instantiated would end up with the same problem.

1) tg is instantiated, tg->bio_cnt_reset_time is set to jiffies.
(after a few jiffies...)
2) the 1st IO gets dispatched and reaches endio.
  2.1) tg->bad_bio_cnt++ #if the IO's latency > threshold.
  2.2) if (jiffies > bio_cnt_reset_time)

At 2.2), (the jiffies at this point > tg->bio_cnt_reset_time).  If
this IO is a bad one, then tg->bad_bio_cnt would become 0 instead of 1
since we do tg->bad_bio_cnt /= 2 in the if statement.

That's kind of an ugly way to use it. How is it any different from when
the tg has been idle for a while? There shouldn't be a need to special
case this.

-- 
Jens Axboe