[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps
From: |
Denis V. Lunev |
Subject: |
Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps |
Date: |
Mon, 20 Feb 2017 12:21:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On 02/20/2017 12:15 PM, Kevin Wolf wrote:
> Am 18.02.2017 um 11:54 hat Denis V. Lunev geschrieben:
>> 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.
> If the contents of so-called persistent bitmaps is lost across
> migration and stale after bdrv_invalidate_cache, that's not "all ok" in
> my book. It's buggy.
>
>> 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.
> Which makes the operation in open/close meaningless: open reads data
> which may still by changed, and close cannot write to the image any more
> because ownership is already given up.
>
> Anyway, all of this isn't really productive. Please, can't you just
> answer that simple question that I asked you above: What problems would
> you get if you used invalidate_cache/inactivate instead of open/close
> for triggering these actions?
>
> If you can bring up some "this would break X", "it would be very hard to
> implement Y correctly then" or "in scenario Z, this would have unwanted
> effects", then we can figure out what to do. But not putting things in
> the proper place just because you don't feel like it isn't a very strong
> argument.
>
> Kevin
This will increase the downtime ~0.5-1 second for migrated VM
or will require very intrusive interface from migration code to
here to avoid bitmap save for inactivation on that path.
No other arguments so far.
Den
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, (continued)
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Kevin Wolf, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Vladimir Sementsov-Ogievskiy, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Kevin Wolf, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Denis V. Lunev, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Kevin Wolf, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Denis V. Lunev, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Kevin Wolf, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Vladimir Sementsov-Ogievskiy, 2017/02/17
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Denis V. Lunev, 2017/02/18
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Kevin Wolf, 2017/02/20
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps,
Denis V. Lunev <=
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Vladimir Sementsov-Ogievskiy, 2017/02/20
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Kevin Wolf, 2017/02/20
- Re: [Qemu-devel] [PATCH v15 08/25] block: introduce auto-loading bitmaps, Vladimir Sementsov-Ogievskiy, 2017/02/20
[Qemu-devel] [PATCH v15 02/25] specs/qcow2: do not use wording 'bitmap header', Vladimir Sementsov-Ogievskiy, 2017/02/15
[Qemu-devel] [PATCH v15 12/25] block/dirty-bitmap: add bdrv_dirty_bitmap_next(), Vladimir Sementsov-Ogievskiy, 2017/02/15
[Qemu-devel] [PATCH v15 04/25] tests: add hbitmap iter test, Vladimir Sementsov-Ogievskiy, 2017/02/15
[Qemu-devel] [PATCH v15 01/25] specs/qcow2: fix bitmap granularity qemu-specific note, Vladimir Sementsov-Ogievskiy, 2017/02/15
[Qemu-devel] [PATCH v15 17/25] qmp: add autoload parameter to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2017/02/15
[Qemu-devel] [PATCH v15 21/25] qcow2-bitmap: refcounts, Vladimir Sementsov-Ogievskiy, 2017/02/15