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