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