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

[Kevin Wolf]

Am 28.09.2015 um 05:29 hat Jeff Cody geschrieben:
> 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 <address@hidden>
> ---
>  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;
> +        }
> +
> +        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;
> +
> +}

Did you leave this additional empty line there intentionally?

With or without it removed, and also with or without Paolo's suggestion
regarding goto vs. return applied:

Reviewed-by: Kevin Wolf <address@hidden>