Comment 5 for bug 1814418

On 2/12/19 5:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.02.2019 4:10, John Snow wrote:
>> On 2/4/19 11:22 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> In-use bitmaps in the image may appear only after some kind of qemu crash. Isn't
>>> it a good reason to call qemu-img check? So, haw about just forbid to start qemu
>>> if there are any in-use bitmaps?
>>>
>>
>> I have wondered this recently.
>>
>> I am against just silently loading and deleting the bitmaps because I
>> don't want any chance for data corruption if the bitmap gets lost
>> accidentally. I like the loud failure.
>>
>> I kind of like the idea of just failing to load the image at all,
>> because it does necessitate user action, but it feels a little user hostile.
>
> Yes, it may be to noisy to have to call qemu-img check after any unexpected process
> kill, and it's not like real machine behave.
>
>>
>> Maybe we can do some kind of soft-load for corrupted bitmaps where they
>> will remain "locked" and cannot be re-written to disk until the user
>> issues a clear command to reset them -- so the user knows full well the
>> backup chain is broken. Maybe this is a good solution to the problem?
>>
>> What do you think?
>
> It should not be just "locked", it should be visibly "corrupted", for user to understand
> the reason of why bitmap is unusable. So, it should be a new state of flag.
>

Right, sure. It will behave similarly to a locked, disabled bitmap. You
can't do anything to it, it doesn't record writes, etc. A new flag is
helpful for the purpose.

> So, instead of just ignoring in-use bitmaps on start, we load them, benefit of
> having them in list, but the only thing which can be done with them is
> block-dirty-bitmap-remove (and it's additional reason, why it can't be "locked" state).
>

I'd say remove or clear, both would make sense. I suppose we only *need*
one and remove covers all cases, so I'll go with this suggestion and
offer clear as an additional patch that we could take or leave.

> I'm not sure that we should load data for such bitmaps, so they may be loaded as
> BdrvDirtyBitmaps with .corrupted = true and .bitmap = NULL.
>

Probably doesn't hurt to just load a blank bitmap instead of trying to
special case it with the NULL, but understood: we don't have to load the
data because it's junk.

>
> Hmm, go and check that it will not break bitmaps migration related logic, which is described
> in BIG comment in block/qcow2.c. Looks like all is ok, and in only valid case when we could
> see in-use bitmaps is
>
> * One remaining possible case when we don't want load bitmaps:
> *
> * 4. Open disk in inactive mode in target vm (bitmaps are migrating or
> * will be loaded on invalidation, no needs try loading them before)
>
>
> and we don't try loading bitmaps in this case:
>
> if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
> < load bitmaps >
>
> So the only thing to do additionally here is enhance the comment, to
> s/no needs/must not do it, as they will be loaded as corrupted/.
>
>

Sounds good to me, I'll get cooking.