[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup.
From: |
Ian Main |
Subject: |
Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup. |
Date: |
Tue, 16 Jul 2013 11:17:49 -0700 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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