[02/16] block: move restarting of throttled reqs to block/throttle-groups.c

We want to remove throttled_reqs from block/io.c.  This is the easy
part---hide the handling of throttled_reqs during disable/enable of
throttling within throttle-groups.c.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/io.c                      | 15 +--------------
 block/throttle-groups.c         | 15 +++++++++++++++
 include/block/throttle-groups.h |  1 +
 3 files changed, 17 insertions(+), 14 deletions(-)

On Tue, 02/16 18:56, Paolo Bonzini wrote:
> We want to remove throttled_reqs from block/io.c.  This is the easy
> part---hide the handling of throttled_reqs during disable/enable of
> throttling within throttle-groups.c.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/io.c                      | 15 +--------------
>  block/throttle-groups.c         | 15 +++++++++++++++
>  include/block/throttle-groups.h |  1 +
>  3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index e58cfe2..5ee5032 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -66,28 +66,15 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>  void bdrv_set_io_limits(BlockDriverState *bs,
>                          ThrottleConfig *cfg)
>  {
> -    int i;
> -
>      throttle_group_config(bs, cfg);
> -
> -    for (i = 0; i < 2; i++) {
> -        qemu_co_enter_next(&bs->throttled_reqs[i]);
> -    }
>  }
>  
>  static void bdrv_start_throttled_reqs(BlockDriverState *bs)
>  {
>      bool enabled = bs->io_limits_enabled;
> -    int i;
>  
>      bs->io_limits_enabled = false;
> -
> -    for (i = 0; i < 2; i++) {
> -        while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
> -            ;
> -        }
> -    }
> -
> +    throttle_group_restart_bs(bs);
>      bs->io_limits_enabled = enabled;
>  }
>  
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 4920e09..eccfc0d 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -313,6 +313,17 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
>      qemu_mutex_unlock(&tg->lock);
>  }
>  
> +void throttle_group_restart_bs(BlockDriverState *bs)
> +{
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
> +            ;
> +        }
> +    }
> +}
> +
>  /* Update the throttle configuration for a particular group. Similar
>   * to throttle_config(), but guarantees atomicity within the
>   * throttling group.
> @@ -335,6 +346,10 @@ void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
>      }
>      throttle_config(ts, tt, cfg);
>      qemu_mutex_unlock(&tg->lock);
> +
> +    aio_context_acquire(bdrv_get_aio_context(bs));
> +    throttle_group_restart_bs(bs);
> +    aio_context_release(bdrv_get_aio_context(bs));

Could you explain why does this hunk belong to this patch?

Otherwise looks good.

Fam

>  }
>  
>  /* Get the throttle configuration from a particular group. Similar to
> diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
> index aba28f3..395f72d 100644
> --- a/include/block/throttle-groups.h
> +++ b/include/block/throttle-groups.h
> @@ -38,6 +38,7 @@ void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);
>  
>  void throttle_group_register_bs(BlockDriverState *bs, const char *groupname);
>  void throttle_group_unregister_bs(BlockDriverState *bs);
> +void throttle_group_restart_bs(BlockDriverState *bs);
>  
>  void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
>                                                          unsigned int bytes,
> -- 
> 2.5.0
> 
> 
>

On 09/03/2016 02:26, Fam Zheng wrote:
>> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
>> index 4920e09..eccfc0d 100644
>> --- a/block/throttle-groups.c
>> +++ b/block/throttle-groups.c
>> @@ -313,6 +313,17 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
>>      qemu_mutex_unlock(&tg->lock);
>>  }
>>  
>> +void throttle_group_restart_bs(BlockDriverState *bs)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
>> +            ;
>> +        }
>> +    }
>> +}
>> +
>>  /* Update the throttle configuration for a particular group. Similar
>>   * to throttle_config(), but guarantees atomicity within the
>>   * throttling group.
>> @@ -335,6 +346,10 @@ void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
>>      }
>>      throttle_config(ts, tt, cfg);
>>      qemu_mutex_unlock(&tg->lock);
>> +
>> +    aio_context_acquire(bdrv_get_aio_context(bs));
>> +    throttle_group_restart_bs(bs);
>> +    aio_context_release(bdrv_get_aio_context(bs));
> 
> Could you explain why does this hunk belong to this patch?
> 
> Otherwise looks good.

Sure.  It's moved from bdrv_set_io_limits, which calls
throttle_group_config, to throttle_group_config itself:

>>  void bdrv_set_io_limits(BlockDriverState *bs,
>>                          ThrottleConfig *cfg)
>>  {
>> -    int i;
>> -
>>      throttle_group_config(bs, cfg);
>> -
>> -    for (i = 0; i < 2; i++) {
>> -        qemu_co_enter_next(&bs->throttled_reqs[i]);
>> -    }
>>  }
>>  

But in v2 I'll change it to only restart the first request so there is
no semantic change.

Paolo