[Qemu-devel] [PATCH v2 0/4] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Qemu-devel] [PATCH v2 0/4] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi!

This is my counter-propasal on
"[PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps" by John.

It does bitmap resize, but don't flush bitmaps during resize, postponing this
controversial thing.

Based-on: https://github.com/jnsnow/qemu bitmaps

John Snow (2):
  block/qcow2-bitmap: Allow resizes with persistent bitmaps
  tests/qemu-iotests: add bitmap resize test 246

Vladimir Sementsov-Ogievskiy (2):
  docs/interop/qcow2: Improve bitmap flag in_use specification
  block/qcow2-bitmap: Don't check size for IN_USE bitmap

 docs/interop/qcow2.txt     |  18 ++-
 block/qcow2.h              |   1 +
 block/qcow2-bitmap.c       |  67 ++++++++-
 block/qcow2.c              |   4 +-
 tests/qemu-iotests/246     | 114 ++++++++++++++
 tests/qemu-iotests/246.out | 295 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 7 files changed, 491 insertions(+), 9 deletions(-)
 create mode 100755 tests/qemu-iotests/246
 create mode 100644 tests/qemu-iotests/246.out

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/4] docs/interop/qcow2: Improve bitmap flag in_use specification

[Vladimir Sementsov-Ogievskiy]

We already use (we didn't notice it) IN_USE flag for marking bitmap
metadata outdated, such as AUTO flag, which mirrors enabled/disabled
bitmaps. No we are going to support bitmap resize, so it's good to
write IN_USE meaning with more details.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/qcow2.txt | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index fb5cb47245..575a5f25e2 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -589,7 +589,19 @@ Structure of a bitmap directory entry:
                     Bit
                       0: in_use
                          The bitmap was not saved correctly and may be
-                         inconsistent.
+                         inconsistent. This inconsitency may touch both bitmap
+                         data and metadata, and this mean that bitmap state
+                         (its data and metadata) was changed but not stored
+                         back to the image. This flag doesn't relate to format
+                         corruption, all fields are still correct from qcow2
+                         point of view, they just may be outdated.
+
+                         Note: Currently, Qemu may change (additionally to
+                         bitmap data) @auto flag and size of the bitmap during
+                         image resize. This mean, that not only bitmap data
+                         may be outdated if @in_use flag set, but also value of
+                         @auto flag and bitmap size (which is indirectly
+                         referenced by @bitmap_table_size).
 
                       1: auto
                          The bitmap must reflect all changes of the virtual
@@ -717,8 +729,8 @@ corresponding range of the virtual disk (see above) was written to while the
 bitmap was 'enabled'. An unset bit means that this range was not written to.
 
 The software doesn't have to sync the bitmap in the image file with its
-representation in RAM after each write. Flag 'in_use' should be set while the
-bitmap is not synced.
+representation in RAM after each write or metadata change. Flag 'in_use'
+should be set while the bitmap is not synced.
 
 In the image file the 'enabled' state is reflected by the 'auto' flag. If this
 flag is set, the software must consider the bitmap as 'enabled' and start
-- 
2.18.0

[Qemu-devel] [PATCH v2 2/4] block/qcow2-bitmap: Don't check size for IN_USE bitmap

[Vladimir Sementsov-Ogievskiy]

We are going to allow image resize when there are persistent bitmaps.
It may lead to appearing of inconsistent bitmaps (IN_USE=1) with
inconsistent size. But we still want to load them as inconsistent.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6adbe06b4d..141bc1e52c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -462,10 +462,25 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
         return len;
     }
 
-    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
-           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
+    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
+        return -EINVAL;
+    }
 
-    return fail ? -EINVAL : 0;
+    if (!(entry->flags & BME_FLAG_IN_USE) &&
+        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits)))
+    {
+        /*
+         * We've loaded valid bitmap (IN_USE not set) or we are going to store
+         * valid bitmap. But allocated bitmap table size is not enough to store
+         * such bitmap.
+         *
+         * Note, that it's OK to have invalid bitmap with invalid size during to
+         * bitmap was not correctly saved after image resize.
+         */
+        return -EINVAL;
+    }
+
+    return 0;
 }
 
 static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
-- 
2.18.0

[Qemu-devel] [PATCH v2 3/4] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

From: John Snow <jsnow@redhat.com>

Since we now load all bitmaps into memory anyway, we can just truncate
them in-memory and then flush them back to disk. Just in case, we will
still check and enforce that this shortcut is valid -- i.e. that any
bitmap described on-disk is indeed in-memory and can be modified.

If there are any inconsistent bitmaps, we refuse to allow the truncate
as we do not actually load these bitmaps into memory, and it isn't safe
or reasonable to attempt to truncate corrupted data.

Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
  [vsementsov: drop bitmap flushing, fix block comments style]
---
 block/qcow2.h        |  1 +
 block/qcow2-bitmap.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        |  4 +---
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 9dd02df831..bbac694a84 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 141bc1e52c..6ffeae36c2 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1174,6 +1174,52 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
     return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
 }
 
+/* Checks to see if it's safe to resize bitmaps */
+int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+    int ret = 0;
+
+    if (s->nb_bitmaps == 0) {
+        return 0;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, errp);
+    if (bm_list == NULL) {
+        return -EINVAL;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+        if (bitmap == NULL) {
+            /*
+             * We rely on all bitmaps being in-memory to be able to resize them,
+             * Otherwise, we'd need to resize them on disk explicitly
+             */
+            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
+                       "were not loaded into memory");
+            ret = -ENOTSUP;
+            goto out;
+        }
+
+        /*
+         * The checks against readonly and busy are redundant, but certainly
+         * do no harm. checks against inconsistent are crucial:
+         */
+        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+            ret = -ENOTSUP;
+            goto out;
+        }
+    }
+
+out:
+    bitmap_list_free(bm_list);
+    return ret;
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitmap table accordingly.
  */
diff --git a/block/qcow2.c b/block/qcow2.c
index 7fb2730f09..da2697f912 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3499,9 +3499,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
     }
 
     /* cannot proceed if image has bitmaps */
-    if (s->nb_bitmaps) {
-        /* TODO: resize bitmaps in the image */
-        error_setg(errp, "Can't resize an image which has bitmaps");
+    if (qcow2_truncate_bitmaps_check(bs, errp)) {
         ret = -ENOTSUP;
         goto fail;
     }
-- 
2.18.0

[Qemu-devel] [PATCH v2 4/4] tests/qemu-iotests: add bitmap resize test 246

[Vladimir Sementsov-Ogievskiy]

From: John Snow <jsnow@redhat.com>

Test that we can actually resize qcow2 images with persistent bitmaps
correctly. Throw some other goofy stuff at the test while we're at it,
like adding bitmaps of different granularities and at different times.

Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
   [vsmentsov: drop \n from the end of test output,
      test output changed a bit: some bitmaps goes in other order
      int the output]
---
 tests/qemu-iotests/246     | 114 ++++++++++++++
 tests/qemu-iotests/246.out | 295 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 410 insertions(+)
 create mode 100755 tests/qemu-iotests/246
 create mode 100644 tests/qemu-iotests/246.out

diff --git a/tests/qemu-iotests/246 b/tests/qemu-iotests/246
new file mode 100755
index 0000000000..b0997a392f
--- /dev/null
+++ b/tests/qemu-iotests/246
@@ -0,0 +1,114 @@
+#!/usr/bin/env python
+#
+# Test persistent bitmap resizing.
+#
+# Copyright (c) 2019 John Snow for Red Hat, Inc.
+#
+# 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/>.
+#
+# owner=jsnow@redhat.com
+
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+size = 64 * 1024 * 1024 * 1024
+gran_small = 32 * 1024
+gran_large = 128 * 1024
+
+def query_bitmaps(vm):
+    res = vm.qmp("query-block")
+    return { "bitmaps": { device['device']: device.get('dirty-bitmaps', []) for
+                          device in res['return'] } }
+
+with iotests.FilePath('img') as img_path, \
+     iotests.VM() as vm:
+
+    log('--- Preparing image & VM ---\n')
+    iotests.qemu_img_create('-f', iotests.imgfmt, img_path, str(size))
+    vm.add_drive(img_path)
+
+
+    log('--- 1st Boot (Establish Baseline Image) ---\n')
+    vm.launch()
+
+    log('\n--- Adding bitmaps Small, Medium, Large, and Transient ---\n')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Small", granularity=gran_small, persistent=True)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Medium", persistent=True)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Large", granularity=gran_large, persistent=True)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Transient", persistent=False)
+
+    log('--- Forcing flush of bitmaps to disk ---\n')
+    log(query_bitmaps(vm), indent=2)
+    vm.shutdown()
+
+
+    log('--- 2nd Boot (Grow Image) ---\n')
+    vm.launch()
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Adding new bitmap, growing image, and adding 2nd new bitmap ---')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="New", persistent=True)
+    vm.qmp_log("human-monitor-command",
+               command_line="block_resize drive0 70G")
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Newtwo", persistent=True)
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Forcing flush of bitmaps to disk ---\n')
+    vm.shutdown()
+
+
+    log('--- 3rd Boot (Shrink Image) ---\n')
+    vm.launch()
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Adding "NewB" bitmap, removing "New" bitmap ---')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="NewB", persistent=True)
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0",
+               name="New")
+
+    log('--- Truncating image ---\n')
+    vm.qmp_log("human-monitor-command",
+               command_line="block_resize drive0 50G")
+
+    log('--- Adding "NewC" bitmap, removing "NewTwo" bitmap ---')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="NewC", persistent=True)
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Newtwo")
+
+    log('--- Forcing flush of bitmaps to disk ---\n')
+    vm.shutdown()
+
+
+    log('--- 4th Boot (Verification and Cleanup) ---\n')
+    vm.launch()
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Removing all Bitmaps ---\n')
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Small")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Medium")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Large")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="NewB")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="NewC")
+    log(query_bitmaps(vm), indent=2)
+
+    log('\n--- Done ---')
+    vm.shutdown()
diff --git a/tests/qemu-iotests/246.out b/tests/qemu-iotests/246.out
new file mode 100644
index 0000000000..6671a11fdd
--- /dev/null
+++ b/tests/qemu-iotests/246.out
@@ -0,0 +1,295 @@
+--- Preparing image & VM ---
+
+--- 1st Boot (Establish Baseline Image) ---
+
+
+--- Adding bitmaps Small, Medium, Large, and Transient ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 32768, "name": "Small", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "Medium", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 131072, "name": "Large", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "Transient", "node": "drive0", "persistent": false}}
+{"return": {}}
+--- Forcing flush of bitmaps to disk ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Transient",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- 2nd Boot (Grow Image) ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Adding new bitmap, growing image, and adding 2nd new bitmap ---
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "New", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "human-monitor-command", "arguments": {"command-line": "block_resize drive0 70G"}}
+{"return": ""}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "Newtwo", "node": "drive0", "persistent": true}}
+{"return": {}}
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Newtwo",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "New",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Forcing flush of bitmaps to disk ---
+
+--- 3rd Boot (Shrink Image) ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "New",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Newtwo",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Adding "NewB" bitmap, removing "New" bitmap ---
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "NewB", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "New", "node": "drive0"}}
+{"return": {}}
+--- Truncating image ---
+
+{"execute": "human-monitor-command", "arguments": {"command-line": "block_resize drive0 50G"}}
+{"return": ""}
+--- Adding "NewC" bitmap, removing "NewTwo" bitmap ---
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "NewC", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Newtwo", "node": "drive0"}}
+{"return": {}}
+--- Forcing flush of bitmaps to disk ---
+
+--- 4th Boot (Verification and Cleanup) ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "NewB",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "NewC",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Removing all Bitmaps ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Small", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Medium", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Large", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "NewB", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "NewC", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "drive0": []
+  }
+}
+
+--- Done ---
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b5ca63cf72..0683f6e10d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -241,3 +241,4 @@
 239 rw auto quick
 240 auto quick
 242 rw auto quick
+246 rw auto quick
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 1/4] docs/interop/qcow2: Improve bitmap flag in_use specification

[Eric Blake]

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

On 3/11/19 1:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> We already use (we didn't notice it) IN_USE flag for marking bitmap
> metadata outdated, such as AUTO flag, which mirrors enabled/disabled
> bitmaps. No we are going to support bitmap resize, so it's good to

s/No/Now/

> write IN_USE meaning with more details.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/interop/qcow2.txt | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index fb5cb47245..575a5f25e2 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -589,7 +589,19 @@ Structure of a bitmap directory entry:
>                      Bit
>                        0: in_use
>                           The bitmap was not saved correctly and may be
> -                         inconsistent.
> +                         inconsistent. This inconsitency may touch both bitmap

inconsistency

> +                         data and metadata, and this mean that bitmap state
> +                         (its data and metadata) was changed but not stored
> +                         back to the image. This flag doesn't relate to format
> +                         corruption, all fields are still correct from qcow2
> +                         point of view, they just may be outdated.
> +
> +                         Note: Currently, Qemu may change (additionally to
> +                         bitmap data) @auto flag and size of the bitmap during
> +                         image resize. This mean, that not only bitmap data
> +                         may be outdated if @in_use flag set, but also value of
> +                         @auto flag and bitmap size (which is indirectly
> +                         referenced by @bitmap_table_size).

Feels wordy. Maybe drop the second paragraph starting with Note, and
merely use this for the first paragraph:

The bitmap was not saved correctly and may be inconsistent. Although the
bitmap metadata is still well-formed from a qcow2 perspective, the
metadata (such as the auto flag or bitmap size) or data contents may be
outdated.

>  
>                        1: auto
>                           The bitmap must reflect all changes of the virtual
> @@ -717,8 +729,8 @@ corresponding range of the virtual disk (see above) was written to while the
>  bitmap was 'enabled'. An unset bit means that this range was not written to.
>  
>  The software doesn't have to sync the bitmap in the image file with its
> -representation in RAM after each write. Flag 'in_use' should be set while the
> -bitmap is not synced.
> +representation in RAM after each write or metadata change. Flag 'in_use'
> +should be set while the bitmap is not synced.
>  
>  In the image file the 'enabled' state is reflected by the 'auto' flag. If this
>  flag is set, the software must consider the bitmap as 'enabled' and start
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2-bitmap: Don't check size for IN_USE bitmap

[Eric Blake]

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

On 3/11/19 1:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to allow image resize when there are persistent bitmaps.
> It may lead to appearing of inconsistent bitmaps (IN_USE=1) with
> inconsistent size. But we still want to load them as inconsistent.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

> +    {
> +        /*
> +         * We've loaded valid bitmap (IN_USE not set) or we are going to store
> +         * valid bitmap. But allocated bitmap table size is not enough to store
> +         * such bitmap.
> +         *
> +         * Note, that it's OK to have invalid bitmap with invalid size during to

s/during/due/

> +         * bitmap was not correctly saved after image resize.

s/bitmap/a bitmap that/

> +         */
> +        return -EINVAL;
> +    }
> +
> +    return 0;
>  }
>  
>  static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/4] docs/interop/qcow2: Improve bitmap flag in_use specification

[Vladimir Sementsov-Ogievskiy]

On 12.03.2019 3:00, Eric Blake wrote:
> On 3/11/19 1:51 PM, Vladimir Sementsov-Ogievskiy wrote:
>> We already use (we didn't notice it) IN_USE flag for marking bitmap
>> metadata outdated, such as AUTO flag, which mirrors enabled/disabled
>> bitmaps. No we are going to support bitmap resize, so it's good to
> 
> s/No/Now/
> 
>> write IN_USE meaning with more details.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/interop/qcow2.txt | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index fb5cb47245..575a5f25e2 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -589,7 +589,19 @@ Structure of a bitmap directory entry:
>>                       Bit
>>                         0: in_use
>>                            The bitmap was not saved correctly and may be
>> -                         inconsistent.
>> +                         inconsistent. This inconsitency may touch both bitmap
> 
> inconsistency
> 
>> +                         data and metadata, and this mean that bitmap state
>> +                         (its data and metadata) was changed but not stored
>> +                         back to the image. This flag doesn't relate to format
>> +                         corruption, all fields are still correct from qcow2
>> +                         point of view, they just may be outdated.
>> +
>> +                         Note: Currently, Qemu may change (additionally to
>> +                         bitmap data) @auto flag and size of the bitmap during
>> +                         image resize. This mean, that not only bitmap data
>> +                         may be outdated if @in_use flag set, but also value of
>> +                         @auto flag and bitmap size (which is indirectly
>> +                         referenced by @bitmap_table_size).
> 
> Feels wordy. Maybe drop the second paragraph starting with Note, and
> merely use this for the first paragraph:
> 
> The bitmap was not saved correctly and may be inconsistent. Although the
> bitmap metadata is still well-formed from a qcow2 perspective, the
> metadata (such as the auto flag or bitmap size) or data contents may be
> outdated.

Sounds good (as always), thanks)

> 
>>   
>>                         1: auto
>>                            The bitmap must reflect all changes of the virtual
>> @@ -717,8 +729,8 @@ corresponding range of the virtual disk (see above) was written to while the
>>   bitmap was 'enabled'. An unset bit means that this range was not written to.
>>   
>>   The software doesn't have to sync the bitmap in the image file with its
>> -representation in RAM after each write. Flag 'in_use' should be set while the
>> -bitmap is not synced.
>> +representation in RAM after each write or metadata change. Flag 'in_use'
>> +should be set while the bitmap is not synced.
>>   
>>   In the image file the 'enabled' state is reflected by the 'auto' flag. If this
>>   flag is set, the software must consider the bitmap as 'enabled' and start
>>
>