| Message ID | 1529550424-105210-1-git-send-email-bo.liu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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?
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
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().
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
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.
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
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;
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(+)