Re: [Qemu-block] [PATCH RFC v3 2/8] block: Add aio_context field in ThrottleGroupMember

[Kevin Wolf]

Am 28.06.2017 um 14:15 hat Manos Pitsidianakis geschrieben:
> On Wed, Jun 28, 2017 at 01:27:36PM +0200, Kevin Wolf wrote:
> >Am 23.06.2017 um 14:46 hat Manos Pitsidianakis geschrieben:
> >>timer_cb() needs to know about the current Aio context of the throttle
> >>request that is woken up. In order to make ThrottleGroupMember backend
> >>agnostic, this information is stored in an aio_context field instead of
> >>accessing it from BlockBackend.
> >>
> >>Signed-off-by: Manos Pitsidianakis 
> >
> >You're copying the AioContext when the BlockBackend is registered for
> >the throttle group, but what keeps both sides in sync when the context
> >is changed later on? Don't we need to update the ThrottleGroupMember in
> >blk_set_aio_context?
> 
> blk_set_aio_context calls throttle_timers_attach_aio_context which
> updates this. Though as Alberto said util/throttle.c should not know
> about ThrottleGroupMember. This is not needed in the later patches
> because the ThrottleGroupMember's aio_context gets updated as a node
> in the driver's bdrv_attach_aio_context
> 
> We can add a new function in block/throttle.c that updates a
> member's aio context but I'm not sure if it's really needed if
> members are only used in throttle nodes.

Oh, I looked at the final state after the series instead of this very
commit, so I missed the existing calls in blk_set_aio_context().

My bad, sorry for the noise.

Kevin


pgpOh7c2AaLnP.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH RFC v3 2/8] block: Add aio_context field in ThrottleGroupMember

[Kevin Wolf]

Am 23.06.2017 um 14:46 hat Manos Pitsidianakis geschrieben:
> timer_cb() needs to know about the current Aio context of the throttle
> request that is woken up. In order to make ThrottleGroupMember backend
> agnostic, this information is stored in an aio_context field instead of
> accessing it from BlockBackend.
> 
> Signed-off-by: Manos Pitsidianakis 

You're copying the AioContext when the BlockBackend is registered for
the throttle group, but what keeps both sides in sync when the context
is changed later on? Don't we need to update the ThrottleGroupMember in
blk_set_aio_context?

Kevin

Re: [Qemu-block] [PATCH RFC v3 2/8] block: Add aio_context field in ThrottleGroupMember

[Alberto Garcia]

On Fri 23 Jun 2017 02:46:54 PM CEST, Manos Pitsidianakis wrote:
> timer_cb() needs to know about the current Aio context of the throttle
> request that is woken up. In order to make ThrottleGroupMember backend
> agnostic, this information is stored in an aio_context field instead of
> accessing it from BlockBackend.
>
> Signed-off-by: Manos Pitsidianakis 

I agree with Stefan's comments, otherwise the patch looks good to me.

Berto

Re: [Qemu-block] [PATCH RFC v3 2/8] block: Add aio_context field in ThrottleGroupMember

[Manos Pitsidianakis]

On Mon, Jun 26, 2017 at 02:36:11PM +0100, Stefan Hajnoczi wrote:

On Fri, Jun 23, 2017 at 03:46:54PM +0300, Manos Pitsidianakis wrote:

timer_cb() needs to know about the current Aio context of the throttle
request that is woken up. In order to make ThrottleGroupMember backend
agnostic, this information is stored in an aio_context field instead of
accessing it from BlockBackend.

Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   |  1 +
 block/throttle-groups.c | 19 +--
 include/qemu/throttle.h |  1 +
 tests/test-throttle.c   | 65 +++--
 util/throttle.c |  4 +++
 5 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 90a7abaa53..1d501ec973 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1928,6 +1928,7 @@ void blk_io_limits_disable(BlockBackend *blk)
 /* should be called before blk_set_io_limits if a limit is set */
 void blk_io_limits_enable(BlockBackend *blk, const char *group)
 {
+blk->public.throttle_group_member.aio_context = blk_get_aio_context(blk);
 assert(!blk->public.throttle_group_member.throttle_state);
 throttle_group_register_tgm(>public.throttle_group_member, group);


Or throttle_group_register_tgm() could take an AioContext* argument, I
think that's a little cleaner than modifying throttle_group_member
fields in multiple source files.


Indeed that is cleaner.




diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 5e9d8fb4d6..7883cbb511 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -71,6 +71,7 @@ static QemuMutex throttle_groups_lock;
 static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
 QTAILQ_HEAD_INITIALIZER(throttle_groups);

+
 /* Increments the reference count of a ThrottleGroup given its name.
  *
  * If no ThrottleGroup is found with the given name a new one is


Spurious whitespace change.  Please do not change whitespace in random
places.


I noticed it too late unfortunately.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH RFC v3 2/8] block: Add aio_context field in ThrottleGroupMember

[Stefan Hajnoczi]

On Fri, Jun 23, 2017 at 03:46:54PM +0300, Manos Pitsidianakis wrote:
> timer_cb() needs to know about the current Aio context of the throttle
> request that is woken up. In order to make ThrottleGroupMember backend
> agnostic, this information is stored in an aio_context field instead of
> accessing it from BlockBackend.
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  block/block-backend.c   |  1 +
>  block/throttle-groups.c | 19 +--
>  include/qemu/throttle.h |  1 +
>  tests/test-throttle.c   | 65 
> +++--
>  util/throttle.c |  4 +++
>  5 files changed, 48 insertions(+), 42 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 90a7abaa53..1d501ec973 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1928,6 +1928,7 @@ void blk_io_limits_disable(BlockBackend *blk)
>  /* should be called before blk_set_io_limits if a limit is set */
>  void blk_io_limits_enable(BlockBackend *blk, const char *group)
>  {
> +blk->public.throttle_group_member.aio_context = blk_get_aio_context(blk);
>  assert(!blk->public.throttle_group_member.throttle_state);
>  throttle_group_register_tgm(>public.throttle_group_member, group);

Or throttle_group_register_tgm() could take an AioContext* argument, I
think that's a little cleaner than modifying throttle_group_member
fields in multiple source files.

> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 5e9d8fb4d6..7883cbb511 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -71,6 +71,7 @@ static QemuMutex throttle_groups_lock;
>  static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
>  QTAILQ_HEAD_INITIALIZER(throttle_groups);
>  
> +
>  /* Increments the reference count of a ThrottleGroup given its name.
>   *
>   * If no ThrottleGroup is found with the given name a new one is

Spurious whitespace change.  Please do not change whitespace in random
places.


signature.asc
Description: PGP signature

[Qemu-block] [PATCH RFC v3 2/8] block: Add aio_context field in ThrottleGroupMember

[Manos Pitsidianakis]

timer_cb() needs to know about the current Aio context of the throttle
request that is woken up. In order to make ThrottleGroupMember backend
agnostic, this information is stored in an aio_context field instead of
accessing it from BlockBackend.

Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   |  1 +
 block/throttle-groups.c | 19 +--
 include/qemu/throttle.h |  1 +
 tests/test-throttle.c   | 65 +++--
 util/throttle.c |  4 +++
 5 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 90a7abaa53..1d501ec973 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1928,6 +1928,7 @@ void blk_io_limits_disable(BlockBackend *blk)
 /* should be called before blk_set_io_limits if a limit is set */
 void blk_io_limits_enable(BlockBackend *blk, const char *group)
 {
+blk->public.throttle_group_member.aio_context = blk_get_aio_context(blk);
 assert(!blk->public.throttle_group_member.throttle_state);
 throttle_group_register_tgm(>public.throttle_group_member, group);
 }
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 5e9d8fb4d6..7883cbb511 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -71,6 +71,7 @@ static QemuMutex throttle_groups_lock;
 static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
 QTAILQ_HEAD_INITIALIZER(throttle_groups);
 
+
 /* Increments the reference count of a ThrottleGroup given its name.
  *
  * If no ThrottleGroup is found with the given name a new one is
@@ -383,9 +384,6 @@ static void coroutine_fn 
throttle_group_restart_queue_entry(void *opaque)
 
 static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool 
is_write)
 {
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
 Coroutine *co;
 RestartData rd = {
 .tgm = tgm,
@@ -393,7 +391,7 @@ static void 
throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write
 };
 
 co = qemu_coroutine_create(throttle_group_restart_queue_entry, );
-aio_co_enter(blk_get_aio_context(blk), co);
+aio_co_enter(tgm->aio_context, co);
 }
 
 void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
@@ -447,13 +445,11 @@ void throttle_group_get_config(ThrottleGroupMember *tgm, 
ThrottleConfig *cfg)
 /* ThrottleTimers callback. This wakes up a request that was waiting because it
  * had been throttled.
  *
- * @blk:   the BlockBackend whose request had been throttled
+ * @tgm:   the ThrottleGroupMember whose request had been throttled
  * @is_write:  the type of operation (read/write)
  */
-static void timer_cb(BlockBackend *blk, bool is_write)
+static void timer_cb(ThrottleGroupMember *tgm, bool is_write)
 {
-BlockBackendPublic *blkp = blk_get_public(blk);
-ThrottleGroupMember *tgm = >throttle_group_member;
 ThrottleState *ts = tgm->throttle_state;
 ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
 
@@ -487,9 +483,6 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
  const char *groupname)
 {
 int i;
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
 ThrottleState *ts = throttle_group_incref(groupname);
 ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
 int clock_type = QEMU_CLOCK_REALTIME;
@@ -512,11 +505,11 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
 QLIST_INSERT_HEAD(>head, tgm, round_robin);
 
 throttle_timers_init(>throttle_timers,
- blk_get_aio_context(blk),
+ tgm->aio_context,
  clock_type,
  read_timer_cb,
  write_timer_cb,
- blk);
+ tgm);
 
 qemu_mutex_unlock(>lock);
 }
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index e99cbfc865..3e92d4d4eb 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -160,6 +160,7 @@ void throttle_account(ThrottleState *ts, bool is_write, 
uint64_t size);
  */
 
 typedef struct ThrottleGroupMember {
+AioContext  *aio_context;
 /* throttled_reqs_lock protects the CoQueues for throttled requests.  */
 CoMutex  throttled_reqs_lock;
 CoQueue  throttled_reqs[2];
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 0f95da2592..d3298234aa 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -24,8 +24,9 @@
 static AioContext *ctx;
 static LeakyBucketbkt;
 static ThrottleConfig cfg;
+static ThrottleGroupMember tgm;
 static ThrottleState  ts;
-static ThrottleTimers tt;
+static ThrottleTimers *tt;
 
 /* useful function */
 static bool double_cmp(double x, double y)
@@ -153,19