Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH RESEND v6 1/8] blk-throttle: fix that io throttle can only work for single bio | From | Yu Kuai <> | Date | Sat, 30 Jul 2022 09:06:37 +0800 |
| |
在 2022/07/30 2:04, Tejun Heo 写道: > Hello, > > On Fri, Jul 29, 2022 at 02:32:36PM +0800, Yu Kuai wrote: >> We need to make sure following conditions is always hold: >> >> 1) If a bio is splited, iops limits should count multiple times, while >> bps limits should only count once. >> 2) If a bio is issued while some bios are already throttled, bps limits >> should not be ignored. >> >> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit") >> fixes that 1) is not hold, while it breaks 2). Root cause is that such >> bio will be flaged in __blk_throtl_bio(), and later >> tg_with_in_bps_limit() will skip flaged bio. >> >> In order to fix this problem, at first, I change that flaged bio won't >> be skipped in tg_with_in_bps_limit(): >> >> - if (!bio_flagged(bio, BIO_THROTTLED)) { >> - tg->bytes_disp[rw] += bio_size; >> - tg->last_bytes_disp[rw] += bio_size; >> - } >> - >> + tg->bytes_disp[rw] += bio_size; >> + tg->last_bytes_disp[rw] += bio_size; >> >> However, this will break that bps limits should only count once. Thus I >> try to restore the overaccounting in __blk_throtl_bio() in such case: >> >> + if (bio_flagged(bio, BIO_THROTTLED)) { >> + unsigned int bio_size = throtl_bio_data_size(bio); >> + >> + if (tg->bytes_disp[rw] >= bio_size) >> + tg->bytes_disp[rw] -= bio_size; >> + if (tg->last_bytes_disp[rw] >= bio_size) >> + tg->last_bytes_disp[rw] -= bio_size; >> + } >> >> If new slice is not started, then the decrement should make sure this >> bio won't be counted again. However, if new slice is started and the >> condition 'bytes_disp >= bio_size' doesn't hold, this bio will end up >> accounting twice. >> >> Pleas let me know if you think this suituation is problematic, I'll try >> to figure out a new way... > > While a bit tricky, I think it's fine but please add comments in the code > explaining what's going on and why. Also, can you please explain why > __blk_throtl_bio() being skipped when iops limit is not set doesn't skew the > result?
Because bps limit is already counted the first time __blk_throtl_bio() is called for the orignal bio. When splited bio is reentered, we only need to throttle it again if iops limit is set.
By the way, I found that this way is better after patch 4:
in __blk_throtl_bio():
if (bio_flagged(bio, BIO_THROTTLED)) { tg->bytes_skipped[rw] += bio_size; if (tg->last_bytes_disp[rw] >= bio_size) tg->last_bytes_disp[rw] -= bio_size; }
The overaccounting can be restored even if new slice is started.
Thanks, Kuai
| ![\](/images/icornerr.gif) |