Re: [Qemu-devel] [PATCH RFC] block: add throttle block filter driver

[Stefan Hajnoczi]

On Wed, Jun 07, 2017 at 04:06:19PM +0300, Manos Pitsidianakis wrote:
> +#define QEMU_OPT_IOPS_TOTAL "iops-total"
> +#define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
> +#define QEMU_OPT_IOPS_TOTAL_MAX_LENGTH "iops-total-max-length"
> +#define QEMU_OPT_IOPS_READ "iops-read"
> +#define QEMU_OPT_IOPS_READ_MAX "iops-read-max"
> +#define QEMU_OPT_IOPS_READ_MAX_LENGTH "iops-read-max-length"
> +#define QEMU_OPT_IOPS_WRITE "iops-write"
> +#define QEMU_OPT_IOPS_WRITE_MAX "iops-write-max"
> +#define QEMU_OPT_IOPS_WRITE_MAX_LENGTH "iops-write-max-length"
> +#define QEMU_OPT_BPS_TOTAL "bps-total"
> +#define QEMU_OPT_BPS_TOTAL_MAX "bps-total-max"
> +#define QEMU_OPT_BPS_TOTAL_MAX_LENGTH "bps-total-max-length"
> +#define QEMU_OPT_BPS_READ "bps-read"
> +#define QEMU_OPT_BPS_READ_MAX "bps-read-max"
> +#define QEMU_OPT_BPS_READ_MAX_LENGTH "bps-read-max-length"
> +#define QEMU_OPT_BPS_WRITE "bps-write"
> +#define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
> +#define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
> +#define QEMU_OPT_IOPS_SIZE "iops-size"
> +#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"

Please reuse include/qemu/throttle-options.h.

> +static QemuMutex throttle_groups_lock;
> +static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
> +    QTAILQ_HEAD_INITIALIZER(throttle_groups);

Please reuse block/throttle-groups.c.  I think throttle-groups.c doesn't
actually need BlockBackend so you can make the code more general.

Define the necessary state:

  typedef struct ThrottleGroupMember {
    /* Protected by AioContext lock.  */
    CoQueue      throttled_reqs[2];

    /* Nonzero if the I/O limits are currently being ignored; generally
     * it is zero.  */
    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;

Then refactor throttle-groups.c to use ThrottleGroupMember instead of
BlockBackend(Public).

> diff --git a/include/block/throttle.h b/include/block/throttle.h
> new file mode 100644
> index 0000000000..fed24f02a6
> --- /dev/null
> +++ b/include/block/throttle.h

What is the purpose of this header?

Header files in include/* are "public" APIs that other parts of QEMU can
use.  There is no user other than block/throttle.c so please keep it
private in the .c file.

> @@ -0,0 +1,59 @@
> +/* The ThrottleGroup structure (with its ThrottleState) is shared
> + * among different BlockDriverStates and it's independent from
> + * AioContext, so in order to use it from different threads it needs
> + * its own locking.
> + *
> + * This locking is however handled internally in block/throttle.c so it's
> + * transparent to outside users.
> + *
> + * The whole ThrottleGroup structure is private and invisible to
> + * outside users, that only use it through its ThrottleState.
> + *
> + * In addition to the ThrottleGroup structure, BlockDriverState has
> + * fields that need to be accessed by other members of the group and
> + * therefore also need to be protected by this lock. Once a
> + * BlockDriverState is registered in a group those fields can be accessed
> + * by other threads any time.
> + *
> + * Again, all this is handled internally in block/throttle.c and is mostly
> + * transparent to the outside. The 'throttle_timers' field however has an
> + * additional constraint because it may be temporarily invalid (see for 
> example
> + * bdrv_set_aio_context()). Therefore block/throttle.c will access some
> + * other BlockDriverState's timers only after verifying that that BDS has
> + * throttled requests in the queue.
> + */
> +
> +typedef struct ThrottleGroup {
> +    char *name; /* This is constant during the lifetime of the group */
> +
> +    QemuMutex lock; /* This lock protects the following four fields */
> +    ThrottleState ts;
> +    QLIST_HEAD(, BDRVThrottleNodeState) head;
> +    struct BDRVThrottleNodeState *tokens[2];
> +    bool any_timer_armed[2];
> +
> +    /* These two are protected by the global throttle_groups_lock */
> +    unsigned refcount;
> +    QTAILQ_ENTRY(ThrottleGroup) list;
> +} ThrottleGroup;
> +
> +typedef struct BDRVThrottleNodeState {
> +    ThrottleGroup *throttle_group;
> +
> +    /* I/O throttling has its own locking, but also some fields are
> +     * protected by the AioContext lock.
> +     */
> +
> +    /* Protected by AioContext lock.  */
> +    CoQueue      throttled_reqs[2];
> +
> +    /* Nonzero if the I/O limits are currently being ignored; generally
> +     * it is zero.  */
> +    unsigned int io_limits_disabled;
> +
> +    ThrottleState *throttle_state;
> +    ThrottleTimers throttle_timers;
> +    unsigned       pending_reqs[2];
> +    QLIST_ENTRY(BDRVThrottleNodeState) round_robin;
> +
> +} BDRVThrottleNodeState;
> -- 
> 2.11.0
>

signature.asc
Description: PGP signature