Re: [PATCH RESEND v6 1/8] blk-throttle: fix that io throttle can only work for single bio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





在 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux