Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] block/qcow2-bitmap: rewrite bitmap reopening logic

[Vladimir Sementsov-Ogievskiy]

30.05.2019 11:23, Vladimir Sementsov-Ogievskiy wrote:
> 29.05.2019 21:08, John Snow wrote:
>> Max has picked this thread up for block discussion, so I'm going to
>> stick to slightly more bitmap related discussion here; we'll resume
>> block discussion in the other tail of this thread.
>>


[..]

>>>> And we mark it as inconsistent because we're not sure how we missed it
>>>> earlier. OK.
>>>>
>>>>> +                }
>>>>> +            } else if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>>>> +                corruption =
>>>>> +                    "Corruption: bitmap '%s' is not marked IN_USE in the 
>>>>> "
>>>>> +                    "image '%s' and not marked readonly in RAM. Will try 
>>>>> to "
>>>>> +                    "set IN_USE flag.";
>>>>> +
>>>>
>>>> And in this case, we find the bitmap but it's not marked readonly for
>>>> some reason.
>>>>
>>>>> +                bdrv_dirty_bitmap_set_readonly(bitmap, true);
>>>>
>>>> Why set it readonly again?
>>>
>>> It is because inconsistance is not synced to the image. "readonly" exactly
>>> means, that for some reasons we did not marked bitmap IN_USE in the image 
>>> and
>>> therefore must not write to it.
>>>
>>
>> That's one way of looking at what readonly means. Another was: "The
>> image this bitmap is attached to is read only, and any writes to this
>> bitmap are a logistical error."
>>
>>> So, yes, here occurs new thing: readonly-inconsistent bitmap. It blocks 
>>> guest
>>> writes until we sync it somehow to the image or remove. And we are going to 
>>> sync
>>> it at the end of this function.
>>>
>>
>> Right, we've not really used readonly in this way before. It makes sense
>> to a point, but it's a bit of a semantic overload -- the disk is
>> actually RW but the bitmap is RO; the problem that I have with this is
>> that we guard RO bitmaps with assertions and not errors,
> 
> Oops, you are right. I thought we have errors for guest writes in this case.

Aha, I was (partially) right, we have in bdrv_aligned_pwritev:
     if (bdrv_has_readonly_bitmaps(bs)) {
           return -EPERM;
       }

but what about discard, ...? seems it's not handled.



-- 
Best regards,
Vladimir