Re: [Qemu-devel] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, 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.

I think, s/delete it/delete the bitmap/, as "it" is the bit after "un-set 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.

hmm, and inconsitent bitmaps don't have this flag, isn't it?

> 
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/qcow2-bitmap.c | 103 ++++++++++++++++++++++---------------------
>   1 file changed, 53 insertions(+), 50 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3ee524da4b..c3b210ede1 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c

[..]

> @@ -962,35 +963,39 @@ 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;
> -            }
> -
> -            if (!(bm->flags & BME_FLAG_AUTO)) {
> -                bdrv_disable_dirty_bitmap(bitmap);
> -            }
> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
> -            bm->flags |= BME_FLAG_IN_USE;
> -            created_dirty_bitmaps =
> -                    g_slist_append(created_dirty_bitmaps, bitmap);
> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> +        if (bitmap == NULL) {
> +            goto fail;
>           }
> -    }
>   
> -    if (created_dirty_bitmaps != NULL) {
> -        if (can_write(bs)) {
> -            /* in_use flags must be updated */
> -            int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> -            if (ret < 0) {
> -                error_setg_errno(errp, -ret, "Can't update bitmap 
> directory");
> -                goto fail;
> -            }
> -            header_updated = true;
> +        if (bm->flags & BME_FLAG_IN_USE) {
> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
>           } else {
> -            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
> -                            (gpointer)true);
> +            /* NB: updated flags only get written if can_write(bs) is true. 
> */
> +            bm->flags |= BME_FLAG_IN_USE;
> +            needs_update = true;
>           }
> +        if (!(bm->flags & BME_FLAG_AUTO)) {
> +            bdrv_disable_dirty_bitmap(bitmap);
> +        }
> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
> +        created_dirty_bitmaps =
> +            g_slist_append(created_dirty_bitmaps, bitmap);
> +    }
> +
> +    if (needs_update && can_write(bs)) {
> +        /* in_use flags must be updated */
> +        int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Can't update bitmap directory");
> +            goto fail;
> +        }
> +        header_updated = true;
> +    }
> +
> +    if (!can_write(bs)) {
> +        g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
> +                        (gpointer)true);

hmmm..

I think, inconsistent bitmaps should have readonly flag always set or always 
unset, independently on can_write,
as we are not going to write something related to inconsistent bitmaps.

And I think, better is always unset, to have inconsistent bitmaps as something 
unrelated to readonly.

Readonly are bitmaps, which are not marked IN_USE on open for some reason..

I understand, that inconsistent bitmaps are some kind of readonly too, as we 
can't write to them..
But we can't read too anyway, and we don't interfere with reopen logic at all. 
So again, better is
don't mark inconsistent bitmaps as readonly.

>       }
>   
>       g_slist_free(created_dirty_bitmaps);
> @@ -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)) {
> +            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;
> +            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;
>           }
>   
> 


-- 
Best regards,
Vladimir