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

[Max Reitz]

On 08.06.2016 11:32, Kevin Wolf 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!

True.

I still consider completely opening the backing chain not correct,
though, at least in absolute-paths mode, because this will result in
having at least two BDSs for single physical image files (once for the
old chain, once for the new one).

So let's go through everything.

== drive-mirror with absolute-paths ==

We already have the backing chain open (around the source BDS), and it's
definitely the correct one. So I think we can always reuse it for the
target.

== drive-mirror with existing ==

You're right, we should probably keep doing bdrv_open_backing_file()
because we cannot check whether the existing image has the same backing
chain as a new absolute-paths image would have had.

This is prone to give you some issues if you actually do want to have
the "default" backing chain, though, because of the multiple BDS thing.
This case is basically guaranteed to break with sync=none and default
image locking.

== blockdev-mirror ==

In theory the simplest one: We just assume the backing chain of the
target has been opened already, and then we blame the user if they have
created multiple BDSs per physical file.

Unluckily in practice, though, we require the target BDS to not have a
backing file at all. blockdev-mirror is just supposed to open the
backing chain after completion, which I really don't like (I don't think
a blockdev- command should do this kind of magic).

Maybe we should allow the target to have a backing file (I really don't
see why it shouldn't have one) and treat the non-backing case like
drive-mirror in existing mode.


Does that sound right?

Max


> 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 <address@hidden>
> 
> Leaving the actual code review for later when we have decided what
> semantics we even want.
> 
> Kevin
>

signature.asc
Description: OpenPGP digital signature