Message ID | 1455645388-32401-4-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, 02/16 18:56, Paolo Bonzini wrote: > Extract the handling of throttling from bdrv_flush_io_queue. Looks good overall. Have two questions below. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block.c | 1 - > block/io.c | 56 +++++++++++++++++++++++++++++------------------ > block/throttle-groups.c | 4 ++++ > include/block/block_int.h | 6 ++--- > 4 files changed, 42 insertions(+), 25 deletions(-) > > diff --git a/block.c b/block.c > index efc3c43..b4f328f 100644 > --- a/block.c > +++ b/block.c > @@ -2314,7 +2314,6 @@ static void swap_feature_fields(BlockDriverState *bs_top, > > assert(!bs_new->throttle_state); > if (bs_top->throttle_state) { > - assert(bs_top->io_limits_enabled); > bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top)); > bdrv_io_limits_disable(bs_top); > } > diff --git a/block/io.c b/block/io.c > index 5ee5032..272caac 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -69,28 +69,43 @@ void bdrv_set_io_limits(BlockDriverState *bs, > throttle_group_config(bs, cfg); > } > > -static void bdrv_start_throttled_reqs(BlockDriverState *bs) > +static void bdrv_no_throttling_begin(BlockDriverState *bs) > { > - bool enabled = bs->io_limits_enabled; > + BdrvChild *child; > + > + QLIST_FOREACH(child, &bs->children, next) { > + bdrv_no_throttling_begin(child->bs); > + } > > - bs->io_limits_enabled = false; > - throttle_group_restart_bs(bs); > - bs->io_limits_enabled = enabled; > + if (bs->io_limits_disabled++ == 0) { > + throttle_group_restart_bs(bs); > + } > +} > + > +static void bdrv_no_throttling_end(BlockDriverState *bs) > +{ > + BdrvChild *child; > + > + --bs->io_limits_disabled; > + > + QLIST_FOREACH(child, &bs->children, next) { > + bdrv_no_throttling_end(child->bs); > + } > } > > void bdrv_io_limits_disable(BlockDriverState *bs) > { > - bs->io_limits_enabled = false; > - bdrv_start_throttled_reqs(bs); > + assert(bs->throttle_state); > + bdrv_no_throttling_begin(bs); > throttle_group_unregister_bs(bs); > + bdrv_no_throttling_end(bs); > } > > /* should be called before bdrv_set_io_limits if a limit is set */ > void bdrv_io_limits_enable(BlockDriverState *bs, const char *group) > { > - assert(!bs->io_limits_enabled); > + assert(!bs->throttle_state); > throttle_group_register_bs(bs, group); > - bs->io_limits_enabled = true; > } > > void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group) > @@ -255,6 +270,7 @@ void bdrv_drain(BlockDriverState *bs) > { > bool busy = true; > > + bdrv_no_throttling_begin(bs); > bdrv_drain_recurse(bs); > while (busy) { > /* Keep iterating */ > @@ -262,6 +278,7 @@ void bdrv_drain(BlockDriverState *bs) > busy = bdrv_requests_pending(bs); > busy |= aio_poll(bdrv_get_aio_context(bs), busy); > } > + bdrv_no_throttling_end(bs); > } > > /* > @@ -284,6 +301,7 @@ void bdrv_drain_all(void) > if (bs->job) { > block_job_pause(bs->job); > } > + bdrv_no_throttling_begin(bs); > bdrv_drain_recurse(bs); > aio_context_release(aio_context); > > @@ -325,6 +343,7 @@ void bdrv_drain_all(void) > AioContext *aio_context = bdrv_get_aio_context(bs); > > aio_context_acquire(aio_context); > + bdrv_no_throttling_end(bs); > if (bs->job) { > block_job_resume(bs->job); > } > @@ -555,11 +574,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset, > * will not fire; so the I/O throttling function has to be disabled here > * if it has been enabled. > */ > - if (bs->io_limits_enabled) { > - fprintf(stderr, "Disabling I/O throttling on '%s' due " > - "to synchronous I/O.\n", bdrv_get_device_name(bs)); > - bdrv_io_limits_disable(bs); > - } > + bdrv_no_throttling_begin(bs); > > if (qemu_in_coroutine()) { > /* Fast-path if already in coroutine context */ > @@ -573,6 +588,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset, > aio_poll(aio_context, true); > } > } > + > + bdrv_no_throttling_end(bs); Does this change the behavior? There wasn't a bdrv_io_limits_enable() here, and the throttle doesn't come back automatically. Just want to make sure it's intended. > return rwco.ret; > } > > @@ -608,13 +625,11 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num, > int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num, > uint8_t *buf, int nb_sectors) > { > - bool enabled; > int ret; > > - enabled = bs->io_limits_enabled; > - bs->io_limits_enabled = false; > + bdrv_no_throttling_begin(bs); > ret = bdrv_read(bs, sector_num, buf, nb_sectors); > - bs->io_limits_enabled = enabled; > + bdrv_no_throttling_end(bs); > return ret; > } > > @@ -952,7 +967,7 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs, > } > > /* throttling disk I/O */ > - if (bs->io_limits_enabled) { > + if (bs->throttle_state) { > throttle_group_co_io_limits_intercept(bs, bytes, false); > } > > @@ -1294,7 +1309,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs, > } > > /* throttling disk I/O */ > - if (bs->io_limits_enabled) { > + if (bs->throttle_state) { > throttle_group_co_io_limits_intercept(bs, bytes, true); > } > > @@ -2749,7 +2764,6 @@ void bdrv_flush_io_queue(BlockDriverState *bs) > } else if (bs->file) { > bdrv_flush_io_queue(bs->file->bs); > } > - bdrv_start_throttled_reqs(bs); > } > > void bdrv_drained_begin(BlockDriverState *bs) > diff --git a/block/throttle-groups.c b/block/throttle-groups.c > index eccfc0d..8fe0a4f 100644 > --- a/block/throttle-groups.c > +++ b/block/throttle-groups.c > @@ -219,6 +219,10 @@ static bool throttle_group_schedule_timer(BlockDriverState *bs, > ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); > bool must_wait; > > + if (bs->io_limits_disabled) { > + return false; > + } > + Before, this function and its direct caller throttle_group_co_io_limits_intercept are not called if !bs->io_limits_enabled, so the accounting is not done. It's better, but just different. Thanks, Fam > /* Check if any of the timers in this group is already armed */ > if (tg->any_timer_armed[is_write]) { > return true; > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 9ef823a..ed2e034 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -412,10 +412,10 @@ struct BlockDriverState { > > /* I/O throttling. > * throttle_state tells us if this BDS has I/O limits configured. > - * io_limits_enabled tells us if they are currently being > - * enforced, but it can be temporarily set to false */ > + * io_limits_disabled tells us if they are currently being enforced */ > CoQueue throttled_reqs[2]; > - bool io_limits_enabled; > + unsigned int io_limits_disabled; > + > /* The following fields are protected by the ThrottleGroup lock. > * See the ThrottleGroup documentation for details. */ > ThrottleState *throttle_state; > -- > 2.5.0 > > >
On 09/03/2016 02:45, Fam Zheng wrote: >> > @@ -555,11 +574,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset, >> > * will not fire; so the I/O throttling function has to be disabled here >> > * if it has been enabled. >> > */ >> > - if (bs->io_limits_enabled) { >> > - fprintf(stderr, "Disabling I/O throttling on '%s' due " >> > - "to synchronous I/O.\n", bdrv_get_device_name(bs)); >> > - bdrv_io_limits_disable(bs); >> > - } >> > + bdrv_no_throttling_begin(bs); >> > >> > if (qemu_in_coroutine()) { >> > /* Fast-path if already in coroutine context */ >> > @@ -573,6 +588,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset, >> > aio_poll(aio_context, true); >> > } >> > } >> > + >> > + bdrv_no_throttling_end(bs); > > Does this change the behavior? There wasn't a bdrv_io_limits_enable() here, and > the throttle doesn't come back automatically. Just want to make sure it's > intended. Yes, it's intended. As long as the I/O stays synchronous, throttling is disabled. If it starts to be asynchronous, it will be re-enabled automatically. Paolo
diff --git a/block.c b/block.c index efc3c43..b4f328f 100644 --- a/block.c +++ b/block.c @@ -2314,7 +2314,6 @@ static void swap_feature_fields(BlockDriverState *bs_top, assert(!bs_new->throttle_state); if (bs_top->throttle_state) { - assert(bs_top->io_limits_enabled); bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top)); bdrv_io_limits_disable(bs_top); } diff --git a/block/io.c b/block/io.c index 5ee5032..272caac 100644 --- a/block/io.c +++ b/block/io.c @@ -69,28 +69,43 @@ void bdrv_set_io_limits(BlockDriverState *bs, throttle_group_config(bs, cfg); } -static void bdrv_start_throttled_reqs(BlockDriverState *bs) +static void bdrv_no_throttling_begin(BlockDriverState *bs) { - bool enabled = bs->io_limits_enabled; + BdrvChild *child; + + QLIST_FOREACH(child, &bs->children, next) { + bdrv_no_throttling_begin(child->bs); + } - bs->io_limits_enabled = false; - throttle_group_restart_bs(bs); - bs->io_limits_enabled = enabled; + if (bs->io_limits_disabled++ == 0) { + throttle_group_restart_bs(bs); + } +} + +static void bdrv_no_throttling_end(BlockDriverState *bs) +{ + BdrvChild *child; + + --bs->io_limits_disabled; + + QLIST_FOREACH(child, &bs->children, next) { + bdrv_no_throttling_end(child->bs); + } } void bdrv_io_limits_disable(BlockDriverState *bs) { - bs->io_limits_enabled = false; - bdrv_start_throttled_reqs(bs); + assert(bs->throttle_state); + bdrv_no_throttling_begin(bs); throttle_group_unregister_bs(bs); + bdrv_no_throttling_end(bs); } /* should be called before bdrv_set_io_limits if a limit is set */ void bdrv_io_limits_enable(BlockDriverState *bs, const char *group) { - assert(!bs->io_limits_enabled); + assert(!bs->throttle_state); throttle_group_register_bs(bs, group); - bs->io_limits_enabled = true; } void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group) @@ -255,6 +270,7 @@ void bdrv_drain(BlockDriverState *bs) { bool busy = true; + bdrv_no_throttling_begin(bs); bdrv_drain_recurse(bs); while (busy) { /* Keep iterating */ @@ -262,6 +278,7 @@ void bdrv_drain(BlockDriverState *bs) busy = bdrv_requests_pending(bs); busy |= aio_poll(bdrv_get_aio_context(bs), busy); } + bdrv_no_throttling_end(bs); } /* @@ -284,6 +301,7 @@ void bdrv_drain_all(void) if (bs->job) { block_job_pause(bs->job); } + bdrv_no_throttling_begin(bs); bdrv_drain_recurse(bs); aio_context_release(aio_context); @@ -325,6 +343,7 @@ void bdrv_drain_all(void) AioContext *aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); + bdrv_no_throttling_end(bs); if (bs->job) { block_job_resume(bs->job); } @@ -555,11 +574,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset, * will not fire; so the I/O throttling function has to be disabled here * if it has been enabled. */ - if (bs->io_limits_enabled) { - fprintf(stderr, "Disabling I/O throttling on '%s' due " - "to synchronous I/O.\n", bdrv_get_device_name(bs)); - bdrv_io_limits_disable(bs); - } + bdrv_no_throttling_begin(bs); if (qemu_in_coroutine()) { /* Fast-path if already in coroutine context */ @@ -573,6 +588,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset, aio_poll(aio_context, true); } } + + bdrv_no_throttling_end(bs); return rwco.ret; } @@ -608,13 +625,11 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num, int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors) { - bool enabled; int ret; - enabled = bs->io_limits_enabled; - bs->io_limits_enabled = false; + bdrv_no_throttling_begin(bs); ret = bdrv_read(bs, sector_num, buf, nb_sectors); - bs->io_limits_enabled = enabled; + bdrv_no_throttling_end(bs); return ret; } @@ -952,7 +967,7 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs, } /* throttling disk I/O */ - if (bs->io_limits_enabled) { + if (bs->throttle_state) { throttle_group_co_io_limits_intercept(bs, bytes, false); } @@ -1294,7 +1309,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs, } /* throttling disk I/O */ - if (bs->io_limits_enabled) { + if (bs->throttle_state) { throttle_group_co_io_limits_intercept(bs, bytes, true); } @@ -2749,7 +2764,6 @@ void bdrv_flush_io_queue(BlockDriverState *bs) } else if (bs->file) { bdrv_flush_io_queue(bs->file->bs); } - bdrv_start_throttled_reqs(bs); } void bdrv_drained_begin(BlockDriverState *bs) diff --git a/block/throttle-groups.c b/block/throttle-groups.c index eccfc0d..8fe0a4f 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -219,6 +219,10 @@ static bool throttle_group_schedule_timer(BlockDriverState *bs, ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); bool must_wait; + if (bs->io_limits_disabled) { + return false; + } + /* Check if any of the timers in this group is already armed */ if (tg->any_timer_armed[is_write]) { return true; diff --git a/include/block/block_int.h b/include/block/block_int.h index 9ef823a..ed2e034 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -412,10 +412,10 @@ struct BlockDriverState { /* I/O throttling. * throttle_state tells us if this BDS has I/O limits configured. - * io_limits_enabled tells us if they are currently being - * enforced, but it can be temporarily set to false */ + * io_limits_disabled tells us if they are currently being enforced */ CoQueue throttled_reqs[2]; - bool io_limits_enabled; + unsigned int io_limits_disabled; + /* The following fields are protected by the ThrottleGroup lock. * See the ThrottleGroup documentation for details. */ ThrottleState *throttle_state;
Extract the handling of throttling from bdrv_flush_io_queue. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block.c | 1 - block/io.c | 56 +++++++++++++++++++++++++++++------------------ block/throttle-groups.c | 4 ++++ include/block/block_int.h | 6 ++--- 4 files changed, 42 insertions(+), 25 deletions(-)