On 17/10/2016 17:46, Alberto Garcia wrote: > In 27ccdd52598290f0f8b58be56e235aff7aebfaf3 the throttling fields were > moved from BlockDriverState to BlockBackend. However in a few cases > the code started using throttling fields from the active BlockBackend > instead of the round-robin token, making the algorithm behave > incorrectly. > > This can cause starvation if there's a throttling group with several > drives but only one of them has I/O. > > Reported-by: Paolo Bonzini <pbonz...@redhat.com> > Signed-off-by: Alberto Garcia <be...@igalia.com> > --- > block/throttle-groups.c | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/block/throttle-groups.c b/block/throttle-groups.c > index 59545e2..17b2efb 100644 > --- a/block/throttle-groups.c > +++ b/block/throttle-groups.c > @@ -168,6 +168,22 @@ static BlockBackend > *throttle_group_next_blk(BlockBackend *blk) > return blk_by_public(next); > } > > +/* > + * Return whether a BlockBackend has pending requests. > + * > + * This assumes that tg->lock is held. > + * > + * @blk: the BlockBackend > + * @is_write: the type of operation (read/write) > + * @ret: whether the BlockBackend has pending requests. > + */ > +static inline bool blk_has_pending_reqs(BlockBackend *blk, > + bool is_write) > +{ > + const BlockBackendPublic *blkp = blk_get_public(blk); > + return blkp->pending_reqs[is_write]; > +} > + > /* Return the next BlockBackend in the round-robin sequence with pending I/O > * requests. > * > @@ -188,7 +204,7 @@ static BlockBackend *next_throttle_token(BlockBackend > *blk, bool is_write) > > /* get next bs round in round robin style */ > token = throttle_group_next_blk(token); > - while (token != start && !blkp->pending_reqs[is_write]) { > + while (token != start && !blk_has_pending_reqs(token, is_write)) { > token = throttle_group_next_blk(token); > } > > @@ -196,10 +212,13 @@ static BlockBackend *next_throttle_token(BlockBackend > *blk, bool is_write) > * then decide the token is the current bs because chances are > * the current bs get the current request queued. > */ > - if (token == start && !blkp->pending_reqs[is_write]) { > + if (token == start && !blk_has_pending_reqs(token, is_write)) { > token = blk; > } > > + /* Either we return the original BB, or one with pending requests */ > + assert(token == blk || blk_has_pending_reqs(token, is_write));
Nice. :-) > return token; > } > > @@ -257,7 +276,7 @@ static void schedule_next_request(BlockBackend *blk, bool > is_write) > > /* Check if there's any pending request to schedule next */ > token = next_throttle_token(blk, is_write); > - if (!blkp->pending_reqs[is_write]) { > + if (!blk_has_pending_reqs(token, is_write)) { > return; > } > > @@ -271,7 +290,7 @@ static void schedule_next_request(BlockBackend *blk, bool > is_write) > qemu_co_queue_next(&blkp->throttled_reqs[is_write])) { > token = blk; > } else { > - ThrottleTimers *tt = &blkp->throttle_timers; > + ThrottleTimers *tt = &blk_get_public(token)->throttle_timers; > int64_t now = qemu_clock_get_ns(tt->clock_type); > timer_mod(tt->timers[is_write], now + 1); > tg->any_timer_armed[is_write] = true; > Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>