[13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()
diff mbox

Message ID 1479835586-74394-14-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Nov. 22, 2016, 5:26 p.m. UTC
Realize block bitmap storing interface, to allow qcow2 images store
persistent bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        |   1 +
 block/qcow2.h        |   1 +
 3 files changed, 453 insertions(+)

Comments

Max Reitz Dec. 9, 2016, 5:05 p.m. UTC | #1
On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Realize block bitmap storing interface, to allow qcow2 images store
> persistent bitmaps.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |   1 +
>  block/qcow2.h        |   1 +
>  3 files changed, 453 insertions(+)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 81be1ca..a975388 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -28,6 +28,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "exec/log.h"
> +#include "qemu/cutils.h"
>  
>  #include "block/block_int.h"
>  #include "block/qcow2.h"
> @@ -43,6 +44,10 @@
>  #define BME_MIN_GRANULARITY_BITS 9
>  #define BME_MAX_NAME_SIZE 1023
>  
> +#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
> +#error In the code bitmap table physical size assumed to fit into int
> +#endif
> +
>  /* Bitmap directory entry flags */
>  #define BME_RESERVED_FLAGS 0xfffffffcU
>  #define BME_FLAG_IN_USE 1
> @@ -74,6 +79,8 @@ typedef struct Qcow2Bitmap {
>      uint8_t granularity_bits;
>      char *name;
>  
> +    BdrvDirtyBitmap *dirty_bitmap;

(I'm not quite happy with the asymmetry of this field (it isn't set by
load_bitmap(), but it is required by store_bitmap()), but making it
symmetric either by making load_bitmap() set it or by store_bitmap() not
reading it (but getting the value through an explicit parameter) makes
the code needlessly more complicated, so I guess I'll have to stay
not-quite-happy.)

> +
>      QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
>  } Qcow2Bitmap;
>  typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
> @@ -87,6 +94,27 @@ static inline bool can_write(BlockDriverState *bs)
>      return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
>  }
>  
> +static int update_header_sync(BlockDriverState *bs)
> +{
> +    int ret;
> +
> +    ret = qcow2_update_header(bs);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* We doesn't return  bdrv_flush error code. Even if it fails, write was

s/doesn't/don't/

(Also, there's a double space after return.)

> +     * successful and it is more logical to consider that header is in the new
> +     * state than in the old.
> +     */
> +    ret = bdrv_flush(bs);
> +    if (ret < 0) {
> +        fprintf(stderr, "Failed to flush qcow2 header");
> +    }
> +
> +    return 0;
> +}
> +
>  static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
>  {
>      size_t i;
> @@ -96,6 +124,15 @@ static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
>      }
>  }
>  
> +static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < size; ++i) {
> +        cpu_to_be64s(&bitmap_table[i]);
> +    }
> +}
> +
>  /* Check table entry specification constraints. If cluster_size is 0, offset
>   * alignment is not checked. */
>  static int check_table_entry(uint64_t entry, int cluster_size)
> @@ -121,6 +158,51 @@ static int check_table_entry(uint64_t entry, int cluster_size)
>      return 0;
>  }
>  
> +static int check_constraints_on_bitmap(BlockDriverState *bs,
> +                                       const char *name,
> +                                       uint32_t granularity)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int granularity_bits = ctz32(granularity);
> +
> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
> +
> +    if (nb_sectors < 0) {
> +        return nb_sectors;
> +    }
> +
> +    uint64_t phys_bitmap_bytes = (nb_sectors << BDRV_SECTOR_BITS) >>

Using bdrv_getlength() would be simpler.

> +                                 granularity_bits;
> +    uint64_t bitmap_table_size = phys_bitmap_bytes / s->cluster_size;

Should be a DIV_ROUND_UP().

> +    size_t name_size = strlen(name);
> +
> +    int fail =

I'd personally like a bool more.

> +            (bitmap_table_size > BME_MAX_TABLE_SIZE) ||
> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> +            (granularity_bits > BME_MAX_GRANULARITY_BITS) ||
> +            (granularity_bits < BME_MIN_GRANULARITY_BITS) ||
> +            (name_size > BME_MAX_NAME_SIZE);
> +
> +    return fail ? -EINVAL : 0;
> +}
> +
> +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
> +                               uint32_t bitmap_table_size)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < bitmap_table_size; ++i) {
> +        uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
> +        if (!addr) {
> +            continue;
> +        }
> +
> +        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
> +        bitmap_table[i] = 0;
> +    }
> +}
> +
>  static int bitmap_table_load(BlockDriverState *bs, Qcow2Bitmap *bm,
>                               uint64_t **bitmap_table)
>  {
> @@ -159,6 +241,28 @@ fail:
>      return ret;
>  }
>  
> +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2Bitmap *bm)
> +{
> +    int ret;
> +    uint64_t *bitmap_table;
> +
> +    ret = bitmap_table_load(bs, bm, &bitmap_table);
> +    if (ret < 0) {
> +        assert(bitmap_table == NULL);
> +        return ret;
> +    }
> +
> +    clear_bitmap_table(bs, bitmap_table, bm->table_size);
> +    qcow2_free_clusters(bs, bm->table_offset, bm->table_size * sizeof(uint64_t),
> +                        QCOW2_DISCARD_OTHER);
> +    g_free(bitmap_table);
> +
> +    bm->table_offset = 0;
> +    bm->table_size = 0;
> +
> +    return 0;
> +}
> +
>  /* This function returns the number of disk sectors covered by a single cluster
>   * of bitmap data. */
>  static uint64_t disk_sectors_in_bitmap_cluster(const BDRVQcow2State *s,
> @@ -603,6 +707,72 @@ fail:
>   * Bitmap List end
>   */
>  
> +static int update_ext_header_and_dir(BlockDriverState *bs,
> +                                     Qcow2BitmapList *bm_list)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int ret;
> +    Qcow2Bitmap *bm;
> +    uint64_t new_offset = 0;
> +    uint64_t new_size = 0;
> +    uint32_t new_nb_bitmaps = 0;
> +    uint64_t old_offset = s->bitmap_directory_offset;
> +    uint64_t old_size = s->bitmap_directory_size;
> +    uint32_t old_nb_bitmaps = s->nb_bitmaps;
> +    uint64_t old_autocl = s->autoclear_features;
> +
> +    if (bm_list != NULL && !QSIMPLEQ_EMPTY(bm_list)) {
> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +            new_nb_bitmaps++;
> +        }
> +
> +        if (new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
> +            return -EINVAL;
> +        }
> +
> +        ret = bitmap_list_store(bs, bm_list, &new_offset, &new_size, false);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        ret = bdrv_flush(bs);
> +        if (ret < 0) {
> +            goto fail;
> +        }

I think this only needs to flush bs->file, not bs itself. Alternatively,
bitmap_list_store() could just use bdrv_pwrite_sync().

> +
> +        s->autoclear_features |= QCOW2_AUTOCLEAR_BITMAPS;
> +    } else {
> +        s->autoclear_features &= ~(uint64_t)QCOW2_AUTOCLEAR_BITMAPS;
> +    }
> +
> +    s->bitmap_directory_offset = new_offset;
> +    s->bitmap_directory_size = new_size;
> +    s->nb_bitmaps = new_nb_bitmaps;
> +
> +    ret = update_header_sync(bs);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    if (old_size > 0) {
> +        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER);
> +    }
> +
> +    return 0;
> +
> +fail:
> +    if (new_offset > 0) {
> +        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
> +    }
> +
> +    s->bitmap_directory_offset = old_offset;
> +    s->bitmap_directory_size = old_size;
> +    s->nb_bitmaps = old_nb_bitmaps;
> +    s->autoclear_features = old_autocl;
> +
> +    return ret;
> +}
> +
>  /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
>  static void release_dirty_bitmap_helper(gpointer bitmap,
>                                          gpointer bs)
> @@ -664,3 +834,284 @@ fail:
>      g_slist_free(created_dirty_bitmaps);
>      bitmap_list_free(bm_list);
>  }
> +
> +/* store_bitmap_data()
> + * Store bitmap to image, filling bitmap table accordingly.
> + */
> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
> +                                   BdrvDirtyBitmap *bitmap,
> +                                   uint32_t *bitmap_table_size, Error **errp)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    int64_t sector;
> +    uint64_t dsc;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
> +    uint8_t *buf = NULL;
> +    BdrvDirtyBitmapIter *dbi;
> +    uint64_t *tb;
> +    uint64_t tb_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
> +
> +    if (tb_size > BME_MAX_TABLE_SIZE ||
> +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {

Alignment to the opening parenthesis, please.

> +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
> +        return NULL;
> +    }
> +
> +    tb = g_try_new0(uint64_t, tb_size);
> +    if (tb == NULL) {
> +        error_setg(errp, "No memory");
> +        return NULL;
> +    }
> +
> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
> +    buf = g_malloc(s->cluster_size);
> +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
> +
> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> +        uint64_t cluster = sector / dsc;
> +        uint64_t end, write_size;
> +        int64_t off;
> +
> +        sector = cluster * dsc;
> +        end = MIN(bm_size, sector + dsc);
> +        write_size =
> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
> +
> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
> +        if (off < 0) {
> +            error_setg_errno(errp, -off,
> +                             "Failed to allocate clusters for bitmap '%s'",
> +                             bm_name);
> +            goto fail;
> +        }
> +        tb[cluster] = off;

Somehow I would feel better with either an assert(cluster < tb_size);
here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error
handling for bdrv_nb_sectors()) above the loop.

> +
> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);
> +        if (write_size < s->cluster_size) {
> +            memset(buf + write_size, 0, s->cluster_size - write_size);
> +        }

Should we assert that write_size <= s->cluster_size?

> +
> +        ret = qcow2_pre_write_overlap_check(bs, 0, off, s->cluster_size);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Qcow2 overlap check failed");
> +            goto fail;
> +        }
> +
> +        ret = bdrv_pwrite(bs->file, off, buf, s->cluster_size);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
> +                             bm_name);
> +            goto fail;
> +        }
> +
> +        if (end >= bm_size) {
> +            break;
> +        }
> +
> +        bdrv_set_dirty_iter(dbi, end);
> +    }
> +
> +    *bitmap_table_size = tb_size;
> +    g_free(buf);
> +    bdrv_dirty_iter_free(dbi);
> +
> +    return tb;
> +
> +fail:
> +    clear_bitmap_table(bs, tb, tb_size);
> +    g_free(buf);
> +    bdrv_dirty_iter_free(dbi);
> +    g_free(tb);
> +
> +    return NULL;
> +}
> +
> +/* store_bitmap()
> + * Store bm->dirty_bitmap to qcow2.
> + * Set bm->table_offset and bm->table_size accordingly.
> + */
> +static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
> +{
> +    int ret;
> +    uint64_t *tb;
> +    int64_t tb_offset;
> +    uint32_t tb_size;
> +    BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
> +    const char *bm_name;
> +
> +    assert(bitmap != NULL);
> +
> +    bm_name = bdrv_dirty_bitmap_name(bitmap);
> +
> +    tb = store_bitmap_data(bs, bitmap, &tb_size, errp);
> +    if (tb == NULL) {
> +        g_free(tb);
> +        return -EINVAL;
> +    }
> +
> +    assert(tb_size <= BME_MAX_TABLE_SIZE);
> +    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));
> +    if (tb_offset < 0) {
> +        error_setg_errno(errp, -tb_offset,
> +                         "Failed to allocate clusters for bitmap '%s'",
> +                         bm_name);
> +        goto fail;
> +    }
> +
> +    ret = qcow2_pre_write_overlap_check(bs, 0, tb_offset,
> +                                        tb_size * sizeof(tb[0]));
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Qcow2 overlap check failed");
> +        goto fail;
> +    }
> +
> +    bitmap_table_to_be(tb, tb_size);
> +    ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0]));
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
> +                         bm_name);
> +        goto fail;
> +    }
> +
> +    g_free(tb);
> +
> +    bm->table_offset = tb_offset;
> +    bm->table_size = tb_size;
> +
> +    return 0;
> +
> +fail:
> +    clear_bitmap_table(bs, tb, tb_size);
> +
> +    if (tb_offset > 0) {
> +        qcow2_free_clusters(bs, tb_offset, tb_size, QCOW2_DISCARD_OTHER);

tb_size should be tb_size * sizeof(uint64_t) (or sizeof(tb[0])).

> +    }
> +
> +    g_free(tb);
> +
> +    return ret;
> +}
> +
> +static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
> +                                        const char *name)
> +{
> +    Qcow2Bitmap *bm;
> +
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        if (strcmp(name, bm->name) == 0) {
> +            return bm;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    BdrvDirtyBitmap *bitmap;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint32_t new_nb_bitmaps = s->nb_bitmaps;
> +    uint64_t new_dir_size = s->bitmap_directory_size;
> +    int ret;
> +    Qcow2BitmapList *bm_list;
> +    Qcow2Bitmap *bm;
> +
> +    if (!bdrv_has_persistent_bitmaps(bs)) {
> +        /* nothing to do */
> +        return;

Maybe there is something to do. See [1].

> +    }
> +
> +    if (s->nb_bitmaps == 0) {
> +        bm_list = bitmap_list_new();
> +    } else {
> +        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                                   s->bitmap_directory_size, errp);
> +        if (bm_list == NULL) {
> +            /* errp is already set */
> +            return;
> +        }
> +    }
> +
> +    /* check constraints and names */
> +    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
> +            bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) {

Alignment to the opening parenthesis, please.

> +        const char *name = bdrv_dirty_bitmap_name(bitmap);
> +        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +        Qcow2Bitmap *bm;
> +
> +        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
> +            continue;
> +        }
> +
> +        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
> +            error_setg(errp, "Too many persistent bitmaps");
> +            goto fail;
> +        }
> +
> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
> +        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
> +            error_setg(errp, "Too large bitmap directory");
> +            goto fail;
> +        }

You only need to increment new_nb_bitmaps and increase new_dir_size if
the bitmap does not already exist in the image (i.e. if
find_bitmap_by_name() below returns NULL).

> +
> +        if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
> +            error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
> +                       name);
> +            goto fail;
> +        }
> +
> +        bm = find_bitmap_by_name(bm_list, name);
> +        if (bm == NULL) {
> +            bm = g_new0(Qcow2Bitmap, 1);
> +            bm->name = g_strdup(name);
> +            QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
> +        } else {
> +            if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) {

Shouldn't we error out right at the beginning of this function if
can_write(bs) is false?

> +                error_setg(errp, "Bitmap '%s' is already exists in the image",
> +                           name);
> +                goto fail;
> +            }
> +            free_bitmap_clusters(bs, bm);

These clusters should not be freed until the new data has been written
successfully.

> +            /* TODO it is better to reuse these clusters */
> +        }
> +        bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
> +        bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
> +        bm->dirty_bitmap = bitmap;
> +    }

[1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a
corresponding BDS bitmap?

If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the
flag, so we should keep it unchanged. That's what this function is
currently doing.

However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely
us who set the IN_USE flag (because otherwise we would have aborted
loading the bitmaps, and thus also aborted bdrv_open_common()).
Therefore, the only explanation is that the bitmap was deleted in the
meantime, and that means we should also delete it in the qcow2 file.

Max

> +
> +    /* allocate clusters and store bitmaps */
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        if (bm->dirty_bitmap == NULL) {
> +            continue;
> +        }
> +
> +        ret = store_bitmap(bs, bm, errp);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +    }
> +
> +    ret = update_ext_header_and_dir(bs, bm_list);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Failed to update bitmap extension");
> +        goto fail;
> +    }
> +
> +    bitmap_list_free(bm_list);
> +    return;
> +
> +fail:
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        if (bm->dirty_bitmap == NULL || bm->table_offset == 0) {
> +            continue;
> +        }
> +
> +        free_bitmap_clusters(bs, bm);
> +    }
> +
> +    bitmap_list_free(bm_list);
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 443be0e..66c7f74 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3515,6 +3515,7 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_attach_aio_context  = qcow2_attach_aio_context,
>  
>      .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
> +    .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps,
>  };
>  
>  static void bdrv_qcow2_init(void)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index bcedf5b..d9a7643 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -615,5 +615,6 @@ void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
>  
>  /* qcow2-bitmap.c functions */
>  void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>  
>  #endif
>
Vladimir Sementsov-Ogievskiy Dec. 9, 2016, 5:55 p.m. UTC | #2
09.12.2016 20:05, Max Reitz wrote:
> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>> Realize block bitmap storing interface, to allow qcow2 images store
>> persistent bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |   1 +
>>   block/qcow2.h        |   1 +
>>   3 files changed, 453 insertions(+)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 81be1ca..a975388 100644
>> --- a/block/qcow2-bitmap.c

[...]

>> +            return;
>> +        }
>> +    }
>> +
>> +    /* check constraints and names */
>> +    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
>> +            bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) {
> Alignment to the opening parenthesis, please.

Hmm.. without an alignment it is not so simple to distinguish for-loop 
header from its body.


[...]

> [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a
> corresponding BDS bitmap?
>
> If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the
> flag, so we should keep it unchanged. That's what this function is
> currently doing.
>
> However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely
> us who set the IN_USE flag (because otherwise we would have aborted
> loading the bitmaps, and thus also aborted bdrv_open_common()).
> Therefore, the only explanation is that the bitmap was deleted in the
> meantime, and that means we should also delete it in the qcow2 file.

Right. Or, alternatively, these bitmaps may be deleted on corresponding 
BdrvDirtyBitmap deletion.
Max Reitz Dec. 10, 2016, 2:53 p.m. UTC | #3
On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2016 20:05, Max Reitz wrote:
>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>> Realize block bitmap storing interface, to allow qcow2 images store
>>> persistent bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2-bitmap.c | 451
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.c        |   1 +
>>>   block/qcow2.h        |   1 +
>>>   3 files changed, 453 insertions(+)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 81be1ca..a975388 100644
>>> --- a/block/qcow2-bitmap.c
> 
> [...]
> 
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    /* check constraints and names */
>>> +    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
>>> +            bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) {
>> Alignment to the opening parenthesis, please.
> 
> Hmm.. without an alignment it is not so simple to distinguish for-loop
> header from its body.

I know, and it's even worse for "if". That is why I usually put the
opening { on a new line if I have to spread an if/while/for header over
multiple lines.

The usual convention for qemu code is to align at an opening parenthesis
if there is one.

Admittedly, the reasoning I gave for changing checkpatch.pl to accept
opening { on a new line in certain cases was that:

(1) We never codified exactly what to allow for multi-line if/while/for
    conditions.
(2) It was existing practice.

(1) applies in your case also; we don't have any explicitly written-out
convention for alignment of wrapped lines. (2) is more difficult, but
there are indeed a handful of cases where lines are wrapped and not
aligned to the opening parenthesis but just indented by an additional
four spaces...

So I guess since I'm insisting on putting the opening { on a new line
for multi-line conditions, you are allowed to indent the consecutive
lines by an additional level. ;-)

(It *is* against existing convention, but I'm not in a position to argue.)

> [...]
> 
>> [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a
>> corresponding BDS bitmap?
>>
>> If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the
>> flag, so we should keep it unchanged. That's what this function is
>> currently doing.
>>
>> However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely
>> us who set the IN_USE flag (because otherwise we would have aborted
>> loading the bitmaps, and thus also aborted bdrv_open_common()).
>> Therefore, the only explanation is that the bitmap was deleted in the
>> meantime, and that means we should also delete it in the qcow2 file.
> 
> Right. Or, alternatively, these bitmaps may be deleted on corresponding
> BdrvDirtyBitmap deletion.

Right, that would work, too.

Max
Vladimir Sementsov-Ogievskiy Dec. 12, 2016, 7:32 a.m. UTC | #4
10.12.2016 17:53, Max Reitz wrote:
> On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote:
>> 09.12.2016 20:05, Max Reitz wrote:
>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>> Realize block bitmap storing interface, to allow qcow2 images store
>>>> persistent bitmaps.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/qcow2-bitmap.c | 451
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    block/qcow2.c        |   1 +
>>>>    block/qcow2.h        |   1 +
>>>>    3 files changed, 453 insertions(+)
>>>>
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index 81be1ca..a975388 100644
>>>> --- a/block/qcow2-bitmap.c
>> [...]
>>
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /* check constraints and names */
>>>> +    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
>>>> +            bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) {
>>> Alignment to the opening parenthesis, please.
>> Hmm.. without an alignment it is not so simple to distinguish for-loop
>> header from its body.
> I know, and it's even worse for "if". That is why I usually put the
> opening { on a new line if I have to spread an if/while/for header over
> multiple lines.
>
> The usual convention for qemu code is to align at an opening parenthesis
> if there is one.
>
> Admittedly, the reasoning I gave for changing checkpatch.pl to accept
> opening { on a new line in certain cases was that:

Good news, didn't know)

>
> (1) We never codified exactly what to allow for multi-line if/while/for
>      conditions.
> (2) It was existing practice.
>
> (1) applies in your case also; we don't have any explicitly written-out
> convention for alignment of wrapped lines. (2) is more difficult, but
> there are indeed a handful of cases where lines are wrapped and not
> aligned to the opening parenthesis but just indented by an additional
> four spaces...
>
> So I guess since I'm insisting on putting the opening { on a new line
> for multi-line conditions, you are allowed to indent the consecutive
> lines by an additional level. ;-)
>
> (It *is* against existing convention, but I'm not in a position to argue.)
>
>> [...]
>>
>>> [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a
>>> corresponding BDS bitmap?
>>>
>>> If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the
>>> flag, so we should keep it unchanged. That's what this function is
>>> currently doing.
>>>
>>> However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely
>>> us who set the IN_USE flag (because otherwise we would have aborted
>>> loading the bitmaps, and thus also aborted bdrv_open_common()).
>>> Therefore, the only explanation is that the bitmap was deleted in the
>>> meantime, and that means we should also delete it in the qcow2 file.
>> Right. Or, alternatively, these bitmaps may be deleted on corresponding
>> BdrvDirtyBitmap deletion.
> Right, that would work, too.
>
> Max
>
Vladimir Sementsov-Ogievskiy Dec. 17, 2016, 2:58 p.m. UTC | #5
09.12.2016 20:05, Max Reitz wrote:
> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>> Realize block bitmap storing interface, to allow qcow2 images store
>> persistent bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |   1 +

[...]

>> +
>> +/* store_bitmap_data()
>> + * Store bitmap to image, filling bitmap table accordingly.
>> + */
>> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
>> +                                   BdrvDirtyBitmap *bitmap,
>> +                                   uint32_t *bitmap_table_size, Error **errp)
>> +{
>> +    int ret;
>> +    BDRVQcow2State *s = bs->opaque;
>> +    int64_t sector;
>> +    uint64_t dsc;
>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>> +    uint8_t *buf = NULL;
>> +    BdrvDirtyBitmapIter *dbi;
>> +    uint64_t *tb;
>> +    uint64_t tb_size =
>> +            size_to_clusters(s,
>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
>> +
>> +    if (tb_size > BME_MAX_TABLE_SIZE ||
>> +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
> Alignment to the opening parenthesis, please.
>
>> +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
>> +        return NULL;
>> +    }
>> +
>> +    tb = g_try_new0(uint64_t, tb_size);
>> +    if (tb == NULL) {
>> +        error_setg(errp, "No memory");
>> +        return NULL;
>> +    }
>> +
>> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
>> +    buf = g_malloc(s->cluster_size);
>> +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
>> +
>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>> +        uint64_t cluster = sector / dsc;
>> +        uint64_t end, write_size;
>> +        int64_t off;
>> +
>> +        sector = cluster * dsc;
>> +        end = MIN(bm_size, sector + dsc);
>> +        write_size =
>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
>> +
>> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
>> +        if (off < 0) {
>> +            error_setg_errno(errp, -off,
>> +                             "Failed to allocate clusters for bitmap '%s'",
>> +                             bm_name);
>> +            goto fail;
>> +        }
>> +        tb[cluster] = off;
> Somehow I would feel better with either an assert(cluster < tb_size);
> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error
> handling for bdrv_nb_sectors()) above the loop.

assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional 
error handling. Right?

>> +
>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);
>> +        if (write_size < s->cluster_size) {
>> +            memset(buf + write_size, 0, s->cluster_size - write_size);
>> +        }
> Should we assert that write_size <= s->cluster_size?

Ok

[...].

>
>> +        const char *name = bdrv_dirty_bitmap_name(bitmap);
>> +        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>> +        Qcow2Bitmap *bm;
>> +
>> +        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
>> +            continue;
>> +        }
>> +
>> +        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
>> +            error_setg(errp, "Too many persistent bitmaps");
>> +            goto fail;
>> +        }
>> +
>> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
>> +        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>> +            error_setg(errp, "Too large bitmap directory");
>> +            goto fail;
>> +        }
> You only need to increment new_nb_bitmaps and increase new_dir_size if
> the bitmap does not already exist in the image (i.e. if
> find_bitmap_by_name() below returns NULL).

Why? No, I need to check the whole sum and the whole size.

>
>> +
>> +        if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
>> +            error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
>> +                       name);
>> +            goto fail;
>> +        }
>> +
>> +        bm = find_bitmap_by_name(bm_list, name);
>> +        if (bm == NULL) {
>> +            bm = g_new0(Qcow2Bitmap, 1);
>> +            bm->name = g_strdup(name);
>> +            QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
>> +        } else {
>> +            if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) {
> Shouldn't we error out right at the beginning of this function if
> can_write(bs) is false?

Hmm, right.

[...]
Max Reitz Dec. 19, 2016, 3:14 p.m. UTC | #6
On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2016 20:05, Max Reitz wrote:
>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>> Realize block bitmap storing interface, to allow qcow2 images store
>>> persistent bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>  block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block/qcow2.c        |   1 +
> 
> [...]
> 
>>> +
>>> +/* store_bitmap_data()
>>> + * Store bitmap to image, filling bitmap table accordingly.
>>> + */
>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
>>> +                                   BdrvDirtyBitmap *bitmap,
>>> +                                   uint32_t *bitmap_table_size, Error **errp)
>>> +{
>>> +    int ret;
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    int64_t sector;
>>> +    uint64_t dsc;
>>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>>> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>>> +    uint8_t *buf = NULL;
>>> +    BdrvDirtyBitmapIter *dbi;
>>> +    uint64_t *tb;
>>> +    uint64_t tb_size =
>>> +            size_to_clusters(s,
>>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
>>> +
>>> +    if (tb_size > BME_MAX_TABLE_SIZE ||
>>> +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
>> Alignment to the opening parenthesis, please.
>>
>>> +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    tb = g_try_new0(uint64_t, tb_size);
>>> +    if (tb == NULL) {
>>> +        error_setg(errp, "No memory");
>>> +        return NULL;
>>> +    }
>>> +
>>> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
>>> +    buf = g_malloc(s->cluster_size);
>>> +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
>>> +
>>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>>> +        uint64_t cluster = sector / dsc;
>>> +        uint64_t end, write_size;
>>> +        int64_t off;
>>> +
>>> +        sector = cluster * dsc;
>>> +        end = MIN(bm_size, sector + dsc);
>>> +        write_size =
>>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
>>> +
>>> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
>>> +        if (off < 0) {
>>> +            error_setg_errno(errp, -off,
>>> +                             "Failed to allocate clusters for bitmap '%s'",
>>> +                             bm_name);
>>> +            goto fail;
>>> +        }
>>> +        tb[cluster] = off;
>> Somehow I would feel better with either an assert(cluster < tb_size);
>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error
>> handling for bdrv_nb_sectors()) above the loop.
> 
> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
> error handling. Right?

Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
necessarily a multiple of dsc. So that should be good. Alternatively, I
think the following would be slightly easier to read:

assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);

> 
>>> +
>>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);
>>> +        if (write_size < s->cluster_size) {
>>> +            memset(buf + write_size, 0, s->cluster_size - write_size);
>>> +        }
>> Should we assert that write_size <= s->cluster_size?
> 
> Ok
> 
> [...].
> 
>>
>>> +        const char *name = bdrv_dirty_bitmap_name(bitmap);
>>> +        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>> +        Qcow2Bitmap *bm;
>>> +
>>> +        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>> +            continue;
>>> +        }
>>> +
>>> +        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
>>> +            error_setg(errp, "Too many persistent bitmaps");
>>> +            goto fail;
>>> +        }
>>> +
>>> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
>>> +        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>>> +            error_setg(errp, "Too large bitmap directory");
>>> +            goto fail;
>>> +        }
>> You only need to increment new_nb_bitmaps and increase new_dir_size if
>> the bitmap does not already exist in the image (i.e. if
>> find_bitmap_by_name() below returns NULL).
> 
> Why? No, I need to check the whole sum and the whole size.

If the bitmap already exists, you don't create a new directory entry but
reuse the existing one. Therefore, the number of bitmaps in the image
and the directory size will not grow then.

Max

>>> +
>>> +        if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
>>> +            error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
>>> +                       name);
>>> +            goto fail;
>>> +        }
>>> +
>>> +        bm = find_bitmap_by_name(bm_list, name);
>>> +        if (bm == NULL) {
>>> +            bm = g_new0(Qcow2Bitmap, 1);
>>> +            bm->name = g_strdup(name);
>>> +            QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
>>> +        } else {
>>> +            if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) {
>> Shouldn't we error out right at the beginning of this function if
>> can_write(bs) is false?
> 
> Hmm, right.
> 
> [...]
> 
> -- 
> Best regards,
> Vladimir
>
Vladimir Sementsov-Ogievskiy Dec. 19, 2016, 3:26 p.m. UTC | #7
19.12.2016 18:14, Max Reitz wrote:
> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
>> 09.12.2016 20:05, Max Reitz wrote:
>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>> Realize block bitmap storing interface, to allow qcow2 images store
>>>> persistent bitmaps.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   block/qcow2.c        |   1 +
>> [...]
>>
>>>> +
>>>> +/* store_bitmap_data()
>>>> + * Store bitmap to image, filling bitmap table accordingly.
>>>> + */
>>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
>>>> +                                   BdrvDirtyBitmap *bitmap,
>>>> +                                   uint32_t *bitmap_table_size, Error **errp)
>>>> +{
>>>> +    int ret;
>>>> +    BDRVQcow2State *s = bs->opaque;
>>>> +    int64_t sector;
>>>> +    uint64_t dsc;
>>>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>>>> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>>>> +    uint8_t *buf = NULL;
>>>> +    BdrvDirtyBitmapIter *dbi;
>>>> +    uint64_t *tb;
>>>> +    uint64_t tb_size =
>>>> +            size_to_clusters(s,
>>>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
>>>> +
>>>> +    if (tb_size > BME_MAX_TABLE_SIZE ||
>>>> +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
>>> Alignment to the opening parenthesis, please.
>>>
>>>> +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    tb = g_try_new0(uint64_t, tb_size);
>>>> +    if (tb == NULL) {
>>>> +        error_setg(errp, "No memory");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
>>>> +    buf = g_malloc(s->cluster_size);
>>>> +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
>>>> +
>>>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>>>> +        uint64_t cluster = sector / dsc;
>>>> +        uint64_t end, write_size;
>>>> +        int64_t off;
>>>> +
>>>> +        sector = cluster * dsc;
>>>> +        end = MIN(bm_size, sector + dsc);
>>>> +        write_size =
>>>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
>>>> +
>>>> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
>>>> +        if (off < 0) {
>>>> +            error_setg_errno(errp, -off,
>>>> +                             "Failed to allocate clusters for bitmap '%s'",
>>>> +                             bm_name);
>>>> +            goto fail;
>>>> +        }
>>>> +        tb[cluster] = off;
>>> Somehow I would feel better with either an assert(cluster < tb_size);
>>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error
>>> handling for bdrv_nb_sectors()) above the loop.
>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
>> error handling. Right?
> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
> necessarily a multiple of dsc. So that should be good. Alternatively, I
> think the following would be slightly easier to read:
>
> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
>
>>>> +
>>>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);
>>>> +        if (write_size < s->cluster_size) {
>>>> +            memset(buf + write_size, 0, s->cluster_size - write_size);
>>>> +        }
>>> Should we assert that write_size <= s->cluster_size?
>> Ok
>>
>> [...].
>>
>>>> +        const char *name = bdrv_dirty_bitmap_name(bitmap);
>>>> +        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>>> +        Qcow2Bitmap *bm;
>>>> +
>>>> +        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
>>>> +            error_setg(errp, "Too many persistent bitmaps");
>>>> +            goto fail;
>>>> +        }
>>>> +
>>>> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
>>>> +        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>>>> +            error_setg(errp, "Too large bitmap directory");
>>>> +            goto fail;
>>>> +        }
>>> You only need to increment new_nb_bitmaps and increase new_dir_size if
>>> the bitmap does not already exist in the image (i.e. if
>>> find_bitmap_by_name() below returns NULL).
>> Why? No, I need to check the whole sum and the whole size.
> If the bitmap already exists, you don't create a new directory entry but
> reuse the existing one. Therefore, the number of bitmaps in the image
> and the directory size will not grow then.

new_nb_bitmaps is not number of "newly created bitmaps", but just new 
value of field nb_bitmaps, so, all bitmaps - old and new are calculated 
into new_nb_bitmaps. Anyway, this misunderstanding shows that variable 
name is bad..

>
> Max
>
>>>> +
>>>> +        if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
>>>> +            error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
>>>> +                       name);
>>>> +            goto fail;
>>>> +        }
>>>> +
>>>> +        bm = find_bitmap_by_name(bm_list, name);
>>>> +        if (bm == NULL) {
>>>> +            bm = g_new0(Qcow2Bitmap, 1);
>>>> +            bm->name = g_strdup(name);
>>>> +            QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
>>>> +        } else {
>>>> +            if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) {
>>> Shouldn't we error out right at the beginning of this function if
>>> can_write(bs) is false?
>> Hmm, right.
>>
>> [...]
>>
>> -- 
>> Best regards,
>> Vladimir
>>
>
Max Reitz Dec. 19, 2016, 3:34 p.m. UTC | #8
On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2016 18:14, Max Reitz wrote:
>> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
>>> 09.12.2016 20:05, Max Reitz wrote:
>>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Realize block bitmap storing interface, to allow qcow2 images store
>>>>> persistent bitmaps.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>   block/qcow2-bitmap.c | 451
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   block/qcow2.c        |   1 +
>>> [...]
>>>
>>>>> +
>>>>> +/* store_bitmap_data()
>>>>> + * Store bitmap to image, filling bitmap table accordingly.
>>>>> + */
>>>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
>>>>> +                                   BdrvDirtyBitmap *bitmap,
>>>>> +                                   uint32_t *bitmap_table_size,
>>>>> Error **errp)
>>>>> +{
>>>>> +    int ret;
>>>>> +    BDRVQcow2State *s = bs->opaque;
>>>>> +    int64_t sector;
>>>>> +    uint64_t dsc;
>>>>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>>>>> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>>>>> +    uint8_t *buf = NULL;
>>>>> +    BdrvDirtyBitmapIter *dbi;
>>>>> +    uint64_t *tb;
>>>>> +    uint64_t tb_size =
>>>>> +            size_to_clusters(s,
>>>>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0,
>>>>> bm_size));
>>>>> +
>>>>> +    if (tb_size > BME_MAX_TABLE_SIZE ||
>>>>> +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
>>>> Alignment to the opening parenthesis, please.
>>>>
>>>>> +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
>>>>> +        return NULL;
>>>>> +    }
>>>>> +
>>>>> +    tb = g_try_new0(uint64_t, tb_size);
>>>>> +    if (tb == NULL) {
>>>>> +        error_setg(errp, "No memory");
>>>>> +        return NULL;
>>>>> +    }
>>>>> +
>>>>> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
>>>>> +    buf = g_malloc(s->cluster_size);
>>>>> +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
>>>>> +
>>>>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>>>>> +        uint64_t cluster = sector / dsc;
>>>>> +        uint64_t end, write_size;
>>>>> +        int64_t off;
>>>>> +
>>>>> +        sector = cluster * dsc;
>>>>> +        end = MIN(bm_size, sector + dsc);
>>>>> +        write_size =
>>>>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector,
>>>>> end - sector);
>>>>> +
>>>>> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
>>>>> +        if (off < 0) {
>>>>> +            error_setg_errno(errp, -off,
>>>>> +                             "Failed to allocate clusters for
>>>>> bitmap '%s'",
>>>>> +                             bm_name);
>>>>> +            goto fail;
>>>>> +        }
>>>>> +        tb[cluster] = off;
>>>> Somehow I would feel better with either an assert(cluster < tb_size);
>>>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the
>>>> error
>>>> handling for bdrv_nb_sectors()) above the loop.
>>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
>>> error handling. Right?
>> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
>> necessarily a multiple of dsc. So that should be good. Alternatively, I
>> think the following would be slightly easier to read:
>>
>> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
>>
>>>>> +
>>>>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end
>>>>> - sector);
>>>>> +        if (write_size < s->cluster_size) {
>>>>> +            memset(buf + write_size, 0, s->cluster_size -
>>>>> write_size);
>>>>> +        }
>>>> Should we assert that write_size <= s->cluster_size?
>>> Ok
>>>
>>> [...].
>>>
>>>>> +        const char *name = bdrv_dirty_bitmap_name(bitmap);
>>>>> +        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>>>> +        Qcow2Bitmap *bm;
>>>>> +
>>>>> +        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>>>> +            continue;
>>>>> +        }
>>>>> +
>>>>> +        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
>>>>> +            error_setg(errp, "Too many persistent bitmaps");
>>>>> +            goto fail;
>>>>> +        }
>>>>> +
>>>>> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
>>>>> +        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>>>>> +            error_setg(errp, "Too large bitmap directory");
>>>>> +            goto fail;
>>>>> +        }
>>>> You only need to increment new_nb_bitmaps and increase new_dir_size if
>>>> the bitmap does not already exist in the image (i.e. if
>>>> find_bitmap_by_name() below returns NULL).
>>> Why? No, I need to check the whole sum and the whole size.
>> If the bitmap already exists, you don't create a new directory entry but
>> reuse the existing one. Therefore, the number of bitmaps in the image
>> and the directory size will not grow then.
> 
> new_nb_bitmaps is not number of "newly created bitmaps", but just new
> value of field nb_bitmaps, so, all bitmaps - old and new are calculated
> into new_nb_bitmaps. Anyway, this misunderstanding shows that variable
> name is bad..

Yes. But when you store a bitmap of the same name as an existing one,
you are replacing it. The number of bitmaps does not grow in that case.

Max
Vladimir Sementsov-Ogievskiy Dec. 19, 2016, 3:50 p.m. UTC | #9
19.12.2016 18:34, Max Reitz wrote:
> On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2016 18:14, Max Reitz wrote:
>>> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
>>>> 09.12.2016 20:05, Max Reitz wrote:
>>>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Realize block bitmap storing interface, to allow qcow2 images store
>>>>>> persistent bitmaps.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>    block/qcow2-bitmap.c | 451
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>    block/qcow2.c        |   1 +
>>>> [...]
>>>>
>>>>>> +
>>>>>> +/* store_bitmap_data()
>>>>>> + * Store bitmap to image, filling bitmap table accordingly.
>>>>>> + */
>>>>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
>>>>>> +                                   BdrvDirtyBitmap *bitmap,
>>>>>> +                                   uint32_t *bitmap_table_size,
>>>>>> Error **errp)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    BDRVQcow2State *s = bs->opaque;
>>>>>> +    int64_t sector;
>>>>>> +    uint64_t dsc;
>>>>>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>>>>>> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>>>>>> +    uint8_t *buf = NULL;
>>>>>> +    BdrvDirtyBitmapIter *dbi;
>>>>>> +    uint64_t *tb;
>>>>>> +    uint64_t tb_size =
>>>>>> +            size_to_clusters(s,
>>>>>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0,
>>>>>> bm_size));
>>>>>> +
>>>>>> +    if (tb_size > BME_MAX_TABLE_SIZE ||
>>>>>> +            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
>>>>> Alignment to the opening parenthesis, please.
>>>>>
>>>>>> +        error_setg(errp, "Bitmap '%s' is too big", bm_name);
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +
>>>>>> +    tb = g_try_new0(uint64_t, tb_size);
>>>>>> +    if (tb == NULL) {
>>>>>> +        error_setg(errp, "No memory");
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +
>>>>>> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
>>>>>> +    buf = g_malloc(s->cluster_size);
>>>>>> +    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
>>>>>> +
>>>>>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>>>>>> +        uint64_t cluster = sector / dsc;
>>>>>> +        uint64_t end, write_size;
>>>>>> +        int64_t off;
>>>>>> +
>>>>>> +        sector = cluster * dsc;
>>>>>> +        end = MIN(bm_size, sector + dsc);
>>>>>> +        write_size =
>>>>>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector,
>>>>>> end - sector);
>>>>>> +
>>>>>> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
>>>>>> +        if (off < 0) {
>>>>>> +            error_setg_errno(errp, -off,
>>>>>> +                             "Failed to allocate clusters for
>>>>>> bitmap '%s'",
>>>>>> +                             bm_name);
>>>>>> +            goto fail;
>>>>>> +        }
>>>>>> +        tb[cluster] = off;
>>>>> Somehow I would feel better with either an assert(cluster < tb_size);
>>>>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the
>>>>> error
>>>>> handling for bdrv_nb_sectors()) above the loop.
>>>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
>>>> error handling. Right?
>>> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
>>> necessarily a multiple of dsc. So that should be good. Alternatively, I
>>> think the following would be slightly easier to read:
>>>
>>> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
>>>
>>>>>> +
>>>>>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end
>>>>>> - sector);
>>>>>> +        if (write_size < s->cluster_size) {
>>>>>> +            memset(buf + write_size, 0, s->cluster_size -
>>>>>> write_size);
>>>>>> +        }
>>>>> Should we assert that write_size <= s->cluster_size?
>>>> Ok
>>>>
>>>> [...].
>>>>
>>>>>> +        const char *name = bdrv_dirty_bitmap_name(bitmap);
>>>>>> +        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>>>>> +        Qcow2Bitmap *bm;
>>>>>> +
>>>>>> +        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>>>>> +            continue;
>>>>>> +        }
>>>>>> +
>>>>>> +        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
>>>>>> +            error_setg(errp, "Too many persistent bitmaps");
>>>>>> +            goto fail;
>>>>>> +        }
>>>>>> +
>>>>>> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
>>>>>> +        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>>>>>> +            error_setg(errp, "Too large bitmap directory");
>>>>>> +            goto fail;
>>>>>> +        }
>>>>> You only need to increment new_nb_bitmaps and increase new_dir_size if
>>>>> the bitmap does not already exist in the image (i.e. if
>>>>> find_bitmap_by_name() below returns NULL).
>>>> Why? No, I need to check the whole sum and the whole size.
>>> If the bitmap already exists, you don't create a new directory entry but
>>> reuse the existing one. Therefore, the number of bitmaps in the image
>>> and the directory size will not grow then.
>> new_nb_bitmaps is not number of "newly created bitmaps", but just new
>> value of field nb_bitmaps, so, all bitmaps - old and new are calculated
>> into new_nb_bitmaps. Anyway, this misunderstanding shows that variable
>> name is bad..
> Yes. But when you store a bitmap of the same name as an existing one,
> you are replacing it. The number of bitmaps does not grow in that case.

Oh, I'm stupid)) I see now, you are right.

>
> Max
>

Patch
diff mbox

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 81be1ca..a975388 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -28,6 +28,7 @@ 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "exec/log.h"
+#include "qemu/cutils.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -43,6 +44,10 @@ 
 #define BME_MIN_GRANULARITY_BITS 9
 #define BME_MAX_NAME_SIZE 1023
 
+#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
+#error In the code bitmap table physical size assumed to fit into int
+#endif
+
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffffffcU
 #define BME_FLAG_IN_USE 1
@@ -74,6 +79,8 @@  typedef struct Qcow2Bitmap {
     uint8_t granularity_bits;
     char *name;
 
+    BdrvDirtyBitmap *dirty_bitmap;
+
     QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
 } Qcow2Bitmap;
 typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
@@ -87,6 +94,27 @@  static inline bool can_write(BlockDriverState *bs)
     return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
 }
 
+static int update_header_sync(BlockDriverState *bs)
+{
+    int ret;
+
+    ret = qcow2_update_header(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* We doesn't return  bdrv_flush error code. Even if it fails, write was
+     * successful and it is more logical to consider that header is in the new
+     * state than in the old.
+     */
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        fprintf(stderr, "Failed to flush qcow2 header");
+    }
+
+    return 0;
+}
+
 static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
 {
     size_t i;
@@ -96,6 +124,15 @@  static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
     }
 }
 
+static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
+{
+    size_t i;
+
+    for (i = 0; i < size; ++i) {
+        cpu_to_be64s(&bitmap_table[i]);
+    }
+}
+
 /* Check table entry specification constraints. If cluster_size is 0, offset
  * alignment is not checked. */
 static int check_table_entry(uint64_t entry, int cluster_size)
@@ -121,6 +158,51 @@  static int check_table_entry(uint64_t entry, int cluster_size)
     return 0;
 }
 
+static int check_constraints_on_bitmap(BlockDriverState *bs,
+                                       const char *name,
+                                       uint32_t granularity)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int granularity_bits = ctz32(granularity);
+
+    int64_t nb_sectors = bdrv_nb_sectors(bs);
+
+    if (nb_sectors < 0) {
+        return nb_sectors;
+    }
+
+    uint64_t phys_bitmap_bytes = (nb_sectors << BDRV_SECTOR_BITS) >>
+                                 granularity_bits;
+    uint64_t bitmap_table_size = phys_bitmap_bytes / s->cluster_size;
+    size_t name_size = strlen(name);
+
+    int fail =
+            (bitmap_table_size > BME_MAX_TABLE_SIZE) ||
+            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
+            (granularity_bits > BME_MAX_GRANULARITY_BITS) ||
+            (granularity_bits < BME_MIN_GRANULARITY_BITS) ||
+            (name_size > BME_MAX_NAME_SIZE);
+
+    return fail ? -EINVAL : 0;
+}
+
+static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
+                               uint32_t bitmap_table_size)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < bitmap_table_size; ++i) {
+        uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
+        if (!addr) {
+            continue;
+        }
+
+        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
+        bitmap_table[i] = 0;
+    }
+}
+
 static int bitmap_table_load(BlockDriverState *bs, Qcow2Bitmap *bm,
                              uint64_t **bitmap_table)
 {
@@ -159,6 +241,28 @@  fail:
     return ret;
 }
 
+static int free_bitmap_clusters(BlockDriverState *bs, Qcow2Bitmap *bm)
+{
+    int ret;
+    uint64_t *bitmap_table;
+
+    ret = bitmap_table_load(bs, bm, &bitmap_table);
+    if (ret < 0) {
+        assert(bitmap_table == NULL);
+        return ret;
+    }
+
+    clear_bitmap_table(bs, bitmap_table, bm->table_size);
+    qcow2_free_clusters(bs, bm->table_offset, bm->table_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_OTHER);
+    g_free(bitmap_table);
+
+    bm->table_offset = 0;
+    bm->table_size = 0;
+
+    return 0;
+}
+
 /* This function returns the number of disk sectors covered by a single cluster
  * of bitmap data. */
 static uint64_t disk_sectors_in_bitmap_cluster(const BDRVQcow2State *s,
@@ -603,6 +707,72 @@  fail:
  * Bitmap List end
  */
 
+static int update_ext_header_and_dir(BlockDriverState *bs,
+                                     Qcow2BitmapList *bm_list)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
+    Qcow2Bitmap *bm;
+    uint64_t new_offset = 0;
+    uint64_t new_size = 0;
+    uint32_t new_nb_bitmaps = 0;
+    uint64_t old_offset = s->bitmap_directory_offset;
+    uint64_t old_size = s->bitmap_directory_size;
+    uint32_t old_nb_bitmaps = s->nb_bitmaps;
+    uint64_t old_autocl = s->autoclear_features;
+
+    if (bm_list != NULL && !QSIMPLEQ_EMPTY(bm_list)) {
+        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+            new_nb_bitmaps++;
+        }
+
+        if (new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
+            return -EINVAL;
+        }
+
+        ret = bitmap_list_store(bs, bm_list, &new_offset, &new_size, false);
+        if (ret < 0) {
+            return ret;
+        }
+
+        ret = bdrv_flush(bs);
+        if (ret < 0) {
+            goto fail;
+        }
+
+        s->autoclear_features |= QCOW2_AUTOCLEAR_BITMAPS;
+    } else {
+        s->autoclear_features &= ~(uint64_t)QCOW2_AUTOCLEAR_BITMAPS;
+    }
+
+    s->bitmap_directory_offset = new_offset;
+    s->bitmap_directory_size = new_size;
+    s->nb_bitmaps = new_nb_bitmaps;
+
+    ret = update_header_sync(bs);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    if (old_size > 0) {
+        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER);
+    }
+
+    return 0;
+
+fail:
+    if (new_offset > 0) {
+        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
+    }
+
+    s->bitmap_directory_offset = old_offset;
+    s->bitmap_directory_size = old_size;
+    s->nb_bitmaps = old_nb_bitmaps;
+    s->autoclear_features = old_autocl;
+
+    return ret;
+}
+
 /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
 static void release_dirty_bitmap_helper(gpointer bitmap,
                                         gpointer bs)
@@ -664,3 +834,284 @@  fail:
     g_slist_free(created_dirty_bitmaps);
     bitmap_list_free(bm_list);
 }
+
+/* store_bitmap_data()
+ * Store bitmap to image, filling bitmap table accordingly.
+ */
+static uint64_t *store_bitmap_data(BlockDriverState *bs,
+                                   BdrvDirtyBitmap *bitmap,
+                                   uint32_t *bitmap_table_size, Error **errp)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    int64_t sector;
+    uint64_t dsc;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
+    uint8_t *buf = NULL;
+    BdrvDirtyBitmapIter *dbi;
+    uint64_t *tb;
+    uint64_t tb_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    if (tb_size > BME_MAX_TABLE_SIZE ||
+            tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
+        error_setg(errp, "Bitmap '%s' is too big", bm_name);
+        return NULL;
+    }
+
+    tb = g_try_new0(uint64_t, tb_size);
+    if (tb == NULL) {
+        error_setg(errp, "No memory");
+        return NULL;
+    }
+
+    dbi = bdrv_dirty_iter_new(bitmap, 0);
+    buf = g_malloc(s->cluster_size);
+    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
+
+    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+        uint64_t cluster = sector / dsc;
+        uint64_t end, write_size;
+        int64_t off;
+
+        sector = cluster * dsc;
+        end = MIN(bm_size, sector + dsc);
+        write_size =
+            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
+
+        off = qcow2_alloc_clusters(bs, s->cluster_size);
+        if (off < 0) {
+            error_setg_errno(errp, -off,
+                             "Failed to allocate clusters for bitmap '%s'",
+                             bm_name);
+            goto fail;
+        }
+        tb[cluster] = off;
+
+        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);
+        if (write_size < s->cluster_size) {
+            memset(buf + write_size, 0, s->cluster_size - write_size);
+        }
+
+        ret = qcow2_pre_write_overlap_check(bs, 0, off, s->cluster_size);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Qcow2 overlap check failed");
+            goto fail;
+        }
+
+        ret = bdrv_pwrite(bs->file, off, buf, s->cluster_size);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
+                             bm_name);
+            goto fail;
+        }
+
+        if (end >= bm_size) {
+            break;
+        }
+
+        bdrv_set_dirty_iter(dbi, end);
+    }
+
+    *bitmap_table_size = tb_size;
+    g_free(buf);
+    bdrv_dirty_iter_free(dbi);
+
+    return tb;
+
+fail:
+    clear_bitmap_table(bs, tb, tb_size);
+    g_free(buf);
+    bdrv_dirty_iter_free(dbi);
+    g_free(tb);
+
+    return NULL;
+}
+
+/* store_bitmap()
+ * Store bm->dirty_bitmap to qcow2.
+ * Set bm->table_offset and bm->table_size accordingly.
+ */
+static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
+{
+    int ret;
+    uint64_t *tb;
+    int64_t tb_offset;
+    uint32_t tb_size;
+    BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+    const char *bm_name;
+
+    assert(bitmap != NULL);
+
+    bm_name = bdrv_dirty_bitmap_name(bitmap);
+
+    tb = store_bitmap_data(bs, bitmap, &tb_size, errp);
+    if (tb == NULL) {
+        g_free(tb);
+        return -EINVAL;
+    }
+
+    assert(tb_size <= BME_MAX_TABLE_SIZE);
+    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));
+    if (tb_offset < 0) {
+        error_setg_errno(errp, -tb_offset,
+                         "Failed to allocate clusters for bitmap '%s'",
+                         bm_name);
+        goto fail;
+    }
+
+    ret = qcow2_pre_write_overlap_check(bs, 0, tb_offset,
+                                        tb_size * sizeof(tb[0]));
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Qcow2 overlap check failed");
+        goto fail;
+    }
+
+    bitmap_table_to_be(tb, tb_size);
+    ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0]));
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
+                         bm_name);
+        goto fail;
+    }
+
+    g_free(tb);
+
+    bm->table_offset = tb_offset;
+    bm->table_size = tb_size;
+
+    return 0;
+
+fail:
+    clear_bitmap_table(bs, tb, tb_size);
+
+    if (tb_offset > 0) {
+        qcow2_free_clusters(bs, tb_offset, tb_size, QCOW2_DISCARD_OTHER);
+    }
+
+    g_free(tb);
+
+    return ret;
+}
+
+static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
+                                        const char *name)
+{
+    Qcow2Bitmap *bm;
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        if (strcmp(name, bm->name) == 0) {
+            return bm;
+        }
+    }
+
+    return NULL;
+}
+
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    BdrvDirtyBitmap *bitmap;
+    BDRVQcow2State *s = bs->opaque;
+    uint32_t new_nb_bitmaps = s->nb_bitmaps;
+    uint64_t new_dir_size = s->bitmap_directory_size;
+    int ret;
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+
+    if (!bdrv_has_persistent_bitmaps(bs)) {
+        /* nothing to do */
+        return;
+    }
+
+    if (s->nb_bitmaps == 0) {
+        bm_list = bitmap_list_new();
+    } else {
+        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                                   s->bitmap_directory_size, errp);
+        if (bm_list == NULL) {
+            /* errp is already set */
+            return;
+        }
+    }
+
+    /* check constraints and names */
+    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
+            bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) {
+        const char *name = bdrv_dirty_bitmap_name(bitmap);
+        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+        Qcow2Bitmap *bm;
+
+        if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
+            continue;
+        }
+
+        if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
+            error_setg(errp, "Too many persistent bitmaps");
+            goto fail;
+        }
+
+        new_dir_size += calc_dir_entry_size(strlen(name), 0);
+        if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
+            error_setg(errp, "Too large bitmap directory");
+            goto fail;
+        }
+
+        if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
+            error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
+                       name);
+            goto fail;
+        }
+
+        bm = find_bitmap_by_name(bm_list, name);
+        if (bm == NULL) {
+            bm = g_new0(Qcow2Bitmap, 1);
+            bm->name = g_strdup(name);
+            QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
+        } else {
+            if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) {
+                error_setg(errp, "Bitmap '%s' is already exists in the image",
+                           name);
+                goto fail;
+            }
+            free_bitmap_clusters(bs, bm);
+            /* TODO it is better to reuse these clusters */
+        }
+        bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
+        bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
+        bm->dirty_bitmap = bitmap;
+    }
+
+    /* allocate clusters and store bitmaps */
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        if (bm->dirty_bitmap == NULL) {
+            continue;
+        }
+
+        ret = store_bitmap(bs, bm, errp);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+
+    ret = update_ext_header_and_dir(bs, bm_list);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to update bitmap extension");
+        goto fail;
+    }
+
+    bitmap_list_free(bm_list);
+    return;
+
+fail:
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        if (bm->dirty_bitmap == NULL || bm->table_offset == 0) {
+            continue;
+        }
+
+        free_bitmap_clusters(bs, bm);
+    }
+
+    bitmap_list_free(bm_list);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 443be0e..66c7f74 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3515,6 +3515,7 @@  BlockDriver bdrv_qcow2 = {
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
     .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
+    .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index bcedf5b..d9a7643 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -615,5 +615,6 @@  void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
 
 /* qcow2-bitmap.c functions */
 void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 
 #endif