Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[John Snow <jsnow@redhat.com>]

On 9/27/19 3:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 2:21, John Snow wrote:
>>
>>
>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> - Correct check for write access to file child, and in correct place
>>>    (only if we want to write).
>>> - Support reopen rw -> rw (which will be used in following commit),
>>>    for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
>>>    bitmap is marked IN_USE in the image.
>>> - Consider unexpected bitmap as a corruption and check other
>>>    combinations of in-image and in-RAM bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2-bitmap.c | 56 +++++++++++++++++++++++++++++---------------
>>>   1 file changed, 37 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index a636dc50ca..e276a95154 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>>>       Qcow2BitmapList *bm_list;
>>>       Qcow2Bitmap *bm;
>>>       GSList *ro_dirty_bitmaps = NULL;
>>> -    int ret = 0;
>>> +    int ret = -EINVAL;
>>> +    bool need_header_update = false;
>>>   
>>>       if (s->nb_bitmaps == 0) {
>>>           /* No bitmaps - nothing to do */
>>>           return 0;
>>>       }
>>>   
>>> -    if (!can_write(bs)) {
>>> -        error_setg(errp, "Can't write to the image on reopening bitmaps rw");
>>> -        return -EINVAL;
>>> -    }
>>> -
>>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>>                                  s->bitmap_directory_size, errp);
>>>       if (bm_list == NULL) {
>>> @@ -1128,32 +1124,54 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>>>   
>>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>           BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>> -        if (bitmap == NULL) {
>>> -            continue;
>>> -        }
>>>   
>>> -        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>> -            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
>>> -                       "not marked as readonly. This is a bug, something went "
>>> -                       "wrong. All of the bitmaps may be corrupted", bm->name);
>>> -            ret = -EINVAL;
>>> +        if (!bitmap) {
>>> +            error_setg(errp, "Unexpected bitmap '%s' in the image '%s'",
>>> +                       bm->name, bs->filename);
>>>               goto out;
>>>           }
>>>   
>>
>> I think you can actually drop the definite article, because the image
>> name is the specifier.
>>
>> "Unexpected bitmap '%s' in image '%s'" is sufficient.
>>
>>> -        bm->flags |= BME_FLAG_IN_USE;
>>> -        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>>> +        if (!(bm->flags & BME_FLAG_IN_USE)) {
>>> +            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>> +                error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE "
>>> +                           "in the image '%s' and not marked readonly in RAM",
>>> +                           bm->name, bs->filename);
>>> +                goto out;
>>> +            }
>>> +            if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
>>> +                error_setg(errp, "Corruption: bitmap '%s' is inconsistent but "
>>> +                           "is not marked IN_USE in the image '%s'", bm->name,
>>> +                           bs->filename);
>>> +                goto out;
>>> +            }
>>
>> We support RW --> RW now, but what happens if something is marked IN_USE
>> on RO --> RW? It's not obvious from this function alone why that's safe
>> to ignore.
> 
> If bitmap is IN_USE in the image, it's always safe to ignore it, as it's marked as
> invalid anyway.
> 

Oh, okay -- we're relying on the initial open to have handled this for
us. I only asked because you're doing a lot of additional "sanity
checking" here and wondered.

(That's one drawback to doing sanity checking -- you give the impression
that we expect these combinations to be able to occur.)

> Consider RO->RW with IN_USE.
> 
> if corresponding BdrvDirtyBitmap is inconsistent, it's OK.
> 
> If not, how can that be? I can't imagine. But we have correct bitmap in RAM, should
> we mark it inconsistent, because of bitmap in image? I don't know.
> 

Yeah, I wouldn't know what to make of this occurrence either. I don't
expect it to be able to happen.

If it's +ro -inconsistent and +in_use, we absolutely have a bug
somewhere and we don't know what the right thing to do is because we're
outside of the expected state machine.

Hmm...

If it was RO, then we know we wouldn't be able to have any changes to
disk since we loaded the bitmap, and yet we have a clear state mismatch.

1) We loaded the bitmap incorrectly. Not very likely, but very severe if
true.
2) The disk changed without QEMU's consent.

It's probably the case that we shouldn't be working on this image
anymore -- this would be a very high-level corruption event; the
qcow2_signal_corruption kind.

AKA: "I have no idea what the hell happened, and since it's not possible
to know, please cease using this image immediately."

--js