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

[Kevin Wolf <kwolf@redhat.com>]

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?

> Thus, instead of invoking bdrv_open_backing_file(), just set the correct
> backing BDS directly via bdrv_set_backing_hd(). Also, do so only when
> mirror_complete() is certain to succeed.
> 
> In contrast to what bdrv_replace_in_backing_chain() did so far, we do
> not need to drop the source's backing file.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Leaving the actual code review for later when we have decided what
semantics we even want.

Kevin