Re: [Qemu-devel] [PATCH v3 1/2] block: allow live commit of active image

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Aug 15, 2013 at 04:14:06PM +0800, Fam Zheng wrote:
> diff --git a/block/commit.c b/block/commit.c
> index 2227fc2..b5e024b 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -17,14 +17,13 @@
>  #include "block/blockjob.h"
>  #include "qemu/ratelimit.h"
>  
> -enum {
> -    /*
> -     * Size of data buffer for populating the image file.  This should be large
> -     * enough to process multiple clusters in a single call, so that populating
> -     * contiguous regions of the image is efficient.
> -     */
> -    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
> -};
> +/*
> + * Size of data buffer for populating the image file.  This should be large
> + * enough to process multiple clusters in a single call, so that populating
> + * contiguous regions of the image is efficient.
> + */
> +#define COMMIT_BUFFER_SECTORS 128
> +#define COMMIT_BUFFER_BYTES (COMMIT_BUFFER_SECTORS * BDRV_SECTOR_SIZE)

Changing from 512 KB to 64 KB can affect performance.  8 times as many
iops may be issued to copy data.

Also, the image's cluster size should really be taken into account.
Otherwise additional inefficiency will be suffered when we populate a
128 KB cluster with a COMMIT_BUFFER_SECTORS (64 KB) write only to
overwrite the remaining part in the next loop iteration.

This can be solved by setting dirty bitmap granularity to cluster size
or 64 KB minimum *and* finding continuous runs of dirty bits so larger
I/Os can be performed by the main loop (up to 512 KB in one request).

>  #define SLICE_TIME 100000000ULL /* ns */
>  
> @@ -34,11 +33,27 @@ typedef struct CommitBlockJob {
>      BlockDriverState *active;
>      BlockDriverState *top;
>      BlockDriverState *base;
> +    BlockDriverState *overlay;
>      BlockdevOnError on_error;
>      int base_flags;
>      int orig_overlay_flags;
> +    bool should_complete;
> +    bool ready;

Why introduce the ready state when the active layer is being committed?

There is no documentation update that mentions the job will not complete
by itself if the top image is active.

> +    for (;;) {
> +        int64_t cnt = bdrv_get_dirty_count(s->top);
> +        if (cnt == 0) {
> +            if (!s->overlay && !s->ready) {
> +                s->ready = true;
> +                block_job_ready(&s->common);
>              }
> -            ret = commit_populate(top, base, sector_num, n, buf);
> -            bytes_written += n * BDRV_SECTOR_SIZE;
> +            /* We can complete if user called complete job or the job is
> +             * committing non-active image */
> +            if (s->should_complete || s->overlay) {
> +                break;

This termination condition is not safe:

A write request only marks the dirty bitmap upon completion.  A guest
write request could still be in flight so we get cnt == 0 but we
actually have not copied all data into the base.

Completing safely is a little tricky because bdrv_drain_all() is
synchronous and we don't want to call that.  But waiting for
bs->tracked_requests to be empty is also bad because it's a busy wait.

Ideally we'd get woken up whenever a write request finishes.