Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup.

[Ian Main]

On Mon, Jul 15, 2013 at 12:50:39PM +0200, Paolo Bonzini wrote:
> Il 08/07/2013 11:21, Fam Zheng ha scritto:
> > > Should the source be "bs" for MIRROR_SYNC_MODE_NONE?  Also in this case
> > > you may want to default the format to "qcow2" instead of bs's format.
> > 
> > Maybe not. "source" only affects when sync=top below here. For reading
> > the uncopied for target from source, we can't simply assign it to "bs"
> > for sync=none and create the new image with with bs->filename as backing
> > file, because that way we don't know how to open it with what we have
> > now: the backing file is already opened RW (as "bs").
> 
> But if we do not set the source, how can you read at all from the
> temporary backup destination?
> 
> If I read the code correctly, target_bs should be opened with
> BDRV_O_NO_BACKING, and then you should set target_bs->backing_hd = bs.
> Which in turn would only work for a format that supports backing files
> (such as qcow2).

OK well, I'll explain here my understanding.  I apologize if I explain
more than needed but it might be good to get this out there anyway.

When we do the create with:

bdrv_img_create(target, format, source->filename,                               
                                                                                
                                  
                source->drv->format_name, NULL,                                 
                                                                                
                                  
                size, flags, &local_err, false);                 

We are creating a new target image using the same backing file as the
original disk image.  Then any new data that has been laid on top of it
since creation is copied in the main backup_run() loop.  There is an
extra check in the 'TOP' case so that we don't bother to copy all the
data of the backing file as it already exists in the target.  This is
where the bdrv_co_is_allocated() is used to determine if the data exists
in the topmost layer or below.

Also any new data being written is intercepted via the write_notifier
hook which ends up calling backup_do_cow() to copy old data out before
it gets overwritten.

> I'm not sure how the "none" mode works with these patches.  It's quite
> possible I'm wrong, of course, but then the explanation should be in the
> code or the commit message.  It should be in the code or the commit
> message even if my suggestions are applied. :)

For mode 'NONE' we create the new target image and only copy in the
original data from the disk image starting from the time the call was
made.  This preserves the point in time data by only copying the parts
that are *going to change* to the target image.  This way we can
reconstruct the final image by checking to see if the given block exists
in the new target image first, and if it does not, you can get it from
the original image.  This is basically an optimization allowing you to
do point-in-time snapshots with low overhead vs the 'FULL' version.

Since there is no old data to copy out the loop in backup_run() for the
NONE case just calls qemu_coroutine_yield() which only wakes up after an
event (usually cancel in this case).  The rest is handled by the
before_write notifier which again calls backup_do_cow() to write out the
old data so it can be preserved.

Does that help?

        Ian