Messages in this thread | | | Date | Mon, 14 Jan 2013 10:25:00 +0800 | From | majianpeng <> | Subject | Re: Re: [PATCH] blkcg: Before starting a new slice, firstly count bps/iops limit in func tg_may_dispatch. |
| |
[maybe you received more, I'm very sorry for that] >On Fri, Jan 11, 2013 at 03:26:37PM +0100, Jens Axboe wrote: >> On 2013-01-11 10:11, majianpeng wrote: >> > In func tg_may_dispatch, >> >> if (throtl_slice_used(td, tg, rw)) >> >> throtl_start_new_slice(td, tg, rw); >> > ... >> >> if (tg_with_in_bps_limit(td, tg, bio, &bps_wait) >> >> && tg_with_in_iops_limit(td, tg, bio, &iops_wait)) { >> > >> > In funcs tg_with_in_(bps/iops)_limit, it used the slice_start to count. >> > So if privious slice expired, it start a new slice.This can cause hung >> > task. >> > >> > The next steps can repeat this bug. >> > 1:echo "8:48 10240" > blkio.throttle.write_bps_devic >> > 2:dd if=/dev/zero of=/dev/sdd bs=1M count=1 oflag=direct >> > >> > Using the blktrace, the messages about throttle: >> > root@kernel:/mnt/programs# blktrace -d /dev/sdd -a notify -o -|blkparse -i - >> > 8,48 1 0 0.000000000 0 m N throtl / [W] new slice start=4294854679 end=4294854779 jiffies=4294854679 >> > 8,48 1 0 0.000000966 0 m N throtl / [W] extend slice start=4294854679 end=4294905900 jiffies=4294854679 >> > 8,48 1 0 0.000002553 0 m N throtl / [W] bio. bdisp=0 sz=524288 bps=10240 iodisp=0 iops=4294967295 queued=0/0 >> > 8,48 1 0 0.000004788 0 m N throtl schedule work. delay=51200 jiffies=4294854679 >> > 8,48 1 0 51.304698681 0 m N throtl dispatch nr_queued=1 read=0 write=1 >> > 8,48 1 0 51.304701979 0 m N throtl / [W] new slice start=4294905984 end=4294906084 jiffies=4294905984 >> > 8,48 1 0 51.304703329 0 m N throtl / [W] extend slice start=4294905984 end=4294957200 jiffies=4294905984 >> > 8,48 1 0 51.304705783 0 m N throtl schedule work. delay=51200 jiffies=4294905984 >> > 8,48 1 0 102.632697082 0 m N throtl dispatch nr_queued=1 read=0 write=1 >> > 8,48 1 0 102.632700544 0 m N throtl / [W] new slice start=4294957312 end=4294957412 jiffies=4294957312 >> > 8,48 1 0 102.632701922 0 m N throtl / [W] extend slice start=4294957312 end=4295008600 jiffies=4294957312 >> > 8,48 1 0 102.632704016 0 m N throtl schedule work. delay=51200 jiffies=4294957312 >> > 8,48 1 0 153.960696503 0 m N throtl dispatch nr_queued=1 read=0 write=1 >> > 8,48 1 0 153.960699797 0 m N throtl / [W] new slice start=4295008640 end=4295008740 jiffies=4295008640 >> > 8,48 1 0 153.960701153 0 m N throtl / [W] extend slice start=4295008640 end=4295059900 jiffies=4295008640 >> > 8,48 1 0 153.960703218 0 m N throtl schedule work. delay=51200 jiffies=4295008640 >> > 8,48 1 0 205.288697067 0 m N throtl dispatch nr_queued=1 read=0 write=1 >> > 8,48 1 0 205.288700268 0 m N throtl / [W] new slice start=4295059968 end=4295060068 jiffies=4295059968 >> > 8,48 1 0 205.288701630 0 m N throtl / [W] extend slice start=4295059968 end=4295111200 jiffies=4295059968 >> > 8,48 1 0 205.288703784 0 m N throtl schedule work. delay=51200 jiffies=4295059968 >> > 8,48 1 0 256.616696184 0 m N throtl dispatch nr_queued=1 read=0 write=1 >> > 8,48 1 0 256.616699266 0 m N throtl / [W] new slice start=4295111296 end=4295111396 jiffies=4295111296 >> > 8,48 1 0 256.616700574 0 m N throtl / [W] extend slice start=4295111296 end=4295162500 jiffies=4295111296 >> > 8,48 1 0 256.616702701 0 m N throtl schedule work. delay=51200 jiffies=4295111296 >> > >> > Signed-off-by: Jianpeng Ma <majianpeng@gmail.com> >> > --- >> > block/blk-throttle.c | 15 ++++++++++----- >> > 1 file changed, 10 insertions(+), 5 deletions(-) >> > >> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c >> > index 3114622..9258789 100644 >> > --- a/block/blk-throttle.c >> > +++ b/block/blk-throttle.c >> > @@ -645,6 +645,15 @@ static bool tg_may_dispatch(struct throtl_data *td, struct throtl_grp *tg, >> > } >> > >> > /* >> > + * If privious slice expired,then start new slice. >> > + * But counting bps and iops limit need privious slice info >> > + * which ->slice_start. >> > + */ >> > + if (tg_with_in_bps_limit(td, tg, bio, &bps_wait) >> > + && tg_with_in_iops_limit(td, tg, bio, &iops_wait)) >> > + if (wait) >> > + *wait = 0; >> > + /* >> > * If previous slice expired, start a new one otherwise renew/extend >> > * existing slice to make sure it is at least throtl_slice interval >> > * long since now. >> > @@ -656,12 +665,8 @@ static bool tg_may_dispatch(struct throtl_data *td, struct throtl_grp *tg, >> > throtl_extend_slice(td, tg, rw, jiffies + throtl_slice); >> > } >> > >> > - if (tg_with_in_bps_limit(td, tg, bio, &bps_wait) >> > - && tg_with_in_iops_limit(td, tg, bio, &iops_wait)) { >> > - if (wait) >> > - *wait = 0; >> > + if (!(bps_wait || iops_wait)) >> > return 1; >> > - } >> > >> > max_wait = max(bps_wait, iops_wait); >> >> Looks pretty sane to me. Vivek? > >Hi Jens, > >This fix will introduce other side affects. And that is when an group >has been idle for few seconds and a new IO gets queued in, we will >not start a new slice and allow dispatch equivalent of those idle >seconds before we throttle the group. So keeping group idle will >become an incentive. > >I have attached a patch which might work better. majianpeng, can >you please give it a try. > >Having said that it is strange that workqueue thread is triggering >so late. Looking at timestamp of traces attached. > >We scheduled a delayed work. Following is trace. > >8,48 1 0 51.304705783 0 m N throtl schedule work. delay=51200 jiffies=4294905984 > >Now a worker should execute after 51.2 seconds (delay=51200). > >8,48 1 0 102.632697082 0 m N throtl dispatch nr_queued=1 read=0 write=1 > >But worker executed at delay of 51.328 seconds (102.632 - 51.304). That >is a delay of around 128 jiffies (51.328 - 51.2). That kind of seems odd. >AFAIK, workers are fairly quick to execute after expiry. And it is >because of this excessive delay that we think slice has expired. > >May be it is something new. CCing Tejun, if he has seen anything like this >and may be it is a known issue. > >Even if it is a worker thread issue, I think applying below patch makes >sense. > >Thanks >Vivek > > >blk-throttle: Do not start a new slice if IO is pending > >It might happen that we queue an IO (because it overshot the rate) >and then wait for certain jiffies to pass. We will set slice_end >accordingly and wait for that time to elapse and worker thread will >kick in, again evaluate whether group can dispatch or not. Now it >might happen that current time is after slice_end and >tg_may_dispatch() will start a new slice. > >This will result in IO not being dispatched and be put back on >wait again. And this will repeat, resulting in hang. > >Do not start a new slice if an IO is pending in that group in >the direction being queired. Instead extend the slice. New slice >is supposed to start when a group has not been doing IO for some >time and a new IO shows up. In that case we want do discard >history and start a new slice. > >Signed-off-by: Vivek Goyal <vgoyal@redhat.com> >--- > block/blk-throttle.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > >Index: linux-2.6/block/blk-throttle.c >=================================================================== >--- linux-2.6.orig/block/blk-throttle.c 2012-10-18 01:52:28.000000000 -0400 >+++ linux-2.6/block/blk-throttle.c 2013-01-14 03:40:41.355731375 -0500 >@@ -648,8 +648,14 @@ static bool tg_may_dispatch(struct throt > * If previous slice expired, start a new one otherwise renew/extend > * existing slice to make sure it is at least throtl_slice interval > * long since now. >+ * >+ * Start a new slice only if there is no bio queued in that direction. >+ * That bio is waiting to be dispatched and slice needs to be >+ * extended. It might happen that bio waited to be dispatched but >+ * workqueue execution got little late it might restart a new slice >+ * instead of taking all the waited time into account. > */ >- if (throtl_slice_used(td, tg, rw)) >+ if (throtl_slice_used(td, tg, rw) && !tg->nr_queued[rw]) > throtl_start_new_slice(td, tg, rw); > else { > if (time_before(tg->slice_end[rw], jiffies + throtl_slice)) Hi vivek, Your patch is ok.But i had a question: What's condition tg->nr_queued[rw] = 0, but bio is not null?
Thanks! Jianpeng | |