Re: [Qemu-devel] [PATCH 1/2] throttle: Correct access to wrong BlockBackendPublic structures

[Paolo Bonzini <pbonzini@redhat.com>]

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 <pbonzini@redhat.com>
> Signed-off-by: Alberto Garcia <berto@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 <pbonzini@redhat.com>