Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps

[Denis V. Lunev]

On 02/17/2017 03:54 PM, Vladimir Sementsov-Ogievskiy wrote:
> 17.02.2017 17:24, Kevin Wolf wrote:
>> Am 17.02.2017 um 14:48 hat Denis V. Lunev geschrieben:
>>> On 02/17/2017 04:34 PM, Kevin Wolf wrote:
>>>> Am 17.02.2017 um 14:22 hat Denis V. Lunev geschrieben:
>>>>> But for sure this is bad from the downtime point of view.
>>>>> On migrate you will have to write to the image and re-read
>>>>> it again on the target. This would be very slow. This will
>>>>> not help for the migration with non-shared disk too.
>>>>>
>>>>> That is why we have specifically worked in a migration,
>>>>> which for a good does not influence downtime at all now.
>>>>>
>>>>> With a write we are issuing several write requests + sync.
>>>>> Our measurements shows that bdrv_drain could take around
>>>>> a second on an averagely loaded conventional system, which
>>>>> seems unacceptable addition to me.
>>>> I'm not arguing against optimising migration, I fully agree with
>>>> you. I
>>>> just think that we should start with a correct if slow base version
>>>> and
>>>> then add optimisation to that, instead of starting with a broken base
>>>> version and adding to that.
>>>>
>>>> Look, whether you do the expensive I/O on open/close and make that a
>>>> slow operation or whether you do it on invalidate_cache/inactivate
>>>> doesn't really make a difference in term of slowness because in
>>>> general
>>>> both operations are called exactly once. But it does make a difference
>>>> in terms of correctness.
>>>>
>>>> Once you do the optimisation, of course, you'll skip writing those
>>>> bitmaps that you transfer using a different channel, no matter whether
>>>> you skip it in bdrv_close() or in bdrv_inactivate().
>>>>
>>>> Kevin
>>> I do not understand this point as in order to optimize this
>>> we will have to create specific code path or option from
>>> the migration code and keep this as an ugly kludge forever.
>> The point that I don't understand is why it makes any difference for the
>> follow-up migration series whether the writeout is in bdrv_close() or
>> bdrv_inactivate(). I don't really see the difference between the two
>> from a migration POV; both need to be skipped if we transfer the bitmap
>> using a different channel.
>>
>> Maybe I would see the reason if I could find the time to look at the
>> migration patches first, but unfortunately I don't have this time at the
>> moment.
>>
>> My point is just that generally we want to have a correctly working qemu
>> after every single patch, and even more importantly after every series.
>> As the migration series is separate from this, I don't think it's a good
>> excuse for doing worse than we could easily do here.
>>
>> Kevin
>
> With open/close all is already ok - bitmaps will not be saved because
> of BDRV_O_INACTIVE  and will not be loaded because of IN_USE.
>
>
in any case load/store happens outside of VM downtime.
The target is running at the moment of close on source,
the source is running at the moment of open on target.

Den