| Message ID | 20230812072116.42321-1-zhuxiaohui.400@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] blk-throttle: fix throttle configuring not effective | expand |
Hi, 在 2023/08/12 15:21, zhuxiaohui 写道: > when updating block throttle limit with persistence and stable io > pressure, especially a relative high io pressure, fio test e.g., > there may never be a change to start a new slice, and carryover_ios & > carryover_bytes will not be cleared. > > As a result, when reconfiguring block throttle limit, we can notice that > the actual iops and throughput is a random value far away from what is > set > > So we need to update carryover value when dispatching bio I don't understand, not clear carryover_bytes/ios is what expected, and how can they affect actual bandwith/iops. Can you give a example how you tested and why current calculation is not correct? Thanks, Kuai > > Signed-off-by: zhuxiaohui <zhuxiaohui.400@bytedance.com> > --- > block/blk-throttle.c | 26 ++++++++++++++++++++++++++ > block/blk-throttle.h | 4 ++-- > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/block/blk-throttle.c b/block/blk-throttle.c > index 7397ff199d66..13c9d87a7201 100644 > --- a/block/blk-throttle.c > +++ b/block/blk-throttle.c > @@ -821,6 +821,30 @@ static void tg_update_carryover(struct throtl_grp *tg) > tg->carryover_ios[READ], tg->carryover_ios[WRITE]); > } > > +static void tg_charge_carryover(struct throtl_grp *tg, struct bio *bio) > +{ > + bool rw = bio_data_dir(bio); > + > + if (unlikely(tg->carryover_bytes[rw])) { > + unsigned int bio_size = throtl_bio_data_size(bio); > + unsigned int carryout_size = abs(tg->carryover_bytes[rw]); > + > + carryout_size = min(carryout_size, bio_size); > + > + if (tg->carryover_bytes[rw] < 0) > + tg->carryover_bytes[rw] += carryout_size; > + else > + tg->carryover_bytes[rw] -= carryout_size; > + } > + > + if (unlikely(tg->carryover_ios[rw])) { > + if (tg->carryover_ios[rw] < 0) > + tg->carryover_ios[rw] += 1; > + else > + tg->carryover_ios[rw] -= 1; > + } > +} > + > static unsigned long tg_within_iops_limit(struct throtl_grp *tg, struct bio *bio, > u32 iops_limit) > { > @@ -965,6 +989,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio) > > tg->io_disp[rw]++; > tg->last_io_disp[rw]++; > + > + tg_charge_carryover(tg, bio); > } > > /** > diff --git a/block/blk-throttle.h b/block/blk-throttle.h > index d1ccbfe9f797..8f1642becb23 100644 > --- a/block/blk-throttle.h > +++ b/block/blk-throttle.h > @@ -127,8 +127,8 @@ struct throtl_grp { > * bytes/ios are waited already in previous configuration, and they will > * be used to calculate wait time under new configuration. > */ > - uint64_t carryover_bytes[2]; > - unsigned int carryover_ios[2]; > + int64_t carryover_bytes[2]; > + int carryover_ios[2]; > > unsigned long last_check_time; > >
+CC Michal. 在 2023/08/12 15:53, Yu Kuai 写道: > Hi, > > 在 2023/08/12 15:21, zhuxiaohui 写道: >> when updating block throttle limit with persistence and stable io >> pressure, especially a relative high io pressure, fio test e.g., >> there may never be a change to start a new slice, and carryover_ios & >> carryover_bytes will not be cleared. >> >> As a result, when reconfiguring block throttle limit, we can notice that >> the actual iops and throughput is a random value far away from what is >> set >> >> So we need to update carryover value when dispatching bio > > I don't understand, not clear carryover_bytes/ios is what expected, and > how can they affect actual bandwith/iops. > > Can you give a example how you tested and why current calculation is not > correct? I can reporduce this, but this patch is obviously wrong. You must explaim how the calculation is not correct. After a quick loock, I found that carryover_bytes/ios is not updated in throtl_trim_slice(), while tg->io/bytes_disp[] can be cleared. This is definitly a problem. Thanks, Kuai > > Thanks, > Kuai > >> >> Signed-off-by: zhuxiaohui <zhuxiaohui.400@bytedance.com> >> --- >> block/blk-throttle.c | 26 ++++++++++++++++++++++++++ >> block/blk-throttle.h | 4 ++-- >> 2 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c >> index 7397ff199d66..13c9d87a7201 100644 >> --- a/block/blk-throttle.c >> +++ b/block/blk-throttle.c >> @@ -821,6 +821,30 @@ static void tg_update_carryover(struct throtl_grp >> *tg) >> tg->carryover_ios[READ], tg->carryover_ios[WRITE]); >> } >> +static void tg_charge_carryover(struct throtl_grp *tg, struct bio *bio) >> +{ >> + bool rw = bio_data_dir(bio); >> + >> + if (unlikely(tg->carryover_bytes[rw])) { >> + unsigned int bio_size = throtl_bio_data_size(bio); >> + unsigned int carryout_size = abs(tg->carryover_bytes[rw]); >> + >> + carryout_size = min(carryout_size, bio_size); >> + >> + if (tg->carryover_bytes[rw] < 0) >> + tg->carryover_bytes[rw] += carryout_size; >> + else >> + tg->carryover_bytes[rw] -= carryout_size; >> + } >> + >> + if (unlikely(tg->carryover_ios[rw])) { >> + if (tg->carryover_ios[rw] < 0) >> + tg->carryover_ios[rw] += 1; >> + else >> + tg->carryover_ios[rw] -= 1; >> + } >> +} >> + >> static unsigned long tg_within_iops_limit(struct throtl_grp *tg, >> struct bio *bio, >> u32 iops_limit) >> { >> @@ -965,6 +989,8 @@ static void throtl_charge_bio(struct throtl_grp >> *tg, struct bio *bio) >> tg->io_disp[rw]++; >> tg->last_io_disp[rw]++; >> + >> + tg_charge_carryover(tg, bio); >> } >> /** >> diff --git a/block/blk-throttle.h b/block/blk-throttle.h >> index d1ccbfe9f797..8f1642becb23 100644 >> --- a/block/blk-throttle.h >> +++ b/block/blk-throttle.h >> @@ -127,8 +127,8 @@ struct throtl_grp { >> * bytes/ios are waited already in previous configuration, and >> they will >> * be used to calculate wait time under new configuration. >> */ >> - uint64_t carryover_bytes[2]; >> - unsigned int carryover_ios[2]; >> + int64_t carryover_bytes[2]; >> + int carryover_ios[2]; >> unsigned long last_check_time; >> > > . >
diff --git a/block/blk-throttle.c b/block/blk-throttle.c index 7397ff199d66..13c9d87a7201 100644 --- a/block/blk-throttle.c +++ b/block/blk-throttle.c @@ -821,6 +821,30 @@ static void tg_update_carryover(struct throtl_grp *tg) tg->carryover_ios[READ], tg->carryover_ios[WRITE]); } +static void tg_charge_carryover(struct throtl_grp *tg, struct bio *bio) +{ + bool rw = bio_data_dir(bio); + + if (unlikely(tg->carryover_bytes[rw])) { + unsigned int bio_size = throtl_bio_data_size(bio); + unsigned int carryout_size = abs(tg->carryover_bytes[rw]); + + carryout_size = min(carryout_size, bio_size); + + if (tg->carryover_bytes[rw] < 0) + tg->carryover_bytes[rw] += carryout_size; + else + tg->carryover_bytes[rw] -= carryout_size; + } + + if (unlikely(tg->carryover_ios[rw])) { + if (tg->carryover_ios[rw] < 0) + tg->carryover_ios[rw] += 1; + else + tg->carryover_ios[rw] -= 1; + } +} + static unsigned long tg_within_iops_limit(struct throtl_grp *tg, struct bio *bio, u32 iops_limit) { @@ -965,6 +989,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio) tg->io_disp[rw]++; tg->last_io_disp[rw]++; + + tg_charge_carryover(tg, bio); } /** diff --git a/block/blk-throttle.h b/block/blk-throttle.h index d1ccbfe9f797..8f1642becb23 100644 --- a/block/blk-throttle.h +++ b/block/blk-throttle.h @@ -127,8 +127,8 @@ struct throtl_grp { * bytes/ios are waited already in previous configuration, and they will * be used to calculate wait time under new configuration. */ - uint64_t carryover_bytes[2]; - unsigned int carryover_ios[2]; + int64_t carryover_bytes[2]; + int carryover_ios[2]; unsigned long last_check_time;
when updating block throttle limit with persistence and stable io pressure, especially a relative high io pressure, fio test e.g., there may never be a change to start a new slice, and carryover_ios & carryover_bytes will not be cleared. As a result, when reconfiguring block throttle limit, we can notice that the actual iops and throughput is a random value far away from what is set So we need to update carryover value when dispatching bio Signed-off-by: zhuxiaohui <zhuxiaohui.400@bytedance.com> --- block/blk-throttle.c | 26 ++++++++++++++++++++++++++ block/blk-throttle.h | 4 ++-- 2 files changed, 28 insertions(+), 2 deletions(-)