| Message ID | 20250417015033.512940-4-wozizhi@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | blk-throttle: Some bugfixes and modifications | expand |
Hi, 在 2025/04/17 9:50, Zizhi Wo 写道: > Now the tg->[bytes/io]_disp type is signed, and calculate_bytes/io_allowed > return type is unsigned. Even if the bps/iops limit is not set to max, the > return value of the function may still exceed INT_MAX or LLONG_MAX, which > can cause overflow in outer variables. In such cases, we can add additional > checks accordingly. > > And in throtl_trim_slice(), if the BPS/IOPS limit is set to max, there's > no need to call calculate_bytes/io_allowed(). Introduces the helper > functions throtl_trim_bps/iops to simplifies the process. For cases when > the calculated trim value exceeds INT_MAX (causing an overflow), we reset > tg->[bytes/io]_disp to zero, so return original tg->[bytes/io]_disp because > it is the size that is actually trimmed. > > Signed-off-by: Zizhi Wo <wozizhi@huawei.com> > --- > block/blk-throttle.c | 94 ++++++++++++++++++++++++++++++++++---------- > 1 file changed, 73 insertions(+), 21 deletions(-) > > diff --git a/block/blk-throttle.c b/block/blk-throttle.c > index 4e4609dce85d..cfb9c47d1af7 100644 > --- a/block/blk-throttle.c > +++ b/block/blk-throttle.c > @@ -571,6 +571,56 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed) > return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ); > } > > +static long long throtl_trim_bps(struct throtl_grp *tg, bool rw, > + unsigned long time_elapsed) > +{ > + u64 bps_limit = tg_bps_limit(tg, rw); > + long long bytes_trim; > + > + if (bps_limit == U64_MAX) > + return 0; > + > + /* Need to consider the case of bytes_allowed overflow. */ > + bytes_trim = calculate_bytes_allowed(bps_limit, time_elapsed); > + if (bytes_trim <= 0) { > + bytes_trim = tg->bytes_disp[rw]; > + tg->bytes_disp[rw] = 0; > + goto out; > + } > + > + if (tg->bytes_disp[rw] >= bytes_trim) > + tg->bytes_disp[rw] -= bytes_trim; > + else > + tg->bytes_disp[rw] = 0; Perhaps if (bytes_trim <=0 || tg->bytes_disp[rw] < bytes_trim) { bytes_trim = tg->bytes_disp[rw]; tg->bytes_disp[rw] = 0; } else { tg->bytes_disp[rw] -= bytes_trim; } And you don't need the out tag. > +out: > + return bytes_trim; > +} > + > +static int throtl_trim_iops(struct throtl_grp *tg, bool rw, > + unsigned long time_elapsed) > +{ > + u32 iops_limit = tg_iops_limit(tg, rw); > + int io_trim; > + > + if (iops_limit == UINT_MAX) > + return 0; > + > + /* Need to consider the case of io_allowed overflow. */ > + io_trim = calculate_io_allowed(iops_limit, time_elapsed); > + if (io_trim <= 0) { > + io_trim = tg->io_disp[rw]; > + tg->io_disp[rw] = 0; > + goto out; > + } > + > + if (tg->io_disp[rw] >= io_trim) > + tg->io_disp[rw] -= io_trim; > + else > + tg->io_disp[rw] = 0; > +out: > + return io_trim; Same as above. Thanks, Kuai > +} > + > /* Trim the used slices and adjust slice start accordingly */ > static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw) > { > @@ -612,22 +662,11 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw) > * one extra slice is preserved for deviation. > */ > time_elapsed -= tg->td->throtl_slice; > - bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw), > - time_elapsed); > - io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed); > - if (bytes_trim <= 0 && io_trim <= 0) > + bytes_trim = throtl_trim_bps(tg, rw, time_elapsed); > + io_trim = throtl_trim_iops(tg, rw, time_elapsed); > + if (!bytes_trim && !io_trim) > return; > > - if ((long long)tg->bytes_disp[rw] >= bytes_trim) > - tg->bytes_disp[rw] -= bytes_trim; > - else > - tg->bytes_disp[rw] = 0; > - > - if ((int)tg->io_disp[rw] >= io_trim) > - tg->io_disp[rw] -= io_trim; > - else > - tg->io_disp[rw] = 0; > - > tg->slice_start[rw] += time_elapsed; > > throtl_log(&tg->service_queue, > @@ -643,6 +682,8 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, > unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw]; > u64 bps_limit = tg_bps_limit(tg, rw); > u32 iops_limit = tg_iops_limit(tg, rw); > + long long bytes_allowed; > + int io_allowed; > > /* > * If the queue is empty, carryover handling is not needed. In such cases, > @@ -661,19 +702,28 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, > * accumulate how many bytes/ios are waited across changes. And use the > * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, which > * will be used to calculate new wait time under new configuration. > + * And we need to consider the case of bytes/io_allowed overflow. > */ > if (bps_limit != U64_MAX) { > - *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - > - tg->bytes_disp[rw]; > - tg->bytes_disp[rw] = -*bytes; > + bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed); > + if (bytes_allowed < 0) { > + tg->bytes_disp[rw] = 0; > + } else { > + *bytes = bytes_allowed - tg->bytes_disp[rw]; > + tg->bytes_disp[rw] = -*bytes; > + } > } else { > tg->bytes_disp[rw] = 0; > } > > if (iops_limit != UINT_MAX) { > - *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) - > - tg->io_disp[rw]; > - tg->io_disp[rw] = -*ios; > + io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed); > + if (io_allowed < 0) { > + tg->io_disp[rw] = 0; > + } else { > + *ios = io_allowed - tg->io_disp[rw]; > + tg->io_disp[rw] = -*ios; > + } > } else { > tg->io_disp[rw] = 0; > } > @@ -741,7 +791,9 @@ static unsigned long tg_within_bps_limit(struct throtl_grp *tg, struct bio *bio, > > jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice); > bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd); > - if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed) > + /* Need to consider the case of bytes_allowed overflow. */ > + if ((bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed) > + || bytes_allowed < 0) > return 0; > > /* Calc approx time to dispatch */ >
在 2025/4/17 10:39, Yu Kuai 写道: > Hi, > > 在 2025/04/17 9:50, Zizhi Wo 写道: >> Now the tg->[bytes/io]_disp type is signed, and >> calculate_bytes/io_allowed >> return type is unsigned. Even if the bps/iops limit is not set to max, >> the >> return value of the function may still exceed INT_MAX or LLONG_MAX, which >> can cause overflow in outer variables. In such cases, we can add >> additional >> checks accordingly. >> >> And in throtl_trim_slice(), if the BPS/IOPS limit is set to max, there's >> no need to call calculate_bytes/io_allowed(). Introduces the helper >> functions throtl_trim_bps/iops to simplifies the process. For cases when >> the calculated trim value exceeds INT_MAX (causing an overflow), we reset >> tg->[bytes/io]_disp to zero, so return original tg->[bytes/io]_disp >> because >> it is the size that is actually trimmed. >> >> Signed-off-by: Zizhi Wo <wozizhi@huawei.com> >> --- >> block/blk-throttle.c | 94 ++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 73 insertions(+), 21 deletions(-) >> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c >> index 4e4609dce85d..cfb9c47d1af7 100644 >> --- a/block/blk-throttle.c >> +++ b/block/blk-throttle.c >> @@ -571,6 +571,56 @@ static u64 calculate_bytes_allowed(u64 bps_limit, >> unsigned long jiffy_elapsed) >> return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ); >> } >> +static long long throtl_trim_bps(struct throtl_grp *tg, bool rw, >> + unsigned long time_elapsed) >> +{ >> + u64 bps_limit = tg_bps_limit(tg, rw); >> + long long bytes_trim; >> + >> + if (bps_limit == U64_MAX) >> + return 0; >> + >> + /* Need to consider the case of bytes_allowed overflow. */ >> + bytes_trim = calculate_bytes_allowed(bps_limit, time_elapsed); >> + if (bytes_trim <= 0) { >> + bytes_trim = tg->bytes_disp[rw]; >> + tg->bytes_disp[rw] = 0; >> + goto out; >> + } >> + >> + if (tg->bytes_disp[rw] >= bytes_trim) >> + tg->bytes_disp[rw] -= bytes_trim; >> + else >> + tg->bytes_disp[rw] = 0; > > Perhaps > > if (bytes_trim <=0 || tg->bytes_disp[rw] < bytes_trim) { > bytes_trim = tg->bytes_disp[rw]; > tg->bytes_disp[rw] = 0; > } else { > tg->bytes_disp[rw] -= bytes_trim; > } > > And you don't need the out tag. >> +out: >> + return bytes_trim; >> +} >> + Yeah, it's definitely simpler. I'll send out the second version shortly. Thanks, Zizhi Wo >> +static int throtl_trim_iops(struct throtl_grp *tg, bool rw, >> + unsigned long time_elapsed) >> +{ >> + u32 iops_limit = tg_iops_limit(tg, rw); >> + int io_trim; >> + >> + if (iops_limit == UINT_MAX) >> + return 0; >> + >> + /* Need to consider the case of io_allowed overflow. */ >> + io_trim = calculate_io_allowed(iops_limit, time_elapsed); >> + if (io_trim <= 0) { >> + io_trim = tg->io_disp[rw]; >> + tg->io_disp[rw] = 0; >> + goto out; >> + } >> + >> + if (tg->io_disp[rw] >= io_trim) >> + tg->io_disp[rw] -= io_trim; >> + else >> + tg->io_disp[rw] = 0; >> +out: >> + return io_trim; > > Same as above. > > Thanks, > Kuai > >> +} >> + >> /* Trim the used slices and adjust slice start accordingly */ >> static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw) >> { >> @@ -612,22 +662,11 @@ static inline void throtl_trim_slice(struct >> throtl_grp *tg, bool rw) >> * one extra slice is preserved for deviation. >> */ >> time_elapsed -= tg->td->throtl_slice; >> - bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw), >> - time_elapsed); >> - io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed); >> - if (bytes_trim <= 0 && io_trim <= 0) >> + bytes_trim = throtl_trim_bps(tg, rw, time_elapsed); >> + io_trim = throtl_trim_iops(tg, rw, time_elapsed); >> + if (!bytes_trim && !io_trim) >> return; >> - if ((long long)tg->bytes_disp[rw] >= bytes_trim) >> - tg->bytes_disp[rw] -= bytes_trim; >> - else >> - tg->bytes_disp[rw] = 0; >> - >> - if ((int)tg->io_disp[rw] >= io_trim) >> - tg->io_disp[rw] -= io_trim; >> - else >> - tg->io_disp[rw] = 0; >> - >> tg->slice_start[rw] += time_elapsed; >> throtl_log(&tg->service_queue, >> @@ -643,6 +682,8 @@ static void __tg_update_carryover(struct >> throtl_grp *tg, bool rw, >> unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw]; >> u64 bps_limit = tg_bps_limit(tg, rw); >> u32 iops_limit = tg_iops_limit(tg, rw); >> + long long bytes_allowed; >> + int io_allowed; >> /* >> * If the queue is empty, carryover handling is not needed. In >> such cases, >> @@ -661,19 +702,28 @@ static void __tg_update_carryover(struct >> throtl_grp *tg, bool rw, >> * accumulate how many bytes/ios are waited across changes. And >> use the >> * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, >> which >> * will be used to calculate new wait time under new configuration. >> + * And we need to consider the case of bytes/io_allowed overflow. >> */ >> if (bps_limit != U64_MAX) { >> - *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - >> - tg->bytes_disp[rw]; >> - tg->bytes_disp[rw] = -*bytes; >> + bytes_allowed = calculate_bytes_allowed(bps_limit, >> jiffy_elapsed); >> + if (bytes_allowed < 0) { >> + tg->bytes_disp[rw] = 0; >> + } else { >> + *bytes = bytes_allowed - tg->bytes_disp[rw]; >> + tg->bytes_disp[rw] = -*bytes; >> + } >> } else { >> tg->bytes_disp[rw] = 0; >> } >> if (iops_limit != UINT_MAX) { >> - *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) - >> - tg->io_disp[rw]; >> - tg->io_disp[rw] = -*ios; >> + io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed); >> + if (io_allowed < 0) { >> + tg->io_disp[rw] = 0; >> + } else { >> + *ios = io_allowed - tg->io_disp[rw]; >> + tg->io_disp[rw] = -*ios; >> + } >> } else { >> tg->io_disp[rw] = 0; >> } >> @@ -741,7 +791,9 @@ static unsigned long tg_within_bps_limit(struct >> throtl_grp *tg, struct bio *bio, >> jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, >> tg->td->throtl_slice); >> bytes_allowed = calculate_bytes_allowed(bps_limit, >> jiffy_elapsed_rnd); >> - if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= >> bytes_allowed) >> + /* Need to consider the case of bytes_allowed overflow. */ >> + if ((bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= >> bytes_allowed) >> + || bytes_allowed < 0) >> return 0; >> /* Calc approx time to dispatch */ >> >
diff --git a/block/blk-throttle.c b/block/blk-throttle.c index 4e4609dce85d..cfb9c47d1af7 100644 --- a/block/blk-throttle.c +++ b/block/blk-throttle.c @@ -571,6 +571,56 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed) return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ); } +static long long throtl_trim_bps(struct throtl_grp *tg, bool rw, + unsigned long time_elapsed) +{ + u64 bps_limit = tg_bps_limit(tg, rw); + long long bytes_trim; + + if (bps_limit == U64_MAX) + return 0; + + /* Need to consider the case of bytes_allowed overflow. */ + bytes_trim = calculate_bytes_allowed(bps_limit, time_elapsed); + if (bytes_trim <= 0) { + bytes_trim = tg->bytes_disp[rw]; + tg->bytes_disp[rw] = 0; + goto out; + } + + if (tg->bytes_disp[rw] >= bytes_trim) + tg->bytes_disp[rw] -= bytes_trim; + else + tg->bytes_disp[rw] = 0; +out: + return bytes_trim; +} + +static int throtl_trim_iops(struct throtl_grp *tg, bool rw, + unsigned long time_elapsed) +{ + u32 iops_limit = tg_iops_limit(tg, rw); + int io_trim; + + if (iops_limit == UINT_MAX) + return 0; + + /* Need to consider the case of io_allowed overflow. */ + io_trim = calculate_io_allowed(iops_limit, time_elapsed); + if (io_trim <= 0) { + io_trim = tg->io_disp[rw]; + tg->io_disp[rw] = 0; + goto out; + } + + if (tg->io_disp[rw] >= io_trim) + tg->io_disp[rw] -= io_trim; + else + tg->io_disp[rw] = 0; +out: + return io_trim; +} + /* Trim the used slices and adjust slice start accordingly */ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw) { @@ -612,22 +662,11 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw) * one extra slice is preserved for deviation. */ time_elapsed -= tg->td->throtl_slice; - bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw), - time_elapsed); - io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed); - if (bytes_trim <= 0 && io_trim <= 0) + bytes_trim = throtl_trim_bps(tg, rw, time_elapsed); + io_trim = throtl_trim_iops(tg, rw, time_elapsed); + if (!bytes_trim && !io_trim) return; - if ((long long)tg->bytes_disp[rw] >= bytes_trim) - tg->bytes_disp[rw] -= bytes_trim; - else - tg->bytes_disp[rw] = 0; - - if ((int)tg->io_disp[rw] >= io_trim) - tg->io_disp[rw] -= io_trim; - else - tg->io_disp[rw] = 0; - tg->slice_start[rw] += time_elapsed; throtl_log(&tg->service_queue, @@ -643,6 +682,8 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw]; u64 bps_limit = tg_bps_limit(tg, rw); u32 iops_limit = tg_iops_limit(tg, rw); + long long bytes_allowed; + int io_allowed; /* * If the queue is empty, carryover handling is not needed. In such cases, @@ -661,19 +702,28 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, * accumulate how many bytes/ios are waited across changes. And use the * calculated carryover (@bytes/@ios) to update [bytes/io]_disp, which * will be used to calculate new wait time under new configuration. + * And we need to consider the case of bytes/io_allowed overflow. */ if (bps_limit != U64_MAX) { - *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - - tg->bytes_disp[rw]; - tg->bytes_disp[rw] = -*bytes; + bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed); + if (bytes_allowed < 0) { + tg->bytes_disp[rw] = 0; + } else { + *bytes = bytes_allowed - tg->bytes_disp[rw]; + tg->bytes_disp[rw] = -*bytes; + } } else { tg->bytes_disp[rw] = 0; } if (iops_limit != UINT_MAX) { - *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) - - tg->io_disp[rw]; - tg->io_disp[rw] = -*ios; + io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed); + if (io_allowed < 0) { + tg->io_disp[rw] = 0; + } else { + *ios = io_allowed - tg->io_disp[rw]; + tg->io_disp[rw] = -*ios; + } } else { tg->io_disp[rw] = 0; } @@ -741,7 +791,9 @@ static unsigned long tg_within_bps_limit(struct throtl_grp *tg, struct bio *bio, jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice); bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd); - if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed) + /* Need to consider the case of bytes_allowed overflow. */ + if ((bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed) + || bytes_allowed < 0) return 0; /* Calc approx time to dispatch */
Now the tg->[bytes/io]_disp type is signed, and calculate_bytes/io_allowed return type is unsigned. Even if the bps/iops limit is not set to max, the return value of the function may still exceed INT_MAX or LLONG_MAX, which can cause overflow in outer variables. In such cases, we can add additional checks accordingly. And in throtl_trim_slice(), if the BPS/IOPS limit is set to max, there's no need to call calculate_bytes/io_allowed(). Introduces the helper functions throtl_trim_bps/iops to simplifies the process. For cases when the calculated trim value exceeds INT_MAX (causing an overflow), we reset tg->[bytes/io]_disp to zero, so return original tg->[bytes/io]_disp because it is the size that is actually trimmed. Signed-off-by: Zizhi Wo <wozizhi@huawei.com> --- block/blk-throttle.c | 94 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 73 insertions(+), 21 deletions(-)