Re: [Qemu-devel] [PATCH v2 4/4] block/dirty-bitmaps: implement inconsistent bit

[Vladimir Sementsov-Ogievskiy]

28.02.2019 2:48, John Snow wrote:
> 
> 
> On 2/25/19 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 23.02.2019 3:22, John Snow wrote:
>>> Set the inconsistent bit on load instead of rejecting such bitmaps.
>>> There is no way to un-set it; the only option is to delete it.
>>>
>>> Obvervations:
>>> - bitmap loading does not need to update the header for in_use bitmaps.
>>> - inconsistent bitmaps don't need to have their data loaded; they're
>>>     glorified corruption sentinels.
>>> - bitmap saving does not need to save inconsistent bitmaps back to disk.
>>> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>>>     bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>>>     being eventually flushed back to disk.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>    block/qcow2-bitmap.c | 77 +++++++++++++++++++++++---------------------
>>>    1 file changed, 40 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 3ee524da4b..d1cc11da88 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState 
>>> *bs,
>>>        uint32_t granularity;
>>>        BdrvDirtyBitmap *bitmap = NULL;
>>>    
>>> +    granularity = 1U << bm->granularity_bits;
>>> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
>>> +    if (bitmap == NULL) {
>>> +        goto fail;
>>> +    }
>>> +
>>>        if (bm->flags & BME_FLAG_IN_USE) {
>>> -        error_setg(errp, "Bitmap '%s' is in use", bm->name);
>>> -        goto fail;
>>> +        /* Data is unusable, skip loading it */
>>> +        return bitmap;
>>>        }
>>>    
>>>        ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
>>> @@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState 
>>> *bs,
>>>            goto fail;
>>>        }
>>>    
>>> -    granularity = 1U << bm->granularity_bits;
>>> -    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
>>> -    if (bitmap == NULL) {
>>> -        goto fail;
>>> -    }
>>> -
>>>        ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
>>>        if (ret < 0) {
>>>            error_setg_errno(errp, -ret, "Could not read bitmap '%s' from 
>>> image",
>>> @@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, 
>>> Error **errp)
>>>        Qcow2Bitmap *bm;
>>>        GSList *created_dirty_bitmaps = NULL;
>>>        bool header_updated = false;
>>> +    bool needs_update = false;
>>>    
>>>        if (s->nb_bitmaps == 0) {
>>>            /* No bitmaps - nothing to do */
>>> @@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, 
>>> Error **errp)
>>>        }
>>>    
>>>        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>>> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>>> -            if (bitmap == NULL) {
>>> -                goto fail;
>>> -            }
>>> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>>> +        if (bitmap == NULL) {
>>> +            goto fail;
>>> +        }
>>>    
>>> -            if (!(bm->flags & BME_FLAG_AUTO)) {
>>> -                bdrv_disable_dirty_bitmap(bitmap);
>>> -            }
>>> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
>>> +        if (bm->flags & BME_FLAG_IN_USE) {
>>> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
>>> +        } else {
>>> +            /* NB: updated flags only get written if can_write(bs) is 
>>> true. */
>>>                bm->flags |= BME_FLAG_IN_USE;
>>> -            created_dirty_bitmaps =
>>> -                    g_slist_append(created_dirty_bitmaps, bitmap);
>>> +            needs_update = true;
>>>            }
>>> +        if (!(bm->flags & BME_FLAG_AUTO)) {
>>> +            bdrv_disable_dirty_bitmap(bitmap);
>>> +        }
>>> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
>>
>> We define persistent bitmaps as which we are going to store.. But we are
>> not going to store inconsistent bitmaps. So inconsistent bitmaps are not 
>> persistent.
>>
> 
> I changed the wording for v3.
> 
>>> +        created_dirty_bitmaps =
>>> +            g_slist_append(created_dirty_bitmaps, bitmap);
>>>        }
>>>    
>>> -    if (created_dirty_bitmaps != NULL) {
>>> +    if (needs_update) {
>>
>> created_dirty_bitmaps list needed only for setting them readonly, if failed 
>> write that
>> they are IN_USE. So, instead of creating additional variable, better is just 
>> not include
>> IN_USE bitmaps to this list. And it may be renamed like set_in_use_list.
>>
> 
> If I don't add them to the list, then one of the bitmaps in the list
> fails, I don't know which bitmap to remove when we go to the error exit.


Oops, you are right.

> 
>>>            if (can_write(bs)) {
>>>                /* in_use flags must be updated */
>>>                int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>>> @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState 
>>> *bs, bool *header_updated,
>>>        }
>>>    
>>>        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>>> -            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>> -            if (bitmap == NULL) {
>>> -                continue;
>>> -            }
>>> -
>>> -            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>> -                error_setg(errp, "Bitmap %s is not readonly but not marked"
>>> -                                 "'IN_USE' in the image. Something went 
>>> wrong,"
>>> -                                 "all the bitmaps may be corrupted", 
>>> bm->name);
>>> -                ret = -EINVAL;
>>> -                goto out;
>>> -            }
>>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>> +        if (bitmap == NULL) {
>>> +            continue;
>>> +        }
>>
>>>    
>>> -            bm->flags |= BME_FLAG_IN_USE;
>>> -            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>>> +        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>
>> so, we'll fail, as we didn't marked inconsistetn bitmaps as readonly above. 
>> Should we?
>> Or we may check here inconsistance directly.
>>
> 
> OH! I didn't mean to do this -- I want to mark all inconsistent bitmaps
> as readonly, yes. I will fix this.
> 
>>> +            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);
>>
>> Hmm, your wording covers invalid case when inconsistent bitmap is not 
>> IN_USE, and sounds
>> better for user, though may be less precise (as it may be not one reopen in 
>> a sequence..).
>> Ok for me.
>>
> 
> With all inconsistent bitmaps marked as readonly, this shouldn't ever
> actually trigger. All persistent bitmaps on a drive that hits
> reopen_rw_hint ought to be readonly, right?

not necessary I think. Some bitmaps may be created through qmp command, they 
will not be readonly,
as they are not flushed to storage.

But it of course doesn't lead to that message. Yes it should never happen I 
think, so it is worded in
manner "This is a bug". On the other hand, assume that someone illegally 
modifies image, while
Qemu is running and clear IN_USE bit. In this case we'll see that message.

> 
> That's my assumption; that we should never see this message.
> 
>>> +            ret = -EINVAL;
>>> +            goto out;
>>>            }
>>> +
>>> +        bm->flags |= BME_FLAG_IN_USE;
>>> +        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>>>        }
>>>    
>>>        if (ro_dirty_bitmaps != NULL) {
>>> @@ -1424,8 +1427,8 @@ void 
>>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>            Qcow2Bitmap *bm;
>>>    
>>>            if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
>>> -            bdrv_dirty_bitmap_readonly(bitmap))
>>> -        {
>>> +            bdrv_dirty_bitmap_readonly(bitmap) ||
>>> +            bdrv_dirty_bitmap_inconsistent(bitmap)) {
>>>                continue;
>>>            }
>>>    
>>>
>>
>>
> 
> Sending a V3 for further critique, but freeze is soon.
> 


-- 
Best regards,
Vladimir