From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49237) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwAHM-0000p2-Sq for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:56:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwAHL-00065Q-Vl for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:56:44 -0400 References: <65c04b17fb38a4d89a599d6d52a1a3f621388d55.1476718804.git.berto@igalia.com> From: Paolo Bonzini Message-ID: <7f03e7a1-5df6-8eff-686f-b4e6efaecdee@redhat.com> Date: Mon, 17 Oct 2016 17:56:33 +0200 MIME-Version: 1.0 In-Reply-To: <65c04b17fb38a4d89a599d6d52a1a3f621388d55.1476718804.git.berto@igalia.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] throttle: Correct access to wrong BlockBackendPublic structures List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, qemu-stable@nongnu.org, Kevin Wolf 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 > Signed-off-by: Alberto Garcia > --- > 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