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

[Max Reitz]

On 08.06.2016 16:38, Max Reitz wrote:
> 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).

Good news: Turns out I was wrong. I was somehow mixing things up with
blockdev-snapshot (don't ask me why, I have no clue).

So I think it'd be fine to rely on the user that the backing chain of
the target is correct.

Max

> 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