[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitm
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps |
Date: |
Sat, 1 Oct 2016 18:26:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
*with the AUTO flag set
> loaded at image open and becomes BdrvDirtyBitmap's for corresponding
"loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive."
> drive. These bitmaps are deleted from Qcow2 image after loading to avoid
*from the Qcow2 image
> conflicts.
>
> Extra data in bitmaps is not supported for now.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> block/qcow2-bitmap.c | 518
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
> block/qcow2.c | 5 +
> block/qcow2.h | 3 +
> 3 files changed, 525 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index cd18b07..760a047 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -25,6 +25,12 @@
[...]
> +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
> + uint32_t bitmap_table_size)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + int cl_size = s->cluster_size;
> + int i;
> +
> + for (i = 0; i < bitmap_table_size; ++i) {
> + uint64_t addr = bitmap_table[i];
> + if (addr <= 1) {
I'd rather add a BME_TABLE_ENTRY_OFFSET_MASK
(0x00fffffffffffe00ull) and then use:
uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
if (!addr) {
> + continue;
> + }
> +
> + qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS);
I think this should rather be QCOW2_DISCARD_OTHER; or you introduce a
new QCOW2_DISCARD_* flag.
(The same applies to all the other QCOW2_DISCARD_ALWAYS users here.)
Also, I don't really see the point of introducing cl_size just for this
place; it doesn't even save you from a line wrap.
> + bitmap_table[i] = 0;
> + }
> +}
> +
> +static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapDirEntry
> *entry,
> + uint64_t **table)
> +{
> + int ret;
> +
> + *table = g_try_new(uint64_t, entry->bitmap_table_size);
> + if (*table == NULL) {
Not that g_try_new() will return NULL if you try to allocate a buffer
with size 0.
Normally, entry->bitmap_table_size shouldn't be 0, but I don't think
you're catching that case anywhere.
> + return -ENOMEM;
> + }
> +
I wouldn't mind an
assert(entry->bitmap_table_size <= UINT32_MAX / sizeof(uint64_t));
here because of the following multiplication (which is extended to 64
bit on 64 bit machines, but stays in 32 bit on 32 bit machines).
(Of course,
assert(entry->bitmap_table_size <= BME_MAX_TABLE_SIZE);
would be fine, too.)
> + ret = bdrv_pread(bs->file, entry->bitmap_table_offset,
> + *table, entry->bitmap_table_size * sizeof(uint64_t));
> + if (ret < 0) {
> + g_free(*table);
> + *table = NULL;
> + return ret;
> + }
> +
> + bitmap_table_to_cpu(*table, entry->bitmap_table_size);
> +
> + return 0;
> +}
> +
> +static void do_free_bitmap_clusters(BlockDriverState *bs,
> + uint64_t table_offset,
> + uint32_t table_size,
> + uint64_t *bitmap_table)
> +{
> + clear_bitmap_table(bs, bitmap_table, table_size);
> +
> + qcow2_free_clusters(bs, table_offset, table_size * sizeof(uint64_t),
> + QCOW2_DISCARD_ALWAYS);
> +}
> +
> +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapDirEntry
> *entry)
> +{
> + int ret;
> + uint64_t *bitmap_table;
> +
> + ret = bitmap_table_load(bs, entry, &bitmap_table);
> + if (ret < 0 || bitmap_table == NULL) {
That should be:
if (ret < 0) {
assert(bitmap_table == NULL);
Or the other way around:
if (bitmap_table == NULL) {
assert(ret < 0);
Otherwise, it looks like bitmap_table may be NULL with ret being 0, and
the next statement would make this function return success even though
it actually failed.
> + return ret;
> + }
> +
> + do_free_bitmap_clusters(bs, entry->bitmap_table_offset,
> + entry->bitmap_table_size, bitmap_table);
> +
> + return 0;
You're leaking bitmap_table here.
> +}
> +
> +/* dirty sectors in cluster is a number of sectors in the image,
> corresponding
> + * to one cluster of bitmap data */
How about:
This function returns the number of sectors covered by a single cluster
of dirty bitmap data.
> +static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s,
> + const BdrvDirtyBitmap *bitmap)
> +{
> + uint32_t sector_granularity =
> + bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> +
> + return (uint64_t)sector_granularity * (s->cluster_size << 3);
> +}
> +
> +static int load_bitmap_data(BlockDriverState *bs,
> + const uint64_t *dirty_bitmap_table,
> + uint32_t dirty_bitmap_table_size,
> + BdrvDirtyBitmap *bitmap)
> +{
> + int ret = 0;
> + 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 really see the reason to put this into a dedicated
variable (I feel like it makes the code harder to read because you have
to keep in mind what cl_size is).
> + uint8_t *buf = NULL;
> + uint32_t i, tab_size =
> + size_to_clusters(s,
> + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
> +
> + if (tab_size != dirty_bitmap_table_size) {
> + return -EINVAL;
> + }
You should also check that tab_size does not exceed BME_MAX_TABLE_SIZE.
And maybe you should check that the physical size of the bitmap data
does not exceed BME_MAX_PHYS_SIZE, but I'm not so sure why you have that
constant at all because it's basically superseded by the existence of
BME_MAX_TABLE_SIZE.
> +
> + bdrv_clear_dirty_bitmap(bitmap, NULL);
> +
> + buf = g_malloc0(cl_size);
g_malloc() would suffice as well, since you'll be overwriting all of it
anyway.
> + dsc = dirty_sectors_in_cluster(s, bitmap);
> + for (i = 0, sector = 0; i < tab_size; ++i, sector += dsc) {
> + uint64_t end = MIN(bm_size, sector + dsc);
> + uint64_t offset = dirty_bitmap_table[i];
> +
> + if (offset == 1) {
> + bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false);
> + } else if (offset > 1) {
As before, I'd organize this differently, e.g. like:
uint64_t table_entry = dirty_bitmap_table[i];
uint64_t offset = table_entry & BME_TABLE_ENTRY_OFFSET_MASK;
if (table_entry & BME_TABLE_ENTRY_RESERVED_MASK) {
ret = -EINVAL;
goto finish;
}
if (!offset) {
if (table_entry & 1) {
bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false);
} else {
/* No need to deserialize zeroes because the dirty bitmap is
* already cleared */
}
} else {
ret = bdrv_pread(...);
...
}
It's more code, yes, but I find it easier to read because it reflects
the specification better.
> + ret = bdrv_pread(bs->file, offset, buf, cl_size);
> + if (ret < 0) {
> + goto finish;
> + }
> + bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end,
> false);
> + }
> + }
> + ret = 0;
> +
> + bdrv_dirty_bitmap_deserialize_finish(bitmap);
> +
> +finish:
> + g_free(buf);
> +
> + return ret;
> +}
> +
> +static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
> + Qcow2BitmapDirEntry *entry, Error **errp)
> +{
> + int ret;
> + uint64_t *bitmap_table = NULL;
> + uint32_t granularity;
> + BdrvDirtyBitmap *bitmap = NULL;
> + char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size);
Maybe you should limit entry->name_size to BME_MAX_NAME_SIZE.
> +
> + ret = bitmap_table_load(bs, entry, &bitmap_table);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "Could not read bitmap_table table from image");
> + goto fail;
> + }
> +
> + granularity = 1U << entry->granularity_bits;
You should be making sure that entry->granularity_bits does not exceed
[BME_MIN_GRANULARITY_BITS, BME_MAX_GRANULARITY_BITS] before this point.
> + bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> + if (bitmap == NULL) {
> + goto fail;
> + }
> +
> + ret = load_bitmap_data(bs, bitmap_table, entry->bitmap_table_size,
> + bitmap);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not read bitmap from image");
> + goto fail;
> + }
> +
> + g_free(name);
> + g_free(bitmap_table);
> + return bitmap;
> +
> +fail:
> + g_free(name);
> + g_free(bitmap_table);
> + if (bitmap != NULL) {
> + bdrv_release_dirty_bitmap(bs, bitmap);
> + }
> +
> + return NULL;
> +}
> +
> +/* directory_read
> + * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
> + * format from BE.
> + */
> +static uint8_t *directory_read(BlockDriverState *bs,
> + uint64_t offset, uint64_t size, Error **errp)
> +{
> + int ret;
> + uint8_t *dir, *dir_end;
> + Qcow2BitmapDirEntry *e;
I think you should return an error if size >
QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE.
> +
> + dir = g_try_malloc(size);
> + if (dir == NULL) {
> + error_setg(errp, "Can't allocate space for bitmap directory.");
Please drop the full stop at the end of the message.
(And I personally don't like contractions ("can't") in user-facing
messages too much, but that is just my taste.)
> + return NULL;
> + }
> + dir_end = dir + size;
> +
> + ret = bdrv_pread(bs->file, offset, dir, size);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Can't read bitmap directory.");
Again, no full stop at the end of error messages, please.
> + goto fail;
> + }
> +
> + /* loop is safe because next entry offset is calculated after conversion
> to
Should it be s/safe/unsafe/?
> + * cpu format */
> + for_each_bitmap_dir_entry_unsafe(e, dir, size) {
> + if ((uint8_t *)(e + 1) > dir_end) {
> + goto broken_dir;
> + }
> +
> + bitmap_dir_entry_to_cpu(e);
> +
> + if ((uint8_t *)next_dir_entry(e) > dir_end) {
> + goto broken_dir;
> + }
> +
> + if (e->extra_data_size != 0) {
> + error_setg(errp, "Can't load bitmap '%.*s' from '%s':"
> + "extra data not supported.", e->name_size,
Full stop again.
Also, I'm not quite sure why you give the device/node name here. You
don't do that anywhere else and I think if we want to emit the
information where something failed, it should be added at some previous
point in the call chain.
> + dir_entry_name_notcstr(e),
> + bdrv_get_device_or_node_name(bs));
> + goto fail;
> + }
> + }
Maybe you should count the number of dirty bitmaps and return an error
if it exceeds QCOW_MAX_DIRTY_BITMAPS.
> +
> + assert((uint8_t *)e == dir_end);
I think this should rather go to broken_dir if they're not equal.
> +
> + return dir;
> +
> +broken_dir:
> + error_setg(errp, "Broken bitmap directory.");
And again.
> +
> +fail:
> + g_free(dir);
> +
> + return NULL;
> +}
> +
> +static int update_header_sync(BlockDriverState *bs)
> +{
> + int ret;
> +
> + ret = qcow2_update_header(bs);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + ret = bdrv_flush(bs);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/* write bitmap directory from the state to new allocated clusters */
> +static int64_t directory_write(BlockDriverState *bs, const uint8_t *dir,
> + size_t size)
> +{
> + int ret = 0;
> + uint8_t *dir_be = NULL;
> + int64_t dir_offset = 0;
> +
> + dir_be = g_try_malloc(size);
> + if (dir_be == NULL) {
> + return -ENOMEM;
> + }
> + memcpy(dir_be, dir, size);
> + bitmap_directory_to_be(dir_be, size);
> +
> + /* Allocate space for the new bitmap directory */
> + dir_offset = qcow2_alloc_clusters(bs, size);
> + if (dir_offset < 0) {
> + ret = dir_offset;
> + goto out;
> + }
> +
> + /* The bitmap directory position has not yet been updated, so these
> + * clusters must indeed be completely free */
> + ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, size);
> + if (ret < 0) {
> + goto out;
> + }
> +
> + ret = bdrv_pwrite(bs->file, dir_offset, dir_be, size);
> + if (ret < 0) {
> + goto out;
> + }
> +
> +out:
> + g_free(dir_be);
> +
> + if (ret < 0) {
> + if (dir_offset > 0) {
> + qcow2_free_clusters(bs, dir_offset, size, QCOW2_DISCARD_ALWAYS);
> + }
> +
> + return ret;
> + }
> +
> + return dir_offset;
> +}
> +
> +static int directory_update(BlockDriverState *bs, uint8_t *new_dir,
> + size_t new_size, uint32_t new_nb_bitmaps)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + int ret;
> + int64_t new_offset = 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 (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
> + return -EINVAL;
> + }
Also, you should probably check new_nb_bitmaps against
QCOW_MAX_DIRTY_BITMAPS, but maybe you'll be adding that check somewhere
else in a future patch. In that case, an assertion wouldn't be too bad
here, though.
> +
> + if ((new_size == 0) != (new_nb_bitmaps == 0)) {
> + return -EINVAL;
> + }
> +
> + if (new_nb_bitmaps > 0) {
> + new_offset = directory_write(bs, new_dir, new_size);
> + if (new_offset < 0) {
> + return new_offset;
> + }
> +
> + ret = bdrv_flush(bs);
> + if (ret < 0) {
> + goto fail;
> + }
> + }
> + 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) {
> + qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
> + }
> +
> + return 0;
> +
> +fail:
> + if (new_offset > 0) {
> + qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
> + s->bitmap_directory_offset = old_offset;
> + s->bitmap_directory_size = old_size;
> + s->nb_bitmaps = old_nb_bitmaps;
> + s->autoclear_features = old_autocl;
Why are you restoring the autoclear features? From a quick glance I
can't see any code path that changes this field here, and if there is
one, it probably has a good reason to do so.
> + }
> +
> + return ret;
> +}
> +
> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> + int ret;
> + BDRVQcow2State *s = bs->opaque;
> + uint8_t *dir, *new_dir, *new_pos;
> + uint64_t dir_size;
I suggest initializing this variable here (to s->bitmap_directory_size)...
> + Qcow2BitmapDirEntry *e;
> + uint32_t new_nb_bitmaps = 0;
> +
> + if (s->nb_bitmaps == 0) {
> + /* No bitmaps - nothing to do */
> + return 0;
> + }
> +
> + new_pos = new_dir = g_try_malloc(s->bitmap_directory_size);
...and then using it here...
> + if (new_dir == NULL) {
> + error_setg(errp, "Can't allocate space for bitmap directory.");
> + return -ENOMEM;
> + }
> +
> + dir_size = s->bitmap_directory_size;
> + dir = directory_read(bs, s->bitmap_directory_offset,
> + s->bitmap_directory_size, errp);
...and here.
Alternatively, of course, you can just drop the variable and use
s->bitmap_directory_size everywhere.
> + if (dir == NULL) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + for_each_bitmap_dir_entry(e, dir, dir_size) {
> + uint32_t fl = e->flags;
> +
> + if (fl & BME_FLAG_AUTO) {
> + BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp);
> + if (bitmap == NULL) {
> + ret = -EINVAL;
> + goto out;
> + }
> + } else {
> + /* leave bitmap in the image */
> + new_pos += copy_dir_entry(new_pos, e);
> + new_nb_bitmaps++;
> + }
> + }
> +
> + if (!can_write(bs)) {
You should be setting ret here (gcc complains about that).
> + goto out;
> + }
> +
> + ret = directory_update(bs, new_dir, new_pos - new_dir, new_nb_bitmaps);
> + if (ret < 0) {
> + error_setg(errp, "Can't update bitmap directory.");
And once more, please no full stops at the end of error messages.
> + goto out;
> + }
> +
> + /* to be consistent, free bitmap only after successfull directory update
> */
*successful
> + for_each_bitmap_dir_entry(e, dir, dir_size) {
> + uint32_t fl = e->flags;
> +
> + if (fl & BME_FLAG_AUTO) {
> + free_bitmap_clusters(bs, e);
> + }
> + }
> +
> +out:
> + g_free(dir);
> + g_free(new_dir);
> +
> + return ret;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 08c4ef9..02ec224 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -213,6 +213,11 @@ static int qcow2_read_extensions(BlockDriverState *bs,
> uint64_t start_offset,
> s->bitmap_directory_size =
> bitmaps_ext.bitmap_directory_size;
>
> + ret = qcow2_read_bitmaps(bs, errp);
> + if (ret < 0) {
> + return ret;
> + }
> +
I think I'd put this directly into qcow2_open(), just like
qcow2_read_snapshots(); but that's an optional suggestion.
Max
> #ifdef DEBUG_EXT
> printf("Qcow2: Got dirty bitmaps extension:"
> " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> diff --git a/block/qcow2.h b/block/qcow2.h
> index c068b2c..482a29f 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -603,6 +603,9 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
> void qcow2_free_snapshots(BlockDriverState *bs);
> int qcow2_read_snapshots(BlockDriverState *bs);
>
> +/* qcow2-bitmap.c functions */
> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
> +
> /* qcow2-cache.c functions */
> Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
> int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps,
Max Reitz <=