Re: [Qemu-devel] [PATCH v4 2/6] add basic backup support to block driver

[Stefan Hajnoczi]

On Thu, Feb 21, 2013 at 05:23:30AM +0000, Dietmar Maurer wrote:
> > > +static void coroutine_fn backup_run(void *opaque) {
> > > +    BackupBlockJob *job = opaque;
> > > +    BlockDriverState *bs = job->common.bs;
> > > +    assert(bs);
> > > +
> > > +    int64_t start, end;
> > > +
> > > +    start = 0;
> > > +    end = (bs->total_sectors + BACKUP_BLOCKS_PER_CLUSTER - 1) /
> > > +        BACKUP_BLOCKS_PER_CLUSTER;
> > > +
> > > +    DPRINTF("backup_run start %s %zd %zd\n", bdrv_get_device_name(bs),
> > > +            start, end);
> > > +
> > > +    int ret = 0;
> > > +
> > > +    for (; start < end; start++) {
> > > +        if (block_job_is_cancelled(&job->common)) {
> > > +            ret = -1;
> > > +            break;
> > > +        }
> > > +
> > > +        /* we need to yield so that qemu_aio_flush() returns.
> > > +         * (without, VM does not reboot)
> > > +        * Note: use 1000 instead of 0 (0 prioritize this task too
> > > + much)
> > 
> > indentation
> > 
> > What does "0 prioritize this task too much" mean?  If no rate limit has 
> > been set
> > the job should run at full speed.  We should not hardcode arbitrary delays 
> > like
> > 1000.
> 
> The VM itself gets somehow slower during backup - do not know why. As 
> workaround sleep 1000 works.

Please find out why, it's a bug that an arbitrary sleep hides but
doesn't fix (plus the sleep makes backup less efficient).

If the VM becomes slow this loop is probably "spinning" without doing
blocking I/O and only doing sleep 0.  I guess that can happen when you
loop over blocks that have already been backed up (bit has been set)?

Stefan