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

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe