Re: [Qemu-block] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

11.03.2019 21:05, John Snow wrote:
> 
> 
> On 3/11/19 1:36 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 11.03.2019 19:18, Eric Blake wrote:
>>> On 3/5/19 5:43 PM, John Snow wrote:
>>>> This series aims to enable block resizes when persistent bitmaps are
>>>> in use. The basic approach here is to recognize that we now load all
>>>> persistent bitmaps in memory, and so we can rely on in-memory resizes
>>>> and then flush the changed metadata back to disk.
>>>>
>>>> One part that is potentially now quite strange is that bitmap resizes
>>>> may happen twice: once during the qcow2 resize event only if persistent
>>>> bitmaps are found, and then again as part of the generic resize callback
>>>> event whether or not we have any persistent bitmaps.
>>>>
>>>> The second round is required if we are not using qcow2 or we have only
>>>> transient bitmaps. The first round is required as we wish to flush the
>>>> bitmaps back to disk atomically with the qcow2 resize to avoid violating
>>>> our invariants for the bitmap metadata which is checked in many places.
>>>>
>>>> This is harmless; hbitmap_truncate will recognize the second round as
>>>> a no-op.
>>>
>>> FWIW - I have not yet reviewed this series closely, but I think it would
>>> be wise to get this initial cut in before softfreeze (we can make
>>> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
>>> lot harder to add the series at all if it misses softfreeze).
>>>
>>
>> I still not convinced that we need bitmap flushing. I think (and Den supports
>> me) that it's a bad idea. It makes things more difficult without a reason,
>> except improving debugging with --force-share which should never be used in
>> production.
>>
> 
> I know you don't want it in the general case, but I think resizing
> represents a genuine case for exactly when we want it.
> 
> Set aside your concerns about general case flushing for now -- this is a
> distraction -- and let us consider ONLY resize events.
> 
> If QEMU crashes after a resize event, we will be unable to even open
> qcow2 images if we don't resize the metadata information and write it
> back out to disk.
> 
> It is far simpler -- logically and technically -- to just write this
> data out to disk during a resize event. It won't happen often, shouldn't
> incur much of an IO penalty, and (I think) is algorithmically
> straightforward.
> 
> If it's really that much of a problem, we can revise it during the RC
> period to eliminate the flush.
> 
>> Bitmaps are stored in qcow2_inactivate() which is true place for flushing 
>> caches.
>>


Hmm, you are so sure, that I can't continue arguing)
What about patch 3, did you test it?
It's just strange for me that we are going to push unreviewed series just to 
have
the feature in 4.0.. Why is it so important?
Also, we have no comments from Max and Kevin who are maintainers of the only 
file,
changed in the series. And why this don't go through their trees?
This all just breaks all patterns I'm used to..

And I hope, I'll send a counter-propasal without flushing in a few minutes...


-- 
Best regards,
Vladimir