[11/22] qcow2-bitmap: add qcow2_store_persistent_bitmaps()
diff mbox

Message ID 1475232808-4852-12-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 30, 2016, 10:53 a.m. UTC
Realize block bitmap stroing interface, to allow qcow2 images store
persistent bitmaps.

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

Comments

Max Reitz Oct. 7, 2016, 7:24 p.m. UTC | #1
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Realize block bitmap stroing interface, to allow qcow2 images store
> persistent bitmaps.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |   2 +
>  block/qcow2.h        |   2 +
>  3 files changed, 245 insertions(+)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 81520cd..a5be25a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -27,6 +27,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/cutils.h"
>  
>  #include "block/block_int.h"
>  #include "block/qcow2.h"
> @@ -96,6 +97,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]);
> +    }
> +}
> +
>  static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size)
>  {
>      return align_offset(sizeof(Qcow2BitmapDirEntry) +
> @@ -564,3 +574,234 @@ out:
>  
>      return ret;
>  }
> +
> +/* store_bitmap_data()
> + * Store bitmap to image, filling bitamp table accordingly.

s/bitamp/bitmap/

> + */
> +static int store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                             uint64_t *bitmap_table, uint32_t bitmap_table_size)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t sector, dsc;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    int cl_size = s->cluster_size;

Once more, I don't think this variable is necessary, and I feel like it
makes reading the code more difficult for no gain.

> +    uint8_t *buf = NULL;
> +    uint32_t tb_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));

Should be uint64_t or you might accidentally get a truncation here. I'm
not sure how you would get such huge bitmaps but it's trivial to use
uint64_t.

> +
> +    BdrvDirtyBitmapIter *dbi;
> +
> +    if (tb_size != bitmap_table_size) {
> +        return -EINVAL;
> +    }
> +
> +    memset(bitmap_table, 0, bitmap_table_size * sizeof(bitmap_table[0]));

On 32 bit machines, this multiplication can overflow. There should at
least be an assertion to prevent this. Other than that, of course, there
needs to be some place which limits bitmap_table_size to some sane value
and emits a real error if it exceeds that value.

> +
> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
> +    buf = g_malloc(cl_size);
> +    dsc = dirty_sectors_in_cluster(s, bitmap);
> +
> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {

sector should be int64_t instead of uint64_t, then.

> +        uint64_t cluster = sector / dsc;
> +        sector = cluster * dsc;

Our coding style does not allow interleaving declarations and
non-declarations.

> +        uint64_t end = MIN(bm_size, sector + dsc);
> +        uint64_t write_size =
> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
> +
> +        int64_t off = qcow2_alloc_clusters(bs, cl_size);
> +        if (off < 0) {
> +            ret = off;
> +            goto finish;
> +        }
> +        bitmap_table[cluster] = off;
> +
> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);

s/end/end - sector/?

> +        if (write_size < cl_size) {
> +            memset(buf + write_size, 0, cl_size - write_size);
> +        }
> +

I guess there should be a metadata overlap check here.

> +        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
> +        if (ret < 0) {
> +            goto finish;
> +        }
> +
> +        if (end >= bm_size) {
> +            break;
> +        }
> +
> +        bdrv_set_dirty_iter(dbi, end);
> +    }
> +    ret = 0; /* writes */

What is that comment supposed to mean?

> +
> +finish:
> +    if (ret < 0) {
> +        clear_bitmap_table(bs, bitmap_table, bitmap_table_size);
> +    }
> +    g_free(buf);
> +    bdrv_dirty_iter_free(dbi);
> +
> +    return ret;

In case you decide to keep BME_MAX_PHYS_SIZE, this function should check
somewhere that the physical size of the bitmap does not exceed that value.

> +}
> +
> +/* store_bitmap()
> + * Store bitmap to qcow2 and set bitmap_table. bitmap_table itself is not
> + * stored to qcow2.

First of all, there is no parameter called "bitmap_table", and second,
yes, the bitmap table is written to the qcow2 file.

> + */
> +static int store_bitmap(BlockDriverState *bs,
> +                        BdrvDirtyBitmap *bitmap,
> +                        Qcow2BitmapDirEntry *entry)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
> +
> +    uint64_t *tb;
> +    int64_t tb_offset;
> +    uint32_t tb_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));

As above, this variable should be of type uint64_t.

Also, you have to check that it does not exceed BME_MAX_TABLE_SIZE.

> +
> +    tb = g_try_new(uint64_t, tb_size);
> +    if (tb == NULL) {
> +        return -ENOMEM;
> +    }
> +
> +    ret = store_bitmap_data(bs, bitmap, tb, tb_size);
> +    if (ret < 0) {
> +        g_free(tb);
> +        return ret;
> +    }
> +
> +    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));

If you don't limit tb_size, then this multiplication can overflow on 32
bit machines.

> +    if (tb_offset < 0) {
> +        ret = tb_offset;
> +        goto fail;
> +    }
> +

There should be a metadata overlap check here.

> +    bitmap_table_to_be(tb, tb_size);
> +    ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0]));
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    g_free(tb);
> +
> +    entry->bitmap_table_offset = tb_offset;
> +    entry->bitmap_table_size = tb_size;
> +    entry->flags = bdrv_dirty_bitmap_granularity(bitmap) ? BME_FLAG_AUTO : 0;

s/granularity/get_autoload/

> +    entry->type = BT_DIRTY_TRACKING_BITMAP;
> +    entry->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));

You should probably check somewhere that the resulting value for
entry->granularity_bits is in the BME_{MIN,MAX}_GRANULARITY_BITS range.

> +    entry->name_size = strlen(bm_name);

And that this length does not exceed BME_MAX_NAME_SIZE.

> +    entry->extra_data_size = 0;
> +    memcpy(entry + 1, bm_name, entry->name_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_ALWAYS);

As before, I'd vote for QCOW2_DISCARD_OTHER.

> +    }
> +
> +    g_free(tb);
> +
> +    return ret;
> +}
> +
> +static Qcow2BitmapDirEntry *find_bitmap_by_name(uint8_t *bitmap_directory,
> +                                                size_t size, const char *name)
> +{
> +    Qcow2BitmapDirEntry *e;
> +
> +    for_each_bitmap_dir_entry(e, bitmap_directory, size) {
> +        if (strncmp((char *)(e + 1), name, e->name_size) == 0) {
> +            return e;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    BdrvDirtyBitmap *bm;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint32_t new_nb_bitmaps = s->nb_bitmaps;
> +    uint64_t new_dir_size = s->bitmap_directory_size;
> +    uint8_t *dir = NULL, *new_dir = NULL;
> +    int ret;
> +    Qcow2BitmapDirEntry *new_pos;
> +
> +    if (s->nb_bitmaps > 0) {
> +        dir = directory_read(bs, s->bitmap_directory_offset,
> +                             s->bitmap_directory_size, errp);
> +        if (dir == NULL) {
> +            goto out;
> +        }
> +    }
> +
> +    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
> +            bm = bdrv_dirty_bitmap_next(bs, bm)) {
> +        const char *name = bdrv_dirty_bitmap_name(bm);
> +
> +        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
> +            continue;
> +        }
> +
> +        if (s->nb_bitmaps > 0 &&
> +                find_bitmap_by_name(dir, s->bitmap_directory_size, name)) {
> +            error_setg(errp,
> +                       "Can't store bitmap '%s' to '%s', as it already exists",
> +                       name, bdrv_get_device_or_node_name(bs));
> +            goto out;
> +        }
> +
> +        new_nb_bitmaps++;
> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
> +    }
> +
> +    if (s->nb_bitmaps == new_nb_bitmaps) {
> +        /* No new bitmaps - nothing to do */
> +        goto out;
> +    }
> +
> +    new_dir = g_try_malloc0(new_dir_size);
> +    if (new_dir == NULL) {
> +        error_setg(errp, "Can't allocate space for bitmap directory.");
> +        goto out;
> +    }
> +
> +    memcpy(new_dir, dir, s->bitmap_directory_size);
> +    new_pos = (Qcow2BitmapDirEntry *)(new_dir + s->bitmap_directory_size);
> +
> +    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
> +            bm = bdrv_dirty_bitmap_next(bs, bm)) {
> +        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
> +            continue;
> +        }
> +
> +        ret = store_bitmap(bs, bm, new_pos);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Can't store bitmap '%s' to '%s'",
> +                             bdrv_dirty_bitmap_name(bm),
> +                             bdrv_get_device_or_node_name(bs));
> +            goto out;
> +        }
> +        new_pos = next_dir_entry(new_pos);
> +    }
> +
> +    ret = directory_update(bs, new_dir, new_dir_size, new_nb_bitmaps);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Can't update bitmap directory in '%s'",
> +                         bdrv_get_device_or_node_name(bs));
> +        goto out;
> +    }
> +
> +out:
> +    g_free(new_dir);
> +    g_free(dir);

This error path leaks all the bitmaps that have been written
successfully (if any). I guess this is more or less fine if
directory_update() failed (because you can't really tell the state of
the image header after directory_update(), so better be safe) but it's
not so fine if just some store_bitmap() failed.

Max

> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 02ec224..8238205 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3493,6 +3493,8 @@ BlockDriver bdrv_qcow2 = {
>  
>      .bdrv_detach_aio_context  = qcow2_detach_aio_context,
>      .bdrv_attach_aio_context  = qcow2_attach_aio_context,
> +
> +    .bdrv_store_persistent_bitmaps = qcow2_store_persistent_bitmaps,
>  };
>  
>  static void bdrv_qcow2_init(void)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 482a29f..dfcf4c6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -627,4 +627,6 @@ int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
>      void **table);
>  void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
>  
> +void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp);
> +
>  #endif
>
Vladimir Sementsov-Ogievskiy Oct. 13, 2016, 4:48 p.m. UTC | #2
On 07.10.2016 22:24, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> Realize block bitmap stroing interface, to allow qcow2 images store

[snip]

>> +        uint64_t end = MIN(bm_size, sector + dsc);
>> +        uint64_t write_size =
>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
>> +
>> +        int64_t off = qcow2_alloc_clusters(bs, cl_size);
>> +        if (off < 0) {
>> +            ret = off;
>> +            goto finish;
>> +        }
>> +        bitmap_table[cluster] = off;
>> +
>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);
> s/end/end - sector/?

o_0 terrible mistake, thank you.

>
>> +        if (write_size < cl_size) {
>> +            memset(buf + write_size, 0, cl_size - write_size);
>> +        }
>> +
> I guess there should be a metadata overlap check here.

What is the general rule of checking it? Should I check it before all my 
extension related writes?
Max Reitz Oct. 15, 2016, 4:40 p.m. UTC | #3
On 13.10.2016 18:48, Vladimir Sementsov-Ogievskiy wrote:
> On 07.10.2016 22:24, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> Realize block bitmap stroing interface, to allow qcow2 images store
> 
> [snip]
> 
>>> +        uint64_t end = MIN(bm_size, sector + dsc);
>>> +        uint64_t write_size =
>>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end
>>> - sector);
>>> +
>>> +        int64_t off = qcow2_alloc_clusters(bs, cl_size);
>>> +        if (off < 0) {
>>> +            ret = off;
>>> +            goto finish;
>>> +        }
>>> +        bitmap_table[cluster] = off;
>>> +
>>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);
>> s/end/end - sector/?
> 
> o_0 terrible mistake, thank you.
> 
>>
>>> +        if (write_size < cl_size) {
>>> +            memset(buf + write_size, 0, cl_size - write_size);
>>> +        }
>>> +
>> I guess there should be a metadata overlap check here.
> 
> What is the general rule of checking it? Should I check it before all my
> extension related writes?

The general rule is supposed to be "One check before every write to
bs->file".

Max
Vladimir Sementsov-Ogievskiy Oct. 17, 2016, 5:19 p.m. UTC | #4
On 07.10.2016 22:24, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> Realize block bitmap stroing interface, to allow qcow2 images store
>> persistent bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++

[...]

>>
>> +        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
>> +        if (ret < 0) {
>> +            goto finish;
>> +        }
>> +
>> +        if (end >= bm_size) {
>> +            break;
>> +        }
>> +
>> +        bdrv_set_dirty_iter(dbi, end);
>> +    }
>> +    ret = 0; /* writes */
> What is that comment supposed to mean?
>
>

Now I think I can drop this assignment, as bdrv_aligned_preadv have 
'return ret < 0 ? ret : 0;' in the end...  Am I right? Can bdrv_pwrite 
and friends return positive value on success?
Vladimir Sementsov-Ogievskiy Oct. 17, 2016, 5:57 p.m. UTC | #5
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Realize block bitmap stroing interface, to allow qcow2 images store
> persistent bitmaps.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |   2 +
>  block/qcow2.h        |   2 +
>  3 files changed, 245 insertions(+)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 81520cd..a5be25a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -27,6 +27,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/cutils.h"
>
>  #include "block/block_int.h"
>  #include "block/qcow2.h"
> @@ -96,6 +97,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]);
> +    }
> +}
> +
>  static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size)
>  {
>      return align_offset(sizeof(Qcow2BitmapDirEntry) +
> @@ -564,3 +574,234 @@ out:
>
>      return ret;
>  }
> +
> +/* store_bitmap_data()
> + * Store bitmap to image, filling bitamp table accordingly.


> + */
> +static int store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                             uint64_t *bitmap_table, uint32_t bitmap_table_size)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t sector, dsc;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    int cl_size = s->cluster_size;



> +    uint8_t *buf = NULL;
> +    uint32_t tb_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));


> +
> +    BdrvDirtyBitmapIter *dbi;
> +
> +    if (tb_size != bitmap_table_size) {
> +        return -EINVAL;
> +    }
> +
> +    memset(bitmap_table, 0, bitmap_table_size * sizeof(bitmap_table[0]));


> +
> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
> +    buf = g_malloc(cl_size);
> +    dsc = dirty_sectors_in_cluster(s, bitmap);
> +
> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {


> +        uint64_t cluster = sector / dsc;
> +        sector = cluster * dsc;



> +        uint64_t end = MIN(bm_size, sector + dsc);
> +        uint64_t write_size =
> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
> +
> +        int64_t off = qcow2_alloc_clusters(bs, cl_size);
> +        if (off < 0) {
> +            ret = off;
> +            goto finish;
> +        }
> +        bitmap_table[cluster] = off;
> +
> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);



> +        if (write_size < cl_size) {
> +            memset(buf + write_size, 0, cl_size - write_size);
> +        }
> +


> +        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
> +        if (ret < 0) {
> +            goto finish;
> +        }
> +
> +        if (end >= bm_size) {
> +            break;
> +        }
> +
> +        bdrv_set_dirty_iter(dbi, end);
> +    }
> +    ret = 0; /* writes */

What is that comment supposed to mean?

> +
> +finish:
> +    if (ret < 0) {
> +        clear_bitmap_table(bs, bitmap_table, bitmap_table_size);
> +    }
> +    g_free(buf);
> +    bdrv_dirty_iter_free(dbi);
> +
> +    return ret;

In case you decide to keep BME_MAX_PHYS_SIZE, this function should check
somewhere that the physical size of the bitmap does not exceed that value.

> +}
> +
> +/* store_bitmap()
> + * Store bitmap to qcow2 and set bitmap_table. bitmap_table itself is not
> + * stored to qcow2.

First of all, there is no parameter called "bitmap_table", and second,
yes, the bitmap table is written to the qcow2 file.

> + */
> +static int store_bitmap(BlockDriverState *bs,
> +                        BdrvDirtyBitmap *bitmap,
> +                        Qcow2BitmapDirEntry *entry)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
> +
> +    uint64_t *tb;
> +    int64_t tb_offset;
> +    uint32_t tb_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));

As above, this variable should be of type uint64_t.

Also, you have to check that it does not exceed BME_MAX_TABLE_SIZE.

> +
> +    tb = g_try_new(uint64_t, tb_size);
> +    if (tb == NULL) {
> +        return -ENOMEM;
> +    }
> +
> +    ret = store_bitmap_data(bs, bitmap, tb, tb_size);
> +    if (ret < 0) {
> +        g_free(tb);
> +        return ret;
> +    }
> +
> +    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));

If you don't limit tb_size, then this multiplication can overflow on 32
bit machines.

> +    if (tb_offset < 0) {
> +        ret = tb_offset;
> +        goto fail;
> +    }
> +

There should be a metadata overlap check here.

> +    bitmap_table_to_be(tb, tb_size);
> +    ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0]));
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    g_free(tb);
> +
> +    entry->bitmap_table_offset = tb_offset;
> +    entry->bitmap_table_size = tb_size;
> +    entry->flags = bdrv_dirty_bitmap_granularity(bitmap) ? BME_FLAG_AUTO : 0;

s/granularity/get_autoload/

> +    entry->type = BT_DIRTY_TRACKING_BITMAP;
> +    entry->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));

You should probably check somewhere that the resulting value for
entry->granularity_bits is in the BME_{MIN,MAX}_GRANULARITY_BITS range.

> +    entry->name_size = strlen(bm_name);

And that this length does not exceed BME_MAX_NAME_SIZE.

> +    entry->extra_data_size = 0;
> +    memcpy(entry + 1, bm_name, entry->name_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_ALWAYS);

As before, I'd vote for QCOW2_DISCARD_OTHER.

> +    }
> +
> +    g_free(tb);
> +
> +    return ret;
> +}
> +
> +static Qcow2BitmapDirEntry *find_bitmap_by_name(uint8_t *bitmap_directory,
> +                                                size_t size, const char *name)
> +{
> +    Qcow2BitmapDirEntry *e;
> +
> +    for_each_bitmap_dir_entry(e, bitmap_directory, size) {
> +        if (strncmp((char *)(e + 1), name, e->name_size) == 0) {
> +            return e;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    BdrvDirtyBitmap *bm;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint32_t new_nb_bitmaps = s->nb_bitmaps;
> +    uint64_t new_dir_size = s->bitmap_directory_size;
> +    uint8_t *dir = NULL, *new_dir = NULL;
> +    int ret;
> +    Qcow2BitmapDirEntry *new_pos;
> +
> +    if (s->nb_bitmaps > 0) {
> +        dir = directory_read(bs, s->bitmap_directory_offset,
> +                             s->bitmap_directory_size, errp);
> +        if (dir == NULL) {
> +            goto out;
> +        }
> +    }
> +
> +    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
> +            bm = bdrv_dirty_bitmap_next(bs, bm)) {
> +        const char *name = bdrv_dirty_bitmap_name(bm);
> +
> +        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
> +            continue;
> +        }
> +
> +        if (s->nb_bitmaps > 0 &&
> +                find_bitmap_by_name(dir, s->bitmap_directory_size, name)) {
> +            error_setg(errp,
> +                       "Can't store bitmap '%s' to '%s', as it already exists",
> +                       name, bdrv_get_device_or_node_name(bs));
> +            goto out;
> +        }
> +
> +        new_nb_bitmaps++;
> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
> +    }
> +
> +    if (s->nb_bitmaps == new_nb_bitmaps) {
> +        /* No new bitmaps - nothing to do */
> +        goto out;
> +    }
> +
> +    new_dir = g_try_malloc0(new_dir_size);
> +    if (new_dir == NULL) {
> +        error_setg(errp, "Can't allocate space for bitmap directory.");
> +        goto out;
> +    }
> +
> +    memcpy(new_dir, dir, s->bitmap_directory_size);
> +    new_pos = (Qcow2BitmapDirEntry *)(new_dir + s->bitmap_directory_size);
> +
> +    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
> +            bm = bdrv_dirty_bitmap_next(bs, bm)) {
> +        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
> +            continue;
> +        }
> +
> +        ret = store_bitmap(bs, bm, new_pos);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Can't store bitmap '%s' to '%s'",
> +                             bdrv_dirty_bitmap_name(bm),
> +                             bdrv_get_device_or_node_name(bs));
> +            goto out;
> +        }
> +        new_pos = next_dir_entry(new_pos);
> +    }
> +
> +    ret = directory_update(bs, new_dir, new_dir_size, new_nb_bitmaps);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Can't update bitmap directory in '%s'",
> +                         bdrv_get_device_or_node_name(bs));
> +        goto out;
> +    }
> +
> +out:
> +    g_free(new_dir);
> +    g_free(dir);

This error path leaks all the bitmaps that have been written
successfully (if any). I guess this is more or less fine if
directory_update() failed (because you can't really tell the state of
the image header after directory_update(), so better be safe) but it's
not so fine if just some store_bitmap() failed.

Max

> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 02ec224..8238205 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3493,6 +3493,8 @@ BlockDriver bdrv_qcow2 = {
>
>      .bdrv_detach_aio_context  = qcow2_detach_aio_context,
>      .bdrv_attach_aio_context  = qcow2_attach_aio_context,
> +
> +    .bdrv_store_persistent_bitmaps = qcow2_store_persistent_bitmaps,
>  };
>
>  static void bdrv_qcow2_init(void)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 482a29f..dfcf4c6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -627,4 +627,6 @@ int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
>      void **table);
>  void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
>
> +void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp);
> +
>  #endif
>
Vladimir Sementsov-Ogievskiy Oct. 17, 2016, 5:58 p.m. UTC | #6
Sorry, this was an accidental reply.

On 17.10.2016 20:57, Vladimir Sementsov-Ogievskiy wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> Realize block bitmap stroing interface, to allow qcow2 images store
>> persistent bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/qcow2-bitmap.c | 241 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  block/qcow2.c        |   2 +
>>  block/qcow2.h        |   2 +
>>  3 files changed, 245 insertions(+)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 81520cd..a5be25a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -27,6 +27,7 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>> +#include "qemu/cutils.h"
>>
>>  #include "block/block_int.h"
>>  #include "block/qcow2.h"
>> @@ -96,6 +97,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]);
>> +    }
>> +}
>> +
>>  static inline int calc_dir_entry_size(size_t name_size, size_t 
>> extra_data_size)
>>  {
>>      return align_offset(sizeof(Qcow2BitmapDirEntry) +
>> @@ -564,3 +574,234 @@ out:
>>
>>      return ret;
>>  }
>> +
>> +/* store_bitmap_data()
>> + * Store bitmap to image, filling bitamp table accordingly.
>
>
>> + */
>> +static int store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap 
>> *bitmap,
>> +                             uint64_t *bitmap_table, uint32_t 
>> bitmap_table_size)
>> +{
>> +    int ret;
>> +    BDRVQcow2State *s = bs->opaque;
>> +    uint64_t sector, dsc;
>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>> +    int cl_size = s->cluster_size;
>
>
>
>> +    uint8_t *buf = NULL;
>> +    uint32_t tb_size =
>> +            size_to_clusters(s,
>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, 
>> bm_size));
>
>
>> +
>> +    BdrvDirtyBitmapIter *dbi;
>> +
>> +    if (tb_size != bitmap_table_size) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    memset(bitmap_table, 0, bitmap_table_size * 
>> sizeof(bitmap_table[0]));
>
>
>> +
>> +    dbi = bdrv_dirty_iter_new(bitmap, 0);
>> +    buf = g_malloc(cl_size);
>> +    dsc = dirty_sectors_in_cluster(s, bitmap);
>> +
>> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>
>
>> +        uint64_t cluster = sector / dsc;
>> +        sector = cluster * dsc;
>
>
>
>> +        uint64_t end = MIN(bm_size, sector + dsc);
>> +        uint64_t write_size =
>> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end 
>> - sector);
>> +
>> +        int64_t off = qcow2_alloc_clusters(bs, cl_size);
>> +        if (off < 0) {
>> +            ret = off;
>> +            goto finish;
>> +        }
>> +        bitmap_table[cluster] = off;
>> +
>> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);
>
>
>
>> +        if (write_size < cl_size) {
>> +            memset(buf + write_size, 0, cl_size - write_size);
>> +        }
>> +
>
>
>> +        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
>> +        if (ret < 0) {
>> +            goto finish;
>> +        }
>> +
>> +        if (end >= bm_size) {
>> +            break;
>> +        }
>> +
>> +        bdrv_set_dirty_iter(dbi, end);
>> +    }
>> +    ret = 0; /* writes */
>
> What is that comment supposed to mean?
>
>> +
>> +finish:
>> +    if (ret < 0) {
>> +        clear_bitmap_table(bs, bitmap_table, bitmap_table_size);
>> +    }
>> +    g_free(buf);
>> +    bdrv_dirty_iter_free(dbi);
>> +
>> +    return ret;
>
> In case you decide to keep BME_MAX_PHYS_SIZE, this function should check
> somewhere that the physical size of the bitmap does not exceed that 
> value.
>
>> +}
>> +
>> +/* store_bitmap()
>> + * Store bitmap to qcow2 and set bitmap_table. bitmap_table itself 
>> is not
>> + * stored to qcow2.
>
> First of all, there is no parameter called "bitmap_table", and second,
> yes, the bitmap table is written to the qcow2 file.
>
>> + */
>> +static int store_bitmap(BlockDriverState *bs,
>> +                        BdrvDirtyBitmap *bitmap,
>> +                        Qcow2BitmapDirEntry *entry)
>> +{
>> +    int ret;
>> +    BDRVQcow2State *s = bs->opaque;
>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>> +
>> +    uint64_t *tb;
>> +    int64_t tb_offset;
>> +    uint32_t tb_size =
>> +            size_to_clusters(s,
>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, 
>> bm_size));
>
> As above, this variable should be of type uint64_t.
>
> Also, you have to check that it does not exceed BME_MAX_TABLE_SIZE.
>
>> +
>> +    tb = g_try_new(uint64_t, tb_size);
>> +    if (tb == NULL) {
>> +        return -ENOMEM;
>> +    }
>> +
>> +    ret = store_bitmap_data(bs, bitmap, tb, tb_size);
>> +    if (ret < 0) {
>> +        g_free(tb);
>> +        return ret;
>> +    }
>> +
>> +    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));
>
> If you don't limit tb_size, then this multiplication can overflow on 32
> bit machines.
>
>> +    if (tb_offset < 0) {
>> +        ret = tb_offset;
>> +        goto fail;
>> +    }
>> +
>
> There should be a metadata overlap check here.
>
>> +    bitmap_table_to_be(tb, tb_size);
>> +    ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * 
>> sizeof(tb[0]));
>> +    if (ret < 0) {
>> +        goto fail;
>> +    }
>> +
>> +    g_free(tb);
>> +
>> +    entry->bitmap_table_offset = tb_offset;
>> +    entry->bitmap_table_size = tb_size;
>> +    entry->flags = bdrv_dirty_bitmap_granularity(bitmap) ? 
>> BME_FLAG_AUTO : 0;
>
> s/granularity/get_autoload/
>
>> +    entry->type = BT_DIRTY_TRACKING_BITMAP;
>> +    entry->granularity_bits = 
>> ctz32(bdrv_dirty_bitmap_granularity(bitmap));
>
> You should probably check somewhere that the resulting value for
> entry->granularity_bits is in the BME_{MIN,MAX}_GRANULARITY_BITS range.
>
>> +    entry->name_size = strlen(bm_name);
>
> And that this length does not exceed BME_MAX_NAME_SIZE.
>
>> +    entry->extra_data_size = 0;
>> +    memcpy(entry + 1, bm_name, entry->name_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_ALWAYS);
>
> As before, I'd vote for QCOW2_DISCARD_OTHER.
>
>> +    }
>> +
>> +    g_free(tb);
>> +
>> +    return ret;
>> +}
>> +
>> +static Qcow2BitmapDirEntry *find_bitmap_by_name(uint8_t 
>> *bitmap_directory,
>> +                                                size_t size, const 
>> char *name)
>> +{
>> +    Qcow2BitmapDirEntry *e;
>> +
>> +    for_each_bitmap_dir_entry(e, bitmap_directory, size) {
>> +        if (strncmp((char *)(e + 1), name, e->name_size) == 0) {
>> +            return e;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp)
>> +{
>> +    BdrvDirtyBitmap *bm;
>> +    BDRVQcow2State *s = bs->opaque;
>> +    uint32_t new_nb_bitmaps = s->nb_bitmaps;
>> +    uint64_t new_dir_size = s->bitmap_directory_size;
>> +    uint8_t *dir = NULL, *new_dir = NULL;
>> +    int ret;
>> +    Qcow2BitmapDirEntry *new_pos;
>> +
>> +    if (s->nb_bitmaps > 0) {
>> +        dir = directory_read(bs, s->bitmap_directory_offset,
>> +                             s->bitmap_directory_size, errp);
>> +        if (dir == NULL) {
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
>> +            bm = bdrv_dirty_bitmap_next(bs, bm)) {
>> +        const char *name = bdrv_dirty_bitmap_name(bm);
>> +
>> +        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
>> +            continue;
>> +        }
>> +
>> +        if (s->nb_bitmaps > 0 &&
>> +                find_bitmap_by_name(dir, s->bitmap_directory_size, 
>> name)) {
>> +            error_setg(errp,
>> +                       "Can't store bitmap '%s' to '%s', as it 
>> already exists",
>> +                       name, bdrv_get_device_or_node_name(bs));
>> +            goto out;
>> +        }
>> +
>> +        new_nb_bitmaps++;
>> +        new_dir_size += calc_dir_entry_size(strlen(name), 0);
>> +    }
>> +
>> +    if (s->nb_bitmaps == new_nb_bitmaps) {
>> +        /* No new bitmaps - nothing to do */
>> +        goto out;
>> +    }
>> +
>> +    new_dir = g_try_malloc0(new_dir_size);
>> +    if (new_dir == NULL) {
>> +        error_setg(errp, "Can't allocate space for bitmap directory.");
>> +        goto out;
>> +    }
>> +
>> +    memcpy(new_dir, dir, s->bitmap_directory_size);
>> +    new_pos = (Qcow2BitmapDirEntry *)(new_dir + 
>> s->bitmap_directory_size);
>> +
>> +    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
>> +            bm = bdrv_dirty_bitmap_next(bs, bm)) {
>> +        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
>> +            continue;
>> +        }
>> +
>> +        ret = store_bitmap(bs, bm, new_pos);
>> +        if (ret < 0) {
>> +            error_setg_errno(errp, -ret, "Can't store bitmap '%s' to 
>> '%s'",
>> +                             bdrv_dirty_bitmap_name(bm),
>> +                             bdrv_get_device_or_node_name(bs));
>> +            goto out;
>> +        }
>> +        new_pos = next_dir_entry(new_pos);
>> +    }
>> +
>> +    ret = directory_update(bs, new_dir, new_dir_size, new_nb_bitmaps);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Can't update bitmap directory 
>> in '%s'",
>> +                         bdrv_get_device_or_node_name(bs));
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    g_free(new_dir);
>> +    g_free(dir);
>
> This error path leaks all the bitmaps that have been written
> successfully (if any). I guess this is more or less fine if
> directory_update() failed (because you can't really tell the state of
> the image header after directory_update(), so better be safe) but it's
> not so fine if just some store_bitmap() failed.
>
> Max
>
>> +}
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 02ec224..8238205 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3493,6 +3493,8 @@ BlockDriver bdrv_qcow2 = {
>>
>>      .bdrv_detach_aio_context  = qcow2_detach_aio_context,
>>      .bdrv_attach_aio_context  = qcow2_attach_aio_context,
>> +
>> +    .bdrv_store_persistent_bitmaps = qcow2_store_persistent_bitmaps,
>>  };
>>
>>  static void bdrv_qcow2_init(void)
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 482a29f..dfcf4c6 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -627,4 +627,6 @@ int qcow2_cache_get_empty(BlockDriverState *bs, 
>> Qcow2Cache *c, uint64_t offset,
>>      void **table);
>>  void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void 
>> **table);
>>
>> +void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error 
>> **errp);
>> +
>>  #endif
>>
>
>
>
Max Reitz Oct. 21, 2016, 7:44 p.m. UTC | #7
On 17.10.2016 19:19, Vladimir Sementsov-Ogievskiy wrote:
> On 07.10.2016 22:24, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> Realize block bitmap stroing interface, to allow qcow2 images store
>>> persistent bitmaps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2-bitmap.c | 241
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> [...]
> 
>>>
>>> +        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
>>> +        if (ret < 0) {
>>> +            goto finish;
>>> +        }
>>> +
>>> +        if (end >= bm_size) {
>>> +            break;
>>> +        }
>>> +
>>> +        bdrv_set_dirty_iter(dbi, end);
>>> +    }
>>> +    ret = 0; /* writes */
>> What is that comment supposed to mean?
>>
>>
> 
> Now I think I can drop this assignment, as bdrv_aligned_preadv have
> 'return ret < 0 ? ret : 0;' in the end...  Am I right? Can bdrv_pwrite
> and friends return positive value on success?

I think so, but if they do return positive value they're supposed to be
equal to the number of bytes written. In practice, though, any
non-negative return value is treated the same (as success).

I was just wondering about the comment, though, not about the assignment
itself.

Max
Eric Blake Oct. 21, 2016, 9:04 p.m. UTC | #8
On 10/21/2016 02:44 PM, Max Reitz wrote:

>> Now I think I can drop this assignment, as bdrv_aligned_preadv have
>> 'return ret < 0 ? ret : 0;' in the end...  Am I right? Can bdrv_pwrite
>> and friends return positive value on success?
> 
> I think so, but if they do return positive value they're supposed to be
> equal to the number of bytes written. In practice, though, any
> non-negative return value is treated the same (as success).

At one point in my conversion to byte access, I ran into test failures
when trying to return number of bytes written; so commit 1a62d0a
hard-codes a return of 0 on success instead.  It will be a long and
painful audit to find which callers expect which semantics, which I did
not want to do.

> 
> I was just wondering about the comment, though, not about the assignment
> itself.
> 
> Max
>

Patch
diff mbox

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 81520cd..a5be25a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -27,6 +27,7 @@ 
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -96,6 +97,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]);
+    }
+}
+
 static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size)
 {
     return align_offset(sizeof(Qcow2BitmapDirEntry) +
@@ -564,3 +574,234 @@  out:
 
     return ret;
 }
+
+/* store_bitmap_data()
+ * Store bitmap to image, filling bitamp table accordingly.
+ */
+static int store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                             uint64_t *bitmap_table, uint32_t bitmap_table_size)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t sector, dsc;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    int cl_size = s->cluster_size;
+    uint8_t *buf = NULL;
+    uint32_t tb_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    BdrvDirtyBitmapIter *dbi;
+
+    if (tb_size != bitmap_table_size) {
+        return -EINVAL;
+    }
+
+    memset(bitmap_table, 0, bitmap_table_size * sizeof(bitmap_table[0]));
+
+    dbi = bdrv_dirty_iter_new(bitmap, 0);
+    buf = g_malloc(cl_size);
+    dsc = dirty_sectors_in_cluster(s, bitmap);
+
+    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+        uint64_t cluster = sector / dsc;
+        sector = cluster * dsc;
+        uint64_t end = MIN(bm_size, sector + dsc);
+        uint64_t write_size =
+            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
+
+        int64_t off = qcow2_alloc_clusters(bs, cl_size);
+        if (off < 0) {
+            ret = off;
+            goto finish;
+        }
+        bitmap_table[cluster] = off;
+
+        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);
+        if (write_size < cl_size) {
+            memset(buf + write_size, 0, cl_size - write_size);
+        }
+
+        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
+        if (ret < 0) {
+            goto finish;
+        }
+
+        if (end >= bm_size) {
+            break;
+        }
+
+        bdrv_set_dirty_iter(dbi, end);
+    }
+    ret = 0; /* writes */
+
+finish:
+    if (ret < 0) {
+        clear_bitmap_table(bs, bitmap_table, bitmap_table_size);
+    }
+    g_free(buf);
+    bdrv_dirty_iter_free(dbi);
+
+    return ret;
+}
+
+/* store_bitmap()
+ * Store bitmap to qcow2 and set bitmap_table. bitmap_table itself is not
+ * stored to qcow2.
+ */
+static int store_bitmap(BlockDriverState *bs,
+                        BdrvDirtyBitmap *bitmap,
+                        Qcow2BitmapDirEntry *entry)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
+
+    uint64_t *tb;
+    int64_t tb_offset;
+    uint32_t tb_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    tb = g_try_new(uint64_t, tb_size);
+    if (tb == NULL) {
+        return -ENOMEM;
+    }
+
+    ret = store_bitmap_data(bs, bitmap, tb, tb_size);
+    if (ret < 0) {
+        g_free(tb);
+        return ret;
+    }
+
+    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));
+    if (tb_offset < 0) {
+        ret = tb_offset;
+        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) {
+        goto fail;
+    }
+
+    g_free(tb);
+
+    entry->bitmap_table_offset = tb_offset;
+    entry->bitmap_table_size = tb_size;
+    entry->flags = bdrv_dirty_bitmap_granularity(bitmap) ? BME_FLAG_AUTO : 0;
+    entry->type = BT_DIRTY_TRACKING_BITMAP;
+    entry->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
+    entry->name_size = strlen(bm_name);
+    entry->extra_data_size = 0;
+    memcpy(entry + 1, bm_name, entry->name_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_ALWAYS);
+    }
+
+    g_free(tb);
+
+    return ret;
+}
+
+static Qcow2BitmapDirEntry *find_bitmap_by_name(uint8_t *bitmap_directory,
+                                                size_t size, const char *name)
+{
+    Qcow2BitmapDirEntry *e;
+
+    for_each_bitmap_dir_entry(e, bitmap_directory, size) {
+        if (strncmp((char *)(e + 1), name, e->name_size) == 0) {
+            return e;
+        }
+    }
+
+    return NULL;
+}
+
+void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    BdrvDirtyBitmap *bm;
+    BDRVQcow2State *s = bs->opaque;
+    uint32_t new_nb_bitmaps = s->nb_bitmaps;
+    uint64_t new_dir_size = s->bitmap_directory_size;
+    uint8_t *dir = NULL, *new_dir = NULL;
+    int ret;
+    Qcow2BitmapDirEntry *new_pos;
+
+    if (s->nb_bitmaps > 0) {
+        dir = directory_read(bs, s->bitmap_directory_offset,
+                             s->bitmap_directory_size, errp);
+        if (dir == NULL) {
+            goto out;
+        }
+    }
+
+    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
+            bm = bdrv_dirty_bitmap_next(bs, bm)) {
+        const char *name = bdrv_dirty_bitmap_name(bm);
+
+        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
+            continue;
+        }
+
+        if (s->nb_bitmaps > 0 &&
+                find_bitmap_by_name(dir, s->bitmap_directory_size, name)) {
+            error_setg(errp,
+                       "Can't store bitmap '%s' to '%s', as it already exists",
+                       name, bdrv_get_device_or_node_name(bs));
+            goto out;
+        }
+
+        new_nb_bitmaps++;
+        new_dir_size += calc_dir_entry_size(strlen(name), 0);
+    }
+
+    if (s->nb_bitmaps == new_nb_bitmaps) {
+        /* No new bitmaps - nothing to do */
+        goto out;
+    }
+
+    new_dir = g_try_malloc0(new_dir_size);
+    if (new_dir == NULL) {
+        error_setg(errp, "Can't allocate space for bitmap directory.");
+        goto out;
+    }
+
+    memcpy(new_dir, dir, s->bitmap_directory_size);
+    new_pos = (Qcow2BitmapDirEntry *)(new_dir + s->bitmap_directory_size);
+
+    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm != NULL;
+            bm = bdrv_dirty_bitmap_next(bs, bm)) {
+        if (!bdrv_dirty_bitmap_get_persistance(bm)) {
+            continue;
+        }
+
+        ret = store_bitmap(bs, bm, new_pos);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Can't store bitmap '%s' to '%s'",
+                             bdrv_dirty_bitmap_name(bm),
+                             bdrv_get_device_or_node_name(bs));
+            goto out;
+        }
+        new_pos = next_dir_entry(new_pos);
+    }
+
+    ret = directory_update(bs, new_dir, new_dir_size, new_nb_bitmaps);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Can't update bitmap directory in '%s'",
+                         bdrv_get_device_or_node_name(bs));
+        goto out;
+    }
+
+out:
+    g_free(new_dir);
+    g_free(dir);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 02ec224..8238205 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3493,6 +3493,8 @@  BlockDriver bdrv_qcow2 = {
 
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+    .bdrv_store_persistent_bitmaps = qcow2_store_persistent_bitmaps,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 482a29f..dfcf4c6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -627,4 +627,6 @@  int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
     void **table);
 void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
 
+void qcow2_store_persistent_bitmaps(BlockDriverState *bs, Error **errp);
+
 #endif