Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Max Reitz]

On 20.10.2016 14:22, Vladimir Sementsov-Ogievskiy wrote:
> On 01.10.2016 19:26, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
> 
> [...]
> 
>> 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
>>
>>
> 
> Snapshots are not header extension.. so it is not the case. Here
> qcow2_read_bitmaps looks like part of header extension loading, and
> header extension fields describe other parts of the extension.. I think
> this is a good point, isn't it?

I said it's optional, so it's optional. :-)

I personally feel like a header extension is just the bit of data in the
qcow2 header. The bitmaps itself are managed by that extension, but not
part of the extension itself. Therefore, I wouldn't load it here but in
the main function qcow2_open().

However, this is a personal opinion and thus it's an optional suggestion
(as I said), so if you disagree (which you apparently do) then don't let
it bother you. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

07.10.2016 22:25, Max Reitz пишет:

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
loaded at image open and becomes BdrvDirtyBitmap's for corresponding
drive. These bitmaps are deleted from Qcow2 image after loading to avoid
conflicts.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  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

[...]


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

What if bdrv_flush() in update_header_sync() failed? Then you cannot be
sure what bitmap directory the header is actually pointing to. In that
case, it would be wrong to assume it's still pointing to the old one and
freeing the new one.

Max


Hmm.. Good point. Any suggestions?

I'm trying to achieve some kind of transaction - if function failed file 
is not changed.. But now I understand that because of flush ability to 
fail I can't achieve this..


Has write and flush any guaranties in case of fail? Would it be 
old-or-new state, or some mixed state is possible?





+}
+
+return ret;
+}



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

On 01.10.2016 19:26, Max Reitz wrote:

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are


[...]


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




Snapshots are not header extension.. so it is not the case. Here 
qcow2_read_bitmaps looks like part of header extension loading, and 
header extension fields describe other parts of the extension.. I think 
this is a good point, isn't it?


--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

On 15.10.2016 20:03, Max Reitz wrote:

On 14.10.2016 20:44, Vladimir Sementsov-Ogievskiy wrote:

On 01.10.2016 19:26, Max Reitz wrote:

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are

...


+goto fail;
+}
+
+/* loop is safe because next entry offset is calculated after
conversion to
Should it be s/safe/unsafe/?

loop is safe. _unsafe is related to absence of assert in a loop, as it
loops through BE data. Bad wording, I'll change it somehow..

Yes, I know that the loop is safe in the sense of the word "safe", but I
meant that it's kind of confusing that the loop uses
"for_each_bitmap_dir_entry_unsafe" and thus is "unsafe", too.

Another idea would be to write "This is actually safe" instead of "loop
is safe".


Finally I've decided to introduce normal list of normal structures like 
in snapshots..





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

For example, how? As I understand, I can emit it only by error_setg, so
actually it would be better to add node and bitmap name to all
error_setg in the code.. or create helper function for it.

error_prepend() would be the function.

This code will be invoked by any code that is opening an image. There
are of course a couple of places where that can be the case: For
external tools such as qemu-img, it's normally pretty clear which image
is meant (and it additionally uses error_reportf_err() to give you more
information).

For -drive, every error message will at least be prepended by the
corresponding -drive parameter, so that will help a bit.

blockdev-add, unfortunately, doesn't do anything like this. But the user
can of course choose to construct the BDS graph node by node and thus
always know which node an error originates from.

Anyway, if you want to add this information to every error message, you
should probably do so in bdrv_open_inherit(). The issue I'd take with
this is that most users probably won't set the node name themselves
(either they don't at all or it's some management tool that does), so
reporting the node name doesn't actually help them at all (and
management tools are not supposed to parse error messages, so it won't
help in that case either).

Max



Thanks for explanations, and for the whole review, it's great! Sorry for 
my laziness and for spelling(

O! I've discovered vim spell, so one problem less, I hope.

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

On 01.10.2016 19:26, Max Reitz wrote:

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are


...


+goto fail;
+}
+
+/* loop is safe because next entry offset is calculated after conversion to
Should it be s/safe/unsafe/?


loop is safe. _unsafe is related to absence of assert in a loop, as it 
loops through BE data. Bad wording, I'll change it somehow..





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


For example, how? As I understand, I can emit it only by error_setg, so 
actually it would be better to add node and bitmap name to all 
error_setg in the code.. or create helper function for it.





+   dir_entry_name_notcstr(e),
+   bdrv_get_device_or_node_name(bs));
+goto fail;
+}
+}


...


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


hmm.. it's an artefact from future). should be moved to later patch.



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Max Reitz]

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
> loaded at image open and becomes BdrvDirtyBitmap's for corresponding
> drive. These bitmaps are deleted from Qcow2 image after loading to avoid
> conflicts.
> 
> Extra data in bitmaps is not supported for now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  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

[...]

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

What if bdrv_flush() in update_header_sync() failed? Then you cannot be
sure what bitmap directory the header is actually pointing to. In that
case, it would be wrong to assume it's still pointing to the old one and
freeing the new one.

Max

> +}
> +
> +return ret;
> +}



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Max Reitz]

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 
> ---
>  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
(0x00fffe00ull) 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, _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 

[Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
loaded at image open and becomes BdrvDirtyBitmap's for corresponding
drive. These bitmaps are deleted from Qcow2 image after loading to avoid
conflicts.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 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 @@
  * THE SOFTWARE.
  */
 
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+
+#include "block/block_int.h"
+#include "block/qcow2.h"
+
 /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
  * _internal_ constants. Please do not use this _internal_ abbreviation for
  * other needs and/or outside of this file. */
@@ -37,11 +43,521 @@
 #define BME_MAX_NAME_SIZE 1023
 
 /* Bitmap directory entry flags */
-#define BME_RESERVED_FLAGS 0x
+#define BME_RESERVED_FLAGS 0xfffd
+#define BME_FLAG_AUTO   (1U << 1)
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001fe
 
+#define for_each_bitmap_dir_entry_unsafe(entry, dir, size) \
+for (entry = (Qcow2BitmapDirEntry *)(dir); \
+ entry < (Qcow2BitmapDirEntry *)((uint8_t *)(dir) + size); \
+ entry = next_dir_entry(entry))
+
+#define for_each_bitmap_dir_entry(entry, dir, size) \
+for (entry = (Qcow2BitmapDirEntry *)(dir); \
+ assert(check_dir_iter(entry, (uint8_t *)(dir) + size)), \
+ entry < (Qcow2BitmapDirEntry *)((uint8_t *)(dir) + size); \
+ entry = next_dir_entry(entry))
+
 typedef enum BitmapType {
 BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
+
+static inline bool can_write(BlockDriverState *bs)
+{
+return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+}
+
+static inline void bitmap_dir_entry_to_cpu(Qcow2BitmapDirEntry *entry)
+{
+be64_to_cpus(>bitmap_table_offset);
+be32_to_cpus(>bitmap_table_size);
+be32_to_cpus(>flags);
+be16_to_cpus(>name_size);
+be32_to_cpus(>extra_data_size);
+}
+
+static inline void bitmap_dir_entry_to_be(Qcow2BitmapDirEntry *entry)
+{
+cpu_to_be64s(>bitmap_table_offset);
+cpu_to_be32s(>bitmap_table_size);
+cpu_to_be32s(>flags);
+cpu_to_be16s(>name_size);
+cpu_to_be32s(>extra_data_size);
+}
+
+static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
+{
+size_t i;
+
+for (i = 0; i < size; ++i) {
+be64_to_cpus(_table[i]);
+}
+}
+
+static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size)
+{
+return align_offset(sizeof(Qcow2BitmapDirEntry) +
+name_size + extra_data_size, 8);
+}
+
+static inline int dir_entry_size(Qcow2BitmapDirEntry *entry)
+{
+return calc_dir_entry_size(entry->name_size, entry->extra_data_size);
+}
+
+static inline const char *dir_entry_name_notcstr(Qcow2BitmapDirEntry *entry)
+{
+return (const char *)(entry + 1) + entry->extra_data_size;
+}
+
+static inline int copy_dir_entry(void *dest, Qcow2BitmapDirEntry *entry)
+{
+int sz = dir_entry_size(entry);
+memcpy(dest, entry, sz);
+return sz;
+}
+
+static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
+{
+return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
+}
+
+static inline bool check_dir_iter(Qcow2BitmapDirEntry *it, void *directory_end)
+{
+return ((void *)it == directory_end) ||
+   (((void *)(it + 1) <= directory_end) &&
+((void *)next_dir_entry(it) <= directory_end));
+}
+
+static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
+{
+uint8_t *end = dir + size;
+while (dir < end) {
+Qcow2BitmapDirEntry *e = (Qcow2BitmapDirEntry *)dir;
+dir += dir_entry_size(e);
+
+bitmap_dir_entry_to_be(e);
+}
+}
+
+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) {
+continue;
+}
+
+qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS);
+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) {
+return -ENOMEM;
+}
+
+ret = bdrv_pread(bs->file, entry->bitmap_table_offset,
+ *table, entry->bitmap_table_size