| Message ID | 20200911224049.GA865564@mtj.thefacebook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,block/for-next] blk-iocost: fix divide-by-zero in transfer_surpluses() | expand |
On 9/11/20 4:40 PM, Tejun Heo wrote: > Conceptually, root_iocg->hweight_donating must be less than WEIGHT_ONE but > all hweight calculations round up and thus it may end up >= WEIGHT_ONE > triggering divide-by-zero and other issues. Bound the value to avoid > surprises. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Fixes: e08d02aa5fc9 ("blk-iocost: implement Andy's method for donation weight updates") > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > Jens, I was flipping between doing max_t(, 1) over the whole divider and > doing min_t(, WEIGHT_ONE - 1) for hweight_donating. I thought that I as > testing after the last change but it obviously wasn't and the previous patch > doesn't compile due to missing type argument. Can you please apply this > patch instead? I can send an incremental patch if that'd be better. My > apologies. Sure, I replaced it. BTW, you had two signed-off-by's in there.
On Fri, Sep 11, 2020 at 04:43:18PM -0600, Jens Axboe wrote: > On 9/11/20 4:40 PM, Tejun Heo wrote: > > Conceptually, root_iocg->hweight_donating must be less than WEIGHT_ONE but > > all hweight calculations round up and thus it may end up >= WEIGHT_ONE > > triggering divide-by-zero and other issues. Bound the value to avoid > > surprises. > > > > Signed-off-by: Tejun Heo <tj@kernel.org> > > Fixes: e08d02aa5fc9 ("blk-iocost: implement Andy's method for donation weight updates") > > > > Signed-off-by: Tejun Heo <tj@kernel.org> > > --- > > Jens, I was flipping between doing max_t(, 1) over the whole divider and > > doing min_t(, WEIGHT_ONE - 1) for hweight_donating. I thought that I as > > testing after the last change but it obviously wasn't and the previous patch > > doesn't compile due to missing type argument. Can you please apply this > > patch instead? I can send an incremental patch if that'd be better. My > > apologies. > > Sure, I replaced it. BTW, you had two signed-off-by's in there. Yeah, sloppy all around. Sorry about that. Gotta slow down more when posting oh-shit-I-fucked-up stuff. Thanks.
diff --git a/block/blk-iocost.c b/block/blk-iocost.c index fc897bb142bcd..6e29b4dcf3566 100644 --- a/block/blk-iocost.c +++ b/block/blk-iocost.c @@ -1881,15 +1881,21 @@ static void transfer_surpluses(struct list_head *surpluses, struct ioc_now *now) /* * Calculate the global donation rate (gamma) - the rate to adjust - * non-donating budgets by. No need to use 64bit multiplication here as - * the first operand is guaranteed to be smaller than WEIGHT_ONE - * (1<<16). + * non-donating budgets by. + * + * No need to use 64bit multiplication here as the first operand is + * guaranteed to be smaller than WEIGHT_ONE (1<<16). + * + * We know that there are beneficiary nodes and the sum of the donating + * hweights can't be whole; however, due to the round-ups during hweight + * calculations, root_iocg->hweight_donating might still end up equal to + * or greater than whole. Limit the range when calculating the divider. * * gamma = (1 - t_r') / (1 - t_r) */ gamma = DIV_ROUND_UP( (WEIGHT_ONE - root_iocg->hweight_after_donation) * WEIGHT_ONE, - WEIGHT_ONE - root_iocg->hweight_donating); + WEIGHT_ONE - min_t(u32, root_iocg->hweight_donating, WEIGHT_ONE - 1)); /* * Calculate adjusted hwi, child_adjusted_sum and inuse for the inner