Re: [PATCH] Block: initialize bio_cnt_ret_time for the first time

[Liu Bo <bo.liu@linux.alibaba.com>]

On Fri, Jun 29, 2018 at 02:46:27PM -0600, Jens Axboe wrote:
> On 6/29/18 2:43 PM, Liu Bo wrote:
> > 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.
> 
> 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.

Yes, we can leave this corner case alone.

thanks,
-liubo