Re: [Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load()

[Vladimir Sementsov-Ogievskiy]

On 07.10.2015 02:01, John Snow wrote:


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 
---
  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.


So, it (hash table approach) may be implemented later




+/* 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 

Re: [Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load()

[Eric Blake]

On 10/06/2015 05:01 PM, John Snow wrote:

>> +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.
> 


If you have to make a decision based on whether an error was detected,
then you MUST pass a local error (as your caller may have passed NULL
because they don't care if you fail, even though you care if your helper
fails).  As in:

Error *err = NULL;
...
if (ret < 0) {
error_setg_errno(, -ret, "Could not read...");
}
finish:
if (err) {
bdrv_release_dirty_bitmap(...);
error_propagate(errp, err);
bitmap = NULL;
}

In short, any code that does *errp is a potential NULL dereference.  The
comments in error.h help explain the paradigms.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load()

[John Snow]

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 
> ---
>  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;
> +}
> +

[Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load()

[Vladimir Sementsov-Ogievskiy]

This function loads block dirty bitmap from qcow2.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 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)
+{
+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;
+}
+
+/* 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);
+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));
+
+if (tab_size > dirty_bitmap_table_size) {
+return -EINVAL;
+}
+
+bdrv_clear_dirty_bitmap(bitmap);
+
+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);
+}
+}
+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,
+