Re: [Qemu-block] [PATCH v2 4/6] block: remove legacy I/O throttling

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:54 PM CEST, Manos Pitsidianakis wrote:
> +void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error 
> **errp)
>  {
> +ThrottleGroupMember *tgm;
> +
>  /* this BB is not part of any group */
> -if (!blk->public.throttle_group_member.throttle_state) {
> +if (!blk->public.throttle_node) {
>  return;
>  }
>  
> +tgm = throttle_get_tgm(blk->public.throttle_node);
>  /* this BB is a part of the same group than the one we want */
> -if 
> (!g_strcmp0(throttle_group_get_name(>public.throttle_group_member),
> -group)) {
> +if (!g_strcmp0(throttle_group_get_name(tgm),
> +   group)) {

You can join these two lines, no need to have 'group' in its own line.

> @@ -2629,6 +2636,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, 
> Error **errp)
>  BlockDriverState *bs;
>  BlockBackend *blk;
>  AioContext *aio_context;
> +BlockDriverState *throttle_node = NULL;
> +ThrottleGroupMember *tgm;
> +Error *local_err = NULL;
>  
>  blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
>arg->has_id ? arg->id : NULL,
> @@ -2704,18 +2714,33 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, 
> Error **errp)
>  if (throttle_enabled()) {
>  /* Enable I/O limits if they're not enabled yet, otherwise
>   * just update the throttling group. */
> -if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
> -blk_io_limits_enable(blk,
> - arg->has_group ? arg->group :
> - arg->has_device ? arg->device :
> - arg->id);
> -} else if (arg->has_group) {
> -blk_io_limits_update_group(blk, arg->group);
> +if (!blk_get_public(blk)->throttle_node) {
> +blk_io_limits_enable(blk, arg->has_group ? arg->group :
> + arg->has_device ? arg->device : arg->id,
> + _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
>  }
> -/* Set the new throttling configuration */
> -blk_set_io_limits(blk, );
> -} else if (blk_get_public(blk)->throttle_group_member.throttle_state) {
> -/* If all throttling settings are set to 0, disable I/O limits */
> +
> +if (arg->has_group) {
> +/* move throttle node membership to arg->group */
> +blk_io_limits_update_group(blk, arg->group, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
> +}

This used to be

   if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
  /* Enable throttling */
   } else if (arg->has_group) {
  /* Update group */
   }

but now it is

   if (!blk_get_throttle_node(blk)) {
  /* Enable throttling */
   }
   if (arg->has_group) {
  /* Update group */
   }

Now if I/O limits are not set then the code sets the same group twice.

Everything else looks fine.

Berto

[Qemu-block] [PATCH v2 4/6] block: remove legacy I/O throttling

[Manos Pitsidianakis]

This commit removes all I/O throttling from block/block-backend.c. In
order to support the existing interface, it is changed to use the
block/throttle.c filter driver.

The throttle filter node that is created by the legacy interface is
stored in a 'throttle_node' field in the BlockBackendPublic of the
device. The legacy throttle node is managed by the legacy interface
completely. More advanced configurations with the filter drive are
possible using the QMP API, but these will be ignored by the legacy
interface.

Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   | 133 
 block/qapi.c|  10 +--
 block/throttle.c|   8 +++
 blockdev.c  |  49 +++
 include/block/throttle-groups.h |   1 +
 include/sysemu/block-backend.h  |   6 +-
 tests/test-throttle.c   |  19 +++---
 7 files changed, 146 insertions(+), 80 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index df0200fc49..61983b7393 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -15,6 +15,7 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
 #include "qapi-event.h"
@@ -282,7 +283,7 @@ static void blk_delete(BlockBackend *blk)
 assert(!blk->refcnt);
 assert(!blk->name);
 assert(!blk->dev);
-if (blk->public.throttle_group_member.throttle_state) {
+if (blk->public.throttle_node) {
 blk_io_limits_disable(blk);
 }
 if (blk->root) {
@@ -593,13 +594,7 @@ BlockBackend *blk_by_public(BlockBackendPublic *public)
  */
 void blk_remove_bs(BlockBackend *blk)
 {
-ThrottleTimers *tt;
-
 notifier_list_notify(>remove_bs_notifiers, blk);
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_detach_aio_context(tt);
-}
 
 blk_update_root_state(blk);
 
@@ -620,12 +615,6 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, 
Error **errp)
 bdrv_ref(bs);
 
 notifier_list_notify(>insert_bs_notifiers, blk);
-if (blk->public.throttle_group_member.throttle_state) {
-throttle_timers_attach_aio_context(
->public.throttle_group_member.throttle_timers,
-bdrv_get_aio_context(bs));
-}
-
 return 0;
 }
 
@@ -983,13 +972,6 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t 
offset,
 }
 
 bdrv_inc_in_flight(bs);
-
-/* throttling disk I/O */
-if (blk->public.throttle_group_member.throttle_state) {
-
throttle_group_co_io_limits_intercept(>public.throttle_group_member,
-bytes, false);
-}
-
 ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
 bdrv_dec_in_flight(bs);
 return ret;
@@ -1010,11 +992,6 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, 
int64_t offset,
 }
 
 bdrv_inc_in_flight(bs);
-/* throttling disk I/O */
-if (blk->public.throttle_group_member.throttle_state) {
-
throttle_group_co_io_limits_intercept(>public.throttle_group_member,
-bytes, true);
-}
 
 if (!blk->enable_write_cache) {
 flags |= BDRV_REQ_FUA;
@@ -1682,16 +1659,9 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB 
*acb)
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
 BlockDriverState *bs = blk_bs(blk);
-ThrottleGroupMember *tgm = >public.throttle_group_member;
 
 if (bs) {
-if (tgm->throttle_state) {
-throttle_group_detach_aio_context(tgm);
-}
 bdrv_set_aio_context(bs, new_context);
-if (tgm->throttle_state) {
-throttle_group_attach_aio_context(tgm, new_context);
-}
 }
 }
 
@@ -1909,45 +1879,98 @@ int blk_commit_all(void)
 /* throttling disk I/O limits */
 void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
 {
-throttle_group_config(>public.throttle_group_member, cfg);
+assert(blk->public.throttle_node);
+throttle_group_config(throttle_get_tgm(blk->public.throttle_node), cfg);
 }
 
 void blk_io_limits_disable(BlockBackend *blk)
 {
-assert(blk->public.throttle_group_member.throttle_state);
-bdrv_drained_begin(blk_bs(blk));
-throttle_group_unregister_tgm(>public.throttle_group_member);
-bdrv_drained_end(blk_bs(blk));
+BlockDriverState *bs, *throttle_node;
+
+throttle_node = blk_get_public(blk)->throttle_node;
+
+assert(throttle_node);
+
+bs = throttle_node->file->bs;
+bdrv_drained_begin(bs);
+
+/* Ref throttle_node's child bs to ensure it won't go away */
+bdrv_ref(bs);
+
+bdrv_child_try_set_perm(throttle_node->file, 0, BLK_PERM_ALL,
+_abort);
+/* Replace throttle_node with bs. While throttle_node was inserted under
+ * blk, at this point