[Qemu-devel] [PATCH v9 00/21] qcow2: persistent dirty bitmaps

[Qemu-devel] [PATCH v9 00/21] qcow2: persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all!

There is a new update of qcow2-bitmap series - v9.

web: https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=qcow2-bitmap-v9
git: https://src.openvz.org/scm/~vsementsov/qemu.git (tag qcow2-bitmap-v9)

v9:

rebase on master!

01-04,06,07: John's r-b
03: rewording in comment ("The concurrent resetting ..."), John's r-b
07: reword error messages
    commit message: dirty bitmap -> bitmap
09: fix uninitialized ret in bitmap_list_store() (fix compilation warning)
18: fix compilation of tests/test-hbitmap

also, change constants names for qcow2: DIRTY_BITMAP -> BITMAP (as it is not
dirty bitmaps extension, but just bitmaps extension), QCOW -> QCOW2 (bitmaps
are only for qcow2)

v8:

Patches 01-06 are mostly unchanged, except for spelling and wording.
02 - add Eric's r-b
03,04 - add Max's r-b

07-09 is refactored "bitmap-read" part of the feature. Main changes:
- introduce bitmap list - normal list, which represents bitmap directory.
  Function bitmap_list_load loads bitmap directory, checks everything and
  creates normal list.
- introduce .bdrv_load_autoloading_dirty_bitmaps : instead of loading bitmaps
  in qcow2_open, lets load them only in bdrv_open, to avoid reloading in 
  bdrv_snapshot_goto
- do not delete bitmaps after read, but mark them IN_USE

10 has contextual changes and rewording of comment. I've added Max's r-b, hope it's ok.

11: add error_report("Persistent bitmaps are lost") in case of failed bitmap store

12: add Max's r-b

13 is refactored "bitmap-store" part of the feature. see 07-09 description
- for now I just free old clusters and allocate new. This will be improved
  with a separate patch.

patch about "delete bitmaps on truncate" is removed. This case will be handled later.
IN_USE, autoclear, check-constraints things are merged into other patches.

14-15 are mew patches, to early check possibility of creating persistent bitmap with
  specified name and granularity in specified BDS

16-17: spelling, rewording, indenting, tiny code simplifying, check can_store (by 14-15),
    s/if (autoload && !persistent)/if (has_autoload && !persistent)/,
    rebase (deleted qmp-commands.hx)

18: alternative to md5 in query-block:
  - separated qmp command -x-debug-block-dirty-bitmap-sha256
  - as adviced by Daniel P. Berrange in my parallel thread:
    - sha256 instead of md5
    - use qcrypto_hash_* instead of GChecksum
  - fix bug =) (size was wrong in hbitmap_md5)

19: s/3999/3fff/, use x-debug-block-dirty-bitmap-sha256

20: new patch to rename and publish inc_refcounts

21: some fixes and refactoring mostyly by Max's comments.

Max, Eric, great tanks for your review!
I hope, I've covered most of your comments by this update.

TODO:
- handle reopening image RO->RW and incoming migration, set IN_USE for existing loaded bitmaps
  in these cases.
- reuse old, already allocated data clusters for bitmaps storing
- truncate bitmaps in the image on truncate


v7:

https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fqcow2-bitmap-v7
based on block-next (https://github.com/XanClic/qemu/commits/block-next)

- a lot of refactoring and reordering of patches.
- dead code removed (bdrv_dirty_bitmap_load, etc.)
- do not maintain extra data for now
- do not store dirty bitmap directory in memory
  (as we use it seldom, we can just reread if needed)

By Kevin's review:
01 - commit message changed: fix->improvement (as it was not a bug)
03 - r-b
04 - r-b
05 - add 21 patch to fix spec, also, removed all (I hope) mentions of
     "Bitmap Header", switch to one unified name for it - "Bitmap
     Directory Entry", to avoid misunderstanding with Qcow2 header.
     (also, add patch 22, to fix it in spec)
v6.06 - improve for_each_dir_entry loop, reorder patches, other small fixes
v6.07 ~> v7.09 - dead code removed, I've moved to one function 
        .bdrv_store_persistent_bitmaps and have wrapper and callback in one
        patch (with also some other staff. If it not ok, I can split them)
v6.08 - about keeping bitmap directory instead of bitmap list: no I don't keep
        it at all.
v6.16 - old bdrv_ bitmap-storing related functions are removed. The new one is
        bdrv_store_persistent_bitmaps.


v6:
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fqcow2-bitmap-v6
based on block-next (https://github.com/XanClic/qemu/commits/block-next)

There are a lot of changes, reorderings and additions in comparement with v5.
One principal thing: now bitmaps are removed from image after loading instead
of marking them in_use. It is simpler and we do not need to store superfluous data.
Also, we are no more interested in command line interface to dirty bitmaps.
So it is dropped.  If someone needs it I can add it later.

Vladimir Sementsov-Ogievskiy (21):
  specs/qcow2: fix bitmap granularity qemu-specific note
  specs/qcow2: do not use wording 'bitmap header'
  hbitmap: improve dirty iter
  tests: add hbitmap iter test
  block: fix bdrv_dirty_bitmap_granularity signature
  block/dirty-bitmap: add deserialize_ones func
  qcow2: add bitmaps extension
  block: introduce auto-loading bitmaps
  qcow2: add .bdrv_load_autoloading_dirty_bitmaps
  block/dirty-bitmap: add autoload field to BdrvDirtyBitmap
  block: introduce persistent dirty bitmaps
  block/dirty-bitmap: add bdrv_dirty_bitmap_next()
  qcow2: add .bdrv_store_persistent_dirty_bitmaps()
  block: add bdrv_can_store_dirty_bitmap
  qcow2: add .bdrv_can_store_dirty_bitmap
  qmp: add persistent flag to block-dirty-bitmap-add
  qmp: add autoload parameter to block-dirty-bitmap-add
  qmp: add x-debug-block-dirty-bitmap-sha256
  iotests: test qcow2 persistent dirty bitmap
  qcow2-refcount: rename inc_refcounts() and make it public
  qcow2-bitmap: refcounts

 block.c                      |   67 +++
 block/Makefile.objs          |    2 +-
 block/dirty-bitmap.c         |   63 ++-
 block/qcow2-bitmap.c         | 1230 ++++++++++++++++++++++++++++++++++++++++++
 block/qcow2-refcount.c       |   59 +-
 block/qcow2.c                |  111 +++-
 block/qcow2.h                |   39 ++
 blockdev.c                   |   64 ++-
 docs/qmp-commands.txt        |    7 +
 docs/specs/qcow2.txt         |    8 +-
 include/block/block.h        |    5 +
 include/block/block_int.h    |    7 +
 include/block/dirty-bitmap.h |   18 +-
 include/qemu/hbitmap.h       |   49 +-
 qapi/block-core.json         |   37 +-
 tests/Makefile.include       |    2 +-
 tests/qemu-iotests/165       |   89 +++
 tests/qemu-iotests/165.out   |    5 +
 tests/qemu-iotests/group     |    1 +
 tests/test-hbitmap.c         |   19 +
 util/hbitmap.c               |   51 +-
 21 files changed, 1872 insertions(+), 61 deletions(-)
 create mode 100644 block/qcow2-bitmap.c
 create mode 100755 tests/qemu-iotests/165
 create mode 100644 tests/qemu-iotests/165.out

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/21] specs/qcow2: fix bitmap granularity qemu-specific note

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 docs/specs/qcow2.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..dda53dd 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -472,8 +472,7 @@ Structure of a bitmap directory entry:
              17:    granularity_bits
                     Granularity bits. Valid values: 0 - 63.
 
-                    Note: Qemu currently doesn't support granularity_bits
-                    greater than 31.
+                    Note: Qemu currently supports only values 9 - 31.
 
                     Granularity is calculated as
                         granularity = 1 << granularity_bits
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/21] specs/qcow2: do not use wording 'bitmap header'

[Vladimir Sementsov-Ogievskiy]

A bitmap directory entry is sometimes called a 'bitmap header'. This
patch leaves only one name - 'bitmap directory entry'. The name 'bitmap
header' creates misunderstandings with 'qcow2 header' and 'qcow2 bitmap
header extension' (which is extension of qcow2 header)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 docs/specs/qcow2.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index dda53dd..8874e8c 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -201,7 +201,7 @@ The fields of the bitmaps extension are:
 
           8 - 15:  bitmap_directory_size
                    Size of the bitmap directory in bytes. It is the cumulative
-                   size of all (nb_bitmaps) bitmap headers.
+                   size of all (nb_bitmaps) bitmap directory entries.
 
          16 - 23:  bitmap_directory_offset
                    Offset into the image file at which the bitmap directory
@@ -426,8 +426,7 @@ Each bitmap saved in the image is described in a bitmap directory entry. The
 bitmap directory is a contiguous area in the image file, whose starting offset
 and length are given by the header extension fields bitmap_directory_offset and
 bitmap_directory_size. The entries of the bitmap directory have variable
-length, depending on the lengths of the bitmap name and extra data. These
-entries are also called bitmap headers.
+length, depending on the lengths of the bitmap name and extra data.
 
 Structure of a bitmap directory entry:
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/21] hbitmap: improve dirty iter

[Vladimir Sementsov-Ogievskiy]

Make dirty iter resistant to resetting bits in corresponding HBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 include/qemu/hbitmap.h | 26 ++++----------------------
 util/hbitmap.c         | 23 ++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index eb46475..2965f01 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -243,10 +243,9 @@ void hbitmap_free(HBitmap *hb);
  * the lowest-numbered bit that is set in @hb, starting at @first.
  *
  * Concurrent setting of bits is acceptable, and will at worst cause the
- * iteration to miss some of those bits.  Resetting bits before the current
- * position of the iterator is also okay.  However, concurrent resetting of
- * bits can lead to unexpected behavior if the iterator has not yet reached
- * those bits.
+ * iteration to miss some of those bits.
+ *
+ * The concurrent resetting of bits is OK.
  */
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
 
@@ -285,24 +284,7 @@ void hbitmap_free_meta(HBitmap *hb);
  * Return the next bit that is set in @hbi's associated HBitmap,
  * or -1 if all remaining bits are zero.
  */
-static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
-{
-    unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-    int64_t item;
-
-    if (cur == 0) {
-        cur = hbitmap_iter_skip_words(hbi);
-        if (cur == 0) {
-            return -1;
-        }
-    }
-
-    /* The next call will resume work from the next bit.  */
-    hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
-    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
-
-    return item << hbi->granularity;
-}
+int64_t hbitmap_iter_next(HBitmapIter *hbi);
 
 /**
  * hbitmap_iter_next_word:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 5d1a21c..48d8b2d 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 
     unsigned long cur;
     do {
-        cur = hbi->cur[--i];
+        i--;
         pos >>= BITS_PER_LEVEL;
+        cur = hbi->cur[i] & hb->levels[i][pos];
     } while (cur == 0);
 
     /* Check for end of iteration.  We always use fewer than BITS_PER_LONG
@@ -139,6 +140,26 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
     return cur;
 }
 
+int64_t hbitmap_iter_next(HBitmapIter *hbi)
+{
+    unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
+            hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
+    int64_t item;
+
+    if (cur == 0) {
+        cur = hbitmap_iter_skip_words(hbi);
+        if (cur == 0) {
+            return -1;
+        }
+    }
+
+    /* The next call will resume work from the next bit.  */
+    hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
+
+    return item << hbi->granularity;
+}
+
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
 {
     unsigned i, bit;
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/21] tests: add hbitmap iter test

[Vladimir Sementsov-Ogievskiy]

Test that hbitmap iter is resistant to bitmap resetting.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/test-hbitmap.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 9b7495c..c76d2b5 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -898,6 +898,22 @@ static void hbitmap_test_add(const char *testpath,
                hbitmap_test_teardown);
 }
 
+static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
+                                        const void *unused)
+{
+    HBitmapIter hbi;
+
+    hbitmap_test_init(data, L1 * 2, 0);
+    hbitmap_set(data->hb, 0, data->size);
+
+    hbitmap_iter_init(&hbi, data->hb, BITS_PER_LONG - 1);
+
+    hbitmap_iter_next(&hbi);
+
+    hbitmap_reset_all(data->hb);
+    hbitmap_iter_next(&hbi);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -955,6 +971,9 @@ int main(int argc, char **argv)
                      test_hbitmap_serialize_part);
     hbitmap_test_add("/hbitmap/serialize/zeroes",
                      test_hbitmap_serialize_zeroes);
+
+    hbitmap_test_add("/hbitmap/iter/iter_and_reset",
+                     test_hbitmap_iter_and_reset);
     g_test_run();
 
     return 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/21] block: fix bdrv_dirty_bitmap_granularity signature

[Vladimir Sementsov-Ogievskiy]

Make getter signature const-correct. This allows other functions with
const dirty bitmap parameter use bdrv_dirty_bitmap_granularity().

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         | 2 +-
 include/block/dirty-bitmap.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 519737c..186941c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
     return granularity;
 }
 
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
     return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14b..7cbe623 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -29,7 +29,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
 uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/21] block/dirty-bitmap: add deserialize_ones func

[Vladimir Sementsov-Ogievskiy]

Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
all-ones.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c         |  7 +++++++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h       | 15 +++++++++++++++
 util/hbitmap.c               | 17 +++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 186941c..90af372 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -499,6 +499,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
     hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
 }
 
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+                                        uint64_t start, uint64_t count,
+                                        bool finish)
+{
+    hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish);
+}
+
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
 {
     hbitmap_deserialize_finish(bitmap->bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 7cbe623..1e17729 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -70,6 +70,9 @@ void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
 void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
                                           uint64_t start, uint64_t count,
                                           bool finish);
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+                                        uint64_t start, uint64_t count,
+                                        bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 2965f01..063ec0e 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -216,6 +216,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
                                 bool finish);
 
 /**
+ * hbitmap_deserialize_ones
+ * @hb: HBitmap to operate on.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Fills the bitmap with ones.
+ *
+ * If @finish is false, caller must call hbitmap_serialize_finish before using
+ * the bitmap.
+ */
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+                              bool finish);
+
+/**
  * hbitmap_deserialize_finish
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 48d8b2d..72623f4 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -531,6 +531,23 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
     }
 }
 
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+                              bool finish)
+{
+    uint64_t el_count;
+    unsigned long *first;
+
+    if (!count) {
+        return;
+    }
+    serialization_chunk(hb, start, count, &first, &el_count);
+
+    memset(first, 0xff, el_count * sizeof(unsigned long));
+    if (finish) {
+        hbitmap_deserialize_finish(hb);
+    }
+}
+
 void hbitmap_deserialize_finish(HBitmap *bitmap)
 {
     int64_t i, size, prev_size;
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy]

Add bitmap extension as specified in docs/specs/qcow2.txt.
For now, just mirror extension header into Qcow2 state and check
constraints.

For now, disable image resize if it has bitmaps. It will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.h |  24 +++++++++++++
 2 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6d5689a..7908657 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -63,6 +63,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
     QCowExtension ext;
     uint64_t offset;
     int ret;
+    Qcow2BitmapHeaderExt bitmaps_ext;
 
 #ifdef DEBUG_EXT
     printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);
@@ -162,6 +164,81 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_BITMAPS:
+            if (ext.len != sizeof(bitmaps_ext)) {
+                error_setg_errno(errp, -ret, "bitmaps_ext: "
+                                 "Invalid extension length");
+                return -EINVAL;
+            }
+
+            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
+                fprintf(stderr,
+                        "WARNING: a program lacking bitmap support modified "
+                        "this file, so all bitmaps are now considered "
+                        "inconsistent");
+                break;
+            }
+
+            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "bitmaps_ext: "
+                                 "Could not read ext header");
+                return ret;
+            }
+
+            if (bitmaps_ext.reserved32 != 0) {
+                error_setg_errno(errp, -ret, "bitmaps_ext: "
+                                 "Reserved field is not zero");
+                return -EINVAL;
+            }
+
+            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
+
+            if (bitmaps_ext.nb_bitmaps > QCOW2_MAX_BITMAPS) {
+                error_setg(errp,
+                           "bitmaps_ext: File %s has %" PRIu32 " bitmaps, "
+                           "exceeding the QEMU supported maximum of %d",
+                           bs->filename, bitmaps_ext.nb_bitmaps,
+                           QCOW2_MAX_BITMAPS);
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.nb_bitmaps == 0) {
+                error_setg(errp, "found bitmaps extension with zero bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
+                error_setg(errp, "bitmaps_ext: "
+                                 "invalid bitmap directory offset");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_size >
+                QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
+                error_setg(errp, "bitmaps_ext: "
+                                 "bitmap directory size (%" PRIu64 ") exceeds "
+                                 "the maximum supported size (%d)",
+                                 bitmaps_ext.bitmap_directory_size,
+                                 QCOW2_MAX_BITMAP_DIRECTORY_SIZE);
+                return -EINVAL;
+            }
+
+            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
+            s->bitmap_directory_offset =
+                    bitmaps_ext.bitmap_directory_offset;
+            s->bitmap_directory_size =
+                    bitmaps_ext.bitmap_directory_size;
+
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got bitmaps extension: "
+                   "offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
+                   s->bitmap_directory_offset, s->nb_bitmaps);
+#endif
+            break;
+
         default:
             /* unknown magic - save it in case we need to rewrite the header */
             {
@@ -1144,8 +1221,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Clear unknown autoclear feature bits */
-    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && s->autoclear_features) {
-        s->autoclear_features = 0;
+    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
+        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
+        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
         ret = qcow2_update_header(bs);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not update qcow2 header");
@@ -1944,6 +2022,24 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    if (s->nb_bitmaps > 0) {
+        Qcow2BitmapHeaderExt bitmaps_header = {
+            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
+            .bitmap_directory_size =
+                    cpu_to_be64(s->bitmap_directory_size),
+            .bitmap_directory_offset =
+                    cpu_to_be64(s->bitmap_directory_offset)
+        };
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_BITMAPS,
+                             &bitmaps_header, sizeof(bitmaps_header),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Keep unknown header extensions */
     QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
         ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
@@ -2514,6 +2610,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
 
+    /* cannot proceed if image has bitmaps */
+    if (s->nb_bitmaps) {
+        /* TODO: resize bitmaps in the image */
+        error_report("Can't resize an image which has bitmaps");
+        return -ENOTSUP;
+    }
+
     /* shrinking is currently not supported */
     if (offset < bs->total_sectors * 512) {
         error_report("qcow2 doesn't support shrinking images yet");
diff --git a/block/qcow2.h b/block/qcow2.h
index 1823414..861b501 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -52,6 +52,10 @@
  * space for snapshot names and IDs */
 #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
 
+/* Bitmap header extension constraints */
+#define QCOW2_MAX_BITMAPS 65535
+#define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
+
 /* indicate that the refcount of the referenced cluster is exactly one. */
 #define QCOW_OFLAG_COPIED     (1ULL << 63)
 /* indicate that the cluster is compressed (they never have the copied flag) */
@@ -195,6 +199,15 @@ enum {
     QCOW2_COMPAT_FEAT_MASK            = QCOW2_COMPAT_LAZY_REFCOUNTS,
 };
 
+/* Autoclear feature bits */
+enum {
+    QCOW2_AUTOCLEAR_BITMAPS_BITNR = 0,
+    QCOW2_AUTOCLEAR_BITMAPS       =
+        1 << QCOW2_AUTOCLEAR_BITMAPS_BITNR,
+
+    QCOW2_AUTOCLEAR_MASK                = QCOW2_AUTOCLEAR_BITMAPS,
+};
+
 enum qcow2_discard_type {
     QCOW2_DISCARD_NEVER = 0,
     QCOW2_DISCARD_ALWAYS,
@@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
 typedef void Qcow2SetRefcountFunc(void *refcount_array,
                                   uint64_t index, uint64_t value);
 
+typedef struct Qcow2BitmapHeaderExt {
+    uint32_t nb_bitmaps;
+    uint32_t reserved32;
+    uint64_t bitmap_directory_size;
+    uint64_t bitmap_directory_offset;
+} QEMU_PACKED Qcow2BitmapHeaderExt;
+
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
@@ -263,6 +283,10 @@ typedef struct BDRVQcow2State {
     unsigned int nb_snapshots;
     QCowSnapshot *snapshots;
 
+    uint32_t nb_bitmaps;
+    uint64_t bitmap_directory_size;
+    uint64_t bitmap_directory_offset;
+
     int flags;
     int qcow_version;
     bool use_lazy_refcounts;
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/21] block: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

Auto loading bitmaps are bitmaps store in the disk image, which should
be loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                   | 14 ++++++++++++++
 include/block/block.h     |  2 ++
 include/block/block_int.h |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/block.c b/block.c
index 39ddea3..450379c 100644
--- a/block.c
+++ b/block.c
@@ -1137,6 +1137,13 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto free_and_fail;
     }
 
+    bdrv_load_autoloading_dirty_bitmaps(bs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto free_and_fail;
+    }
+
     assert(bdrv_opt_mem_align(bs) != 0);
     assert(bdrv_min_mem_align(bs) != 0);
     assert(is_power_of_2(bs->bl.request_alignment));
@@ -4093,3 +4100,10 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
 
     parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
+
+void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    if (bs->drv && bs->drv->bdrv_load_autoloading_dirty_bitmaps) {
+        bs->drv->bdrv_load_autoloading_dirty_bitmaps(bs, errp);
+    }
+}
diff --git a/include/block/block.h b/include/block/block.h
index 49bb0b2..f701da2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -552,4 +552,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
                     Error **errp);
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
+void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 83a423c..ca955f0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -320,6 +320,9 @@ struct BlockDriver {
     void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
                            Error **errp);
 
+    void (*bdrv_load_autoloading_dirty_bitmaps)(BlockDriverState *bs,
+                                                Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
are loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/Makefile.objs  |   2 +-
 block/qcow2-bitmap.c | 663 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        |   2 +
 block/qcow2.h        |   3 +
 4 files changed, 669 insertions(+), 1 deletion(-)
 create mode 100644 block/qcow2-bitmap.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 67a036a..b8b6cf3 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
 block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 0000000..0f797e6
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,663 @@
+/*
+ * Bitmaps for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2016 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "exec/log.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. */
+
+/* Bitmap directory entry constraints */
+#define BME_MAX_TABLE_SIZE 0x8000000
+#define BME_MAX_PHYS_SIZE 0x20000000 /* restrict BdrvDirtyBitmap size in RAM */
+#define BME_MAX_GRANULARITY_BITS 31
+#define BME_MIN_GRANULARITY_BITS 9
+#define BME_MAX_NAME_SIZE 1023
+
+/* Bitmap directory entry flags */
+#define BME_RESERVED_FLAGS 0xfffffffcU
+#define BME_FLAG_IN_USE 1
+#define BME_FLAG_AUTO   (1U << 1)
+
+/* bits [1, 8] U [56, 63] are reserved */
+#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
+#define BME_TABLE_ENTRY_OFFSET_MASK 0x00fffffffffffe00ULL
+
+typedef struct QEMU_PACKED Qcow2BitmapDirEntry {
+    /* header is 8 byte aligned */
+    uint64_t bitmap_table_offset;
+
+    uint32_t bitmap_table_size;
+    uint32_t flags;
+
+    uint8_t type;
+    uint8_t granularity_bits;
+    uint16_t name_size;
+    uint32_t extra_data_size;
+    /* extra data follows  */
+    /* name follows  */
+} Qcow2BitmapDirEntry;
+
+typedef struct Qcow2Bitmap {
+    uint64_t table_offset;
+    uint32_t table_size;
+    uint32_t flags;
+    uint8_t granularity_bits;
+    char *name;
+
+    QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
+} Qcow2Bitmap;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
+
+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_table_to_cpu(uint64_t *bitmap_table, size_t size)
+{
+    size_t i;
+
+    for (i = 0; i < size; ++i) {
+        be64_to_cpus(&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)
+{
+    uint64_t offset;
+
+    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
+        return -EINVAL;
+    }
+
+    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+    if (offset != 0) {
+        /* if offset specified, bit 0 must is reserved */
+        if (entry & 1) {
+            return -EINVAL;
+        }
+
+        if ((cluster_size != 0) && (entry % cluster_size != 0)) {
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
+static int bitmap_table_load(BlockDriverState *bs, Qcow2Bitmap *bm,
+                             uint64_t **bitmap_table)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint32_t i;
+    uint64_t *table;
+
+    assert(bm->table_size != 0);
+    table = g_try_new(uint64_t, bm->table_size);
+    if (table == NULL) {
+        return -ENOMEM;
+    }
+
+    assert(bm->table_size <= BME_MAX_TABLE_SIZE);
+    ret = bdrv_pread(bs->file, bm->table_offset,
+                     table, bm->table_size * sizeof(uint64_t));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    for (i = 0; i < bm->table_size; ++i) {
+        be64_to_cpus(&table[i]);
+        ret = check_table_entry(table[i], s->cluster_size);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+
+    *bitmap_table = table;
+    return 0;
+
+fail:
+    g_free(table);
+
+    return ret;
+}
+
+/* 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,
+                                               const BdrvDirtyBitmap *bitmap)
+{
+    uint32_t sector_granularity =
+            bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+
+    return (uint64_t)sector_granularity * (s->cluster_size << 3);
+}
+
+/* bitmap table entries must satisfy specification constraints */
+static int load_bitmap_data(BlockDriverState *bs,
+                            const uint64_t *bitmap_table,
+                            uint32_t 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);
+    uint8_t *buf = NULL;
+    uint64_t i, tab_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    if (tab_size != bitmap_table_size ||
+            tab_size > BME_MAX_TABLE_SIZE) {
+        return -EINVAL;
+    }
+
+    bdrv_clear_dirty_bitmap(bitmap, NULL);
+
+    buf = g_malloc(s->cluster_size);
+    dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
+    for (i = 0, sector = 0; i < tab_size; ++i, sector += dsc) {
+        uint64_t count = MIN(bm_size - sector, dsc);
+        uint64_t entry = bitmap_table[i];
+        uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+
+        assert(check_table_entry(entry, s->cluster_size) == 0);
+
+        if (offset == 0) {
+            if (entry & 1) {
+                bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, count,
+                                                   false);
+            } else {
+                /* No need to deserialize zeros because the dirty bitmap is
+                 * already cleared */
+            }
+        } else {
+            ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
+            if (ret < 0) {
+                goto finish;
+            }
+            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, count,
+                                               false);
+        }
+    }
+    ret = 0;
+
+    bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+    g_free(buf);
+
+    return ret;
+}
+
+static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
+                                    Qcow2Bitmap *bm, Error **errp)
+{
+    int ret;
+    uint64_t *bitmap_table = NULL;
+    uint32_t granularity;
+    BdrvDirtyBitmap *bitmap = NULL;
+
+    if (bm->flags & BME_FLAG_IN_USE) {
+        error_setg(errp, "Bitmap '%s' is in use", bm->name);
+        goto fail;
+    }
+
+    ret = bitmap_table_load(bs, bm, &bitmap_table);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Could not read bitmap_table table from image for "
+                         "bitmap '%s'", bm->name);
+        goto fail;
+    }
+
+    granularity = 1U << bm->granularity_bits;
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+    if (bitmap == NULL) {
+        goto fail;
+    }
+
+    ret = load_bitmap_data(bs, bitmap_table, bm->table_size,
+                           bitmap);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
+                         bm->name);
+        goto fail;
+    }
+
+    g_free(bitmap_table);
+    return bitmap;
+
+fail:
+    g_free(bitmap_table);
+    if (bitmap != NULL) {
+        bdrv_release_dirty_bitmap(bs, bitmap);
+    }
+
+    return NULL;
+}
+
+/*
+ * Bitmap List
+ */
+
+/*
+ * Bitmap List private functions
+ * Only Bitmap List knows about bitmap directory structure in Qcow2.
+ */
+
+static inline void bitmap_dir_entry_to_cpu(Qcow2BitmapDirEntry *entry)
+{
+    be64_to_cpus(&entry->bitmap_table_offset);
+    be32_to_cpus(&entry->bitmap_table_size);
+    be32_to_cpus(&entry->flags);
+    be16_to_cpus(&entry->name_size);
+    be32_to_cpus(&entry->extra_data_size);
+}
+
+static inline void bitmap_dir_entry_to_be(Qcow2BitmapDirEntry *entry)
+{
+    cpu_to_be64s(&entry->bitmap_table_offset);
+    cpu_to_be32s(&entry->bitmap_table_size);
+    cpu_to_be32s(&entry->flags);
+    cpu_to_be16s(&entry->name_size);
+    cpu_to_be32s(&entry->extra_data_size);
+}
+
+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_field(Qcow2BitmapDirEntry *entry)
+{
+    return (const char *)(entry + 1) + entry->extra_data_size;
+}
+
+static inline char *dir_entry_copy_name(Qcow2BitmapDirEntry *entry)
+{
+    const char *name_field = dir_entry_name_field(entry);
+    return g_strndup(name_field, entry->name_size);
+}
+
+static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
+{
+    return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
+}
+
+static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
+{
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t phys_bitmap_bytes =
+            (uint64_t)entry->bitmap_table_size * s->cluster_size;
+    uint64_t max_virtual_bits =
+            (phys_bitmap_bytes * 8) << entry->granularity_bits;
+    int64_t nb_sectors = bdrv_nb_sectors(bs);
+
+    if (nb_sectors < 0) {
+        return nb_sectors;
+    }
+
+    int fail =
+            (entry->bitmap_table_size == 0) ||
+            (entry->bitmap_table_offset == 0) ||
+            (entry->bitmap_table_offset % s->cluster_size) ||
+            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
+            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
+            (entry->bitmap_table_offset != 0 &&
+                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
+            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
+            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
+            (entry->flags & BME_RESERVED_FLAGS) ||
+            (entry->name_size > BME_MAX_NAME_SIZE) ||
+            (entry->type != BT_DIRTY_TRACKING_BITMAP);
+
+    return fail ? -EINVAL : 0;
+}
+
+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);
+    }
+}
+
+/*
+ * Bitmap List public functions
+ */
+
+static void bitmap_free(Qcow2Bitmap *bm)
+{
+    g_free(bm->name);
+    g_free(bm);
+}
+
+static void bitmap_list_free(Qcow2BitmapList *bm_list)
+{
+    Qcow2Bitmap *bm;
+
+    if (bm_list == NULL) {
+        return;
+    }
+
+    while ((bm = QSIMPLEQ_FIRST(bm_list)) != NULL) {
+        QSIMPLEQ_REMOVE_HEAD(bm_list, entry);
+        bitmap_free(bm);
+    }
+
+    g_free(bm_list);
+}
+
+static Qcow2BitmapList *bitmap_list_new(void)
+{
+    Qcow2BitmapList *bm_list = g_new(Qcow2BitmapList, 1);
+    QSIMPLEQ_INIT(bm_list);
+
+    return bm_list;
+}
+
+/* bitmap_list_load
+ * Get bitmap list from qcow2 image. Actually reads bitmap directory,
+ * checks it and convert to bitmap list.
+ */
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
+                                         uint64_t size, Error **errp)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint8_t *dir, *dir_end;
+    Qcow2BitmapDirEntry *e;
+    uint32_t nb_dir_entries = 0;
+    Qcow2BitmapList *bm_list = NULL;
+
+    if (size == 0) {
+        error_setg(errp, "Requested bitmap directory size is zero");
+        return NULL;
+    }
+
+    if (size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
+        error_setg(errp, "Requested bitmap directory size is too big");
+        return NULL;
+    }
+
+    dir = g_try_malloc(size);
+    if (dir == NULL) {
+        error_setg(errp, "Failed to allocate space for bitmap directory");
+        return NULL;
+    }
+    dir_end = dir + size;
+
+    ret = bdrv_pread(bs->file, offset, dir, size);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to read bitmap directory");
+        goto fail;
+    }
+
+    bm_list = bitmap_list_new();
+    for (e = (Qcow2BitmapDirEntry *)dir;
+             e < (Qcow2BitmapDirEntry *)dir_end; e = next_dir_entry(e)) {
+        Qcow2Bitmap *bm;
+
+        if ((uint8_t *)(e + 1) > dir_end) {
+            goto broken_dir;
+        }
+
+        if (++nb_dir_entries > s->nb_bitmaps) {
+            error_setg(errp, "More bitmaps found than specified in header"
+                       " extension");
+            goto fail;
+        }
+        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, "Bitmap extra data is not supported");
+            goto fail;
+        }
+
+        ret = check_dir_entry(bs, e);
+        if (ret < 0) {
+            error_setg(errp, "Bitmap '%.*s' doesn't satisfy the constraints",
+                       e->name_size, dir_entry_name_field(e));
+            goto fail;
+        }
+
+        bm = g_new(Qcow2Bitmap, 1);
+        bm->table_offset = e->bitmap_table_offset;
+        bm->table_size = e->bitmap_table_size;
+        bm->flags = e->flags;
+        bm->granularity_bits = e->granularity_bits;
+        bm->name = dir_entry_copy_name(e);
+        QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
+    }
+
+    if (nb_dir_entries != s->nb_bitmaps) {
+        error_setg(errp, "Less bitmaps found than specified in header"
+                         " extension");
+        goto fail;
+    }
+
+    if ((uint8_t *)e != dir_end) {
+        goto broken_dir;
+    }
+
+    g_free(dir);
+    return bm_list;
+
+broken_dir:
+    ret = -EINVAL;
+    error_setg(errp, "Broken bitmap directory");
+
+fail:
+    g_free(dir);
+    bitmap_list_free(bm_list);
+
+    return NULL;
+}
+
+/* bitmap_list_store
+ * Store bitmap list to qcow2 image as a bitmap directory.
+ * Everything is checked.
+ */
+static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
+                             uint64_t *offset, uint64_t *size, bool in_place)
+{
+    int ret;
+    uint8_t *dir;
+    int64_t dir_offset = 0;
+    uint64_t dir_size = 0;
+    Qcow2Bitmap *bm;
+    Qcow2BitmapDirEntry *e;
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        dir_size += calc_dir_entry_size(strlen(bm->name), 0);
+    }
+
+    if (dir_size == 0 || dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
+        return -EINVAL;
+    }
+
+    if (in_place) {
+        if (*size != dir_size || *offset == 0) {
+            return -EINVAL;
+        }
+
+        dir_offset = *offset;
+    }
+
+    dir = g_try_malloc(dir_size);
+    if (dir == NULL) {
+        return -ENOMEM;
+    }
+
+    e = (Qcow2BitmapDirEntry *)dir;
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        e->bitmap_table_offset = bm->table_offset;
+        e->bitmap_table_size = bm->table_size;
+        e->flags = bm->flags;
+        e->type = BT_DIRTY_TRACKING_BITMAP;
+        e->granularity_bits = bm->granularity_bits;
+        e->name_size = strlen(bm->name);
+        e->extra_data_size = 0;
+        memcpy(e + 1, bm->name, e->name_size);
+
+        if (check_dir_entry(bs, e) < 0) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        e = next_dir_entry(e);
+    }
+
+    bitmap_directory_to_be(dir, dir_size);
+
+    if (!in_place) {
+        dir_offset = qcow2_alloc_clusters(bs, dir_size);
+        if (dir_offset < 0) {
+            ret = dir_offset;
+            goto fail;
+        }
+    }
+
+    ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = bdrv_pwrite(bs->file, dir_offset, dir, dir_size);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    g_free(dir);
+
+    if (!in_place) {
+        *size = dir_size;
+        *offset = dir_offset;
+    }
+
+    return 0;
+
+fail:
+    g_free(dir);
+
+    if (dir_offset > 0) {
+        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);
+    }
+
+    return ret;
+}
+
+/*
+ * Bitmap List end
+ */
+
+/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
+static void release_dirty_bitmap_helper(gpointer bitmap,
+                                        gpointer bs)
+{
+    bdrv_release_dirty_bitmap(bs, bitmap);
+}
+
+void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+    GSList *created_dirty_bitmaps = NULL;
+
+    if (s->nb_bitmaps == 0) {
+        /* No bitmaps - nothing to do */
+        return;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, errp);
+    if (bm_list == NULL) {
+        return;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        if ((bm->flags & BME_FLAG_AUTO) && !(bm->flags & BME_FLAG_IN_USE)) {
+            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+            if (bitmap == NULL) {
+                goto fail;
+            }
+            bm->flags |= BME_FLAG_IN_USE;
+            created_dirty_bitmaps =
+                    g_slist_append(created_dirty_bitmaps, bitmap);
+        }
+    }
+
+    if (created_dirty_bitmaps != NULL && can_write(bs)) {
+        /* in_use flags must be updated */
+        int ret = bitmap_list_store(bs, bm_list,
+                                    &s->bitmap_directory_offset,
+                                    &s->bitmap_directory_size, true);
+        if (ret < 0) {
+            error_setg(errp, "Can't update bitmap directory");
+            goto fail;
+        }
+    }
+
+    g_slist_free(created_dirty_bitmaps);
+    bitmap_list_free(bm_list);
+
+    return;
+
+fail:
+    g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
+    g_slist_free(created_dirty_bitmaps);
+    bitmap_list_free(bm_list);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 7908657..443be0e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3513,6 +3513,8 @@ BlockDriver bdrv_qcow2 = {
 
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+    .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 861b501..bcedf5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -613,4 +613,7 @@ int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
     void **table);
 void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
 
+/* qcow2-bitmap.c functions */
+void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/21] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

Mirror AUTO flag from Qcow2 bitmap in BdrvDirtyBitmap. This will be
needed in future, to save this flag back to Qcow2 for persistent
bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/dirty-bitmap.c         | 16 ++++++++++++++++
 block/qcow2-bitmap.c         |  2 ++
 include/block/dirty-bitmap.h |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af372..2d27494 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
     int active_iterators;       /* How many iterators are active */
+    bool autoload;              /* For persistent bitmaps: bitmap must be
+                                   autoloaded on image opening */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -70,6 +72,8 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
     assert(!bdrv_dirty_bitmap_frozen(bitmap));
     g_free(bitmap->name);
     bitmap->name = NULL;
+
+    bitmap->autoload = false;
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
@@ -238,6 +242,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->name = NULL;
     successor->name = name;
     bitmap->successor = NULL;
+    successor->autoload = bitmap->autoload;
+    bitmap->autoload = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
     return hbitmap_count(bitmap->meta);
 }
+
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
+{
+    bitmap->autoload = autoload;
+}
+
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->autoload;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0f797e6..787bf03 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -634,6 +634,8 @@ void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             if (bitmap == NULL) {
                 goto fail;
             }
+
+            bdrv_dirty_bitmap_set_autoload(bitmap, true);
             bm->flags |= BME_FLAG_IN_USE;
             created_dirty_bitmaps =
                     g_slist_append(created_dirty_bitmaps, bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729..45a389a 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -75,4 +75,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
                                         bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/21] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
on bdrv_close, using format driver. Format driver should maintain bitmap
storing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                      | 31 +++++++++++++++++++++++++++++++
 block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
 block/qcow2-bitmap.c         |  1 +
 include/block/block.h        |  1 +
 include/block/block_int.h    |  2 ++
 include/block/dirty-bitmap.h |  6 ++++++
 6 files changed, 67 insertions(+)

diff --git a/block.c b/block.c
index 450379c..4e1f16a 100644
--- a/block.c
+++ b/block.c
@@ -2321,6 +2321,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 static void bdrv_close(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
+    Error *local_err = NULL;
 
     assert(!bs->job);
     assert(!bs->refcnt);
@@ -2329,6 +2330,11 @@ static void bdrv_close(BlockDriverState *bs)
     bdrv_flush(bs);
     bdrv_drain(bs); /* in case flush left pending I/O */
 
+    bdrv_store_persistent_dirty_bitmaps(bs, &local_err);
+    if (local_err != NULL) {
+        error_report_err(local_err);
+        error_report("Persistent bitmaps are lost");
+    }
     bdrv_release_named_dirty_bitmaps(bs);
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
@@ -4107,3 +4113,28 @@ void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         bs->drv->bdrv_load_autoloading_dirty_bitmaps(bs, errp);
     }
 }
+
+void bdrv_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    BlockDriver *drv = bs->drv;
+
+    if (!bdrv_has_persistent_bitmaps(bs)) {
+        return;
+    }
+
+    if (!drv) {
+        error_setg_errno(errp, ENOMEDIUM,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return;
+    }
+
+    if (!drv->bdrv_store_persistent_dirty_bitmaps) {
+        error_setg_errno(errp, ENOTSUP,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return;
+    }
+
+    drv->bdrv_store_persistent_dirty_bitmaps(bs, errp);
+}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 2d27494..4d026df 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,7 @@ struct BdrvDirtyBitmap {
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
     int active_iterators;       /* How many iterators are active */
+    bool persistent;            /* bitmap must be saved to owner disk image */
     bool autoload;              /* For persistent bitmaps: bitmap must be
                                    autoloaded on image opening */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -73,6 +74,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
     g_free(bitmap->name);
     bitmap->name = NULL;
 
+    bitmap->persistent = false;
     bitmap->autoload = false;
 }
 
@@ -242,6 +244,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->name = NULL;
     successor->name = name;
     bitmap->successor = NULL;
+    successor->persistent = bitmap->persistent;
+    bitmap->persistent = false;
     successor->autoload = bitmap->autoload;
     bitmap->autoload = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
@@ -556,3 +560,25 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
 {
     return bitmap->autoload;
 }
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
+{
+    bitmap->persistent = persistent;
+}
+
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->persistent;
+}
+
+bool bdrv_has_persistent_bitmaps(BlockDriverState *bs)
+{
+    BdrvDirtyBitmap *bm;
+    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+        if (bm->persistent) {
+            return true;
+        }
+    }
+
+    return false;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 787bf03..81be1ca 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -635,6 +635,7 @@ void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
                 goto fail;
             }
 
+            bdrv_dirty_bitmap_set_persistance(bitmap, true);
             bdrv_dirty_bitmap_set_autoload(bitmap, true);
             bm->flags |= BME_FLAG_IN_USE;
             created_dirty_bitmaps =
diff --git a/include/block/block.h b/include/block/block.h
index f701da2..e3175b5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -553,5 +553,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
 void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void bdrv_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ca955f0..e8d9210 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -322,6 +322,8 @@ struct BlockDriver {
 
     void (*bdrv_load_autoloading_dirty_bitmaps)(BlockDriverState *bs,
                                                 Error **errp);
+    void (*bdrv_store_persistent_dirty_bitmaps)(BlockDriverState *bs,
+                                                Error **errp);
 
     QLIST_ENTRY(BlockDriver) list;
 };
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 45a389a..8dbd16b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -77,4 +77,10 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+                                                bool persistent);
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+
+bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/21] block/dirty-bitmap: add bdrv_dirty_bitmap_next()

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/dirty-bitmap.c         | 7 +++++++
 include/block/dirty-bitmap.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4d026df..fe34d48 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -582,3 +582,10 @@ bool bdrv_has_persistent_bitmaps(BlockDriverState *bs)
 
     return false;
 }
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+                                        BdrvDirtyBitmap *bitmap)
+{
+    return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
+                            QLIST_NEXT(bitmap, list);
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8dbd16b..d71edc4 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -81,6 +81,9 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                                 bool persistent);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
 
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+                                        BdrvDirtyBitmap *bitmap);
+
 bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
 
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

Realize block bitmap storing interface, to allow qcow2 images store
persistent bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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;
+
     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
+     * 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) >>
+                                 granularity_bits;
+    uint64_t bitmap_table_size = phys_bitmap_bytes / s->cluster_size;
+    size_t name_size = strlen(name);
+
+    int fail =
+            (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;
+        }
+
+        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) {
+        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;
+
+        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);
+        }
+
+        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);
+    }
+
+    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;
+    }
+
+    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)) {
+        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;
+        }
+
+        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)) {
+                error_setg(errp, "Bitmap '%s' is already exists in the image",
+                           name);
+                goto fail;
+            }
+            free_bitmap_clusters(bs, bm);
+            /* 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;
+    }
+
+    /* 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
-- 
1.8.3.1

[Qemu-devel] [PATCH 14/21] block: add bdrv_can_store_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

This will be needed to check some restrictions before making bitmap
persistent in qmp-block-dirty-bitmap-add (this functionality will be
added by future patch)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                   | 22 ++++++++++++++++++++++
 include/block/block.h     |  2 ++
 include/block/block_int.h |  2 ++
 3 files changed, 26 insertions(+)

diff --git a/block.c b/block.c
index 4e1f16a..2f002b0 100644
--- a/block.c
+++ b/block.c
@@ -4138,3 +4138,25 @@ void bdrv_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 
     drv->bdrv_store_persistent_dirty_bitmaps(bs, errp);
 }
+
+bool bdrv_can_store_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                 uint32_t granularity, Error **errp)
+{
+    BlockDriver *drv = bs->drv;
+
+    if (!drv) {
+        error_setg_errno(errp, ENOMEDIUM,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return false;
+    }
+
+    if (!drv->bdrv_can_store_dirty_bitmap) {
+        error_setg_errno(errp, ENOTSUP,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return false;
+    }
+
+    return drv->bdrv_can_store_dirty_bitmap(bs, name, granularity, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index e3175b5..08ffa8e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -554,5 +554,7 @@ void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
 void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 void bdrv_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool bdrv_can_store_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                 uint32_t granularity, Error **errp);
 
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e8d9210..2cdf709 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -324,6 +324,8 @@ struct BlockDriver {
                                                 Error **errp);
     void (*bdrv_store_persistent_dirty_bitmaps)(BlockDriverState *bs,
                                                 Error **errp);
+    bool (*bdrv_can_store_dirty_bitmap)(BlockDriverState *bs, const char *name,
+                                        uint32_t granularity, Error **errp);
 
     QLIST_ENTRY(BlockDriver) list;
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/21] qcow2: add .bdrv_can_store_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_can_store_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 40 ++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        |  1 +
 block/qcow2.h        |  4 ++++
 3 files changed, 45 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a975388..55b1112 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1115,3 +1115,43 @@ fail:
 
     bitmap_list_free(bm_list);
 }
+
+bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
+                                  const char *name,
+                                  uint32_t granularity,
+                                  Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    const char *reason = NULL;
+    bool found;
+    Qcow2BitmapList *bm_list;
+
+    if (check_constraints_on_bitmap(bs, name, granularity) != 0) {
+        reason = "it doesn't satisfy the constraints";
+        goto common_errp;
+    }
+
+    if (s->nb_bitmaps == 0) {
+        return true;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, errp);
+    if (bm_list == NULL) {
+        return false;
+    }
+
+    found = !!find_bitmap_by_name(bm_list, name);
+    bitmap_list_free(bm_list);
+    if (found) {
+        reason = "bitmap with the same name is already stored";
+        goto common_errp;
+    }
+
+    return true;
+
+common_errp:
+    error_setg(errp, "Can't make bitmap '%s' persistent in '%s', as %s.",
+               name, bdrv_get_device_or_node_name(bs), reason);
+    return false;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 66c7f74..cb9c2a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3516,6 +3516,7 @@ BlockDriver bdrv_qcow2 = {
 
     .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
     .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps,
+    .bdrv_can_store_dirty_bitmap = qcow2_can_store_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index d9a7643..e7a44a1 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -616,5 +616,9 @@ 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);
+bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
+                                  const char *name,
+                                  uint32_t granularity,
+                                  Error **errp);
 
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 16/21] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 blockdev.c            | 17 ++++++++++++++++-
 docs/qmp-commands.txt |  3 +++
 qapi/block-core.json  |  7 ++++++-
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 245e1e1..3876d1d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1967,6 +1967,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
     /* AIO context taken and released within qmp_block_dirty_bitmap_add */
     qmp_block_dirty_bitmap_add(action->node, action->name,
                                action->has_granularity, action->granularity,
+                               action->has_persistent, action->persistent,
                                &local_err);
 
     if (!local_err) {
@@ -2696,10 +2697,12 @@ out:
 
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
                                 bool has_granularity, uint32_t granularity,
+                                bool has_persistent, bool persistent,
                                 Error **errp)
 {
     AioContext *aio_context;
     BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2725,7 +2728,19 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         granularity = bdrv_get_default_bitmap_granularity(bs);
     }
 
-    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (!has_persistent) {
+        persistent = false;
+    }
+
+    if (persistent &&
+            !bdrv_can_store_dirty_bitmap(bs, name, granularity, errp)) {
+        goto out;
+    }
+
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (bitmap != NULL) {
+        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    }
 
  out:
     aio_context_release(aio_context);
diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index abf210a..c4ad1e4 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -1015,6 +1015,9 @@ Arguments:
 - "node": device/node on which to create dirty bitmap (json-string)
 - "name": name of the new dirty bitmap (json-string)
 - "granularity": granularity to track writes with (int, optional)
+- "persistent": bitmap will be saved to the corresponding block device image
+                file on its close. For now only Qcow2 disks support persistent
+                bitmaps. (json-bool, optional, default false) (Since 2.8)
 
 Example:
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c29bef7..cec312c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1231,10 +1231,15 @@
 # @granularity: #optional the bitmap granularity, default is 64k for
 #               block-dirty-bitmap-add
 #
+# @persistent: #optional the bitmap is persistent, i.e. it will be saved to the
+#              corresponding block device image file on its close. Default is
+#              false. (Since 2.8)
+#
 # Since 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
-  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
+            '*persistent': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add
-- 
1.8.3.1

[Qemu-devel] [PATCH 17/21] qmp: add autoload parameter to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Optional. Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 blockdev.c            | 18 ++++++++++++++++--
 docs/qmp-commands.txt |  4 ++++
 qapi/block-core.json  |  6 +++++-
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3876d1d..3891d86 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1968,6 +1968,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
     qmp_block_dirty_bitmap_add(action->node, action->name,
                                action->has_granularity, action->granularity,
                                action->has_persistent, action->persistent,
+                               action->has_autoload, action->autoload,
                                &local_err);
 
     if (!local_err) {
@@ -2698,6 +2699,7 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
                                 bool has_granularity, uint32_t granularity,
                                 bool has_persistent, bool persistent,
+                                bool has_autoload, bool autoload,
                                 Error **errp)
 {
     AioContext *aio_context;
@@ -2731,6 +2733,15 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     if (!has_persistent) {
         persistent = false;
     }
+    if (!has_autoload) {
+        autoload = false;
+    }
+
+    if (has_autoload && !persistent) {
+        error_setg(errp, "Autoload flag must be used only for persistent "
+                         "bitmaps");
+        goto out;
+    }
 
     if (persistent &&
             !bdrv_can_store_dirty_bitmap(bs, name, granularity, errp)) {
@@ -2738,10 +2749,13 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
-    if (bitmap != NULL) {
-        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    if (bitmap == NULL) {
+        goto out;
     }
 
+    bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
+
  out:
     aio_context_release(aio_context);
 }
diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index c4ad1e4..dda2911 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -1018,6 +1018,10 @@ Arguments:
 - "persistent": bitmap will be saved to the corresponding block device image
                 file on its close. For now only Qcow2 disks support persistent
                 bitmaps. (json-bool, optional, default false) (Since 2.8)
+- "autoload": the bitmap will be automatically loaded when the image it is
+              stored in is opened. This flag may only be specified for
+              persistent bitmaps (json-bool, optional, default false)
+              (Since 2.8)
 
 Example:
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cec312c..648f94a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1235,11 +1235,15 @@
 #              corresponding block device image file on its close. Default is
 #              false. (Since 2.8)
 #
+# @autoload: #optional the bitmap will be automatically loaded when the image
+#            it is stored in is opened. This flag may only be specified for
+#            persistent bitmaps. Default is false. (Since 2.8)
+#
 # Since 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-            '*persistent': 'bool' } }
+            '*persistent': 'bool', '*autoload': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add
-- 
1.8.3.1

[Qemu-devel] [PATCH 18/21] qmp: add x-debug-block-dirty-bitmap-sha256

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         |  5 +++++
 blockdev.c                   | 33 +++++++++++++++++++++++++++++++++
 include/block/dirty-bitmap.h |  2 ++
 include/qemu/hbitmap.h       |  8 ++++++++
 qapi/block-core.json         | 26 ++++++++++++++++++++++++++
 tests/Makefile.include       |  2 +-
 util/hbitmap.c               | 11 +++++++++++
 7 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index fe34d48..775181c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -589,3 +589,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
     return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
                             QLIST_NEXT(bitmap, list);
 }
+
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
+{
+    return hbitmap_sha256(bitmap->bitmap, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index 3891d86..f129ccf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2819,6 +2819,39 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
     aio_context_release(aio_context);
 }
 
+/**
+ * Completely clear a bitmap, for the purposes of synchronizing a bitmap
+ * immediately after a full backup operation.
+ */
+BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
+                                                              const char *name,
+                                                              Error **errp)
+{
+    AioContext *aio_context;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+    BlockDirtyBitmapSha256 *ret = NULL;
+    char *sha256;
+
+    bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context, errp);
+    if (!bitmap || !bs) {
+        return NULL;
+    }
+
+    sha256 = bdrv_dirty_bitmap_sha256(bitmap, errp);
+    if (sha256 == NULL) {
+        goto out;
+    }
+
+    ret = g_new(BlockDirtyBitmapSha256, 1);
+    ret->sha256 = sha256;
+
+out:
+    aio_context_release(aio_context);
+
+    return ret;
+}
+
 void hmp_drive_del(Monitor *mon, const QDict *qdict)
 {
     const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index d71edc4..b022b34 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -86,4 +86,6 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
 
 bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
 
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 063ec0e..750346c 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -240,6 +240,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
 void hbitmap_deserialize_finish(HBitmap *hb);
 
 /**
+ * hbitmap_sha256:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns SHA256 hash of the last level.
+ */
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 648f94a..76f6c60 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1289,6 +1289,32 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @BlockDirtyBitmapSha256:
+#
+# SHA256 hash of dirty bitmap data
+#
+# @sha256: bitmap SHA256 hash
+#
+# Since: 2.8
+##
+  { 'struct': 'BlockDirtyBitmapSha256',
+    'data': {'sha256': 'str'} }
+
+##
+# @x-debug-block-dirty-bitmap-sha256
+#
+# Get bitmap SHA256
+#
+# Returns: BlockDirtyBitmapSha256 on success
+#          If @node is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explanation
+#
+# Since 2.8
+##
+  { 'command': 'x-debug-block-dirty-bitmap-sha256',
+    'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+
+##
 # @blockdev-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e98d3b6..b85ff78 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -496,7 +496,7 @@ tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-u
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
-tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
+tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
 tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o page_cache.o $(test-util-obj-y)
 tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 72623f4..96af071 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -13,6 +13,7 @@
 #include "qemu/hbitmap.h"
 #include "qemu/host-utils.h"
 #include "trace.h"
+#include "crypto/hash.h"
 
 /* HBitmaps provides an array of bits.  The bits are stored as usual in an
  * array of unsigned longs, but HBitmap is also optimized to provide fast
@@ -707,3 +708,13 @@ void hbitmap_free_meta(HBitmap *hb)
     hbitmap_free(hb->meta);
     hb->meta = NULL;
 }
+
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
+{
+    size_t size = bitmap->sizes[HBITMAP_LEVELS - 1] * sizeof(unsigned long);
+    char *data = (char *)bitmap->levels[HBITMAP_LEVELS - 1];
+    char *hash = NULL;
+    qcrypto_hash_digest(QCRYPTO_HASH_ALG_SHA256, data, size, &hash, errp);
+
+    return hash;
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH 19/21] iotests: test qcow2 persistent dirty bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/165     | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/165.out |  5 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 95 insertions(+)
 create mode 100755 tests/qemu-iotests/165
 create mode 100644 tests/qemu-iotests/165.out

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
new file mode 100755
index 0000000..0b04b04
--- /dev/null
+++ b/tests/qemu-iotests/165
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for persistent dirty bitmaps.
+#
+# Copyright: Vladimir Sementsov-Ogievskiy 2015-2016
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+disk_size = 0x40000000 # 1G
+
+# regions for qemu_io: (start, count) in bytes
+regions1 = ((0,        0x100000),
+            (0x200000, 0x100000))
+
+regions2 = ((0x10000000, 0x20000),
+            (0x3fff0000, 0x10000))
+
+class TestPersistentDirtyBitmap(iotests.QMPTestCase):
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+
+    def tearDown(self):
+        os.remove(disk)
+
+    def mkVm(self):
+        return iotests.VM().add_drive(disk)
+
+    def getSha256(self):
+        result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+                             node='drive0', name='bitmap0')
+        return result['return']['sha256']
+
+    def checkBitmap(self, sha256):
+        result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+                             node='drive0', name='bitmap0')
+        self.assert_qmp(result, 'return/sha256', sha256);
+
+    def writeRegions(self, regions):
+        for r in regions:
+          self.vm.hmp_qemu_io('drive0',
+                              'write %d %d' % r)
+
+    def qmpAddBitmap(self):
+        self.vm.qmp('block-dirty-bitmap-add', node='drive0',
+                    name='bitmap0', persistent=True, autoload=True)
+
+    def test_persistent(self):
+        self.vm = self.mkVm()
+        self.vm.launch()
+        self.qmpAddBitmap()
+
+        self.writeRegions(regions1)
+        sha256 = self.getSha256()
+
+        self.vm.shutdown()
+        self.vm = self.mkVm()
+        self.vm.launch()
+
+        self.checkBitmap(sha256)
+        self.writeRegions(regions2)
+        sha256 = self.getSha256()
+
+        self.vm.shutdown()
+        self.vm.launch()
+
+        self.checkBitmap(sha256)
+
+        self.vm.shutdown()
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/165.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 866c1a0..8bd7a84 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -162,6 +162,7 @@
 159 rw auto quick
 160 rw auto quick
 162 auto quick
+165 rw auto quick
 170 rw auto quick
 171 rw auto quick
 172 auto
-- 
1.8.3.1

[Qemu-devel] [PATCH 20/21] qcow2-refcount: rename inc_refcounts() and make it public

[Vladimir Sementsov-Ogievskiy]

This is needed for the following patch, which will introduce refcounts
checking for qcow2 bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 53 ++++++++++++++++++++++++++------------------------
 block/qcow2.h          |  4 ++++
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cbfb3fe..14a736d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1309,11 +1309,10 @@ static int realloc_refcount_array(BDRVQcow2State *s, void **array,
  *
  * Modifies the number of errors in res.
  */
-static int inc_refcounts(BlockDriverState *bs,
-                         BdrvCheckResult *res,
-                         void **refcount_table,
-                         int64_t *refcount_table_size,
-                         int64_t offset, int64_t size)
+int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
+                             void **refcount_table,
+                             int64_t *refcount_table_size,
+                             int64_t offset, int64_t size)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t start, last, cluster_offset, k, refcount;
@@ -1406,8 +1405,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             nb_csectors = ((l2_entry >> s->csize_shift) &
                            s->csize_mask) + 1;
             l2_entry &= s->cluster_offset_mask;
-            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-                                l2_entry & ~511, nb_csectors * 512);
+            ret = qcow2_inc_refcounts_imrt(bs, res,
+                                           refcount_table, refcount_table_size,
+                                           l2_entry & ~511, nb_csectors * 512);
             if (ret < 0) {
                 goto fail;
             }
@@ -1445,8 +1445,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             }
 
             /* Mark cluster as used */
-            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-                                offset, s->cluster_size);
+            ret = qcow2_inc_refcounts_imrt(bs, res,
+                                           refcount_table, refcount_table_size,
+                                           offset, s->cluster_size);
             if (ret < 0) {
                 goto fail;
             }
@@ -1498,8 +1499,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
     l1_size2 = l1_size * sizeof(uint64_t);
 
     /* Mark L1 table as used */
-    ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-                        l1_table_offset, l1_size2);
+    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
+                                   l1_table_offset, l1_size2);
     if (ret < 0) {
         goto fail;
     }
@@ -1528,8 +1529,9 @@ static int check_refcounts_l1(BlockDriverState *bs,
         if (l2_offset) {
             /* Mark L2 table as used */
             l2_offset &= L1E_OFFSET_MASK;
-            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-                                l2_offset, s->cluster_size);
+            ret = qcow2_inc_refcounts_imrt(bs, res,
+                                           refcount_table, refcount_table_size,
+                                           l2_offset, s->cluster_size);
             if (ret < 0) {
                 goto fail;
             }
@@ -1744,14 +1746,15 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
                 }
 
                 res->corruptions_fixed++;
-                ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-                                    offset, s->cluster_size);
+                ret = qcow2_inc_refcounts_imrt(bs, res,
+                                               refcount_table, nb_clusters,
+                                               offset, s->cluster_size);
                 if (ret < 0) {
                     return ret;
                 }
                 /* No need to check whether the refcount is now greater than 1:
                  * This area was just allocated and zeroed, so it can only be
-                 * exactly 1 after inc_refcounts() */
+                 * exactly 1 after qcow2_inc_refcounts_imrt() */
                 continue;
 
 resize_fail:
@@ -1766,8 +1769,8 @@ resize_fail:
         }
 
         if (offset != 0) {
-            ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-                                offset, s->cluster_size);
+            ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
+                                           offset, s->cluster_size);
             if (ret < 0) {
                 return ret;
             }
@@ -1807,8 +1810,8 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
     /* header */
-    ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-                        0, s->cluster_size);
+    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
+                                   0, s->cluster_size);
     if (ret < 0) {
         return ret;
     }
@@ -1829,16 +1832,16 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
             return ret;
         }
     }
-    ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-                        s->snapshots_offset, s->snapshots_size);
+    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
+                                   s->snapshots_offset, s->snapshots_size);
     if (ret < 0) {
         return ret;
     }
 
     /* refcount data */
-    ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-                        s->refcount_table_offset,
-                        s->refcount_table_size * sizeof(uint64_t));
+    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
+                                   s->refcount_table_offset,
+                                   s->refcount_table_size * sizeof(uint64_t));
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index e7a44a1..c52da79 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -543,6 +543,10 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                                  int64_t size);
 int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
                                   int64_t size);
+int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
+                             void **refcount_table,
+                             int64_t *refcount_table_size,
+                             int64_t offset, int64_t size);
 
 int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
                                 BlockDriverAmendStatusCB *status_cb,
-- 
1.8.3.1

[Qemu-devel] [PATCH 21/21] qcow2-bitmap: refcounts

[Vladimir Sementsov-Ogievskiy]

Calculate refcounts for qcow2 bitmaps. It is needed for qcow2's qemu-img
check implementation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c   | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2-refcount.c |  6 +++++
 block/qcow2.h          |  3 +++
 3 files changed, 82 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 55b1112..0e0ddf3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1155,3 +1155,76 @@ common_errp:
                name, bdrv_get_device_or_node_name(bs), reason);
     return false;
 }
+
+int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+                                  void **refcount_table,
+                                  int64_t *refcount_table_size)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+
+    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
+                                   s->bitmap_directory_offset,
+                                   s->bitmap_directory_size);
+    if (ret < 0) {
+        return ret;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, NULL);
+    if (bm_list == NULL) {
+        res->corruptions++;
+        return -EINVAL;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        uint64_t *bitmap_table = NULL;
+        int i;
+
+        ret = qcow2_inc_refcounts_imrt(bs, res,
+                                       refcount_table, refcount_table_size,
+                                       bm->table_offset,
+                                       bm->table_size * sizeof(uint64_t));
+        if (ret < 0) {
+            goto out;
+        }
+
+        ret = bitmap_table_load(bs, bm, &bitmap_table);
+        if (ret < 0) {
+            res->corruptions++;
+            goto out;
+        }
+
+        for (i = 0; i < bm->table_size; ++i) {
+            uint64_t entry = bitmap_table[i];
+            uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+
+            if (check_table_entry(entry, s->cluster_size) < 0) {
+                res->corruptions++;
+                ret = -EINVAL;
+                continue;
+            }
+
+            if (offset == 0) {
+                continue;
+            }
+
+            ret = qcow2_inc_refcounts_imrt(bs, res,
+                                           refcount_table, refcount_table_size,
+                                           offset, s->cluster_size);
+            if (ret < 0) {
+                g_free(bitmap_table);
+                goto out;
+            }
+        }
+
+        g_free(bitmap_table);
+    }
+
+out:
+    bitmap_list_free(bm_list);
+
+    return ret;
+}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 14a736d..cdb74ba 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1846,6 +1846,12 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
         return ret;
     }
 
+    /* bitmaps */
+    ret = qcow2_check_bitmaps_refcounts(bs, res, refcount_table, nb_clusters);
+    if (ret < 0) {
+        return ret;
+    }
+
     return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index c52da79..29c3d31 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -624,5 +624,8 @@ bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
                                   const char *name,
                                   uint32_t granularity,
                                   Error **errp);
+int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+                                  void **refcount_table,
+                                  int64_t *refcount_table_size);
 
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Add bitmap extension as specified in docs/specs/qcow2.txt.
> For now, just mirror extension header into Qcow2 state and check
> constraints.
> 
> For now, disable image resize if it has bitmaps. It will be fixed later.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  block/qcow2.h |  24 +++++++++++++
>  2 files changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6d5689a..7908657 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -63,6 +63,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_END 0
>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> +#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>  
>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
> @@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>      QCowExtension ext;
>      uint64_t offset;
>      int ret;
> +    Qcow2BitmapHeaderExt bitmaps_ext;
>  
>  #ifdef DEBUG_EXT
>      printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);
> @@ -162,6 +164,81 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_BITMAPS:
> +            if (ext.len != sizeof(bitmaps_ext)) {
> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
> +                                 "Invalid extension length");
> +                return -EINVAL;
> +            }
> +
> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
> +                fprintf(stderr,
> +                        "WARNING: a program lacking bitmap support modified "
> +                        "this file, so all bitmaps are now considered "
> +                        "inconsistent");
> +                break;
> +            }

I think It would be nice if qemu-img check could remove all bitmaps and
related data if this bit is not set. Otherwise, we're practically
leaking data.

(But this can be added in a later patch.)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 08/21] block: introduce auto-loading bitmaps

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps store in the disk image, which should

s/store/stored/

> be loaded when the image is opened and become BdrvDirtyBitmaps for the
> corresponding drive.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 14 ++++++++++++++
>  include/block/block.h     |  2 ++
>  include/block/block_int.h |  3 +++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 39ddea3..450379c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1137,6 +1137,13 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
>          goto free_and_fail;
>      }
>  
> +    bdrv_load_autoloading_dirty_bitmaps(bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto free_and_fail;
> +    }
> +
>      assert(bdrv_opt_mem_align(bs) != 0);
>      assert(bdrv_min_mem_align(bs) != 0);
>      assert(is_power_of_2(bs->bl.request_alignment));

These asserts should stay directly after the bdrv_refresh_limits() block
(i.e. the bdrv_load_autoloading_dirty_bitmaps() call should be moved
here, after them).

The rest looks good.

Max

> @@ -4093,3 +4100,10 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>  
>      parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>  }
> +
> +void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    if (bs->drv && bs->drv->bdrv_load_autoloading_dirty_bitmaps) {
> +        bs->drv->bdrv_load_autoloading_dirty_bitmaps(bs, errp);
> +    }
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index 49bb0b2..f701da2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -552,4 +552,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>                      Error **errp);
>  void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>  
> +void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +
>  #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 83a423c..ca955f0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -320,6 +320,9 @@ struct BlockDriver {
>      void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
>                             Error **errp);
>  
> +    void (*bdrv_load_autoloading_dirty_bitmaps)(BlockDriverState *bs,
> +                                                Error **errp);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 16/21] qmp: add persistent flag to block-dirty-bitmap-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

On 11/22/2016 11:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
> Default is false.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---

> +++ b/qapi/block-core.json
> @@ -1231,10 +1231,15 @@
>  # @granularity: #optional the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
>  #
> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to the
> +#              corresponding block device image file on its close. Default is
> +#              false. (Since 2.8)

s/2.8/2.9/

> +#
>  # Since 2.4
>  ##
>  { 'struct': 'BlockDirtyBitmapAdd',
> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> +            '*persistent': 'bool' } }
>  
>  ##
>  # @block-dirty-bitmap-add
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5093 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
> are loaded when the image is opened and become BdrvDirtyBitmaps for the
> corresponding drive.
> 
> Extra data in bitmaps is not supported for now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/Makefile.objs  |   2 +-
>  block/qcow2-bitmap.c | 663 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |   2 +
>  block/qcow2.h        |   3 +
>  4 files changed, 669 insertions(+), 1 deletion(-)
>  create mode 100644 block/qcow2-bitmap.c
> 

[...]

> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> new file mode 100644
> index 0000000..0f797e6
> --- /dev/null
> +++ b/block/qcow2-bitmap.c
> @@ -0,0 +1,663 @@

[...]

> +/* 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)
> +{
> +    uint64_t offset;
> +
> +    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
> +        return -EINVAL;
> +    }
> +
> +    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
> +    if (offset != 0) {
> +        /* if offset specified, bit 0 must is reserved */

-must

> +        if (entry & 1) {
> +            return -EINVAL;
> +        }
> +
> +        if ((cluster_size != 0) && (entry % cluster_size != 0)) {

Why would cluster_size be 0? Also, shouldn't it be offset instead of entry?

> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}

[...]

> +/* bitmap table entries must satisfy specification constraints */
> +static int load_bitmap_data(BlockDriverState *bs,
> +                            const uint64_t *bitmap_table,
> +                            uint32_t 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);
> +    uint8_t *buf = NULL;
> +    uint64_t i, tab_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
> +
> +    if (tab_size != bitmap_table_size ||
> +            tab_size > BME_MAX_TABLE_SIZE) {

This line should be aligned to the opening parenthesis.

[...]

> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t phys_bitmap_bytes =
> +            (uint64_t)entry->bitmap_table_size * s->cluster_size;
> +    uint64_t max_virtual_bits =
> +            (phys_bitmap_bytes * 8) << entry->granularity_bits;

I think this shift can overflow, even if bitmap_table_size, cluster_size
and granularity_bits are all within their respective limits.

> +    int64_t nb_sectors = bdrv_nb_sectors(bs);

Just using bdrv_getlength() would probably be simpler.

> +
> +    if (nb_sectors < 0) {
> +        return nb_sectors;
> +    }
> +
> +    int fail =

I'd prefer a boolean.

> +            (entry->bitmap_table_size == 0) ||
> +            (entry->bitmap_table_offset == 0) ||
> +            (entry->bitmap_table_offset % s->cluster_size) ||
> +            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> +            (entry->bitmap_table_offset != 0 &&
> +                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
> +            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
> +            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
> +            (entry->flags & BME_RESERVED_FLAGS) ||
> +            (entry->name_size > BME_MAX_NAME_SIZE) ||
> +            (entry->type != BT_DIRTY_TRACKING_BITMAP);
> +
> +    return fail ? -EINVAL : 0;
> +}

[...]

> +/* bitmap_list_load
> + * Get bitmap list from qcow2 image. Actually reads bitmap directory,
> + * checks it and convert to bitmap list.
> + */
> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
> +                                         uint64_t size, Error **errp)
> +{

[...]

> +    bm_list = bitmap_list_new();
> +    for (e = (Qcow2BitmapDirEntry *)dir;
> +             e < (Qcow2BitmapDirEntry *)dir_end; e = next_dir_entry(e)) {

This line should be aligned to the opening parenthesis.

[...]

> +
> +/* bitmap_list_store
> + * Store bitmap list to qcow2 image as a bitmap directory.
> + * Everything is checked.
> + */
> +static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
> +                             uint64_t *offset, uint64_t *size, bool in_place)
> +{

[...]

> +fail:
> +    g_free(dir);
> +
> +    if (dir_offset > 0) {
> +        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);

I think this should only be freed if in_place is false.

Max

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 11/21] block: introduce persistent dirty bitmaps

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
> on bdrv_close, using format driver. Format driver should maintain bitmap
> storing.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                      | 31 +++++++++++++++++++++++++++++++
>  block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>  block/qcow2-bitmap.c         |  1 +
>  include/block/block.h        |  1 +
>  include/block/block_int.h    |  2 ++
>  include/block/dirty-bitmap.h |  6 ++++++
>  6 files changed, 67 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 450379c..4e1f16a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2321,6 +2321,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>  static void bdrv_close(BlockDriverState *bs)
>  {
>      BdrvAioNotifier *ban, *ban_next;
> +    Error *local_err = NULL;
>  
>      assert(!bs->job);
>      assert(!bs->refcnt);
> @@ -2329,6 +2330,11 @@ static void bdrv_close(BlockDriverState *bs)
>      bdrv_flush(bs);
>      bdrv_drain(bs); /* in case flush left pending I/O */
>  
> +    bdrv_store_persistent_dirty_bitmaps(bs, &local_err);
> +    if (local_err != NULL) {
> +        error_report_err(local_err);
> +        error_report("Persistent bitmaps are lost");

:C

Reviewed-by: Max Reitz <mreitz@redhat.com>

But could you maybe add at least the node name to the error message?

Max

> +    }
>      bdrv_release_named_dirty_bitmaps(bs);
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 08/21] block: introduce auto-loading bitmaps

[John Snow]

On 11/22/2016 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps store in the disk image, which should
> be loaded when the image is opened and become BdrvDirtyBitmaps for the
> corresponding drive.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 14 ++++++++++++++
>  include/block/block.h     |  2 ++
>  include/block/block_int.h |  3 +++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 39ddea3..450379c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1137,6 +1137,13 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
>          goto free_and_fail;
>      }
>  
> +    bdrv_load_autoloading_dirty_bitmaps(bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto free_and_fail;
> +    }
> +
>      assert(bdrv_opt_mem_align(bs) != 0);
>      assert(bdrv_min_mem_align(bs) != 0);
>      assert(is_power_of_2(bs->bl.request_alignment));
> @@ -4093,3 +4100,10 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>  
>      parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>  }
> +
> +void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    if (bs->drv && bs->drv->bdrv_load_autoloading_dirty_bitmaps) {
> +        bs->drv->bdrv_load_autoloading_dirty_bitmaps(bs, errp);
> +    }
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index 49bb0b2..f701da2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -552,4 +552,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>                      Error **errp);
>  void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>  
> +void bdrv_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +
>  #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 83a423c..ca955f0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -320,6 +320,9 @@ struct BlockDriver {
>      void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
>                             Error **errp);
>  
> +    void (*bdrv_load_autoloading_dirty_bitmaps)(BlockDriverState *bs,
> +                                                Error **errp);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> 

If you address Max's comments:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 19853 bytes --]

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 <vsementsov@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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 14/21] block: add bdrv_can_store_dirty_bitmap

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> This will be needed to check some restrictions before making bitmap
> persistent in qmp-block-dirty-bitmap-add (this functionality will be
> added by future patch)
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 22 ++++++++++++++++++++++
>  include/block/block.h     |  2 ++
>  include/block/block_int.h |  2 ++
>  3 files changed, 26 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 15/21] qcow2: add .bdrv_can_store_dirty_bitmap

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3562 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Realize .bdrv_can_store_dirty_bitmap interface.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |  1 +
>  block/qcow2.h        |  4 ++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index a975388..55b1112 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1115,3 +1115,43 @@ fail:
>  
>      bitmap_list_free(bm_list);
>  }
> +
> +bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
> +                                  const char *name,
> +                                  uint32_t granularity,
> +                                  Error **errp)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    const char *reason = NULL;
> +    bool found;
> +    Qcow2BitmapList *bm_list;
> +
> +    if (check_constraints_on_bitmap(bs, name, granularity) != 0) {
> +        reason = "it doesn't satisfy the constraints";
> +        goto common_errp;
> +    }
> +
> +    if (s->nb_bitmaps == 0) {
> +        return true;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                               s->bitmap_directory_size, errp);

Maybe it would make sense to keep the bitmap list in memory...

> +    if (bm_list == NULL) {
> +        return false;
> +    }
> +
> +    found = !!find_bitmap_by_name(bm_list, name);

You can omit the !!, storing it in a bool will do that for you.

> +    bitmap_list_free(bm_list);
> +    if (found) {
> +        reason = "bitmap with the same name is already stored";
> +        goto common_errp;
> +    }
> +
> +    return true;
> +
> +common_errp:
> +    error_setg(errp, "Can't make bitmap '%s' persistent in '%s', as %s.",
> +               name, bdrv_get_device_or_node_name(bs), reason);

Hm, so this function isn't for checking whether a bitmap can be stored,
but whether it can be made persistent. That's a difference; you can
store a bitmap of the same name in the file if you want to overwrite it
(i.e. it's an auto-load bitmap), but you cannot create a new bitmap with
the same name.

Maybe rename it from bdrv_can_store_dirty_bitmap to
bdrv_can_store_new_dirty_bitmap?

Max

> +    return false;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 66c7f74..cb9c2a2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3516,6 +3516,7 @@ BlockDriver bdrv_qcow2 = {
>  
>      .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
>      .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps,
> +    .bdrv_can_store_dirty_bitmap = qcow2_can_store_dirty_bitmap,
>  };
>  
>  static void bdrv_qcow2_init(void)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index d9a7643..e7a44a1 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -616,5 +616,9 @@ 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);
> +bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
> +                                  const char *name,
> +                                  uint32_t granularity,
> +                                  Error **errp);
>  
>  #endif
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 16/21] qmp: add persistent flag to block-dirty-bitmap-add

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3997 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
> Default is false.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  blockdev.c            | 17 ++++++++++++++++-
>  docs/qmp-commands.txt |  3 +++
>  qapi/block-core.json  |  7 ++++++-
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 245e1e1..3876d1d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1967,6 +1967,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>      /* AIO context taken and released within qmp_block_dirty_bitmap_add */
>      qmp_block_dirty_bitmap_add(action->node, action->name,
>                                 action->has_granularity, action->granularity,
> +                               action->has_persistent, action->persistent,
>                                 &local_err);
>  
>      if (!local_err) {
> @@ -2696,10 +2697,12 @@ out:
>  
>  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>                                  bool has_granularity, uint32_t granularity,
> +                                bool has_persistent, bool persistent,
>                                  Error **errp)
>  {
>      AioContext *aio_context;
>      BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
>  
>      if (!name || name[0] == '\0') {
>          error_setg(errp, "Bitmap name cannot be empty");
> @@ -2725,7 +2728,19 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>          granularity = bdrv_get_default_bitmap_granularity(bs);
>      }
>  
> -    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (!has_persistent) {
> +        persistent = false;
> +    }
> +
> +    if (persistent &&
> +            !bdrv_can_store_dirty_bitmap(bs, name, granularity, errp)) {

Alignment to the opening parenthesis, please.

(Also, this may need a change depending on whether you want to rename it
to bdrv_can_store_new_dirty_bitmap().)

With that and the s/2\.8/2.9/ below fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +        goto out;
> +    }
> +
> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (bitmap != NULL) {
> +        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
> +    }
>  
>   out:
>      aio_context_release(aio_context);
> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> index abf210a..c4ad1e4 100644
> --- a/docs/qmp-commands.txt
> +++ b/docs/qmp-commands.txt
> @@ -1015,6 +1015,9 @@ Arguments:
>  - "node": device/node on which to create dirty bitmap (json-string)
>  - "name": name of the new dirty bitmap (json-string)
>  - "granularity": granularity to track writes with (int, optional)
> +- "persistent": bitmap will be saved to the corresponding block device image
> +                file on its close. For now only Qcow2 disks support persistent
> +                bitmaps. (json-bool, optional, default false) (Since 2.8)
>  
>  Example:
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c29bef7..cec312c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1231,10 +1231,15 @@
>  # @granularity: #optional the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
>  #
> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to the
> +#              corresponding block device image file on its close. Default is
> +#              false. (Since 2.8)
> +#
>  # Since 2.4
>  ##
>  { 'struct': 'BlockDirtyBitmapAdd',
> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> +            '*persistent': 'bool' } }
>  
>  ##
>  # @block-dirty-bitmap-add
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

09.12.2016 20:05, Max Reitz wrote:
> 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 <vsementsov@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

[...]

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

Hmm.. without an alignment it is not so simple to distinguish for-loop 
header from its body.


[...]

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

Right. Or, alternatively, these bitmaps may be deleted on corresponding 
BdrvDirtyBitmap deletion.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 17/21] qmp: add autoload parameter to block-dirty-bitmap-add

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4110 bytes --]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Optional. Default is false.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  blockdev.c            | 18 ++++++++++++++++--
>  docs/qmp-commands.txt |  4 ++++
>  qapi/block-core.json  |  6 +++++-
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 3876d1d..3891d86 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1968,6 +1968,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>      qmp_block_dirty_bitmap_add(action->node, action->name,
>                                 action->has_granularity, action->granularity,
>                                 action->has_persistent, action->persistent,
> +                               action->has_autoload, action->autoload,
>                                 &local_err);
>  
>      if (!local_err) {
> @@ -2698,6 +2699,7 @@ out:
>  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>                                  bool has_granularity, uint32_t granularity,
>                                  bool has_persistent, bool persistent,
> +                                bool has_autoload, bool autoload,
>                                  Error **errp)
>  {
>      AioContext *aio_context;
> @@ -2731,6 +2733,15 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>      if (!has_persistent) {
>          persistent = false;
>      }
> +    if (!has_autoload) {
> +        autoload = false;
> +    }
> +
> +    if (has_autoload && !persistent) {
> +        error_setg(errp, "Autoload flag must be used only for persistent "
> +                         "bitmaps");
> +        goto out;
> +    }
>  
>      if (persistent &&
>              !bdrv_can_store_dirty_bitmap(bs, name, granularity, errp)) {
> @@ -2738,10 +2749,13 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>      }
>  
>      bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> -    if (bitmap != NULL) {
> -        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
> +    if (bitmap == NULL) {
> +        goto out;
>      }
>  
> +    bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
> +    bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
> +
>   out:
>      aio_context_release(aio_context);
>  }
> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> index c4ad1e4..dda2911 100644
> --- a/docs/qmp-commands.txt
> +++ b/docs/qmp-commands.txt
> @@ -1018,6 +1018,10 @@ Arguments:
>  - "persistent": bitmap will be saved to the corresponding block device image
>                  file on its close. For now only Qcow2 disks support persistent
>                  bitmaps. (json-bool, optional, default false) (Since 2.8)
> +- "autoload": the bitmap will be automatically loaded when the image it is
> +              stored in is opened. This flag may only be specified for
> +              persistent bitmaps (json-bool, optional, default false)
> +              (Since 2.8)

*2.9

>  
>  Example:
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index cec312c..648f94a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1235,11 +1235,15 @@
>  #              corresponding block device image file on its close. Default is
>  #              false. (Since 2.8)
>  #
> +# @autoload: #optional the bitmap will be automatically loaded when the image
> +#            it is stored in is opened. This flag may only be specified for
> +#            persistent bitmaps. Default is false. (Since 2.8)

*2.9

With both fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +#
>  # Since 2.4
>  ##
>  { 'struct': 'BlockDirtyBitmapAdd',
>    'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> -            '*persistent': 'bool' } }
> +            '*persistent': 'bool', '*autoload': 'bool' } }
>  
>  ##
>  # @block-dirty-bitmap-add
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 15/21] qcow2: add .bdrv_can_store_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

09.12.2016 20:28, Max Reitz wrote:
> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>> Realize .bdrv_can_store_dirty_bitmap interface.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |  1 +
>>   block/qcow2.h        |  4 ++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index a975388..55b1112 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1115,3 +1115,43 @@ fail:
>>   
>>       bitmap_list_free(bm_list);
>>   }
>> +
>> +bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
>> +                                  const char *name,
>> +                                  uint32_t granularity,
>> +                                  Error **errp)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    const char *reason = NULL;
>> +    bool found;
>> +    Qcow2BitmapList *bm_list;
>> +
>> +    if (check_constraints_on_bitmap(bs, name, granularity) != 0) {
>> +        reason = "it doesn't satisfy the constraints";
>> +        goto common_errp;
>> +    }
>> +
>> +    if (s->nb_bitmaps == 0) {
>> +        return true;
>> +    }
>> +
>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> +                               s->bitmap_directory_size, errp);
> Maybe it would make sense to keep the bitmap list in memory...
>
>> +    if (bm_list == NULL) {
>> +        return false;
>> +    }
>> +
>> +    found = !!find_bitmap_by_name(bm_list, name);
> You can omit the !!, storing it in a bool will do that for you.
>
>> +    bitmap_list_free(bm_list);
>> +    if (found) {
>> +        reason = "bitmap with the same name is already stored";
>> +        goto common_errp;
>> +    }
>> +
>> +    return true;
>> +
>> +common_errp:
>> +    error_setg(errp, "Can't make bitmap '%s' persistent in '%s', as %s.",
>> +               name, bdrv_get_device_or_node_name(bs), reason);
> Hm, so this function isn't for checking whether a bitmap can be stored,
> but whether it can be made persistent. That's a difference; you can
> store a bitmap of the same name in the file if you want to overwrite it
> (i.e. it's an auto-load bitmap), but you cannot create a new bitmap with
> the same name.
>
> Maybe rename it from bdrv_can_store_dirty_bitmap to
> bdrv_can_store_new_dirty_bitmap?

Yes, I'll rename it.

>
> Max
>
>> +    return false;
>> +}
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 66c7f74..cb9c2a2 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3516,6 +3516,7 @@ BlockDriver bdrv_qcow2 = {
>>   
>>       .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps,
>>       .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps,
>> +    .bdrv_can_store_dirty_bitmap = qcow2_can_store_dirty_bitmap,
>>   };
>>   
>>   static void bdrv_qcow2_init(void)
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index d9a7643..e7a44a1 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -616,5 +616,9 @@ 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);
>> +bool qcow2_can_store_dirty_bitmap(BlockDriverState *bs,
>> +                                  const char *name,
>> +                                  uint32_t granularity,
>> +                                  Error **errp);
>>   
>>   #endif
>>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2016 20:05, Max Reitz wrote:
>> 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 <vsementsov@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
> 
> [...]
> 
>>> +            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.
> 
> Hmm.. without an alignment it is not so simple to distinguish for-loop
> header from its body.

I know, and it's even worse for "if". That is why I usually put the
opening { on a new line if I have to spread an if/while/for header over
multiple lines.

The usual convention for qemu code is to align at an opening parenthesis
if there is one.

Admittedly, the reasoning I gave for changing checkpatch.pl to accept
opening { on a new line in certain cases was that:

(1) We never codified exactly what to allow for multi-line if/while/for
    conditions.
(2) It was existing practice.

(1) applies in your case also; we don't have any explicitly written-out
convention for alignment of wrapped lines. (2) is more difficult, but
there are indeed a handful of cases where lines are wrapped and not
aligned to the opening parenthesis but just indented by an additional
four spaces...

So I guess since I'm insisting on putting the opening { on a new line
for multi-line conditions, you are allowed to indent the consecutive
lines by an additional level. ;-)

(It *is* against existing convention, but I'm not in a position to argue.)

> [...]
> 
>> [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.
> 
> Right. Or, alternatively, these bitmaps may be deleted on corresponding
> BdrvDirtyBitmap deletion.

Right, that would work, too.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

10.12.2016 17:53, Max Reitz wrote:
> On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote:
>> 09.12.2016 20:05, Max Reitz wrote:
>>> 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 <vsementsov@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
>> [...]
>>
>>>> +            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.
>> Hmm.. without an alignment it is not so simple to distinguish for-loop
>> header from its body.
> I know, and it's even worse for "if". That is why I usually put the
> opening { on a new line if I have to spread an if/while/for header over
> multiple lines.
>
> The usual convention for qemu code is to align at an opening parenthesis
> if there is one.
>
> Admittedly, the reasoning I gave for changing checkpatch.pl to accept
> opening { on a new line in certain cases was that:

Good news, didn't know)

>
> (1) We never codified exactly what to allow for multi-line if/while/for
>      conditions.
> (2) It was existing practice.
>
> (1) applies in your case also; we don't have any explicitly written-out
> convention for alignment of wrapped lines. (2) is more difficult, but
> there are indeed a handful of cases where lines are wrapped and not
> aligned to the opening parenthesis but just indented by an additional
> four spaces...
>
> So I guess since I'm insisting on putting the opening { on a new line
> for multi-line conditions, you are allowed to indent the consecutive
> lines by an additional level. ;-)
>
> (It *is* against existing convention, but I'm not in a position to argue.)
>
>> [...]
>>
>>> [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.
>> Right. Or, alternatively, these bitmaps may be deleted on corresponding
>> BdrvDirtyBitmap deletion.
> Right, that would work, too.
>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 18/21] qmp: add x-debug-block-dirty-bitmap-sha256

[Max Reitz]

On 2016-11-22 at 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c         |  5 +++++
>  blockdev.c                   | 33 +++++++++++++++++++++++++++++++++
>  include/block/dirty-bitmap.h |  2 ++
>  include/qemu/hbitmap.h       |  8 ++++++++
>  qapi/block-core.json         | 26 ++++++++++++++++++++++++++
>  tests/Makefile.include       |  2 +-
>  util/hbitmap.c               | 11 +++++++++++
>  7 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index fe34d48..775181c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -589,3 +589,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>      return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
>                              QLIST_NEXT(bitmap, list);
>  }
> +
> +char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
> +{
> +    return hbitmap_sha256(bitmap->bitmap, errp);
> +}
> diff --git a/blockdev.c b/blockdev.c
> index 3891d86..f129ccf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2819,6 +2819,39 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>      aio_context_release(aio_context);
>  }
>
> +/**
> + * Completely clear a bitmap, for the purposes of synchronizing a bitmap
> + * immediately after a full backup operation.

This comment doesn't seem quite right.

> + */
> +BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
> +                                                              const char *name,
> +                                                              Error **errp)
> +{
> +    AioContext *aio_context;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +    BlockDirtyBitmapSha256 *ret = NULL;
> +    char *sha256;
> +
> +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context, errp);
> +    if (!bitmap || !bs) {
> +        return NULL;
> +    }
> +
> +    sha256 = bdrv_dirty_bitmap_sha256(bitmap, errp);
> +    if (sha256 == NULL) {
> +        goto out;
> +    }
> +
> +    ret = g_new(BlockDirtyBitmapSha256, 1);
> +    ret->sha256 = sha256;
> +
> +out:
> +    aio_context_release(aio_context);
> +
> +    return ret;
> +}
> +
>  void hmp_drive_del(Monitor *mon, const QDict *qdict)
>  {
>      const char *id = qdict_get_str(qdict, "id");
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index d71edc4..b022b34 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -86,4 +86,6 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>
>  bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
>
> +char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> +
>  #endif
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 063ec0e..750346c 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -240,6 +240,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
>  void hbitmap_deserialize_finish(HBitmap *hb);
>
>  /**
> + * hbitmap_sha256:
> + * @bitmap: HBitmap to operate on.
> + *
> + * Returns SHA256 hash of the last level.
> + */
> +char *hbitmap_sha256(const HBitmap *bitmap, Error **errp);
> +
> +/**
>   * hbitmap_free:
>   * @hb: HBitmap to operate on.
>   *
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 648f94a..76f6c60 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1289,6 +1289,32 @@
>    'data': 'BlockDirtyBitmap' }
>
>  ##
> +# @BlockDirtyBitmapSha256:
> +#
> +# SHA256 hash of dirty bitmap data
> +#
> +# @sha256: bitmap SHA256 hash
> +#
> +# Since: 2.8

*2.9

> +##
> +  { 'struct': 'BlockDirtyBitmapSha256',
> +    'data': {'sha256': 'str'} }
> +
> +##
> +# @x-debug-block-dirty-bitmap-sha256

Should end in a colon ("@x-debug-block-dirty-bitmap-sha256:").

> +#
> +# Get bitmap SHA256
> +#
> +# Returns: BlockDirtyBitmapSha256 on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation

Theoretically, hashing can also fail, but, well...

> +#
> +# Since 2.8

*2.9 and also "Since: 2.9" (with colon).

Looks good apart from these minor things.

Max

> +##
> +  { 'command': 'x-debug-block-dirty-bitmap-sha256',
> +    'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
> +
> +##
>  # @blockdev-mirror
>  #
>  # Start mirroring a block device's writes to a new destination.
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e98d3b6..b85ff78 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -496,7 +496,7 @@ tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-u
>  tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
>  tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
>  tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
> -tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
> +tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
>  tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
>  tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o page_cache.o $(test-util-obj-y)
>  tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 72623f4..96af071 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -13,6 +13,7 @@
>  #include "qemu/hbitmap.h"
>  #include "qemu/host-utils.h"
>  #include "trace.h"
> +#include "crypto/hash.h"
>
>  /* HBitmaps provides an array of bits.  The bits are stored as usual in an
>   * array of unsigned longs, but HBitmap is also optimized to provide fast
> @@ -707,3 +708,13 @@ void hbitmap_free_meta(HBitmap *hb)
>      hbitmap_free(hb->meta);
>      hb->meta = NULL;
>  }
> +
> +char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
> +{
> +    size_t size = bitmap->sizes[HBITMAP_LEVELS - 1] * sizeof(unsigned long);
> +    char *data = (char *)bitmap->levels[HBITMAP_LEVELS - 1];
> +    char *hash = NULL;
> +    qcrypto_hash_digest(QCRYPTO_HASH_ALG_SHA256, data, size, &hash, errp);
> +
> +    return hash;
> +}
>

Re: [Qemu-devel] [PATCH 19/21] iotests: test qcow2 persistent dirty bitmap

[Max Reitz]

On 2016-11-22 at 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/165     | 89 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/165.out |  5 +++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 95 insertions(+)
>  create mode 100755 tests/qemu-iotests/165
>  create mode 100644 tests/qemu-iotests/165.out
>
> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
> new file mode 100755
> index 0000000..0b04b04
> --- /dev/null
> +++ b/tests/qemu-iotests/165
> @@ -0,0 +1,89 @@
> +#!/usr/bin/env python
> +#
> +# Tests for persistent dirty bitmaps.
> +#
> +# Copyright: Vladimir Sementsov-Ogievskiy 2015-2016
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +from iotests import qemu_img
> +
> +disk = os.path.join(iotests.test_dir, 'disk')
> +disk_size = 0x40000000 # 1G
> +
> +# regions for qemu_io: (start, count) in bytes
> +regions1 = ((0,        0x100000),
> +            (0x200000, 0x100000))
> +
> +regions2 = ((0x10000000, 0x20000),
> +            (0x3fff0000, 0x10000))
> +
> +class TestPersistentDirtyBitmap(iotests.QMPTestCase):
> +
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
> +
> +    def tearDown(self):
> +        os.remove(disk)
> +
> +    def mkVm(self):
> +        return iotests.VM().add_drive(disk)
> +
> +    def getSha256(self):
> +        result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
> +                             node='drive0', name='bitmap0')
> +        return result['return']['sha256']
> +
> +    def checkBitmap(self, sha256):
> +        result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
> +                             node='drive0', name='bitmap0')
> +        self.assert_qmp(result, 'return/sha256', sha256);
> +
> +    def writeRegions(self, regions):
> +        for r in regions:
> +          self.vm.hmp_qemu_io('drive0',

Indentation should be two more spaces. With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +                              'write %d %d' % r)

Re: [Qemu-devel] [PATCH 20/21] qcow2-refcount: rename inc_refcounts() and make it public

[Max Reitz]

On 2016-11-22 at 18:26, Vladimir Sementsov-Ogievskiy wrote:
> This is needed for the following patch, which will introduce refcounts
> checking for qcow2 bitmaps.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-refcount.c | 53 ++++++++++++++++++++++++++------------------------
>  block/qcow2.h          |  4 ++++
>  2 files changed, 32 insertions(+), 25 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 21/21] qcow2-bitmap: refcounts

[Max Reitz]

On 2016-11-22 at 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Calculate refcounts for qcow2 bitmaps. It is needed for qcow2's qemu-img
> check implementation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c   | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2-refcount.c |  6 +++++
>  block/qcow2.h          |  3 +++
>  3 files changed, 82 insertions(+)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 55b1112..0e0ddf3 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1155,3 +1155,76 @@ common_errp:
>                 name, bdrv_get_device_or_node_name(bs), reason);
>      return false;
>  }
> +
> +int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> +                                  void **refcount_table,
> +                                  int64_t *refcount_table_size)
> +{

[...]

> +        for (i = 0; i < bm->table_size; ++i) {
> +            uint64_t entry = bitmap_table[i];
> +            uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
> +
> +            if (check_table_entry(entry, s->cluster_size) < 0) {
> +                res->corruptions++;
> +                ret = -EINVAL;
> +                continue;

First an optional suggestion: You could just assign 
check_table_entry()'s return value directly to ret, then you wouldn't 
have to set it manually.

Second, why are you setting ret at all? If any of the next iterations is 
successful, it will get overwritten and be set to 0 again.

Max

> +            }
> +
> +            if (offset == 0) {
> +                continue;
> +            }
> +
> +            ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                           refcount_table, refcount_table_size,
> +                                           offset, s->cluster_size);
> +            if (ret < 0) {
> +                g_free(bitmap_table);
> +                goto out;
> +            }
> +        }
> +
> +        g_free(bitmap_table);
> +    }
> +
> +out:
> +    bitmap_list_free(bm_list);
> +
> +    return ret;
> +}

Re: [Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy]

07.12.2016 21:25, Max Reitz wrote:
> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>> Add bitmap extension as specified in docs/specs/qcow2.txt.
>> For now, just mirror extension header into Qcow2 state and check
>> constraints.
>>
>> For now, disable image resize if it has bitmaps. It will be fixed later.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   block/qcow2.h |  24 +++++++++++++
>>   2 files changed, 129 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 6d5689a..7908657 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -63,6 +63,7 @@ typedef struct {
>>   #define  QCOW2_EXT_MAGIC_END 0
>>   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>> +#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>>   
>>   static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>   {
>> @@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>       QCowExtension ext;
>>       uint64_t offset;
>>       int ret;
>> +    Qcow2BitmapHeaderExt bitmaps_ext;
>>   
>>   #ifdef DEBUG_EXT
>>       printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);
>> @@ -162,6 +164,81 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>               }
>>               break;
>>   
>> +        case QCOW2_EXT_MAGIC_BITMAPS:
>> +            if (ext.len != sizeof(bitmaps_ext)) {
>> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
>> +                                 "Invalid extension length");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
>> +                fprintf(stderr,
>> +                        "WARNING: a program lacking bitmap support modified "
>> +                        "this file, so all bitmaps are now considered "
>> +                        "inconsistent");
>> +                break;
>> +            }
> I think It would be nice if qemu-img check could remove all bitmaps and
> related data if this bit is not set. Otherwise, we're practically
> leaking data.

Is it safe? Can old version somehow reuse these clusters? Actually, all 
data will be removed on check, as there will be no refcounts if this bit 
is not set.

So, IMHO, only extension header should be deleted additionally in 
qemu-img check.

>
> (But this can be added in a later patch.)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

07.12.2016 23:51, Max Reitz wrote:
> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
>> are loaded when the image is opened and become BdrvDirtyBitmaps for the
>> corresponding drive.
>>
>> Extra data in bitmaps is not supported for now.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/Makefile.objs  |   2 +-
>>   block/qcow2-bitmap.c | 663 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |   2 +
>>   block/qcow2.h        |   3 +
>>   4 files changed, 669 insertions(+), 1 deletion(-)
>>   create mode 100644 block/qcow2-bitmap.c
>>
> [...]
>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> new file mode 100644
>> index 0000000..0f797e6
>> --- /dev/null
>> +++ b/block/qcow2-bitmap.c
>> @@ -0,0 +1,663 @@
> [...]
>
>> +/* 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)
>> +{
>> +    uint64_t offset;
>> +
>> +    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
>> +    if (offset != 0) {
>> +        /* if offset specified, bit 0 must is reserved */
> -must
>
>> +        if (entry & 1) {
>> +            return -EINVAL;
>> +        }
>> +
>> +        if ((cluster_size != 0) && (entry % cluster_size != 0)) {
> Why would cluster_size be 0? Also, shouldn't it be offset instead of entry?

the comment says: "If cluster_size is 0, offset alignment is not checked"
yes, should be offset.

>
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
> [...]
>
>> +/* bitmap table entries must satisfy specification constraints */
>> +static int load_bitmap_data(BlockDriverState *bs,
>> +                            const uint64_t *bitmap_table,
>> +                            uint32_t 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);
>> +    uint8_t *buf = NULL;
>> +    uint64_t i, tab_size =
>> +            size_to_clusters(s,
>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
>> +
>> +    if (tab_size != bitmap_table_size ||
>> +            tab_size > BME_MAX_TABLE_SIZE) {
> This line should be aligned to the opening parenthesis.
>
> [...]
>
>> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    uint64_t phys_bitmap_bytes =
>> +            (uint64_t)entry->bitmap_table_size * s->cluster_size;
>> +    uint64_t max_virtual_bits =
>> +            (phys_bitmap_bytes * 8) << entry->granularity_bits;
> I think this shift can overflow, even if bitmap_table_size, cluster_size
> and granularity_bits are all within their respective limits.

Hmm. But it can't, if phys_bitmap_bytes <= MAX_PHYS_SIZE. I can just 
move this calculation down.

>
>> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
> Just using bdrv_getlength() would probably be simpler.
>
>> +
>> +    if (nb_sectors < 0) {
>> +        return nb_sectors;
>> +    }
>> +
>> +    int fail =
> I'd prefer a boolean.

and mixed definition/code)

>
>> +            (entry->bitmap_table_size == 0) ||
>> +            (entry->bitmap_table_offset == 0) ||
>> +            (entry->bitmap_table_offset % s->cluster_size) ||
>> +            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
>> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
>> +            (entry->bitmap_table_offset != 0 &&

hmm, can be dropped, as checked above

>> +                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
>> +            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
>> +            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
>> +            (entry->flags & BME_RESERVED_FLAGS) ||
>> +            (entry->name_size > BME_MAX_NAME_SIZE) ||
>> +            (entry->type != BT_DIRTY_TRACKING_BITMAP);
>> +
>> +    return fail ? -EINVAL : 0;
>> +}
> [...]
>
>> +/* bitmap_list_load
>> + * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>> + * checks it and convert to bitmap list.
>> + */
>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>> +                                         uint64_t size, Error **errp)
>> +{
> [...]
>
>> +    bm_list = bitmap_list_new();
>> +    for (e = (Qcow2BitmapDirEntry *)dir;
>> +             e < (Qcow2BitmapDirEntry *)dir_end; e = next_dir_entry(e)) {
> This line should be aligned to the opening parenthesis.
>
> [...]
>
>> +
>> +/* bitmap_list_store
>> + * Store bitmap list to qcow2 image as a bitmap directory.
>> + * Everything is checked.
>> + */
>> +static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>> +                             uint64_t *offset, uint64_t *size, bool in_place)
>> +{
> [...]
>
>> +fail:
>> +    g_free(dir);
>> +
>> +    if (dir_offset > 0) {
>> +        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);
> I think this should only be freed if in_place is false.

Reasonable.. And this also leads me to the think that we should clear 
autoclear bit in the header before inplace updating of bitmap directory 
and set it after successful update.

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


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

On 14.12.2016 13:23, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2016 21:25, Max Reitz wrote:
>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>> Add bitmap extension as specified in docs/specs/qcow2.txt.
>>> For now, just mirror extension header into Qcow2 state and check
>>> constraints.
>>>
>>> For now, disable image resize if it has bitmaps. It will be fixed later.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   block/qcow2.c | 107
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>   block/qcow2.h |  24 +++++++++++++
>>>   2 files changed, 129 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 6d5689a..7908657 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -63,6 +63,7 @@ typedef struct {
>>>   #define  QCOW2_EXT_MAGIC_END 0
>>>   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>> +#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>>>     static int qcow2_probe(const uint8_t *buf, int buf_size, const
>>> char *filename)
>>>   {
>>> @@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState
>>> *bs, uint64_t start_offset,
>>>       QCowExtension ext;
>>>       uint64_t offset;
>>>       int ret;
>>> +    Qcow2BitmapHeaderExt bitmaps_ext;
>>>     #ifdef DEBUG_EXT
>>>       printf("qcow2_read_extensions: start=%ld end=%ld\n",
>>> start_offset, end_offset);
>>> @@ -162,6 +164,81 @@ static int
>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>               }
>>>               break;
>>>   +        case QCOW2_EXT_MAGIC_BITMAPS:
>>> +            if (ext.len != sizeof(bitmaps_ext)) {
>>> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
>>> +                                 "Invalid extension length");
>>> +                return -EINVAL;
>>> +            }
>>> +
>>> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
>>> +                fprintf(stderr,
>>> +                        "WARNING: a program lacking bitmap support
>>> modified "
>>> +                        "this file, so all bitmaps are now considered "
>>> +                        "inconsistent");
>>> +                break;
>>> +            }
>> I think It would be nice if qemu-img check could remove all bitmaps and
>> related data if this bit is not set. Otherwise, we're practically
>> leaking data.
> 
> Is it safe? Can old version somehow reuse these clusters?

You're right, it can. It's not so easy then.

>                                                           Actually, all
> data will be removed on check, as there will be no refcounts if this bit
> is not set.

The refcounts will be there, but no references, right. So an old
qemu-img check -r leaks will free the clusters.

We could still remove this extension field, right? Then the clusters are
at least really leaked so that even a new qemu-img check -r leaks will
free them.

> So, IMHO, only extension header should be deleted additionally in
> qemu-img check.

I think we could remove it here instead of only in qemu-img check, but
either way should be fine. Doing it in qemu-img check is probably
cleaner, actually.

If you do it there, however, the warning should probably include a hint
like "(Use 'qemu-img check -r leaks' to reclaim the space used for the
bitmaps)".

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6293 bytes --]

On 14.12.2016 16:54, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2016 23:51, Max Reitz wrote:
>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
>>> are loaded when the image is opened and become BdrvDirtyBitmaps for the
>>> corresponding drive.
>>>
>>> Extra data in bitmaps is not supported for now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/Makefile.objs  |   2 +-
>>>   block/qcow2-bitmap.c | 663
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.c        |   2 +
>>>   block/qcow2.h        |   3 +
>>>   4 files changed, 669 insertions(+), 1 deletion(-)
>>>   create mode 100644 block/qcow2-bitmap.c
>>>
>> [...]
>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> new file mode 100644
>>> index 0000000..0f797e6
>>> --- /dev/null
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -0,0 +1,663 @@
>> [...]
>>
>>> +/* 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)
>>> +{
>>> +    uint64_t offset;
>>> +
>>> +    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
>>> +    if (offset != 0) {
>>> +        /* if offset specified, bit 0 must is reserved */
>> -must
>>
>>> +        if (entry & 1) {
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +        if ((cluster_size != 0) && (entry % cluster_size != 0)) {
>> Why would cluster_size be 0? Also, shouldn't it be offset instead of
>> entry?
> 
> the comment says: "If cluster_size is 0, offset alignment is not checked"

Oops, right. Is there any place where this function is called with
cluster_size being 0, though?

> yes, should be offset.
> 
>>
>>> +            return -EINVAL;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>> [...]
>>
>>> +/* bitmap table entries must satisfy specification constraints */
>>> +static int load_bitmap_data(BlockDriverState *bs,
>>> +                            const uint64_t *bitmap_table,
>>> +                            uint32_t 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);
>>> +    uint8_t *buf = NULL;
>>> +    uint64_t i, tab_size =
>>> +            size_to_clusters(s,
>>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0,
>>> bm_size));
>>> +
>>> +    if (tab_size != bitmap_table_size ||
>>> +            tab_size > BME_MAX_TABLE_SIZE) {
>> This line should be aligned to the opening parenthesis.
>>
>> [...]
>>
>>> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry
>>> *entry)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    uint64_t phys_bitmap_bytes =
>>> +            (uint64_t)entry->bitmap_table_size * s->cluster_size;
>>> +    uint64_t max_virtual_bits =
>>> +            (phys_bitmap_bytes * 8) << entry->granularity_bits;
>> I think this shift can overflow, even if bitmap_table_size, cluster_size
>> and granularity_bits are all within their respective limits.
> 
> Hmm. But it can't, if phys_bitmap_bytes <= MAX_PHYS_SIZE. I can just
> move this calculation down.

Yes, you're right, I missed the check on phys_bitmap_bytes. It's all
good then.

>>> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
>> Just using bdrv_getlength() would probably be simpler.
>>
>>> +
>>> +    if (nb_sectors < 0) {
>>> +        return nb_sectors;
>>> +    }
>>> +
>>> +    int fail =
>> I'd prefer a boolean.
> 
> and mixed definition/code)

...now that you mention it... :-)

>>> +            (entry->bitmap_table_size == 0) ||
>>> +            (entry->bitmap_table_offset == 0) ||
>>> +            (entry->bitmap_table_offset % s->cluster_size) ||
>>> +            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
>>> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
>>> +            (entry->bitmap_table_offset != 0 &&
> 
> hmm, can be dropped, as checked above
> 
>>> +                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
>>> +            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
>>> +            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
>>> +            (entry->flags & BME_RESERVED_FLAGS) ||
>>> +            (entry->name_size > BME_MAX_NAME_SIZE) ||
>>> +            (entry->type != BT_DIRTY_TRACKING_BITMAP);
>>> +
>>> +    return fail ? -EINVAL : 0;
>>> +}
>> [...]
>>
>>> +/* bitmap_list_load
>>> + * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>> + * checks it and convert to bitmap list.
>>> + */
>>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>>> uint64_t offset,
>>> +                                         uint64_t size, Error **errp)
>>> +{
>> [...]
>>
>>> +    bm_list = bitmap_list_new();
>>> +    for (e = (Qcow2BitmapDirEntry *)dir;
>>> +             e < (Qcow2BitmapDirEntry *)dir_end; e =
>>> next_dir_entry(e)) {
>> This line should be aligned to the opening parenthesis.
>>
>> [...]
>>
>>> +
>>> +/* bitmap_list_store
>>> + * Store bitmap list to qcow2 image as a bitmap directory.
>>> + * Everything is checked.
>>> + */
>>> +static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList
>>> *bm_list,
>>> +                             uint64_t *offset, uint64_t *size, bool
>>> in_place)
>>> +{
>> [...]
>>
>>> +fail:
>>> +    g_free(dir);
>>> +
>>> +    if (dir_offset > 0) {
>>> +        qcow2_free_clusters(bs, dir_offset, dir_size,
>>> QCOW2_DISCARD_OTHER);
>> I think this should only be freed if in_place is false.
> 
> Reasonable.. And this also leads me to the think that we should clear
> autoclear bit in the header before inplace updating of bitmap directory
> and set it after successful update.

Not a bad idea, actually. I don't think it's absolutely necessary but it
would indeed be nice.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

09.12.2016 20:05, Max Reitz wrote:
> 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<vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |   1 +

[...]

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

assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional 
error handling. Right?

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

Ok

[...].

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

Why? No, I need to check the whole sum and the whole size.

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

Hmm, right.

[...]

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

16.12.2016 17:37, Max Reitz wrote:
> On 14.12.2016 16:54, Vladimir Sementsov-Ogievskiy wrote:
>> 07.12.2016 23:51, Max Reitz wrote:
>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
>>>> are loaded when the image is opened and become BdrvDirtyBitmaps for the
>>>> corresponding drive.
>>>>
>>>> Extra data in bitmaps is not supported for now.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/Makefile.objs  |   2 +-
>>>>    block/qcow2-bitmap.c | 663
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    block/qcow2.c        |   2 +
>>>>    block/qcow2.h        |   3 +
>>>>    4 files changed, 669 insertions(+), 1 deletion(-)
>>>>    create mode 100644 block/qcow2-bitmap.c
>>>>
>>> [...]
>>>
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> new file mode 100644
>>>> index 0000000..0f797e6
>>>> --- /dev/null
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -0,0 +1,663 @@
>>> [...]
>>>
>>>> +/* 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)
>>>> +{
>>>> +    uint64_t offset;
>>>> +
>>>> +    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
>>>> +    if (offset != 0) {
>>>> +        /* if offset specified, bit 0 must is reserved */
>>> -must
>>>
>>>> +        if (entry & 1) {
>>>> +            return -EINVAL;
>>>> +        }
>>>> +
>>>> +        if ((cluster_size != 0) && (entry % cluster_size != 0)) {
>>> Why would cluster_size be 0? Also, shouldn't it be offset instead of
>>> entry?
>> the comment says: "If cluster_size is 0, offset alignment is not checked"
> Oops, right. Is there any place where this function is called with
> cluster_size being 0, though?

Hmm, I can't find it. Will remove this extra feature.

[...]


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2016 20:05, Max Reitz wrote:
>> 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 <vsementsov@virtuozzo.com>
>>> ---
>>>  block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block/qcow2.c        |   1 +
> 
> [...]
> 
>>> +
>>> +/* 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.
> 
> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
> error handling. Right?

Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
necessarily a multiple of dsc. So that should be good. Alternatively, I
think the following would be slightly easier to read:

assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);

> 
>>> +
>>> +        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?
> 
> Ok
> 
> [...].
> 
>>
>>> +        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).
> 
> Why? No, I need to check the whole sum and the whole size.

If the bitmap already exists, you don't create a new directory entry but
reuse the existing one. Therefore, the number of bitmaps in the image
and the directory size will not grow then.

Max

>>> +
>>> +        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?
> 
> Hmm, right.
> 
> [...]
> 
> -- 
> Best regards,
> Vladimir
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

19.12.2016 18:14, Max Reitz wrote:
> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
>> 09.12.2016 20:05, Max Reitz wrote:
>>> 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 <vsementsov@virtuozzo.com>
>>>> ---
>>>>   block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   block/qcow2.c        |   1 +
>> [...]
>>
>>>> +
>>>> +/* 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.
>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
>> error handling. Right?
> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
> necessarily a multiple of dsc. So that should be good. Alternatively, I
> think the following would be slightly easier to read:
>
> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
>
>>>> +
>>>> +        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?
>> Ok
>>
>> [...].
>>
>>>> +        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).
>> Why? No, I need to check the whole sum and the whole size.
> If the bitmap already exists, you don't create a new directory entry but
> reuse the existing one. Therefore, the number of bitmaps in the image
> and the directory size will not grow then.

new_nb_bitmaps is not number of "newly created bitmaps", but just new 
value of field nb_bitmaps, so, all bitmaps - old and new are calculated 
into new_nb_bitmaps. Anyway, this misunderstanding shows that variable 
name is bad..

>
> Max
>
>>>> +
>>>> +        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?
>> Hmm, right.
>>
>> [...]
>>
>> -- 
>> Best regards,
>> Vladimir
>>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5433 bytes --]

On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2016 18:14, Max Reitz wrote:
>> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
>>> 09.12.2016 20:05, Max Reitz wrote:
>>>> 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 <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>   block/qcow2-bitmap.c | 451
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   block/qcow2.c        |   1 +
>>> [...]
>>>
>>>>> +
>>>>> +/* 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.
>>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
>>> error handling. Right?
>> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
>> necessarily a multiple of dsc. So that should be good. Alternatively, I
>> think the following would be slightly easier to read:
>>
>> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
>>
>>>>> +
>>>>> +        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?
>>> Ok
>>>
>>> [...].
>>>
>>>>> +        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).
>>> Why? No, I need to check the whole sum and the whole size.
>> If the bitmap already exists, you don't create a new directory entry but
>> reuse the existing one. Therefore, the number of bitmaps in the image
>> and the directory size will not grow then.
> 
> new_nb_bitmaps is not number of "newly created bitmaps", but just new
> value of field nb_bitmaps, so, all bitmaps - old and new are calculated
> into new_nb_bitmaps. Anyway, this misunderstanding shows that variable
> name is bad..

Yes. But when you store a bitmap of the same name as an existing one,
you are replacing it. The number of bitmaps does not grow in that case.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

19.12.2016 18:34, Max Reitz wrote:
> On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2016 18:14, Max Reitz wrote:
>>> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
>>>> 09.12.2016 20:05, Max Reitz wrote:
>>>>> 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 <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>    block/qcow2-bitmap.c | 451
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>    block/qcow2.c        |   1 +
>>>> [...]
>>>>
>>>>>> +
>>>>>> +/* 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.
>>>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
>>>> error handling. Right?
>>> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
>>> necessarily a multiple of dsc. So that should be good. Alternatively, I
>>> think the following would be slightly easier to read:
>>>
>>> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
>>>
>>>>>> +
>>>>>> +        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?
>>>> Ok
>>>>
>>>> [...].
>>>>
>>>>>> +        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).
>>>> Why? No, I need to check the whole sum and the whole size.
>>> If the bitmap already exists, you don't create a new directory entry but
>>> reuse the existing one. Therefore, the number of bitmaps in the image
>>> and the directory size will not grow then.
>> new_nb_bitmaps is not number of "newly created bitmaps", but just new
>> value of field nb_bitmaps, so, all bitmaps - old and new are calculated
>> into new_nb_bitmaps. Anyway, this misunderstanding shows that variable
>> name is bad..
> Yes. But when you store a bitmap of the same name as an existing one,
> you are replacing it. The number of bitmaps does not grow in that case.

Oh, I'm stupid)) I see now, you are right.

>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 07/21] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy]

16.12.2016 17:25, Max Reitz wrote:
> On 14.12.2016 13:23, Vladimir Sementsov-Ogievskiy wrote:
>> 07.12.2016 21:25, Max Reitz wrote:
>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add bitmap extension as specified in docs/specs/qcow2.txt.
>>>> For now, just mirror extension header into Qcow2 state and check
>>>> constraints.
>>>>
>>>> For now, disable image resize if it has bitmaps. It will be fixed later.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    block/qcow2.c | 107
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>    block/qcow2.h |  24 +++++++++++++
>>>>    2 files changed, 129 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 6d5689a..7908657 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>    #define  QCOW2_EXT_MAGIC_END 0
>>>>    #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>    #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>> +#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>>>>      static int qcow2_probe(const uint8_t *buf, int buf_size, const
>>>> char *filename)
>>>>    {
>>>> @@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState
>>>> *bs, uint64_t start_offset,
>>>>        QCowExtension ext;
>>>>        uint64_t offset;
>>>>        int ret;
>>>> +    Qcow2BitmapHeaderExt bitmaps_ext;
>>>>      #ifdef DEBUG_EXT
>>>>        printf("qcow2_read_extensions: start=%ld end=%ld\n",
>>>> start_offset, end_offset);
>>>> @@ -162,6 +164,81 @@ static int
>>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>                }
>>>>                break;
>>>>    +        case QCOW2_EXT_MAGIC_BITMAPS:
>>>> +            if (ext.len != sizeof(bitmaps_ext)) {
>>>> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
>>>> +                                 "Invalid extension length");
>>>> +                return -EINVAL;
>>>> +            }
>>>> +
>>>> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
>>>> +                fprintf(stderr,
>>>> +                        "WARNING: a program lacking bitmap support
>>>> modified "
>>>> +                        "this file, so all bitmaps are now considered "
>>>> +                        "inconsistent");
>>>> +                break;
>>>> +            }
>>> I think It would be nice if qemu-img check could remove all bitmaps and
>>> related data if this bit is not set. Otherwise, we're practically
>>> leaking data.
>> Is it safe? Can old version somehow reuse these clusters?
> You're right, it can. It's not so easy then.
>
>>                                                            Actually, all
>> data will be removed on check, as there will be no refcounts if this bit
>> is not set.
> The refcounts will be there, but no references, right. So an old
> qemu-img check -r leaks will free the clusters.
>
> We could still remove this extension field, right? Then the clusters are
> at least really leaked so that even a new qemu-img check -r leaks will
> free them.
>
>> So, IMHO, only extension header should be deleted additionally in
>> qemu-img check.
> I think we could remove it here instead of only in qemu-img check, but
> either way should be fine. Doing it in qemu-img check is probably
> cleaner, actually.

Now I think it would be more deterministic to clear extension header 
here, as, actually, it will be cleared on any future qcow2_update_header().

>
> If you do it there, however, the warning should probably include a hint
> like "(Use 'qemu-img check -r leaks' to reclaim the space used for the
> bitmaps)".
>
> Max
>


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps()

[Vladimir Sementsov-Ogievskiy]

Realize block bitmap storing interface, to allow qcow2 images store
persistent bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 f06de8e..4230af0 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;
+
     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
+     * 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) >>
+                                 granularity_bits;
+    uint64_t bitmap_table_size = phys_bitmap_bytes / s->cluster_size;
+    size_t name_size = strlen(name);
+
+    int fail =
+            (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,
@@ -602,6 +706,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 > QCOW_MAX_DIRTY_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;
+        }
+
+        s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
+    } else {
+        s->autoclear_features &= ~(uint64_t)QCOW2_AUTOCLEAR_DIRTY_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)
@@ -663,3 +833,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) {
+        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;
+
+        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);
+        }
+
+        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);
+    }
+
+    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;
+    }
+
+    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)) {
+        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 > QCOW_MAX_DIRTY_BITMAPS) {
+            error_setg(errp, "Too many persistent bitmaps");
+            goto fail;
+        }
+
+        new_dir_size += calc_dir_entry_size(strlen(name), 0);
+        if (new_dir_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+            error_setg(errp, "Too large bitmap directory");
+            goto fail;
+        }
+
+        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)) {
+                error_setg(errp, "Bitmap '%s' is already exists in the image",
+                           name);
+                goto fail;
+            }
+            free_bitmap_clusters(bs, bm);
+            /* 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;
+    }
+
+    /* 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 cf1f201..8c28bb7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3509,6 +3509,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 601c6ea..6dfa0c4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -617,5 +617,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
-- 
1.8.3.1