Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature

[John Snow]

On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> Adds dirty-bitmaps feature to qcow2 format as specified in
> docs/specs/qcow2.txt
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/Makefile.objs        |   2 +-
>  block/qcow2-dirty-bitmap.c | 503 
> +++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c              |  56 +++++
>  block/qcow2.h              |  50 +++++
>  include/block/block_int.h  |  10 +
>  5 files changed, 620 insertions(+), 1 deletion(-)
>  create mode 100644 block/qcow2-dirty-bitmap.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 0d8c2a4..bff12b4 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -1,5 +1,5 @@
>  block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
> -block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o
> +block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o qcow2-dirty-bitmap.o
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
> new file mode 100644
> index 0000000..bc0167c
> --- /dev/null
> +++ b/block/qcow2-dirty-bitmap.c
> @@ -0,0 +1,503 @@
> +/*
> + * Dirty bitmpas for the QCOW version 2 format
> + *
> + * Copyright (c) 2014-2015 Vladimir Sementsov-Ogievskiy
> + *
> + * This file is derived from qcow2-snapshot.c, original copyright:
> + * Copyright (c) 2004-2006 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu-common.h"
> +#include "block/block_int.h"
> +#include "block/qcow2.h"
> +
> +void qcow2_free_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
> +        g_free(s->dirty_bitmaps[i].name);
> +    }
> +    g_free(s->dirty_bitmaps);
> +    s->dirty_bitmaps = NULL;
> +    s->nb_dirty_bitmaps = 0;
> +}
> +
> +int qcow2_read_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QCowDirtyBitmapHeader h;
> +    QCowDirtyBitmap *bm;
> +    int i, name_size;
> +    int64_t offset;
> +    int ret;
> +
> +    if (!s->nb_dirty_bitmaps) {
> +        s->dirty_bitmaps = NULL;
> +        s->dirty_bitmaps_size = 0;
> +        return 0;
> +    }
> +
> +    offset = s->dirty_bitmaps_offset;
> +    s->dirty_bitmaps = g_new0(QCowDirtyBitmap, s->nb_dirty_bitmaps);
> +
> +    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
> +        /* Read statically sized part of the dirty_bitmap header */
> +        offset = align_offset(offset, 8);
> +        ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +
> +        offset += sizeof(h);
> +        bm = s->dirty_bitmaps + i;
> +        bm->l1_table_offset = be64_to_cpu(h.l1_table_offset);
> +        bm->l1_size = be32_to_cpu(h.l1_size);
> +        bm->bitmap_granularity = be32_to_cpu(h.bitmap_granularity);
> +        bm->bitmap_size = be64_to_cpu(h.bitmap_size);
> +
> +        name_size = be16_to_cpu(h.name_size);
> +
> +        /* Read dirty_bitmap name */
> +        bm->name = g_malloc(name_size + 1);
> +        ret = bdrv_pread(bs->file, offset, bm->name, name_size);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        offset += name_size;
> +        bm->name[name_size] = '\0';
> +
> +        if (offset - s->dirty_bitmaps_offset > QCOW_MAX_DIRTY_BITMAPS_SIZE) {
> +            ret = -EFBIG;
> +            goto fail;
> +        }
> +    }
> +
> +    assert(offset - s->dirty_bitmaps_offset <= INT_MAX);
> +    s->dirty_bitmaps_size = offset - s->dirty_bitmaps_offset;
> +    return 0;
> +
> +fail:
> +    qcow2_free_dirty_bitmaps(bs);
> +    return ret;
> +}
> +
> +/* Add at the end of the file a new table of dirty bitmaps */
> +static int qcow2_write_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QCowDirtyBitmap *bm;
> +    QCowDirtyBitmapHeader h;
> +    int i, name_size, dirty_bitmaps_size;
> +    int64_t offset, dirty_bitmaps_offset = 0;
> +    int ret;
> +
> +    int old_dirty_bitmaps_size = s->dirty_bitmaps_size;
> +    int64_t old_dirty_bitmaps_offset = s->dirty_bitmaps_offset;
> +
> +    /* Compute the size of the dirty bitmaps table */
> +    offset = 0;
> +    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
> +        bm = s->dirty_bitmaps + i;
> +        offset = align_offset(offset, 8);
> +        offset += sizeof(h);
> +        offset += strlen(bm->name);
> +
> +        if (offset > QCOW_MAX_DIRTY_BITMAPS_SIZE) {
> +            ret = -EFBIG;
> +            goto fail;
> +        }
> +    }
> +
> +    assert(offset <= INT_MAX);
> +    dirty_bitmaps_size = offset;
> +
> +    /* Allocate space for the new dirty bitmap table */
> +    dirty_bitmaps_offset = qcow2_alloc_clusters(bs, dirty_bitmaps_size);
> +    offset = dirty_bitmaps_offset;
> +    if (offset < 0) {
> +        ret = offset;
> +        goto fail;
> +    }
> +    ret = bdrv_flush(bs);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    /* The dirty bitmap table position has not yet been updated, so these
> +     * clusters must indeed be completely free */
> +    ret = qcow2_pre_write_overlap_check(bs, 0, offset, dirty_bitmaps_size);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    /* Write all dirty bitmaps to the new table */
> +    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
> +        bm = s->dirty_bitmaps + i;
> +        memset(&h, 0, sizeof(h));
> +        h.l1_table_offset = cpu_to_be64(bm->l1_table_offset);
> +        h.l1_size = cpu_to_be32(bm->l1_size);
> +        h.bitmap_granularity = cpu_to_be32(bm->bitmap_granularity);
> +        h.bitmap_size = cpu_to_be64(bm->bitmap_size);
> +
> +        name_size = strlen(bm->name);
> +        assert(name_size <= UINT16_MAX);
> +        h.name_size = cpu_to_be16(name_size);
> +        offset = align_offset(offset, 8);
> +
> +        ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        offset += sizeof(h);
> +
> +        ret = bdrv_pwrite(bs->file, offset, bm->name, name_size);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        offset += name_size;
> +    }
> +
> +    /*
> +     * Update the header extension to point to the new dirty bitmap table. 
> This
> +     * requires the new table and its refcounts to be stable on disk.
> +     */
> +    ret = bdrv_flush(bs);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    s->dirty_bitmaps_offset = dirty_bitmaps_offset;
> +    s->dirty_bitmaps_size = dirty_bitmaps_size;
> +    ret = qcow2_update_header(bs);
> +    if (ret < 0) {
> +        fprintf(stderr, "Could not update qcow2 header\n");
> +        goto fail;
> +    }
> +
> +    /* Free old dirty bitmap table */
> +    qcow2_free_clusters(bs, old_dirty_bitmaps_offset, old_dirty_bitmaps_size,
> +                        QCOW2_DISCARD_ALWAYS);
> +    return 0;
> +
> +fail:
> +    if (dirty_bitmaps_offset > 0) {
> +        qcow2_free_clusters(bs, dirty_bitmaps_offset, dirty_bitmaps_size,
> +                            QCOW2_DISCARD_ALWAYS);
> +    }
> +    return ret;
> +}
> +
> +static int find_dirty_bitmap_by_name(BlockDriverState *bs,
> +                                     const char *name)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
> +        if (!strcmp(s->dirty_bitmaps[i].name, name)) {
> +            return i;
> +        }
> +    }
> +
> +    return -1;
> +}
> +
> +uint8_t *qcow2_dirty_bitmap_load(BlockDriverState *bs,
> +                            const char *name, uint64_t size,
> +                            int granularity)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i, dirty_bitmap_index, ret;
> +    uint64_t offset;
> +    QCowDirtyBitmap *bm;
> +    uint64_t *l1_table;
> +    uint8_t *buf;
> +
> +    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
> +    if (dirty_bitmap_index < 0) {
> +        return NULL;
> +    }
> +    bm = &s->dirty_bitmaps[dirty_bitmap_index];
> +
> +    if (size != bm->bitmap_size || granularity != bm->bitmap_granularity) {
> +        return NULL;
> +    }
> +
> +    l1_table = g_malloc(bm->l1_size * sizeof(uint64_t));
> +    ret = bdrv_pread(bs->file, bm->l1_table_offset, l1_table,
> +                     bm->l1_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    buf = g_malloc0(bm->l1_size * s->cluster_size);
> +    for (i = 0; i < bm->l1_size; ++i) {
> +        offset = be64_to_cpu(l1_table[i]);
> +        if (!(offset & 1)) {
> +            ret = bdrv_pread(bs->file, offset, buf + i * s->cluster_size,
> +                             s->cluster_size);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +        }
> +    }
> +
> +    g_free(l1_table);
> +    return buf;
> +
> +fail:
> +    g_free(l1_table);
> +    return NULL;
> +}
> +
> +int qcow2_dirty_bitmap_store(BlockDriverState *bs, uint8_t *buf,
> +                            const char *name, uint64_t size,
> +                            int granularity)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int cl_size = s->cluster_size;
> +    int i, dirty_bitmap_index, ret = 0, n;
> +    uint64_t *l1_table;
> +    QCowDirtyBitmap *bm;
> +    uint64_t buf_size;
> +    uint8_t *p;
> +    int sector_granularity = granularity >> BDRV_SECTOR_BITS;
> +
> +    /* find/create dirty bitmap */
> +    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
> +    if (dirty_bitmap_index >= 0) {
> +        bm = s->dirty_bitmaps + dirty_bitmap_index;
> +
> +        if (size != bm->bitmap_size ||
> +            granularity != bm->bitmap_granularity) {
> +            qcow2_dirty_bitmap_delete(bs, name, NULL);
> +            dirty_bitmap_index = -1;
> +        }
> +    }
> +    if (dirty_bitmap_index < 0) {
> +        qcow2_dirty_bitmap_create(bs, name, size, granularity);
> +        dirty_bitmap_index = s->nb_dirty_bitmaps - 1;
> +    }
> +    bm = s->dirty_bitmaps + dirty_bitmap_index;

I catch a segfault right around here if I do the following:

./x86_64-softmmu/qemu-system-x86_64 --dirty-bitmap
file=bitmaps.qcow2,name=bitmap0,drive=drive0 -drive
if=none,file=hda.qcow2,id=drive0 -device ide-hd,drive=drive0

hda.qcow2 and bitmaps.qcow2 are both empty files, but bitmaps.qcow2 has
a size of '0'.

Then when I click close in the QEMU GTK frontend, we hit a segfault when
trying to close because s->dirty_bitmaps is NULL, because it appears as
if we've never actually tried to add the (empty) bitmap to the (empty) file.

Your iotest works, but I am not actually sure why, because I don't
actually know how to *create* a persistent bitmap. I thought that the
-dirty-bitmap CLI would create one in the file specified with file=, but
it apparently only creates an in-memory bitmap and sets the file
pointer, but never initializes any of these structures. Then, when we go
to close, it gets confused and everything breaks a bit.

> +
> +    /* read l1 table */
> +    l1_table = g_malloc(bm->l1_size * sizeof(uint64_t));
> +    ret = bdrv_pread(bs->file, bm->l1_table_offset, l1_table,
> +                     bm->l1_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        goto finish;
> +    }
> +
> +    buf_size = (((size - 1) / sector_granularity) >> 3) + 1;
> +    buf_size = align_offset(buf_size, 4);
> +    n = buf_size / cl_size;
> +    p = buf;
> +    for (i = 0; i < bm->l1_size; ++i) {
> +        uint64_t addr = be64_to_cpu(l1_table[i]) & ~511;
> +        int write_size = (i == n ? (buf_size % cl_size) : cl_size);
> +
> +        if (buffer_is_zero(p, write_size)) {
> +            if (addr) {
> +                qcow2_free_clusters(bs, addr, cl_size,
> +                                    QCOW2_DISCARD_ALWAYS);
> +            }
> +            l1_table[i] = cpu_to_be64(1);
> +        } else {
> +            if (!addr) {
> +                addr = qcow2_alloc_clusters(bs, cl_size);
> +                l1_table[i] = cpu_to_be64(addr);
> +            }
> +
> +            ret = bdrv_pwrite(bs->file, addr, p, write_size);
> +            if (ret < 0) {
> +                goto finish;
> +            }
> +        }
> +
> +        p += cl_size;
> +    }
> +
> +    ret = bdrv_pwrite(bs->file, bm->l1_table_offset, l1_table,
> +                      bm->l1_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        goto finish;
> +    }
> +
> +finish:
> +    g_free(l1_table);
> +    return ret;
> +}
> +/* if no id is provided, a new one is constructed */
> +int qcow2_dirty_bitmap_create(BlockDriverState *bs, const char *name,
> +                              uint64_t size, int granularity)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QCowDirtyBitmap *new_dirty_bitmap_list = NULL;
> +    QCowDirtyBitmap *old_dirty_bitmap_list = NULL;
> +    QCowDirtyBitmap sn1, *bm = &sn1;
> +    int i, ret;
> +    uint64_t *l1_table = NULL;
> +    int64_t l1_table_offset;
> +    int sector_granularity = granularity >> BDRV_SECTOR_BITS;
> +
> +    if (s->nb_dirty_bitmaps >= QCOW_MAX_DIRTY_BITMAPS) {
> +        return -EFBIG;
> +    }
> +
> +    memset(bm, 0, sizeof(*bm));
> +
> +    /* Check that the ID is unique */
> +    if (find_dirty_bitmap_by_name(bs, name) >= 0) {
> +        return -EEXIST;
> +    }
> +
> +    /* Populate bm with passed data */
> +    bm->name = g_strdup(name);
> +    bm->bitmap_granularity = granularity;
> +    bm->bitmap_size = size;
> +
> +    bm->l1_size =
> +        size_to_clusters(s, (((size - 1) / sector_granularity) >> 3) + 1);
> +    l1_table_offset =
> +        qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
> +    if (l1_table_offset < 0) {
> +        ret = l1_table_offset;
> +        goto fail;
> +    }
> +    bm->l1_table_offset = l1_table_offset;
> +
> +    l1_table = g_try_new(uint64_t, bm->l1_size);
> +    if (l1_table == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
> +
> +    /* initialize with zero clusters */
> +    for (i = 0; i < s->l1_size; i++) {
> +        l1_table[i] = cpu_to_be64(1);

bm->l1_size here in my crash output is just "1",
but s->l1_size is 16, so we crash all over this array.

I assume you meant bm->l1_size here. This is a good case to make against
calling everything "L1."

> +    }
> +
> +    ret = qcow2_pre_write_overlap_check(bs, 0, bm->l1_table_offset,
> +                                        s->l1_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    ret = bdrv_pwrite(bs->file, bm->l1_table_offset, l1_table,
> +                      s->l1_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    g_free(l1_table);

I can also catch a segfault here by doing something like this:

./x86_64-softmmu/qemu-system-x86_64 -drive
if=none,format=qcow2,cache=writethrough,file=hda.qcow2,id=drive0
-dirty-bitmap name=bitmap,drive=drive0

Trying to mimick your iotest which does not use an external bitmap file
-- it uses the implicit self-storage.

In this case, hda.qcow2 is still empty (but sized as 8GB) and I try to
quit before any writes occur.

freeing l1_table here causes memory corruption and even valgrind goes
down in flames:

==13284== Invalid write of size 8
==13284==    at 0x53A15E: qcow2_dirty_bitmap_create
(qcow2-dirty-bitmap.c:406)
==13284==    by 0x539D15: qcow2_dirty_bitmap_store
(qcow2-dirty-bitmap.c:307)
==13284==    by 0x505F27: bdrv_store_dirty_bitmap (block.c:3176)
==13284==    by 0x50306D: bdrv_close (block.c:1739)
==13284==    by 0x5032FF: bdrv_close_all (block.c:1797)
==13284==    by 0x3049DC: main (vl.c:4577)
==13284==  Address 0x239b7978 is 0 bytes after a block of size 8 alloc'd
==13284==    at 0x4A06BCF: malloc (vg_replace_malloc.c:296)
==13284==    by 0x300111: malloc_and_trace (vl.c:2706)
==13284==    by 0x62B954E: g_try_malloc (gmem.c:242)
==13284==    by 0x53A11E: qcow2_dirty_bitmap_create
(qcow2-dirty-bitmap.c:398)
==13284==    by 0x539D15: qcow2_dirty_bitmap_store
(qcow2-dirty-bitmap.c:307)
==13284==    by 0x505F27: bdrv_store_dirty_bitmap (block.c:3176)
==13284==    by 0x50306D: bdrv_close (block.c:1739)
==13284==    by 0x5032FF: bdrv_close_all (block.c:1797)
==13284==    by 0x3049DC: main (vl.c:4577)
==13284==
--13284-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11
(SIGSEGV) - exiting
--13284-- si_code=80;  Faulting address: 0x0;  sp: 0x8090a1de0

valgrind: the 'impossible' happened:
   Killed by fatal signal

> +    l1_table = NULL;
> +
> +    /* Append the new dirty bitmap to the dirty bitmap list */
> +    new_dirty_bitmap_list = g_new(QCowDirtyBitmap, s->nb_dirty_bitmaps + 1);
> +    if (s->dirty_bitmaps) {
> +        memcpy(new_dirty_bitmap_list, s->dirty_bitmaps,
> +               s->nb_dirty_bitmaps * sizeof(QCowDirtyBitmap));
> +        old_dirty_bitmap_list = s->dirty_bitmaps;
> +    }
> +    s->dirty_bitmaps = new_dirty_bitmap_list;
> +    s->dirty_bitmaps[s->nb_dirty_bitmaps++] = *bm;
> +
> +    ret = qcow2_write_dirty_bitmaps(bs);
> +    if (ret < 0) {
> +        g_free(s->dirty_bitmaps);
> +        s->dirty_bitmaps = old_dirty_bitmap_list;
> +        s->nb_dirty_bitmaps--;
> +        goto fail;
> +    }
> +
> +    g_free(old_dirty_bitmap_list);
> +
> +    return 0;
> +
Disk is 8GiB, 16,777,216 sectors, and bm->bitmap_size matches that.
> +fail:
> +    g_free(bm->name);
> +    g_free(l1_table);
> +
> +    return ret;
> +}
> +
> +static int qcow2_dirty_bitmap_free_clusters(BlockDriverState *bs,
> +                                            QCowDirtyBitmap *bm)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int ret, i;
> +    uint64_t *l1_table = g_new(uint64_t, bm->l1_size);
> +
> +    ret = bdrv_pread(bs->file, bm->l1_table_offset, l1_table,
> +                     bm->l1_size * sizeof(uint64_t));
> +    if (ret < 0) {
> +        g_free(l1_table);
> +        return ret;
> +    }
> +
> +    for (i = 0; i < bm->l1_size; ++i) {
> +        uint64_t addr = be64_to_cpu(l1_table[i]);
> +        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_ALWAYS);
> +    }
> +
> +    qcow2_free_clusters(bs, bm->l1_table_offset, bm->l1_size * 
> sizeof(uint64_t),
> +                        QCOW2_DISCARD_ALWAYS);
> +
> +    g_free(l1_table);
> +    return 0;
> +}
> +
> +int qcow2_dirty_bitmap_delete(BlockDriverState *bs,
> +                              const char *name,
> +                              Error **errp)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QCowDirtyBitmap bm;
> +    int dirty_bitmap_index, ret = 0;
> +
> +    /* Search the dirty_bitmap */
> +    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
> +    if (dirty_bitmap_index < 0) {
> +        error_setg(errp, "Can't find the dirty bitmap");
> +        return -ENOENT;
> +    }
> +    bm = s->dirty_bitmaps[dirty_bitmap_index];
> +
> +    /* Remove it from the dirty_bitmap list */
> +    memmove(s->dirty_bitmaps + dirty_bitmap_index,
> +            s->dirty_bitmaps + dirty_bitmap_index + 1,
> +            (s->nb_dirty_bitmaps - dirty_bitmap_index - 1) * sizeof(bm));
> +    s->nb_dirty_bitmaps--;
> +    ret = qcow2_write_dirty_bitmaps(bs);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "Failed to remove dirty bitmap"
> +                         " from dirty bitmap list");
> +        return ret;
> +    }
> +
> +    qcow2_dirty_bitmap_free_clusters(bs, &bm);
> +    g_free(bm.name);
> +
> +    return ret;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b9a72e3..406e55d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -61,6 +61,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_END 0
>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> +#define  QCOW2_EXT_MAGIC_DIRTY_BITMAPS 0x23852875
>  
>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char 
> *filename)
>  {
> @@ -90,6 +91,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>      QCowExtension ext;
>      uint64_t offset;
>      int ret;
> +    Qcow2DirtyBitmapHeaderExt dirty_bitmaps_ext;
>  
>  #ifdef DEBUG_EXT
>      printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, 
> end_offset);
> @@ -160,6 +162,33 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
> +            ret = bdrv_pread(bs->file, offset, &dirty_bitmaps_ext, ext.len);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "ERROR: dirty_bitmaps_ext: "
> +                                 "Could not read ext header");
> +                return ret;
> +            }
> +
> +            be64_to_cpus(&dirty_bitmaps_ext.dirty_bitmaps_offset);
> +            be32_to_cpus(&dirty_bitmaps_ext.nb_dirty_bitmaps);
> +
> +            s->dirty_bitmaps_offset = dirty_bitmaps_ext.dirty_bitmaps_offset;
> +            s->nb_dirty_bitmaps = dirty_bitmaps_ext.nb_dirty_bitmaps;
> +
> +            ret = qcow2_read_dirty_bitmaps(bs);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "Could not read dirty bitmaps");
> +                return ret;
> +            }
> +
> +#ifdef DEBUG_EXT
> +            printf("Qcow2: Got dirty bitmaps extension:"
> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> +                   s->dirty_bitmaps_offset, s->nb_dirty_bitmaps);
> +#endif
> +            break;
> +
>          default:
>              /* unknown magic - save it in case we need to rewrite the header 
> */
>              {
> @@ -1000,6 +1029,7 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      g_free(s->unknown_header_fields);
>      cleanup_unknown_header_ext(bs);
>      qcow2_free_snapshots(bs);
> +    qcow2_free_dirty_bitmaps(bs);
>      qcow2_refcount_close(bs);
>      qemu_vfree(s->l1_table);
>      /* else pre-write overlap checks in cache_destroy may crash */
> @@ -1466,6 +1496,7 @@ static void qcow2_close(BlockDriverState *bs)
>      qemu_vfree(s->cluster_data);
>      qcow2_refcount_close(bs);
>      qcow2_free_snapshots(bs);
> +    qcow2_free_dirty_bitmaps(bs);
>  }
>  
>  static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
> @@ -1667,6 +1698,21 @@ int qcow2_update_header(BlockDriverState *bs)
>      buf += ret;
>      buflen -= ret;
>  
> +    if (s->nb_dirty_bitmaps > 0) {
> +        Qcow2DirtyBitmapHeaderExt dirty_bitmaps_header = {
> +            .nb_dirty_bitmaps = cpu_to_be32(s->nb_dirty_bitmaps),
> +            .dirty_bitmaps_offset = cpu_to_be64(s->dirty_bitmaps_offset)
> +        };
> +        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
> +                             &dirty_bitmaps_header, 
> sizeof(dirty_bitmaps_header),
> +                             buflen);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        buf += ret;
> +        buflen -= ret;
> +    }
> +
>      /* Keep unknown header extensions */
>      QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
>          ret = header_ext_add(buf, uext->magic, uext->data, uext->len, 
> buflen);
> @@ -2176,6 +2222,12 @@ static int qcow2_truncate(BlockDriverState *bs, 
> int64_t offset)
>          return -ENOTSUP;
>      }
>  
> +    /* cannot proceed if image has dirty_bitmaps */
> +    if (s->nb_dirty_bitmaps) {
> +        error_report("Can't resize an image which has dirty bitmaps");
> +        return -ENOTSUP;
> +    }
> +
>      /* shrinking is currently not supported */
>      if (offset < bs->total_sectors * 512) {
>          error_report("qcow2 doesn't support shrinking images yet");
> @@ -2952,6 +3004,10 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_get_info          = qcow2_get_info,
>      .bdrv_get_specific_info = qcow2_get_specific_info,
>  
> +    .bdrv_dirty_bitmap_load = qcow2_dirty_bitmap_load,
> +    .bdrv_dirty_bitmap_store = qcow2_dirty_bitmap_store,
> +    .bdrv_dirty_bitmap_delete = qcow2_dirty_bitmap_delete,
> +
>      .bdrv_save_vmstate    = qcow2_save_vmstate,
>      .bdrv_load_vmstate    = qcow2_load_vmstate,
>  
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 422b825..24beee0 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -39,6 +39,7 @@
>  
>  #define QCOW_MAX_CRYPT_CLUSTERS 32
>  #define QCOW_MAX_SNAPSHOTS 65536
> +#define QCOW_MAX_DIRTY_BITMAPS 65536
>  
>  /* 8 MB refcount table is enough for 2 PB images at 64k cluster size
>   * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
> @@ -52,6 +53,8 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>  
> +#define QCOW_MAX_DIRTY_BITMAPS_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
> +
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) 
> */
> @@ -138,6 +141,19 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
>      /* name follows  */
>  } QCowSnapshotHeader;
>  
> +typedef struct QEMU_PACKED QCowDirtyBitmapHeader {
> +    /* header is 8 byte aligned */
> +    uint64_t l1_table_offset;
> +
> +    uint32_t l1_size;
> +    uint32_t bitmap_granularity;
> +
> +    uint64_t bitmap_size;
> +    uint16_t name_size;
> +
> +    /* name follows  */
> +} QCowDirtyBitmapHeader;
> +
>  typedef struct QEMU_PACKED QCowSnapshotExtraData {
>      uint64_t vm_state_size_large;
>      uint64_t disk_size;
> @@ -156,6 +172,14 @@ typedef struct QCowSnapshot {
>      uint64_t vm_clock_nsec;
>  } QCowSnapshot;
>  
> +typedef struct QCowDirtyBitmap {
> +    uint64_t l1_table_offset;
> +    uint32_t l1_size;
> +    char *name;
> +    int bitmap_granularity;
> +    uint64_t bitmap_size;
> +} QCowDirtyBitmap;
> +
>  struct Qcow2Cache;
>  typedef struct Qcow2Cache Qcow2Cache;
>  
> @@ -218,6 +242,11 @@ typedef uint64_t Qcow2GetRefcountFunc(const void 
> *refcount_array,
>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>                                    uint64_t index, uint64_t value);
>  
> +typedef struct Qcow2DirtyBitmapHeaderExt {
> +    uint32_t nb_dirty_bitmaps;
> +    uint64_t dirty_bitmaps_offset;
> +} QEMU_PACKED Qcow2DirtyBitmapHeaderExt;
> +
>  typedef struct BDRVQcowState {
>      int cluster_bits;
>      int cluster_size;
> @@ -259,6 +288,11 @@ typedef struct BDRVQcowState {
>      unsigned int nb_snapshots;
>      QCowSnapshot *snapshots;
>  
> +    uint64_t dirty_bitmaps_offset;
> +    int dirty_bitmaps_size;
> +    unsigned int nb_dirty_bitmaps;
> +    QCowDirtyBitmap *dirty_bitmaps;
> +
>      int flags;
>      int qcow_version;
>      bool use_lazy_refcounts;
> @@ -570,6 +604,22 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
>  void qcow2_free_snapshots(BlockDriverState *bs);
>  int qcow2_read_snapshots(BlockDriverState *bs);
>  
> +/* qcow2-dirty-bitmap.c functions */
> +int qcow2_dirty_bitmap_store(BlockDriverState *bs, uint8_t *buf,
> +                             const char *name, uint64_t size,
> +                             int granularity);
> +uint8_t *qcow2_dirty_bitmap_load(BlockDriverState *bs,
> +                                 const char *name, uint64_t size,
> +                                 int granularity);
> +int qcow2_dirty_bitmap_create(BlockDriverState *bs, const char *name,
> +                              uint64_t size, int granularity);
> +int qcow2_dirty_bitmap_delete(BlockDriverState *bs,
> +                              const char *name,
> +                              Error **errp);
> +
> +void qcow2_free_dirty_bitmaps(BlockDriverState *bs);
> +int qcow2_read_dirty_bitmaps(BlockDriverState *bs);
> +
>  /* qcow2-cache.c functions */
>  Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
>  int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index db29b74..88855b4 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -206,6 +206,16 @@ struct BlockDriver {
>      int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>      ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
>  
> +    int (*bdrv_dirty_bitmap_store)(BlockDriverState *bs, uint8_t *buf,
> +                                   const char *name, uint64_t size,
> +                                   int granularity);
> +    uint8_t *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
> +                                       const char *name, uint64_t size,
> +                                       int granularity);
> +    int (*bdrv_dirty_bitmap_delete)(BlockDriverState *bs,
> +                                    const char *name,
> +                                    Error **errp);
> +
>      int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
>                               int64_t pos);
>      int (*bdrv_load_vmstate)(BlockDriverState *bs, uint8_t *buf,
> 


In light of this, some "sanity" tests that test cases like no writes,
empty bitmaps, empty files, etc I think will be appropriate.