On Wed, 09/04 14:35, Stefan Hajnoczi wrote:
> 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.
Can we mark the dirty bitmap immediately upon getting guest write request?
Fam
> 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.
