On 09/05/2015 12:43 PM, Vladimir Sementsov-Ogievskiy wrote:
This function loads block dirty bitmap from qcow2.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
block/qcow2-dirty-bitmap.c | 155 +++++++++++++++++++++++++++++++++++++++++++++
block/qcow2.c | 2 +
block/qcow2.h | 5 ++
include/block/block_int.h | 5 ++
4 files changed, 167 insertions(+)
diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
index 1260d1d..ea50137 100644
--- a/block/qcow2-dirty-bitmap.c
+++ b/block/qcow2-dirty-bitmap.c
@@ -99,6 +99,13 @@ static int check_constraints(int cluster_size,
return fail ? -EINVAL : 0;
}
+static QCowDirtyBitmapHeader *bitmap_header(BDRVQcowState *s,
+ QCowDirtyBitmap *bitmap)
+{
BDRVQcow2State here and everywhere below, again.
+ return (QCowDirtyBitmapHeader *)
+ (s->dirty_bitmap_directory + bitmap->offset);
+}
+
static int directory_read(BlockDriverState *bs)
{
int ret;
@@ -195,3 +202,151 @@ out:
}
return ret;
}
+
+static QCowDirtyBitmap *find_dirty_bitmap_by_name(BlockDriverState *bs,
+ const char *name)
+{
+ BDRVQcowState *s = bs->opaque;
+ QCowDirtyBitmap *bm, *end = s->dirty_bitmaps + s->nb_dirty_bitmaps;
+
+ for (bm = s->dirty_bitmaps; bm < end; ++bm) {
+ if (strcmp(bm->name, name) == 0) {
+ return bm;
+ }
+ }
+
+ return NULL;
+}
+
Whoops. This says to me we really need to prohibit bitmaps with the same
name from being stored in the same file, and mention this in the spec,
and test for it on load.
Perhaps we can create a hash-table and fail verification on open if
there's a collision. We can then use that hash-table here for
find_dirty_bitmap_by_name to speed up lookup since we already went
through the trouble of loading it.
Might help for large cases where we're approaching 64K bitmaps, will not
be too big of a performance hit for casual use.
+/* dirty sectors in cluster is a number of sectors in the image, corresponding
+ * to one cluster of bitmap data */
+static uint64_t dirty_sectors_in_cluster(const BDRVQcowState *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);
+}
+
+/* load_bitmap()
+ * load dirty bitmap from Dirty Bitmap Table
+ * Dirty Bitmap Table entries are assumed to be in big endian format */
+static int load_bitmap(BlockDriverState *bs,
+ const uint64_t *dirty_bitmap_table,
+ uint32_t dirty_bitmap_table_size,
+ BdrvDirtyBitmap *bitmap)
+{
+ int ret = 0;
+ BDRVQcowState *s = bs->opaque;
+ uint64_t sector, dsc;
+ uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
I found some of this hard to unwind, bear with me:
AKA, the number of sectors that bitmap tracks ...
+ int cl_size = s->cluster_size;
+ uint8_t *buf = NULL;
+ uint32_t i, tab_size =
+ size_to_clusters(s, bdrv_dirty_bitmap_data_size(bitmap, bm_size));
+
bdrv_dirty_bitmap_data_size(bitmap, COUNT) calculates for us how much
actual real size the lowest level of the hbitmap actually takes.
Then size_to_clusters tells us how many clusters we need to store that,
and therefore should map back to be the same as the predicted value,
dirty_bitmap_table_size.
+ if (tab_size > dirty_bitmap_table_size) {
+ return -EINVAL;
+ }
+
I assume this is not == because the real table size might have padding
or other such things, but if the calculated tab size is bigger than the
actual then we have a problem.
But I think that you've passed in "birty_ditmap_table_size" as the total
byte count of the table, but "tab_size" is computed here as the number
of entries. I think you should multiply tab_size by uint64_t and test if
they're equal.
+ bdrv_clear_dirty_bitmap(bitmap);
+
Clear takes the aio_context for the associated BDS and then releases it...
+ buf = g_malloc0(cl_size);
+ 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 = be64_to_cpu(dirty_bitmap_table[i]);
+
+ if (offset & DBM_TABLE_ENTRY_RESERVED_MASK) {
+ ret = -EINVAL;
+ goto finish;
+ }
+
+ /* zero offset means cluster unallocated */
+ if (offset) {
+ ret = bdrv_pread(bs->file, offset, buf, cl_size);
+ if (ret < 0) {
+ goto finish;
+ }
+ bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end);
...but at this point, I believe we're editing this bitmap without its
associated lock, which might be a problem when we go to add QMP commands
later.
+ }
+ }
+ ret = 0;
+
+ bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+ g_free(buf);
+
+ return ret;
+}
+
+BdrvDirtyBitmap * qcow2_dirty_bitmap_load(BlockDriverState *bs_for,
+ BlockDriverState *bs_file,
+ const char *name,
+ Error **errp)
+{
+ BDRVQcowState *s = bs_file->opaque;
+ int ret;
+ QCowDirtyBitmap *bm;
+ QCowDirtyBitmapHeader *bmh;
+ uint64_t *dirty_bitmap_table = NULL;
+ uint32_t granularity;
+ uint64_t size = bdrv_nb_sectors(bs_for);
+ BdrvDirtyBitmap *bitmap = NULL;
+
+ bm = find_dirty_bitmap_by_name(bs_file, name);
+ if (bm == NULL) {
+ error_setg(errp, "Could not find bitmap '%s' in the node '%s'", name,
+ bdrv_get_device_or_node_name(bs_file));
+ return NULL;
+ }
+ bmh = bitmap_header(s, bm);
+
+ if (size != bmh->nb_virtual_bits) {
+ error_setg(errp,
+ "Bitmap '%s' in the node '%s' has size = %" PRIu64
+ "when requested size (for node %s) = %" PRIu64,
+ name, bdrv_get_device_or_node_name(bs_file),
+ bmh->nb_virtual_bits,
+ bdrv_get_device_or_node_name(bs_for), size);
+ return NULL;
+ }
+
+
+ dirty_bitmap_table = g_try_malloc(bmh->dirty_bitmap_table_size *
sizeof(uint64_t));
+ if (dirty_bitmap_table == NULL) {
+ error_setg_errno(errp, -ENOMEM, "Could not allocate Dirty Bitmap
Table");
+ return NULL;
+ }
+
+ ret = bdrv_pread(bs_file->file, bmh->dirty_bitmap_table_offset,
dirty_bitmap_table,
+ bmh->dirty_bitmap_table_size * sizeof(uint64_t));
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not read dirty_bitmap_table table from
image");
+ goto finish;
+ }
+
+ granularity = BDRV_SECTOR_SIZE << bmh->granularity_bits;
+ bitmap = bdrv_create_dirty_bitmap(bs_for, granularity, name, errp);
+ if (bitmap == NULL) {
+ error_setg_errno(errp, -ENOMEM, "Could not create dirty bitmap");
why -ENOMEM? create can fail for a number of reasons ... since we've
been given an errp parameter in this function, and we can trust
bdrv_create_dirty_bitmap to have set it, we can just return NULL here
and the caller can check errp to see what went wrong.
+ goto finish;
+ }
+
Do we need to mark this bitmap as temporarily unusable until we complete
the load? I guess not in the context of bdrv_open at boot time ...
+ ret = load_bitmap(bs_file, dirty_bitmap_table,
bmh->dirty_bitmap_table_size, bitmap);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not read bitmap from image");
+ goto finish;
+ }
+
+finish:
+ if (*errp != NULL) {
+ bdrv_release_dirty_bitmap(bs_for, bitmap);
+ bitmap = NULL;
+ }
+ g_free(dirty_bitmap_table);
I think we're not supposed to be reaching into errp to check its
implementation detail like this ... the usual paradigm I see is just
"goto fail" or similar statements instead of checking for
error-or-success in a shared return block.
finish:
g_free(dirty_bitmap_table);
return bitmap;
fail:
g_free(dirty_bitmap_table);
bdrv_release_dirty_bitmap(bs_for, bitmap);
return NULL;
+
+ return bitmap;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 76c331b..58ebdd3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2965,6 +2965,8 @@ BlockDriver bdrv_qcow2 = {
.bdrv_get_info = qcow2_get_info,
.bdrv_get_specific_info = qcow2_get_specific_info,
+ .bdrv_dirty_bitmap_load = qcow2_dirty_bitmap_load,
+
.bdrv_save_vmstate = qcow2_save_vmstate,
.bdrv_load_vmstate = qcow2_load_vmstate,
diff --git a/block/qcow2.h b/block/qcow2.h
index 5016fa1..51d1907 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -608,6 +608,11 @@ int qcow2_read_snapshots(BlockDriverState *bs);
void qcow2_free_dirty_bitmaps(BlockDriverState *bs);
int qcow2_read_dirty_bitmaps(BlockDriverState *bs);
+BdrvDirtyBitmap *qcow2_dirty_bitmap_load(BlockDriverState *bs_for,
+ BlockDriverState *bs_file,
+ const char *name,
+ Error **errp);
+
/* qcow2-cache.c functions */
Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 14ad4c3..f982adc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -204,6 +204,11 @@ struct BlockDriver {
int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
+ BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs_for,
+ BlockDriverState *bs_file,
+ const char *name,
+ Error **errp);
+
int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
int64_t pos);
int (*bdrv_load_vmstate)(BlockDriverState *bs, uint8_t *buf,
Looking good, thanks!
--js