* Re: [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror
2016-06-22 12:35 [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror Denis V. Lunev
@ 2016-06-28 11:21 ` Denis V. Lunev
2016-06-29 16:08 ` Max Reitz
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2016-06-28 11:21 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: Jeff Cody, Kevin Wolf, Max Reitz
On 06/22/2016 03:35 PM, Denis V. Lunev wrote:
> There are 2 deficiencies here:
> - mirror_iteration could start several requests inside. Thus we could
> simply have more in_flight requests than MAX_IN_FLIGHT.
> - keeping this in mind throttling in mirror_run which is checking
> s->in_flight == MAX_IN_FLIGHT is wrong.
>
> The patch adds the check and throttling into mirror_iteration and fixes
> the check in mirror_run() to be sure.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
> block/mirror.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..e881ef6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> }
> }
>
> + while (s->in_flight >= MAX_IN_FLIGHT) {
> + trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
> + mirror_wait_for_io(s);
> + }
> +
> mirror_clip_sectors(s, sector_num, &io_sectors);
> switch (mirror_method) {
> case MIRROR_METHOD_COPY:
> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < SLICE_TIME &&
> s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> - if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> + if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> (cnt == 0 && s->in_flight > 0)) {
> trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> mirror_wait_for_io(s);
ping
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror
2016-06-22 12:35 [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror Denis V. Lunev
2016-06-28 11:21 ` Denis V. Lunev
@ 2016-06-29 16:08 ` Max Reitz
2016-06-29 16:12 ` Denis V. Lunev
2016-06-30 16:17 ` Jeff Cody
2016-06-30 16:18 ` Jeff Cody
3 siblings, 1 reply; 6+ messages in thread
From: Max Reitz @ 2016-06-29 16:08 UTC (permalink / raw)
To: Denis V. Lunev, qemu-block, qemu-devel; +Cc: Jeff Cody, Kevin Wolf
[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]
On 22.06.2016 14:35, Denis V. Lunev wrote:
> There are 2 deficiencies here:
> - mirror_iteration could start several requests inside. Thus we could
> simply have more in_flight requests than MAX_IN_FLIGHT.
> - keeping this in mind throttling in mirror_run which is checking
> s->in_flight == MAX_IN_FLIGHT is wrong.
>
> The patch adds the check and throttling into mirror_iteration and fixes
> the check in mirror_run() to be sure.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
> block/mirror.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..e881ef6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> }
> }
>
> + while (s->in_flight >= MAX_IN_FLIGHT) {
> + trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
> + mirror_wait_for_io(s);
> + }
> +
> mirror_clip_sectors(s, sector_num, &io_sectors);
> switch (mirror_method) {
> case MIRROR_METHOD_COPY:
> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < SLICE_TIME &&
> s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> - if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> + if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> (cnt == 0 && s->in_flight > 0)) {
> trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> mirror_wait_for_io(s);
>
Using >= seems fine to me, but with the first hunk applied I can't
imagine how s->in_flight should grow beyond MAX_IN_FLIGHT. Don't get me
wrong, I myself like to use >= even where the > case should never
happen, I'm just wondering if I'm missing something here.
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror
2016-06-29 16:08 ` Max Reitz
@ 2016-06-29 16:12 ` Denis V. Lunev
0 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2016-06-29 16:12 UTC (permalink / raw)
To: Max Reitz, qemu-block, qemu-devel; +Cc: Jeff Cody, Kevin Wolf
On 06/29/2016 07:08 PM, Max Reitz wrote:
> On 22.06.2016 14:35, Denis V. Lunev wrote:
>> There are 2 deficiencies here:
>> - mirror_iteration could start several requests inside. Thus we could
>> simply have more in_flight requests than MAX_IN_FLIGHT.
>> - keeping this in mind throttling in mirror_run which is checking
>> s->in_flight == MAX_IN_FLIGHT is wrong.
>>
>> The patch adds the check and throttling into mirror_iteration and fixes
>> the check in mirror_run() to be sure.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>> block/mirror.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index a04ed9c..e881ef6 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>> }
>> }
>>
>> + while (s->in_flight >= MAX_IN_FLIGHT) {
>> + trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
>> + mirror_wait_for_io(s);
>> + }
>> +
>> mirror_clip_sectors(s, sector_num, &io_sectors);
>> switch (mirror_method) {
>> case MIRROR_METHOD_COPY:
>> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
>> */
>> if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < SLICE_TIME &&
>> s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>> - if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>> + if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>> (cnt == 0 && s->in_flight > 0)) {
>> trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
>> mirror_wait_for_io(s);
>>
> Using >= seems fine to me, but with the first hunk applied I can't
> imagine how s->in_flight should grow beyond MAX_IN_FLIGHT. Don't get me
> wrong, I myself like to use >= even where the > case should never
> happen, I'm just wondering if I'm missing something here.
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
I think that this should not happen anymore,
but I'd like to stay on the safe side.
Den
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror
2016-06-22 12:35 [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror Denis V. Lunev
2016-06-28 11:21 ` Denis V. Lunev
2016-06-29 16:08 ` Max Reitz
@ 2016-06-30 16:17 ` Jeff Cody
2016-06-30 16:18 ` Jeff Cody
3 siblings, 0 replies; 6+ messages in thread
From: Jeff Cody @ 2016-06-30 16:17 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-block, qemu-devel, Kevin Wolf, Max Reitz
On Wed, Jun 22, 2016 at 03:35:27PM +0300, Denis V. Lunev wrote:
> There are 2 deficiencies here:
> - mirror_iteration could start several requests inside. Thus we could
> simply have more in_flight requests than MAX_IN_FLIGHT.
> - keeping this in mind throttling in mirror_run which is checking
> s->in_flight == MAX_IN_FLIGHT is wrong.
>
> The patch adds the check and throttling into mirror_iteration and fixes
> the check in mirror_run() to be sure.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
> block/mirror.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..e881ef6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> }
> }
>
> + while (s->in_flight >= MAX_IN_FLIGHT) {
> + trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
> + mirror_wait_for_io(s);
> + }
> +
> mirror_clip_sectors(s, sector_num, &io_sectors);
> switch (mirror_method) {
> case MIRROR_METHOD_COPY:
> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < SLICE_TIME &&
> s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> - if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> + if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> (cnt == 0 && s->in_flight > 0)) {
> trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> mirror_wait_for_io(s);
> --
> 2.1.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror
2016-06-22 12:35 [Qemu-devel] [PATCH 1/1] mirror: fix request throttling in drive-mirror Denis V. Lunev
` (2 preceding siblings ...)
2016-06-30 16:17 ` Jeff Cody
@ 2016-06-30 16:18 ` Jeff Cody
3 siblings, 0 replies; 6+ messages in thread
From: Jeff Cody @ 2016-06-30 16:18 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-block, qemu-devel, Kevin Wolf, Max Reitz
On Wed, Jun 22, 2016 at 03:35:27PM +0300, Denis V. Lunev wrote:
> There are 2 deficiencies here:
> - mirror_iteration could start several requests inside. Thus we could
> simply have more in_flight requests than MAX_IN_FLIGHT.
> - keeping this in mind throttling in mirror_run which is checking
> s->in_flight == MAX_IN_FLIGHT is wrong.
>
> The patch adds the check and throttling into mirror_iteration and fixes
> the check in mirror_run() to be sure.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
> block/mirror.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..e881ef6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -399,6 +399,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> }
> }
>
> + while (s->in_flight >= MAX_IN_FLIGHT) {
> + trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
> + mirror_wait_for_io(s);
> + }
> +
> mirror_clip_sectors(s, sector_num, &io_sectors);
> switch (mirror_method) {
> case MIRROR_METHOD_COPY:
> @@ -634,7 +639,7 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - last_pause_ns < SLICE_TIME &&
> s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> - if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> + if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
> (cnt == 0 && s->in_flight > 0)) {
> trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> mirror_wait_for_io(s);
> --
> 2.1.4
>
Thanks,
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc.git block
-Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread