[PATCH] Block: initialize bio_cnt_ret_time for the first time

[PATCH] Block: initialize bio_cnt_ret_time for the first time

[Liu Bo]

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;
}

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 block/blk-throttle.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 82282e6fdcf8..8bd54118dc0a 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2329,6 +2329,8 @@ void blk_throtl_bio_endio(struct bio *bio)
 		tg->bio_cnt++;
 	}
 
+	if (unlikely(!tg->bio_cnt_reset_time))
+		tg->bio_cnt_reset_time = jiffies;
 	if (time_after(jiffies, tg->bio_cnt_reset_time) || tg->bio_cnt > 1024) {
 		tg->bio_cnt_reset_time = tg->td->throtl_slice + jiffies;
 		tg->bio_cnt /= 2;
-- 
1.8.3.1

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

[Liu Bo]

Hi Jens,

This one is kind of obvious, could you please also take it thru your tree?

thanks,
liubo


On Wed, Jun 20, 2018 at 8:07 PM, Liu Bo <bo.liu@linux.alibaba.com> 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;
> }
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  block/blk-throttle.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 82282e6fdcf8..8bd54118dc0a 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -2329,6 +2329,8 @@ void blk_throtl_bio_endio(struct bio *bio)
>                 tg->bio_cnt++;
>         }
>
> +       if (unlikely(!tg->bio_cnt_reset_time))
> +               tg->bio_cnt_reset_time = jiffies;
>         if (time_after(jiffies, tg->bio_cnt_reset_time) || tg->bio_cnt > 1024) {
>                 tg->bio_cnt_reset_time = tg->td->throtl_slice + jiffies;
>                 tg->bio_cnt /= 2;
> --
> 1.8.3.1
>

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

[Jens Axboe]

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?

-- 
Jens Axboe

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

[Liu Bo]

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.

thanks,
-liubo

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

[Jens Axboe]

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().

-- 
Jens Axboe

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

[Liu Bo]

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

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

[Jens Axboe]

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

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

[Liu Bo]

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