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

[Manos Pitsidianakis]

On Thu, Jun 08, 2017 at 08:22:28AM +0100, Stefan Hajnoczi wrote:
> 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.

This would make the options too verbose, ie -drive 
driver=throttle,file=..,throttling.iops-total=..,throttling. Eventually 
of course throttle-options.h will be replaced.

However, this can't be done at this point anyway, since the 
throttling.[...] options are consumed by the already existing throttling 
code, and are unavailable for the driver.
> > +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).

I've already done this, it was just in a different branch.

> > 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.

Eventually the qmp/hmp/qom related stuff will be in a header file, and 
this is why I put them from the beginning separately. But yes, in this 
case it is not needed. Also, these probably should go to 
include/qemu/throttle.h instead of a separate header 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