Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS

[Kevin Wolf]

Am 08.06.2016 um 17:39 hat Nir Soffer geschrieben:
> On Wed, Jun 8, 2016 at 12:32 PM, Kevin Wolf <address@hidden> wrote:
> > Am 06.06.2016 um 16:42 hat Max Reitz geschrieben:
> >> Currently, we are trying to move the backing BDS from the source to the
> >> target in bdrv_replace_in_backing_chain() which is called from
> >> mirror_exit(). However, mirror_complete() already tries to open the
> >> target's backing chain with a call to bdrv_open_backing_file().
> >>
> >> First, we should only set the target's backing BDS once. Second, the
> >> mirroring block job has a better idea of what to set it to than the
> >> generic code in bdrv_replace_in_backing_chain() (in fact, the latter's
> >> conditions on when to move the backing BDS from source to target are not
> >> really correct).
> >>
> >> Therefore, remove that code from bdrv_replace_in_backing_chain() and
> >> leave it to mirror_complete().
> >>
> >> However, mirror_complete() in turn pursues a questionable strategy by
> >> employing bdrv_open_backing_file(): On the one hand, because this may
> >> open the wrong backing file with drive-mirror in "existing" mode, or
> >> because it will not override a possibly wrong backing file in the
> >> blockdev-mirror case.
> >>
> >> On the other hand, we want to reuse the existing backing chain of the
> >> source instead of opening everything anew, because the latter results in
> >> having multiple BDSs for a single physical file and thus potentially
> >> concurrent access which we should try to avoid.
> >
> > Careful, this "wrong" backing file might actually be intended!
> >
> > Consider a case where you want to move an image with its whole backing
> > chain to different storage. In that case, you would copy all of the
> > backing files (cp is good enough, they are read-only), create the
> > destination image which already points at the copied backing chain, and
> > then mirror in "existing" mode.
> >
> > The intention is obviously that after the job completion the new backing
> > chain is used and not the old one.
> >
> > I know that such cases were discussed when mirroring was introduced, I'm
> > not sure whether it's actually used. We need some input there:
> >
> > Eric, can you tell us whether libvirt makes use of such a setup?
> >
> > Nir, I'm not sure who is the right person in oVirt these days, but do
> > you either know yourself whether oVirt requires this to work, or do you
> > know who else would know?
> 
> I'm the right person, thanks for keeping me in the loop.
> 
> What you describe is how we migrate a disk from one storage to another:
> 
> 1. Create a vm snapshot
> 2. Create a volume on the destination storage for the snapshot
> 3. Start mirroring from the source snapshot to the destination snapshot
>     using libvirt virDomainBlockCopy:
>     https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockCopy

With VIR_DOMAIN_BLOCK_COPY_SHALLOW set, right? (That is, sync=top in QMP
speech.)

> 4. Copy the reset of the chain from source to destination using qemu-img 
> convert
> 5. Pivot to the new chain using libvirt virDomainBlockJobAbort
>     
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockJobAbort
> 6. Remove the old chain
> 
> source and target can be files or block device, and we plan to support also
> rbd and gluster volumes as target, maybe also as source.

Thanks, Nir, we should then do our best not to break it.

Max, maybe we can add a qemu-iotests case that does the exact same thing
as oVirt does?

Kevin