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 <vsement...@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
>
signature.asc
Description: OpenPGP digital signature
