[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS |
Date: |
Wed, 8 Jun 2016 18:54:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 |
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