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

[Ian Main]

On Mon, Jul 15, 2013 at 10:47:25PM +0200, Paolo Bonzini wrote:
> Il 15/07/2013 19:49, Ian Main ha scritto:
> > 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.
> 
> No problem, it's better to be verbose than to have an extra iteration.
> 
> > 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.
> 
> Yes, so far I understood it.
> 
> >> 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?
> 
> Yes, it helps---I think I'm right. :)
> 
> For mode 'NONE' we only copy in the original data from the disk image,
> but what makes us read the old image for the sectors we haven't copied
> yet?  For that to work, we need to set target_bs->backing_hd.
> 
> Now, the question is whether to do it for mode 'NONE' only, or also for
> the others.  It is certainly required for mode 'NONE', because this mode
> will never get complete data in the destination.  But it may actually be
> a good idea for other sync modes.  This is because block-backup is a
> sort of live snapshot, except that the active image remains the
> pre-snapshot one.  Thus it makes sense to have the same defaults as
> blockdev-snapshot-live, including making "qcow2" the default format.
> 
> A user that wants to use an NBD client target for backup (this is
> different from fleecing, which uses a temporary file + an NBD server)
> should specify a "raw" format anyway, independent of the default we
> choose, so this change doesn't affect other use cases.
> 
> On top of this, target_bs->backing_hd needs to be set manually to bs
> itself during the copy (*even if the source used in bdrv_img_create is
> bs->backing_hd, or is NULL!*), and reset just before closing target_bs.
>  This way, if the target is exposed via the NBD server, it reads as a
> full copy of the image even while the backup is ongoing.  This can be
> done using BDRV_O_NO_BACKING.
>
> Paolo

Yes I see what you are saying now.  I really like this idea.  I will
make up a new patch and include a separate patch to default to qcow2 as
well.

        Ian