On Fri, Jun 29, 2018 at 02:26:07PM -0600, Jens Axboe wrote:
> On 6/29/18 2:23 PM, Liu Bo wrote:
> > On Fri, Jun 29, 2018 at 02:00:01PM -0600, Jens Axboe wrote:
> >> On 6/20/18 9:07 PM, Liu Bo wrote:
> >>> 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.
thanks,
-liubo
