Re: [Qemu-devel] [PATCH 2/3] block: mirror - split out part of mirror_run()

[Paolo Bonzini <pbonzini@redhat.com>]

On 28/09/2015 05:29, Jeff Cody wrote:
> This is code relocation, to pull the part of mirror_run() that
> calls mirror_iteration out into a separate function.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/mirror.c | 206 ++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 110 insertions(+), 96 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 8b3e89b..405e5c4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -318,6 +318,115 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>      return delay_ns;
>  }
>  
> +static int mirror_do_iteration(MirrorBlockJob *s, uint64_t last_pause_ns)
> +{
> +    int ret;
> +
> +    BlockDriverState *bs = s->common.bs;
> +
> +    for (;;) {
> +        uint64_t delay_ns = 0;
> +        int64_t cnt;
> +        bool should_complete;
> +
> +        if (s->ret < 0) {
> +            ret = s->ret;
> +            goto immediate_exit;
> +        }

You might as well replace the gotos with returns (either "return ret;"
or "return s->ret;") and the breaks with "return 0;").

> +
> +        cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> +        /* s->common.offset contains the number of bytes already processed so
> +         * far, cnt is the number of dirty sectors remaining and
> +         * s->sectors_in_flight is the number of sectors currently being
> +         * processed; together those are the current total operation length */
> +        s->common.len = s->common.offset +
> +                        (cnt + s->sectors_in_flight) * BDRV_SECTOR_SIZE;
> +
> +        /* Note that even when no rate limit is applied we need to yield
> +         * periodically with no pending I/O so that bdrv_drain_all() returns.
> +         * We do so every SLICE_TIME nanoseconds, or when there is an error,
> +         * or when the source is clean, whichever comes first.
> +         */
> +        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 ||
> +                (cnt == 0 && s->in_flight > 0)) {
> +                trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> +                s->waiting_for_io = true;
> +                qemu_coroutine_yield();
> +                s->waiting_for_io = false;
> +                continue;
> +            } else if (cnt != 0) {
> +                delay_ns = mirror_iteration(s);
> +            }
> +        }
> +
> +        should_complete = false;
> +        if (s->in_flight == 0 && cnt == 0) {
> +            trace_mirror_before_flush(s);
> +            ret = bdrv_flush(s->target);
> +            if (ret < 0) {
> +                if (mirror_error_action(s, false, -ret) ==
> +                    BLOCK_ERROR_ACTION_REPORT) {
> +                    goto immediate_exit;
> +                }
> +            } else {
> +                /* We're out of the streaming phase.  From now on, if the job
> +                 * is cancelled we will actually complete all pending I/O and
> +                 * report completion.  This way, block-job-cancel will leave
> +                 * the target in a consistent state.
> +                 */
> +                if (!s->synced) {
> +                    block_job_event_ready(&s->common);
> +                    s->synced = true;
> +                }
> +
> +                should_complete = s->should_complete ||
> +                    block_job_is_cancelled(&s->common);
> +                cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> +            }
> +        }
> +
> +        if (cnt == 0 && should_complete) {
> +            /* The dirty bitmap is not updated while operations are pending.
> +             * If we're about to exit, wait for pending operations before
> +             * calling bdrv_get_dirty_count(bs), or we may exit while the
> +             * source has dirty data to copy!
> +             *
> +             * Note that I/O can be submitted by the guest while
> +             * mirror_populate runs.
> +             */
> +            trace_mirror_before_drain(s, cnt);
> +            bdrv_drain(bs);
> +            cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> +        }
> +
> +        ret = 0;
> +        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
> +        if (!s->synced) {
> +            block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
> +            if (block_job_is_cancelled(&s->common)) {
> +                break;
> +            }
> +        } else if (!should_complete) {
> +            delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
> +            block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
> +        } else if (cnt == 0) {
> +            /* The two disks are in sync.  Exit and report successful
> +             * completion.
> +             */
> +            assert(QLIST_EMPTY(&bs->tracked_requests));
> +            s->common.cancelled = false;
> +            break;
> +        }
> +        last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +    }
> +
> +immediate_exit:
> +    return ret;
> +
> +}
> +
>  static void mirror_free_init(MirrorBlockJob *s)
>  {
>      int granularity = s->granularity;
> @@ -485,103 +594,8 @@ static void coroutine_fn mirror_run(void *opaque)
>      }
>  
>      bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> -    for (;;) {
> -        uint64_t delay_ns = 0;
> -        int64_t cnt;
> -        bool should_complete;
>  
> -        if (s->ret < 0) {
> -            ret = s->ret;
> -            goto immediate_exit;
> -        }
> -
> -        cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> -        /* s->common.offset contains the number of bytes already processed so
> -         * far, cnt is the number of dirty sectors remaining and
> -         * s->sectors_in_flight is the number of sectors currently being
> -         * processed; together those are the current total operation length */
> -        s->common.len = s->common.offset +
> -                        (cnt + s->sectors_in_flight) * BDRV_SECTOR_SIZE;
> -
> -        /* Note that even when no rate limit is applied we need to yield
> -         * periodically with no pending I/O so that bdrv_drain_all() returns.
> -         * We do so every SLICE_TIME nanoseconds, or when there is an error,
> -         * or when the source is clean, whichever comes first.
> -         */
> -        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 ||
> -                (cnt == 0 && s->in_flight > 0)) {
> -                trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> -                s->waiting_for_io = true;
> -                qemu_coroutine_yield();
> -                s->waiting_for_io = false;
> -                continue;
> -            } else if (cnt != 0) {
> -                delay_ns = mirror_iteration(s);
> -            }
> -        }
> -
> -        should_complete = false;
> -        if (s->in_flight == 0 && cnt == 0) {
> -            trace_mirror_before_flush(s);
> -            ret = bdrv_flush(s->target);
> -            if (ret < 0) {
> -                if (mirror_error_action(s, false, -ret) ==
> -                    BLOCK_ERROR_ACTION_REPORT) {
> -                    goto immediate_exit;
> -                }
> -            } else {
> -                /* We're out of the streaming phase.  From now on, if the job
> -                 * is cancelled we will actually complete all pending I/O and
> -                 * report completion.  This way, block-job-cancel will leave
> -                 * the target in a consistent state.
> -                 */
> -                if (!s->synced) {
> -                    block_job_event_ready(&s->common);
> -                    s->synced = true;
> -                }
> -
> -                should_complete = s->should_complete ||
> -                    block_job_is_cancelled(&s->common);
> -                cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> -            }
> -        }
> -
> -        if (cnt == 0 && should_complete) {
> -            /* The dirty bitmap is not updated while operations are pending.
> -             * If we're about to exit, wait for pending operations before
> -             * calling bdrv_get_dirty_count(bs), or we may exit while the
> -             * source has dirty data to copy!
> -             *
> -             * Note that I/O can be submitted by the guest while
> -             * mirror_populate runs.
> -             */
> -            trace_mirror_before_drain(s, cnt);
> -            bdrv_drain(bs);
> -            cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> -        }
> -
> -        ret = 0;
> -        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
> -        if (!s->synced) {
> -            block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
> -            if (block_job_is_cancelled(&s->common)) {
> -                break;
> -            }
> -        } else if (!should_complete) {
> -            delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
> -            block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
> -        } else if (cnt == 0) {
> -            /* The two disks are in sync.  Exit and report successful
> -             * completion.
> -             */
> -            assert(QLIST_EMPTY(&bs->tracked_requests));
> -            s->common.cancelled = false;
> -            break;
> -        }
> -        last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -    }
> +    ret = mirror_do_iteration(s, last_pause_ns);
>  
>  immediate_exit:
>      if (s->in_flight > 0) {
>