[Qemu-devel] [PATCH 0/6] Configure metadata overlap checks at runtime

[Qemu-devel] [PATCH 0/6] Configure metadata overlap checks at runtime

[Max Reitz]

This series changes the way of selecting what metadata overlap checks to
perform from (currently) using a macro to using a variable contained in
BDRVQcowState which can be configured at runtime through several command
line options.

Although this does not seem necessary anymore regarding performance
(because I could not find any performance hits using cached overlap
checks), it seems cleaner to me this way.

This series depends on:
 - qcow2: Correct snapshots size for overlap check

Max Reitz (6):
  qcow2: Use negated overflow check mask
  qcow2: Make overlap check mask variable
  qcow2: Add overlap-check options
  qcow2: Array assigning options to OL check bits
  qcow2: Add more overlap check bitmask macros
  qcow2: Evaluate overlap check options

 block/qcow2-cache.c    |  8 ++---
 block/qcow2-cluster.c  | 16 ++++-----
 block/qcow2-refcount.c | 22 ++++++------
 block/qcow2-snapshot.c | 12 +++----
 block/qcow2.c          | 93 +++++++++++++++++++++++++++++++++++++++++++++++---
 block/qcow2.h          | 30 ++++++++++++----
 6 files changed, 137 insertions(+), 44 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] qcow2: Use negated overflow check mask

[Max Reitz]

In qcow2_check_metadata_overlap and qcow2_pre_write_overlap_check,
change the parameter signifying the checks to perform from its current
positive form to a negative one, i.e., it will no longer explicitly
specify every check to perform but rather a mask of checks not to
perform.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cache.c    |  8 +++-----
 block/qcow2-cluster.c  | 16 +++++++---------
 block/qcow2-refcount.c | 22 ++++++++++------------
 block/qcow2-snapshot.c | 12 +++++-------
 block/qcow2.c          |  7 +++----
 block/qcow2.h          |  4 ++--
 6 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 40a5a3f..8ecbb5b 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -115,15 +115,13 @@ static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i)
     }
 
     if (c == s->refcount_block_cache) {
-        ret = qcow2_pre_write_overlap_check(bs,
-                QCOW2_OL_DEFAULT & ~QCOW2_OL_REFCOUNT_BLOCK,
+        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_REFCOUNT_BLOCK,
                 c->entries[i].offset, s->cluster_size);
     } else if (c == s->l2_table_cache) {
-        ret = qcow2_pre_write_overlap_check(bs,
-                QCOW2_OL_DEFAULT & ~QCOW2_OL_ACTIVE_L2,
+        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
                 c->entries[i].offset, s->cluster_size);
     } else {
-        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
+        ret = qcow2_pre_write_overlap_check(bs, 0,
                 c->entries[i].offset, s->cluster_size);
     }
 
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 738ff73..6577de1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -82,8 +82,8 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
 
     /* the L1 position has not yet been updated, so these clusters must
      * indeed be completely free */
-    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
-                                        new_l1_table_offset, new_l1_size2);
+    ret = qcow2_pre_write_overlap_check(bs, 0, new_l1_table_offset,
+                                        new_l1_size2);
     if (ret < 0) {
         goto fail;
     }
@@ -157,8 +157,7 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
         buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);
     }
 
-    ret = qcow2_pre_write_overlap_check(bs,
-            QCOW2_OL_DEFAULT & ~QCOW2_OL_ACTIVE_L1,
+    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,
             s->l1_table_offset + 8 * l1_start_index, sizeof(buf));
     if (ret < 0) {
         return ret;
@@ -383,7 +382,7 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
                         &s->aes_encrypt_key);
     }
 
-    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
+    ret = qcow2_pre_write_overlap_check(bs, 0,
             cluster_offset + n_start * BDRV_SECTOR_SIZE, n * BDRV_SECTOR_SIZE);
     if (ret < 0) {
         goto out;
@@ -1592,8 +1591,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 }
             }
 
-            ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
-                                                offset, s->cluster_size);
+            ret = qcow2_pre_write_overlap_check(bs, 0, offset, s->cluster_size);
             if (ret < 0) {
                 qcow2_free_clusters(bs, offset, s->cluster_size,
                         QCOW2_DISCARD_ALWAYS);
@@ -1628,8 +1626,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
             }
         } else {
             if (l2_dirty) {
-                ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT &
-                        ~(QCOW2_OL_INACTIVE_L2 | QCOW2_OL_ACTIVE_L2), l2_offset,
+                ret = qcow2_pre_write_overlap_check(bs,
+                        QCOW2_OL_INACTIVE_L2 | QCOW2_OL_ACTIVE_L2, l2_offset,
                         s->cluster_size);
                 if (ret < 0) {
                     goto fail;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4264148..2fa6acb 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1311,9 +1311,8 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
         }
 
         if (l2_dirty) {
-            ret = qcow2_pre_write_overlap_check(bs,
-                    QCOW2_OL_DEFAULT & ~QCOW2_OL_ACTIVE_L2, l2_offset,
-                    s->cluster_size);
+            ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
+                                                l2_offset, s->cluster_size);
             if (ret < 0) {
                 fprintf(stderr, "ERROR: Could not write L2 table; metadata "
                         "overlap check failed: %s\n", strerror(-ret));
@@ -1354,8 +1353,7 @@ static int write_reftable_entry(BlockDriverState *bs, int rt_index)
         buf[i] = cpu_to_be64(s->refcount_table[rt_start_index + i]);
     }
 
-    ret = qcow2_pre_write_overlap_check(bs,
-            QCOW2_OL_DEFAULT & ~QCOW2_OL_REFCOUNT_TABLE,
+    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_REFCOUNT_TABLE,
             s->refcount_table_offset + rt_start_index * sizeof(uint64_t),
             sizeof(buf));
     if (ret < 0) {
@@ -1406,8 +1404,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
 
     /* new block has not yet been entered into refcount table, therefore it is
      * no refcount block yet (regarding this check) */
-    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, new_offset,
-            s->cluster_size);
+    ret = qcow2_pre_write_overlap_check(bs, 0, new_offset, s->cluster_size);
     if (ret < 0) {
         fprintf(stderr, "Could not write refcount block; metadata overlap "
                 "check failed: %s\n", strerror(-ret));
@@ -1640,8 +1637,8 @@ fail:
  * looking for overlaps with important metadata sections (L1/L2 tables etc.),
  * i.e. a sanity check without relying on the refcount tables.
  *
- * The chk parameter specifies exactly what checks to perform (being a bitmask
- * of QCow2MetadataOverlap values).
+ * The ign parameter specifies what checks not to perform (being a bitmask of
+ * QCow2MetadataOverlap values), i.e., what sections to ignore.
  *
  * Returns:
  * - 0 if writing to this offset will not affect the mentioned metadata
@@ -1649,10 +1646,11 @@ fail:
  * - a negative value (-errno) indicating an error while performing a check,
  *   e.g. when bdrv_read failed on QCOW2_OL_INACTIVE_L2
  */
-int qcow2_check_metadata_overlap(BlockDriverState *bs, int chk, int64_t offset,
+int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                                  int64_t size)
 {
     BDRVQcowState *s = bs->opaque;
+    int chk = QCOW2_OL_DEFAULT & ~ign;
     int i, j;
 
     if (!size) {
@@ -1769,10 +1767,10 @@ static const char *metadata_ol_names[] = {
  * Returns 0 if there were neither overlaps nor errors while checking for
  * overlaps; or a negative value (-errno) on error.
  */
-int qcow2_pre_write_overlap_check(BlockDriverState *bs, int chk, int64_t offset,
+int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
                                   int64_t size)
 {
-    int ret = qcow2_check_metadata_overlap(bs, chk, offset, size);
+    int ret = qcow2_check_metadata_overlap(bs, ign, offset, size);
 
     if (ret < 0) {
         return ret;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5e8a779..f4b3eac 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -191,8 +191,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
     /* The snapshot list position has not yet been updated, so these clusters
      * must indeed be completely free */
-    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, offset,
-                                        snapshots_size);
+    ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size);
     if (ret < 0) {
         return ret;
     }
@@ -388,8 +387,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         l1_table[i] = cpu_to_be64(s->l1_table[i]);
     }
 
-    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
-            sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
+    ret = qcow2_pre_write_overlap_check(bs, 0, sn->l1_table_offset,
+                                        s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
         goto fail;
     }
@@ -513,9 +512,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         goto fail;
     }
 
-    ret = qcow2_pre_write_overlap_check(bs,
-            QCOW2_OL_DEFAULT & ~QCOW2_OL_ACTIVE_L1,
-            s->l1_table_offset, cur_l1_bytes);
+    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,
+                                        s->l1_table_offset, cur_l1_bytes);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 318d95d..13c9d5e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -965,7 +965,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
                 cur_nr_sectors * 512);
         }
 
-        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
+        ret = qcow2_pre_write_overlap_check(bs, 0,
                 cluster_offset + index_in_cluster * BDRV_SECTOR_SIZE,
                 cur_nr_sectors * BDRV_SECTOR_SIZE);
         if (ret < 0) {
@@ -1739,7 +1739,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
 
-        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
+        ret = qcow2_pre_write_overlap_check(bs, 0,
                 sector_num * BDRV_SECTOR_SIZE,
                 s->cluster_sectors * BDRV_SECTOR_SIZE);
         if (ret < 0) {
@@ -1759,8 +1759,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
         }
         cluster_offset &= s->cluster_offset_mask;
 
-        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
-                cluster_offset, out_len);
+        ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/qcow2.h b/block/qcow2.h
index c90e5d6..b5a158f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -433,9 +433,9 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
 
 void qcow2_process_discards(BlockDriverState *bs, int ret);
 
-int qcow2_check_metadata_overlap(BlockDriverState *bs, int chk, int64_t offset,
+int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                                  int64_t size);
-int qcow2_pre_write_overlap_check(BlockDriverState *bs, int chk, int64_t offset,
+int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
                                   int64_t size);
 
 /* qcow2-cluster.c functions */
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/6] qcow2: Make overlap check mask variable

[Max Reitz]

Replace the QCOW2_OL_DEFAULT macro by a variable overlap_check in
BDRVQcowState.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 2 +-
 block/qcow2.c          | 2 ++
 block/qcow2.h          | 5 ++---
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2fa6acb..3edd558 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1650,7 +1650,7 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                                  int64_t size)
 {
     BDRVQcowState *s = bs->opaque;
-    int chk = QCOW2_OL_DEFAULT & ~ign;
+    int chk = s->overlap_check & ~ign;
     int i, j;
 
     if (!size) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 13c9d5e..a1b854c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -631,6 +631,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     s->discard_passthrough[QCOW2_DISCARD_OTHER] =
         qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
 
+    s->overlap_check = QCOW2_OL_CACHED;
+
     qemu_opts_del(opts);
 
     if (s->use_lazy_refcounts && s->qcow_version < 3) {
diff --git a/block/qcow2.h b/block/qcow2.h
index b5a158f..7dd787d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -203,6 +203,8 @@ typedef struct BDRVQcowState {
 
     bool discard_passthrough[QCOW2_DISCARD_MAX];
 
+    int overlap_check; /* bitmask of Qcow2MetadataOverlap values */
+
     uint64_t incompatible_features;
     uint64_t compatible_features;
     uint64_t autoclear_features;
@@ -321,9 +323,6 @@ typedef enum QCow2MetadataOverlap {
      QCOW2_OL_REFCOUNT_TABLE | QCOW2_OL_REFCOUNT_BLOCK | \
      QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_INACTIVE_L1)
 
-/* The default checks to perform */
-#define QCOW2_OL_DEFAULT QCOW2_OL_CACHED
-
 #define L1E_OFFSET_MASK 0x00ffffffffffff00ULL
 #define L2E_OFFSET_MASK 0x00ffffffffffff00ULL
 #define L2E_COMPRESSED_OFFSET_SIZE_MASK 0x3fffffffffffffffULL
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/6] qcow2: Add overlap-check options

[Max Reitz]

Add runtime options to tune the overlap checks to be performed before
write accesses.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h |  9 +++++++++
 2 files changed, 55 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index a1b854c..e74f269 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -354,6 +354,52 @@ static QemuOptsList qcow2_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Generate discard requests when other clusters are freed",
         },
+        {
+            .name = QCOW2_OPT_OVERLAP,
+            .type = QEMU_OPT_STRING,
+            .help = "Selects which overlap checks to perform from a range of "
+                    "templates (none, constant, cached, all)",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_MAIN_HEADER,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into the main qcow2 header",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_ACTIVE_L1,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into the active L1 table",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_ACTIVE_L2,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into an active L2 table",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into the refcount table",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into a refcount block",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into the snapshot table",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_INACTIVE_L1,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into an inactive L1 table",
+        },
+        {
+            .name = QCOW2_OPT_OVERLAP_INACTIVE_L2,
+            .type = QEMU_OPT_BOOL,
+            .help = "Check for unintended writes into an inactive L2 table",
+        },
         { /* end of list */ }
     },
 };
diff --git a/block/qcow2.h b/block/qcow2.h
index 7dd787d..067ed2f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -63,6 +63,15 @@
 #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
 #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
 #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other"
+#define QCOW2_OPT_OVERLAP "overlap-check"
+#define QCOW2_OPT_OVERLAP_MAIN_HEADER "overlap-check.main-header"
+#define QCOW2_OPT_OVERLAP_ACTIVE_L1 "overlap-check.active-l1"
+#define QCOW2_OPT_OVERLAP_ACTIVE_L2 "overlap-check.active-l2"
+#define QCOW2_OPT_OVERLAP_REFCOUNT_TABLE "overlap-check.refcount-table"
+#define QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK "overlap-check.refcount-block"
+#define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
+#define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
+#define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
 
 typedef struct QCowHeader {
     uint32_t magic;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/6] qcow2: Array assigning options to OL check bits

[Max Reitz]

Add an array which assigns the option string to its corresponding
overlap check bit.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index e74f269..66afeca 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -404,6 +404,17 @@ static QemuOptsList qcow2_runtime_opts = {
     },
 };
 
+static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
+    [QCOW2_OL_MAIN_HEADER_BITNR]    = QCOW2_OPT_OVERLAP_MAIN_HEADER,
+    [QCOW2_OL_ACTIVE_L1_BITNR]      = QCOW2_OPT_OVERLAP_ACTIVE_L1,
+    [QCOW2_OL_ACTIVE_L2_BITNR]      = QCOW2_OPT_OVERLAP_ACTIVE_L2,
+    [QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
+    [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
+    [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
+    [QCOW2_OL_INACTIVE_L1_BITNR]    = QCOW2_OPT_OVERLAP_INACTIVE_L1,
+    [QCOW2_OL_INACTIVE_L2_BITNR]    = QCOW2_OPT_OVERLAP_INACTIVE_L2,
+};
+
 static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/6] qcow2: Add more overlap check bitmask macros

[Max Reitz]

Introduces the macros QCOW2_OL_CONSTANT and QCOW2_OL_ALL in addition to
the already existing QCOW2_OL_CACHED, signifying all metadata overlap
checks that can be performed in constant time (regardless of image size
etc.) and truly all available overlap checks, respectively.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 067ed2f..098c14f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -326,11 +326,19 @@ typedef enum QCow2MetadataOverlap {
     QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
 } QCow2MetadataOverlap;
 
+/* Perform all overlap checks which can be done in constant time */
+#define QCOW2_OL_CONSTANT \
+    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
+     QCOW2_OL_SNAPSHOT_TABLE)
+
 /* Perform all overlap checks which don't require disk access */
 #define QCOW2_OL_CACHED \
-    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_ACTIVE_L2 | \
-     QCOW2_OL_REFCOUNT_TABLE | QCOW2_OL_REFCOUNT_BLOCK | \
-     QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_INACTIVE_L1)
+    (QCOW2_OL_CONSTANT | QCOW2_OL_ACTIVE_L2 | QCOW2_OL_REFCOUNT_BLOCK | \
+     QCOW2_OL_SNAPSHOT_TABLE)
+
+/* Perform all overlap checks */
+#define QCOW2_OL_ALL \
+    (QCOW2_OL_CACHED | QCOW2_OL_INACTIVE_L2)
 
 #define L1E_OFFSET_MASK 0x00ffffffffffff00ULL
 #define L2E_OFFSET_MASK 0x00ffffffffffff00ULL
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/6] qcow2: Evaluate overlap check options

[Max Reitz]

Evaluate the runtime overlap check options and set
BDRVQcowState.overlap_check appropriately.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 66afeca..df3dc9d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -425,6 +425,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
+    const char *opt_overlap_check;
+    int overlap_check_template = 0;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
@@ -688,7 +690,32 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     s->discard_passthrough[QCOW2_DISCARD_OTHER] =
         qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
 
-    s->overlap_check = QCOW2_OL_CACHED;
+    opt_overlap_check = qemu_opt_get(opts, "overlap-check") ?: "cached";
+    if (!strcmp(opt_overlap_check, "none")) {
+        overlap_check_template = 0;
+    } else if (!strcmp(opt_overlap_check, "constant")) {
+        overlap_check_template = QCOW2_OL_CONSTANT;
+    } else if (!strcmp(opt_overlap_check, "cached")) {
+        overlap_check_template = QCOW2_OL_CACHED;
+    } else if (!strcmp(opt_overlap_check, "all")) {
+        overlap_check_template = QCOW2_OL_ALL;
+    } else {
+        error_setg(errp, "Unsupported value '%s' for qcow2 option "
+                   "'overlap-check'. Allowed are either of the following: "
+                   "none, constant, cached, all", opt_overlap_check);
+        qemu_opts_del(opts);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    s->overlap_check = 0;
+    for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
+        /* overlap-check defines a template bitmask, but every flag may be
+         * overwritten through the associated boolean option */
+        s->overlap_check |=
+            qemu_opt_get_bool(opts, overlap_bool_option_names[i],
+                              overlap_check_template & (1 << i)) << i;
+    }
 
     qemu_opts_del(opts);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/6] Configure metadata overlap checks at runtime

[Max Reitz]

On 2013-09-20 10:37, Max Reitz wrote:
> This series changes the way of selecting what metadata overlap checks to
> perform from (currently) using a macro to using a variable contained in
> BDRVQcowState which can be configured at runtime through several command
> line options.
>
> Although this does not seem necessary anymore regarding performance
> (because I could not find any performance hits using cached overlap
> checks), it seems cleaner to me this way.
>
> This series depends on:
>   - qcow2: Correct snapshots size for overlap check
>
> Max Reitz (6):
>    qcow2: Use negated overflow check mask
>    qcow2: Make overlap check mask variable
>    qcow2: Add overlap-check options
>    qcow2: Array assigning options to OL check bits
>    qcow2: Add more overlap check bitmask macros
>    qcow2: Evaluate overlap check options
>
>   block/qcow2-cache.c    |  8 ++---
>   block/qcow2-cluster.c  | 16 ++++-----
>   block/qcow2-refcount.c | 22 ++++++------
>   block/qcow2-snapshot.c | 12 +++----
>   block/qcow2.c          | 93 +++++++++++++++++++++++++++++++++++++++++++++++---
>   block/qcow2.h          | 30 ++++++++++++----
>   6 files changed, 137 insertions(+), 44 deletions(-)

Ping – does anyone have comments on this series?

It doesn't really seem to be required, since the current default setting 
is fast enough (although I still have to do a performance test regarding 
the host CPU usage). However, this series introduces a cleaner interface 
to the overlap checks besides the option to change the overlap checks at 
startup (which isn't just useful to users, but also a requirement for 
I/O tests on overlap checks on inactive L1 tables).

Max

Re: [Qemu-devel] [PATCH 5/6] qcow2: Add more overlap check bitmask macros

[Kevin Wolf]

Am 20.09.2013 um 10:37 hat Max Reitz geschrieben:
> Introduces the macros QCOW2_OL_CONSTANT and QCOW2_OL_ALL in addition to
> the already existing QCOW2_OL_CACHED, signifying all metadata overlap
> checks that can be performed in constant time (regardless of image size
> etc.) and truly all available overlap checks, respectively.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 067ed2f..098c14f 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -326,11 +326,19 @@ typedef enum QCow2MetadataOverlap {
>      QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>  } QCow2MetadataOverlap;
>  
> +/* Perform all overlap checks which can be done in constant time */
> +#define QCOW2_OL_CONSTANT \
> +    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
> +     QCOW2_OL_SNAPSHOT_TABLE)
> +
>  /* Perform all overlap checks which don't require disk access */
>  #define QCOW2_OL_CACHED \
> -    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_ACTIVE_L2 | \
> -     QCOW2_OL_REFCOUNT_TABLE | QCOW2_OL_REFCOUNT_BLOCK | \
> -     QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_INACTIVE_L1)
> +    (QCOW2_OL_CONSTANT | QCOW2_OL_ACTIVE_L2 | QCOW2_OL_REFCOUNT_BLOCK | \
> +     QCOW2_OL_SNAPSHOT_TABLE)

QCOW2_OL_INACTIVE_L1 is lost here.

> +/* Perform all overlap checks */
> +#define QCOW2_OL_ALL \
> +    (QCOW2_OL_CACHED | QCOW2_OL_INACTIVE_L2)

Kevin

Re: [Qemu-devel] [PATCH 5/6] qcow2: Add more overlap check bitmask macros

[Max Reitz]

On 2013-10-09 15:07, Kevin Wolf wrote:
> Am 20.09.2013 um 10:37 hat Max Reitz geschrieben:
>> Introduces the macros QCOW2_OL_CONSTANT and QCOW2_OL_ALL in addition to
>> the already existing QCOW2_OL_CACHED, signifying all metadata overlap
>> checks that can be performed in constant time (regardless of image size
>> etc.) and truly all available overlap checks, respectively.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2.h | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 067ed2f..098c14f 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -326,11 +326,19 @@ typedef enum QCow2MetadataOverlap {
>>       QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>   } QCow2MetadataOverlap;
>>   
>> +/* Perform all overlap checks which can be done in constant time */
>> +#define QCOW2_OL_CONSTANT \
>> +    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
>> +     QCOW2_OL_SNAPSHOT_TABLE)
>> +
>>   /* Perform all overlap checks which don't require disk access */
>>   #define QCOW2_OL_CACHED \
>> -    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_ACTIVE_L2 | \
>> -     QCOW2_OL_REFCOUNT_TABLE | QCOW2_OL_REFCOUNT_BLOCK | \
>> -     QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_INACTIVE_L1)
>> +    (QCOW2_OL_CONSTANT | QCOW2_OL_ACTIVE_L2 | QCOW2_OL_REFCOUNT_BLOCK | \
>> +     QCOW2_OL_SNAPSHOT_TABLE)
> QCOW2_OL_INACTIVE_L1 is lost here.

Oh, right, I'll fix it. Thanks for reviewing, by the way. :)

Max

>> +/* Perform all overlap checks */
>> +#define QCOW2_OL_ALL \
>> +    (QCOW2_OL_CACHED | QCOW2_OL_INACTIVE_L2)
> Kevin