Messages in this thread | | | Subject | Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio | From | Yu Kuai <> | Date | Mon, 22 Aug 2022 11:06:44 +0800 |
| |
Hi,
在 2022/08/18 9:23, Yu Kuai 写道: > Hi, Tejun! > > 在 2022/08/18 1:50, Tejun Heo 写道: >> Hello, >> >> On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote: >>>> So, as a fix for the immediate problem, I guess this might do but >>>> this feels >>>> really fragile. How can we be certain that re-entering only happens >>>> because >>>> of splitting? What if future core development changes that? It seems >>>> to be >>>> solving the problem in the wrong place. Shouldn't we flag the bio >>>> indicating >>>> that it's split when we're splitting the bio so that we only limit >>>> them for >>>> iops in the first place? >>> >>> Splited bio is tracked in __bio_clone: >> >> As the word is used in commit messages and comments, the past perfect >> form >> of the verb "split" is "split". It looks like "splitted" is used in rare >> cases but dictionary says it's an archaic form. > > Ok, thanks for pointing it out, I'll change that in next iteration. >> >>> if (bio_flagged(bio_src, BIO_THROTTLED)) >>> bio_set_flag(bio, BIO_THROTTLED);
While implementing the new method, I found that there seems to be a misunderstanding here, the code seems to try to add flag to split bio so that it won't be throttled again for bps limit, however:
1) for blk throttle, split bio is issued directly and will never be throttled again, while orignal bio will go through throttle path again. 2) if cloned bio is directed to a new disk, the flag is cleared anyway. >>> >>> And currenty, the iops limit and bps limit are treated differently, >>> however there are only one flag 'BIO_THROTTLED' and they can't be >>> distinguished. >>> >>> Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and >>> BIO_BPS_THROTTLED, this way only iops limit can be handled and bps >>> limit can be skipped for splited bio. >>> >>> What do you think? >> >> I think the code would be a lot more intuitive and less fragile if we >> used >> two flags but the bits in the bi_flags field are a scarce resource >> unfortunately. Even then, I think the right thing to do here is using two >> flags. > > Yes, the field 'bio->bi_flags' is unsigned short, and there are only two > bits left. I'll use the new sulution which will acquire a new bit. > > Thanks, > Kuai >> >> Thanks. >> > > . >
| |