Re: [Qemu-block] [PATCH RFC v3 1/8] block: move ThrottleGroup membership to ThrottleGroupMember

[Alberto Garcia]

On Fri 23 Jun 2017 02:46:53 PM CEST, Manos Pitsidianakis wrote:
> This commit gathers ThrottleGroup membership details from
> BlockBackendPublic into ThrottleGroupMember and refactors existing code
> to use the structure.
>
> Signed-off-by: Manos Pitsidianakis <address@hidden>

Hey Manos, thanks for the patch. It looks good to me in general, I only
have a couple of comments:

>      /* If no IO are queued for scheduling on the next round robin token
> -     * then decide the token is the current bs because chances are
> -     * the current bs get the current request queued.
> +     * then decide the token is the current tgm because chances are
> +     * the current tgm get the current request queued.

I wonder if it's not better to use 'member' instead of 'tgm' in general.
My impression is that the former is clearer and not too long, but I
don't have a very strong opinion so you can keep it like this if you
want.

> -/* Check if an I/O request needs to be throttled, wait and set a timer
> - * if necessary, and schedule the next request using a round robin
> - * algorithm.
> +/* Check if an I/O request needs to be throttled, wait and set a timer if
> + * necessary, and schedule the next request using a round robin algorithm.
>   *

There's a few cases like this when you reformat a paragraph but don't
actually change the text. I think it just adds unnecessary noise to the
diff...

> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -27,6 +27,7 @@
>  
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/coroutine.h"
>  
>  #define THROTTLE_VALUE_MAX 1000000000000000LL
>  
> @@ -153,4 +154,29 @@ bool throttle_schedule_timer(ThrottleState *ts,
>  
>  void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
>  
> +
> +/* The ThrottleGroupMember structure indicates membership in a ThrottleGroup
> + * and holds related data.
> + */
> +
> +typedef struct ThrottleGroupMember {
> +    /* throttled_reqs_lock protects the CoQueues for throttled requests.  */
> +    CoMutex      throttled_reqs_lock;
> +    CoQueue      throttled_reqs[2];
> +
> +    /* Nonzero if the I/O limits are currently being ignored; generally
> +     * it is zero.  Accessed with atomic operations.
> +     */
> +    unsigned int io_limits_disabled;
> +
> +    /* The following fields are protected by the ThrottleGroup lock.
> +     * See the ThrottleGroup documentation for details.
> +     * throttle_state tells us if I/O limits are configured. */
> +    ThrottleState *throttle_state;
> +    ThrottleTimers throttle_timers;
> +    unsigned       pending_reqs[2];
> +    QLIST_ENTRY(ThrottleGroupMember) round_robin;
> +
> +} ThrottleGroupMember;
> +

Any reason why you add this to throttle.h (which is generic throttling
code independent from the block layer) and not to throttle-groups.h?

Otherwise it all looks good to me, thanks!

Berto