Re: [Qemu-devel] [PATCH 3/3] block: mirror - zero unallocated target sectors when zero init not present

[Jeff Cody <jcody@redhat.com>]

On Wed, Sep 30, 2015 at 05:26:28PM +0200, Kevin Wolf wrote:
> Am 30.09.2015 um 16:43 hat Jeff Cody geschrieben:
> > On Tue, Sep 29, 2015 at 12:52:33PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 29/09/2015 11:35, Kevin Wolf wrote:
> > > > The caller could be copying the backing file in the background and it
> > > > may not yet be finished.
> > > 
> > > Yes, and this is permitted (the destination file of mirroring is opened
> > > with BDRV_O_NO_BACKING).
> > > 
> > > Some more assumptions arise when block-job-complete is invoked, because
> > > at this point the content must not change under the guest's feet.
> > > Because block-job-complete does bdrv_open_backing_file on the
> > > destination, for sync!='full' it means that either 1) the image has no
> > > backing file, but it starts with the content of the backing file or 2)
> > > the image's backing file is complete at the time block-job-complete is
> > > invoked.
> > > 
> > > For mode!='existing' it is always case (2), and the backing file is
> > > complete all the time; for mode=='existing' the backing file could be
> > > copied in the background, and case (1) could happen as well.  An example
> > > of case (1) is replacing sync=='full' with a "fast copy" of the backing
> > > file (e.g. via btrfs's COW copies) and sync=='top'.  This should be valid.
> > 
> > One issue is that QEMU will do mode!='existing' && sync!='full' for
> > drivers that do not support backing files (raw host devices, for
> > instance).  We could refuse to start a mirror in the case of:
> > 
> >     mode != 'existing' && sync != 'full' && !target->drv->supports_backing
> > 
> > Alternatively, we could do the two-pass zero approach in this patch,
> > except under the following conditions:
> > 
> >     sync == 'full' || (mode != 'existing' && !target->drv->supports_backing)
> > 
> > (In the sync == 'full' case, we could also just queue all sectors, as
> > Kevin suggested)
> 
> I don't think that mode == 'existing' should play any role in the
> behaviour of any block job. There's no reason why doing an external
> 'qemu-img create' should make it do anything different compared to
> images created using the monitor.
>

As a general rule for blockjobs, I disagree.

Right away, there is a key difference: we don't know that the image is
(or should be) empty.  With mode != "existing", we know the image
should be empty, since we just created it (although for a host device,
it may have extraneous data in it).  So I think it is not so much what
we can assume about an existing image, as it is what we cannot assume.
And that could potentially influence some block jobs.

That said, I think I agree with simplifying the mirror case, as you
suggested earlier.  Namely, just adding all sectors into the dirty
bitmap when sync=='full'. That approach obviates the need for patches
1 & 2, and makes the single resulting patch pretty small.

The case of: 
             sync!='full' && 
             mode!='existing' && 
             !target->drv->supports_backing &&
             !bdrv_has_zero_init(target)

will result in an image with possible extraneous data, but I think I
agree with Paolo that it is either A) unimportant for the use case, or
B) user error.