| Message ID | 20250417015033.512940-2-wozizhi@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | blk-throttle: Some bugfixes and modifications | expand |
On Thu, Apr 17, 2025 at 09:50:31AM +0800, Zizhi Wo wrote: > In commit 6cc477c36875 ("blk-throttle: carry over directly"), the carryover > bytes/ios was be carried to [bytes/io]_disp. However, its update mechanism > has some issues. > > In __tg_update_carryover(), we calculate "bytes" and "ios" to represent the > carryover, but the computation when updating [bytes/io]_disp is incorrect. > This patch fixes the issue. > > And if the bps/iops limit was previously set to max and later changed to a > smaller value, we may not update tg->[bytes/io]_disp to 0 in > tg_update_carryover(). Relying solely on throtl_trim_slice() is not > sufficient, which can lead to subsequent bio dispatches not behaving as > expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case. > The same handling applies when nr_queued is 0. > > Fixes: 6cc477c36875 ("blk-throttle: carry over directly") > Signed-off-by: Zizhi Wo <wozizhi@huawei.com> > --- > block/blk-throttle.c | 33 +++++++++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 8 deletions(-) > > diff --git a/block/blk-throttle.c b/block/blk-throttle.c > index 91dab43c65ab..df9825eb83be 100644 > --- a/block/blk-throttle.c > +++ b/block/blk-throttle.c > @@ -644,20 +644,39 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, > u64 bps_limit = tg_bps_limit(tg, rw); > u32 iops_limit = tg_iops_limit(tg, rw); > > + /* > + * If the queue is empty, carryover handling is not needed. In such cases, > + * tg->[bytes/io]_disp should be reset to 0 to avoid impacting the dispatch > + * of subsequent bios. The same handling applies when the previous BPS/IOPS > + * limit was set to max. > + */ > + if (tg->service_queue.nr_queued[rw] == 0) { > + tg->bytes_disp[rw] = 0; > + tg->io_disp[rw] = 0; > + return; > + } > + > /* > * If config is updated while bios are still throttled, calculate and > * accumulate how many bytes/ios are waited across changes. And > * carryover_bytes/ios will be used to calculate new wait time under new > * configuration. > */ > - if (bps_limit != U64_MAX) > + if (bps_limit != U64_MAX) { > *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - > tg->bytes_disp[rw]; > - if (iops_limit != UINT_MAX) > + tg->bytes_disp[rw] = -*bytes; > + } else { > + tg->bytes_disp[rw] = 0; > + } It should be fine to do 'tg->bytes_disp[rw] = -*bytes;' directly because `*bytes` is initialized as zero. > + > + if (iops_limit != UINT_MAX) { > *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) - > tg->io_disp[rw]; > - tg->bytes_disp[rw] -= *bytes; > - tg->io_disp[rw] -= *ios; > + tg->io_disp[rw] = -*ios; > + } else { > + tg->io_disp[rw] = 0; > + } Same with above. Otherwise, this patch looks fine. thanks, Ming
在 2025/4/17 10:27, Ming Lei 写道: > On Thu, Apr 17, 2025 at 09:50:31AM +0800, Zizhi Wo wrote: >> In commit 6cc477c36875 ("blk-throttle: carry over directly"), the carryover >> bytes/ios was be carried to [bytes/io]_disp. However, its update mechanism >> has some issues. >> >> In __tg_update_carryover(), we calculate "bytes" and "ios" to represent the >> carryover, but the computation when updating [bytes/io]_disp is incorrect. >> This patch fixes the issue. >> >> And if the bps/iops limit was previously set to max and later changed to a >> smaller value, we may not update tg->[bytes/io]_disp to 0 in >> tg_update_carryover(). Relying solely on throtl_trim_slice() is not >> sufficient, which can lead to subsequent bio dispatches not behaving as >> expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case. >> The same handling applies when nr_queued is 0. >> >> Fixes: 6cc477c36875 ("blk-throttle: carry over directly") >> Signed-off-by: Zizhi Wo <wozizhi@huawei.com> >> --- >> block/blk-throttle.c | 33 +++++++++++++++++++++++++-------- >> 1 file changed, 25 insertions(+), 8 deletions(-) >> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c >> index 91dab43c65ab..df9825eb83be 100644 >> --- a/block/blk-throttle.c >> +++ b/block/blk-throttle.c >> @@ -644,20 +644,39 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, >> u64 bps_limit = tg_bps_limit(tg, rw); >> u32 iops_limit = tg_iops_limit(tg, rw); >> >> + /* >> + * If the queue is empty, carryover handling is not needed. In such cases, >> + * tg->[bytes/io]_disp should be reset to 0 to avoid impacting the dispatch >> + * of subsequent bios. The same handling applies when the previous BPS/IOPS >> + * limit was set to max. >> + */ >> + if (tg->service_queue.nr_queued[rw] == 0) { >> + tg->bytes_disp[rw] = 0; >> + tg->io_disp[rw] = 0; >> + return; >> + } >> + >> /* >> * If config is updated while bios are still throttled, calculate and >> * accumulate how many bytes/ios are waited across changes. And >> * carryover_bytes/ios will be used to calculate new wait time under new >> * configuration. >> */ >> - if (bps_limit != U64_MAX) >> + if (bps_limit != U64_MAX) { >> *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - >> tg->bytes_disp[rw]; >> - if (iops_limit != UINT_MAX) >> + tg->bytes_disp[rw] = -*bytes; >> + } else { >> + tg->bytes_disp[rw] = 0; >> + } > > It should be fine to do 'tg->bytes_disp[rw] = -*bytes;' directly > because `*bytes` is initialized as zero. Indeed, I didn't notice that the incoming bytes/io is initialized to 0. Thanks, Zizhi Wo > >> + >> + if (iops_limit != UINT_MAX) { >> *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) - >> tg->io_disp[rw]; >> - tg->bytes_disp[rw] -= *bytes; >> - tg->io_disp[rw] -= *ios; >> + tg->io_disp[rw] = -*ios; >> + } else { >> + tg->io_disp[rw] = 0; >> + } > > Same with above. > > Otherwise, this patch looks fine. > > > thanks, > Ming > >
Hi, 在 2025/04/17 10:27, Ming Lei 写道: > On Thu, Apr 17, 2025 at 09:50:31AM +0800, Zizhi Wo wrote: >> In commit 6cc477c36875 ("blk-throttle: carry over directly"), the carryover >> bytes/ios was be carried to [bytes/io]_disp. However, its update mechanism >> has some issues. >> >> In __tg_update_carryover(), we calculate "bytes" and "ios" to represent the >> carryover, but the computation when updating [bytes/io]_disp is incorrect. >> This patch fixes the issue. >> >> And if the bps/iops limit was previously set to max and later changed to a >> smaller value, we may not update tg->[bytes/io]_disp to 0 in >> tg_update_carryover(). Relying solely on throtl_trim_slice() is not >> sufficient, which can lead to subsequent bio dispatches not behaving as >> expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case. >> The same handling applies when nr_queued is 0. >> >> Fixes: 6cc477c36875 ("blk-throttle: carry over directly") >> Signed-off-by: Zizhi Wo <wozizhi@huawei.com> >> --- >> block/blk-throttle.c | 33 +++++++++++++++++++++++++-------- >> 1 file changed, 25 insertions(+), 8 deletions(-) >> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c >> index 91dab43c65ab..df9825eb83be 100644 >> --- a/block/blk-throttle.c >> +++ b/block/blk-throttle.c >> @@ -644,20 +644,39 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, >> u64 bps_limit = tg_bps_limit(tg, rw); >> u32 iops_limit = tg_iops_limit(tg, rw); >> >> + /* >> + * If the queue is empty, carryover handling is not needed. In such cases, >> + * tg->[bytes/io]_disp should be reset to 0 to avoid impacting the dispatch >> + * of subsequent bios. The same handling applies when the previous BPS/IOPS >> + * limit was set to max. >> + */ >> + if (tg->service_queue.nr_queued[rw] == 0) { >> + tg->bytes_disp[rw] = 0; >> + tg->io_disp[rw] = 0; >> + return; >> + } >> + >> /* >> * If config is updated while bios are still throttled, calculate and >> * accumulate how many bytes/ios are waited across changes. And >> * carryover_bytes/ios will be used to calculate new wait time under new >> * configuration. >> */ >> - if (bps_limit != U64_MAX) >> + if (bps_limit != U64_MAX) { >> *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - >> tg->bytes_disp[rw]; >> - if (iops_limit != UINT_MAX) >> + tg->bytes_disp[rw] = -*bytes; >> + } else { >> + tg->bytes_disp[rw] = 0; >> + } > > It should be fine to do 'tg->bytes_disp[rw] = -*bytes;' directly > because `*bytes` is initialized as zero. It took me a while to understand, I think you mean if () *bytes =xxx; tg->bytes_disp[rw] = -*bytes; I'm good with or without this change. Reviewed-by: Yu Kuai <yukuai3@huawei.com> > >> + >> + if (iops_limit != UINT_MAX) { >> *ios = calculate_io_allowed(iops_limit, jiffy_elapsed) - >> tg->io_disp[rw]; >> - tg->bytes_disp[rw] -= *bytes; >> - tg->io_disp[rw] -= *ios; >> + tg->io_disp[rw] = -*ios; >> + } else { >> + tg->io_disp[rw] = 0; >> + } > > Same with above. > > Otherwise, this patch looks fine. > > > thanks, > Ming > > > . >
diff --git a/block/blk-throttle.c b/block/blk-throttle.c index 91dab43c65ab..df9825eb83be 100644 --- a/block/blk-throttle.c +++ b/block/blk-throttle.c @@ -644,20 +644,39 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw, u64 bps_limit = tg_bps_limit(tg, rw); u32 iops_limit = tg_iops_limit(tg, rw); + /* + * If the queue is empty, carryover handling is not needed. In such cases, + * tg->[bytes/io]_disp should be reset to 0 to avoid impacting the dispatch + * of subsequent bios. The same handling applies when the previous BPS/IOPS + * limit was set to max. + */ + if (tg->service_queue.nr_queued[rw] == 0) { + tg->bytes_disp[rw] = 0; + tg->io_disp[rw] = 0; + return; + } + /* * If config is updated while bios are still throttled, calculate and * accumulate how many bytes/ios are waited across changes. And * carryover_bytes/ios will be used to calculate new wait time under new * configuration. */ - if (bps_limit != U64_MAX) + if (bps_limit != U64_MAX) { *bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - tg->bytes_disp[rw]; - if (iops_limit != UINT_MAX) + 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->bytes_disp[rw] -= *bytes; - tg->io_disp[rw] -= *ios; + tg->io_disp[rw] = -*ios; + } else { + tg->io_disp[rw] = 0; + } } static void tg_update_carryover(struct throtl_grp *tg) @@ -665,10 +684,8 @@ static void tg_update_carryover(struct throtl_grp *tg) long long bytes[2] = {0}; int ios[2] = {0}; - if (tg->service_queue.nr_queued[READ]) - __tg_update_carryover(tg, READ, &bytes[READ], &ios[READ]); - if (tg->service_queue.nr_queued[WRITE]) - __tg_update_carryover(tg, WRITE, &bytes[WRITE], &ios[WRITE]); + __tg_update_carryover(tg, READ, &bytes[READ], &ios[READ]); + __tg_update_carryover(tg, WRITE, &bytes[WRITE], &ios[WRITE]); /* see comments in struct throtl_grp for meaning of these fields. */ throtl_log(&tg->service_queue, "%s: %lld %lld %d %d\n", __func__,
In commit 6cc477c36875 ("blk-throttle: carry over directly"), the carryover bytes/ios was be carried to [bytes/io]_disp. However, its update mechanism has some issues. In __tg_update_carryover(), we calculate "bytes" and "ios" to represent the carryover, but the computation when updating [bytes/io]_disp is incorrect. This patch fixes the issue. And if the bps/iops limit was previously set to max and later changed to a smaller value, we may not update tg->[bytes/io]_disp to 0 in tg_update_carryover(). Relying solely on throtl_trim_slice() is not sufficient, which can lead to subsequent bio dispatches not behaving as expected. We should set tg->[bytes/io]_disp to 0 in non_carryover case. The same handling applies when nr_queued is 0. Fixes: 6cc477c36875 ("blk-throttle: carry over directly") Signed-off-by: Zizhi Wo <wozizhi@huawei.com> --- block/blk-throttle.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)