[Qemu-devel] [PATCH v3 0/7] bitmaps: add inconsistent bit

[Qemu-devel] [PATCH v3 0/7] bitmaps: add inconsistent bit

[John Snow]

Allow QEMU to read in bitmaps that have the in-use bit set, for the
purposes of allowing users to delete those bitmaps.

This is chosen in preference to a hard error on load to minimize
impact for a non-critical error, but to force the user or management
utility to acknowledge that the bitmap is no longer viable.

1. Changed wording of meaning of persistent bit, inconsistent bit
   Declining to optimize to avoid allocations for this revision.

2. Add Reviewed-by from Eric.

3. Split into several patches that are more single-purpose, which
   highlights the individual fixes more clearly;

   - Prohibit BUSY or INCONSISTENT bitmaps from being merge sources.

4. Declining feedback to prohibit disabling or enabling readonly bitmaps,
   on the basis that users may wish to enable/disable them prior to
   remounting their backing storage RW.

   Decided to prohibit attempting to remove readonly bitmaps, so the
   failure happens earlier.

   Prohibit sync=incremental backups using readonly bitmaps, because
   they're not capable of clearing the bitmap on success.
   sync=differential would be acceptable here. (Good spot, Vladimir.)

John Snow (7):
  block/dirty-bitmaps: add inconsistent bit
  block/dirty-bitmap: add inconsistent status
  block/dirty-bitmaps: add block_dirty_bitmap_check function
  block/dirty-bitmaps: prohibit readonly bitmaps for backups
  block/dirty-bitmaps: prohibit removing readonly bitmaps
  block/dirty-bitmaps: disallow busy bitmaps as merge source
  block/dirty-bitmaps: implement inconsistent bit

 qapi/block-core.json           |  20 +++++--
 include/block/dirty-bitmap.h   |  15 ++++-
 block/dirty-bitmap.c           |  63 +++++++++++++++++---
 block/qcow2-bitmap.c           | 103 +++++++++++++++++----------------
 blockdev.c                     |  50 ++++------------
 migration/block-dirty-bitmap.c |  12 +---
 nbd/server.c                   |   3 +-
 7 files changed, 151 insertions(+), 115 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[John Snow]

Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
that have been marked as "in use".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json         | 13 +++++++++----
 include/block/dirty-bitmap.h |  2 ++
 block/dirty-bitmap.c         | 19 +++++++++++++++++++
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6e543594b3..e639ef6d1c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -467,15 +467,20 @@
 #        and cannot be modified via QMP or used by another operation.
 #        Replaces `locked` and `frozen` statuses. (since 4.0)
 #
-# @persistent: true if the bitmap will eventually be flushed to persistent
-#              storage (since 4.0)
+# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
+#              on disk, or both. (since 4.0)
+#
+# @inconsistent: true if this is a persistent bitmap that was improperly
+#                stored. Implies @persistent to be true; @recording and
+#                @busy to be false. This bitmap cannot be used. To remove
+#                it, use @block-dirty-bitmap-remove. (Since 4.0)
 #
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-           'recording': 'bool', 'busy': 'bool',
-           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
+           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
+           'persistent': 'bool', '*inconsistent': 'bool' } }
 
 ##
 # @Qcow2BitmapInfoFlags:
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ba8477b73f..bd1b6479df 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
+void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp);
@@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 980cae4fa3..9e8630e1ac 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
                                    and this bitmap must remain unchanged while
                                    this flag is set. */
     bool persistent;            /* bitmap must be saved to owner disk image */
+    bool inconsistent;          /* bitmap is persistent, but inconsistent.
+                                 * It cannot be used at all in any way, except
+                                 * a QMP user can remove it. */
     bool migration;             /* Bitmap is selected for migration, it should
                                    not be stored on the next inactivation
                                    (persistent flag doesn't matter until next
@@ -464,6 +467,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->recording = bdrv_dirty_bitmap_recording(bm);
         info->busy = bdrv_dirty_bitmap_busy(bm);
         info->persistent = bm->persistent;
+        info->has_inconsistent = bm->inconsistent;
+        info->inconsistent = bm->inconsistent;
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
@@ -711,6 +716,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
     qemu_mutex_unlock(bitmap->mutex);
 }
 
+/* Called with BQL taken. */
+void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
+{
+    qemu_mutex_lock(bitmap->mutex);
+    bitmap->inconsistent = true;
+    bitmap->disabled = true;
+    qemu_mutex_unlock(bitmap->mutex);
+}
+
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
 {
@@ -724,6 +738,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
     return bitmap->persistent && !bitmap->migration;
 }
 
+bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->inconsistent;
+}
+
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm;
-- 
2.17.2

[Qemu-devel] [PATCH v3 2/7] block/dirty-bitmap: add inconsistent status

[John Snow]

Even though the status field is deprecated, we still have to support
it for a few more releases. Since this is a very new kind of bitmap
state, it makes sense for it to have its own status field.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json | 7 ++++++-
 block/dirty-bitmap.c | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e639ef6d1c..ae55cd0704 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -442,10 +442,15 @@
 #          recording new writes. If the bitmap was @disabled, it is not
 #          recording new writes. (Since 2.12)
 #
+# @inconsistent: This is a persistent dirty bitmap that was marked in-use on
+#                disk, and is unusable by QEMU. It can only be deleted.
+#                Please rely on the inconsistent field in @BlockDirtyInfo
+#                instead, as the status field is deprecated. (Since 4.0)
+#
 # Since: 2.4
 ##
 { 'enum': 'DirtyBitmapStatus',
-  'data': ['active', 'disabled', 'frozen', 'locked'] }
+  'data': ['active', 'disabled', 'frozen', 'locked', 'inconsistent'] }
 
 ##
 # @BlockDirtyInfo:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9e8630e1ac..71e0098396 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,10 +209,15 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
  *               or it can be Disabled and not recording writes.
  * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
  *               in any way from the monitor.
+ * (5) Inconsistent: This is a persistent bitmap whose "in use" bit is set, and
+ *                   is unusable by QEMU. It can be deleted to remove it from
+ *                   the qcow2.
  */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
-    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+    if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+        return DIRTY_BITMAP_STATUS_INCONSISTENT;
+    } else if (bdrv_dirty_bitmap_has_successor(bitmap)) {
         return DIRTY_BITMAP_STATUS_FROZEN;
     } else if (bdrv_dirty_bitmap_busy(bitmap)) {
         return DIRTY_BITMAP_STATUS_LOCKED;
-- 
2.17.2

[Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[John Snow]

Instead of checking against busy, inconsistent, or read only directly,
use a check function with permissions bits that let us streamline the
checks without reproducing them in many places.

Included in this patch are permissions changes that simply add the
inconsistent check to existing permissions call spots, without
addressing existing bugs.

In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
which checks against all three conditions. busy-only checks become
BDRV_BITMAP_ALLOW_RO.

Notably, remove allows inconsistent bitmaps, so it doesn't follow the pattern.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 include/block/dirty-bitmap.h   | 13 ++++++++-
 block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
 blockdev.c                     | 49 +++++++---------------------------
 migration/block-dirty-bitmap.c | 12 +++------
 nbd/server.c                   |  3 +--
 5 files changed, 54 insertions(+), 61 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index bd1b6479df..2a78243954 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -5,6 +5,16 @@
 #include "qapi/qapi-types-block-core.h"
 #include "qemu/hbitmap.h"
 
+typedef enum BitmapCheckFlags {
+    BDRV_BITMAP_BUSY = 1,
+    BDRV_BITMAP_RO = 2,
+    BDRV_BITMAP_INCONSISTENT = 4,
+} BitmapCheckFlags;
+
+#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO |        \
+                             BDRV_BITMAP_INCONSISTENT)
+#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
@@ -24,6 +34,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
+int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
+                            Error **errp);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
@@ -93,7 +105,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
-bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
                                         BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 71e0098396..769668ccdc 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
     return bitmap->successor;
 }
 
-bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap *bitmap) {
     return bitmap->busy;
 }
 
@@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
                                  !bitmap->successor->disabled);
 }
 
+int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
+                            Error **errp)
+{
+    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is currently in use by another"
+                   " operation and cannot be used", bitmap->name);
+        return -1;
+    }
+
+    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+                   bitmap->name);
+        return -1;
+    }
+
+    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
+        bdrv_dirty_bitmap_inconsistent(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
+                   bitmap->name);
+        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
+                          "this bitmap from disk");
+        return -1;
+    }
+
+    return 0;
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an operation.
  * Requires that the bitmap is not marked busy and has no successor.
@@ -794,17 +821,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
     qemu_mutex_lock(dest->mutex);
 
-    if (bdrv_dirty_bitmap_busy(dest)) {
-        error_setg(errp, "Bitmap '%s' is currently in use by another"
-        " operation and cannot be modified", dest->name);
+    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
         goto out;
     }
 
-    if (bdrv_dirty_bitmap_readonly(dest)) {
-        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
-                   dest->name);
-        goto out;
-    }
 
     if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
         error_setg(errp, "Bitmaps are incompatible and can't be merged");
diff --git a/blockdev.c b/blockdev.c
index cbce44877d..5d74479ba7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2007,11 +2007,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-        error_setg(errp, "Cannot modify a bitmap in use by another operation");
-        return;
-    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
-        error_setg(errp, "Cannot clear a readonly bitmap");
+    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
         return;
     }
 
@@ -2056,10 +2052,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be enabled", action->name);
+    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -2097,10 +2090,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be disabled", action->name);
+    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation and"
-                   " cannot be removed", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
         return;
     }
 
@@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be cleared", name);
-        return;
-    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
-        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
         return;
     }
 
@@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be enabled", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be disabled", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
             bdrv_unref(target_bs);
             goto out;
         }
-        if (bdrv_dirty_bitmap_busy(bmap)) {
-            error_setg(errp,
-                       "Bitmap '%s' is currently in use by another operation"
-                       " and cannot be used for backup", backup->bitmap);
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto out;
         }
     }
@@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
             error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
             goto out;
         }
-        if (bdrv_dirty_bitmap_busy(bmap)) {
-            error_setg(errp,
-                       "Bitmap '%s' is currently in use by another operation"
-                       " and cannot be used for backup", backup->bitmap);
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto out;
         }
     }
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7057fff242..0fcf897f32 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
     BdrvDirtyBitmap *bitmap;
     DirtyBitmapMigBitmapState *dbms;
     BdrvNextIterator it;
+    Error *local_err = NULL;
 
     dirty_bitmap_mig_state.bulk_completed = false;
     dirty_bitmap_mig_state.prev_bs = NULL;
@@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
                 goto fail;
             }
 
-            if (bdrv_dirty_bitmap_busy(bitmap)) {
-                error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
-                             bdrv_dirty_bitmap_name(bitmap));
-                goto fail;
-            }
-
-            if (bdrv_dirty_bitmap_readonly(bitmap)) {
-                error_report("Can't migrate read-only dirty bitmap: '%s",
-                             bdrv_dirty_bitmap_name(bitmap));
+            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
+                error_report_err(local_err);
                 goto fail;
             }
 
diff --git a/nbd/server.c b/nbd/server.c
index 02773e2836..9b87c7f243 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
             goto fail;
         }
 
-        if (bdrv_dirty_bitmap_busy(bm)) {
-            error_setg(errp, "Bitmap '%s' is in use", bitmap);
+        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto fail;
         }
 
-- 
2.17.2

[Qemu-devel] [PATCH v3 4/7] block/dirty-bitmaps: prohibit readonly bitmaps for backups

[John Snow]

drive and blockdev backup cannot use readonly bitmaps, because the
sync=incremental mechanism actually edits the bitmaps on success.

If you really want to do this operation, use a copied bitmap.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5d74479ba7..c8255dda0b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3526,7 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
             bdrv_unref(target_bs);
             goto out;
         }
-        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
             goto out;
         }
     }
@@ -3636,7 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
             error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
             goto out;
         }
-        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
             goto out;
         }
     }
-- 
2.17.2

[Qemu-devel] [PATCH v3 5/7] block/dirty-bitmaps: prohibit removing readonly bitmaps

[John Snow]

Remove is an inherently RW operation, so this will fail anyway, but
we can fail it very quickly instead of trying and failing, so do so.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index c8255dda0b..a9a059c570 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2881,7 +2881,8 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
+                                errp)) {
         return;
     }
 
-- 
2.17.2

[Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[John Snow]

We didn't do any state checking on source bitmaps at all,
so this adds inconsistent and busy checks. readonly is
allowed, so you can still copy a readonly bitmap to a new
destination to use it for operations like drive-backup.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 769668ccdc..8403c9981d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
         goto out;
     }
 
+    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
+        goto out;
+    }
 
     if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
         error_setg(errp, "Bitmaps are incompatible and can't be merged");
-- 
2.17.2

[Qemu-devel] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[John Snow]

Set the inconsistent bit on load instead of rejecting such bitmaps.
There is no way to un-set it; the only option is to delete it.

Obvervations:
- bitmap loading does not need to update the header for in_use bitmaps.
- inconsistent bitmaps don't need to have their data loaded; they're
  glorified corruption sentinels.
- bitmap saving does not need to save inconsistent bitmaps back to disk.
- bitmap reopening DOES need to drop the readonly flag from inconsistent
  bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
  being eventually flushed back to disk.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 103 ++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3ee524da4b..c3b210ede1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
     uint32_t granularity;
     BdrvDirtyBitmap *bitmap = NULL;
 
+    granularity = 1U << bm->granularity_bits;
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+    if (bitmap == NULL) {
+        goto fail;
+    }
+
     if (bm->flags & BME_FLAG_IN_USE) {
-        error_setg(errp, "Bitmap '%s' is in use", bm->name);
-        goto fail;
+        /* Data is unusable, skip loading it */
+        return bitmap;
     }
 
     ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
@@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
         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",
@@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     Qcow2Bitmap *bm;
     GSList *created_dirty_bitmaps = NULL;
     bool header_updated = false;
+    bool needs_update = false;
 
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
@@ -962,35 +963,39 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-            if (bitmap == NULL) {
-                goto fail;
-            }
-
-            if (!(bm->flags & BME_FLAG_AUTO)) {
-                bdrv_disable_dirty_bitmap(bitmap);
-            }
-            bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            bm->flags |= BME_FLAG_IN_USE;
-            created_dirty_bitmaps =
-                    g_slist_append(created_dirty_bitmaps, bitmap);
+        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+        if (bitmap == NULL) {
+            goto fail;
         }
-    }
 
-    if (created_dirty_bitmaps != NULL) {
-        if (can_write(bs)) {
-            /* in_use flags must be updated */
-            int ret = update_ext_header_and_dir_in_place(bs, bm_list);
-            if (ret < 0) {
-                error_setg_errno(errp, -ret, "Can't update bitmap directory");
-                goto fail;
-            }
-            header_updated = true;
+        if (bm->flags & BME_FLAG_IN_USE) {
+            bdrv_dirty_bitmap_set_inconsistent(bitmap);
         } else {
-            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
-                            (gpointer)true);
+            /* NB: updated flags only get written if can_write(bs) is true. */
+            bm->flags |= BME_FLAG_IN_USE;
+            needs_update = true;
         }
+        if (!(bm->flags & BME_FLAG_AUTO)) {
+            bdrv_disable_dirty_bitmap(bitmap);
+        }
+        bdrv_dirty_bitmap_set_persistance(bitmap, true);
+        created_dirty_bitmaps =
+            g_slist_append(created_dirty_bitmaps, bitmap);
+    }
+
+    if (needs_update && can_write(bs)) {
+        /* in_use flags must be updated */
+        int ret = update_ext_header_and_dir_in_place(bs, bm_list);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Can't update bitmap directory");
+            goto fail;
+        }
+        header_updated = true;
+    }
+
+    if (!can_write(bs)) {
+        g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
+                        (gpointer)true);
     }
 
     g_slist_free(created_dirty_bitmaps);
@@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-            if (bitmap == NULL) {
-                continue;
-            }
-
-            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-                error_setg(errp, "Bitmap %s is not readonly but not marked"
-                                 "'IN_USE' in the image. Something went wrong,"
-                                 "all the bitmaps may be corrupted", bm->name);
-                ret = -EINVAL;
-                goto out;
-            }
+        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+        if (bitmap == NULL) {
+            continue;
+        }
 
-            bm->flags |= BME_FLAG_IN_USE;
-            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
+                       "not marked as readonly. This is a bug, something went "
+                       "wrong. All of the bitmaps may be corrupted", bm->name);
+            ret = -EINVAL;
+            goto out;
         }
+
+        bm->flags |= BME_FLAG_IN_USE;
+        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
     }
 
     if (ro_dirty_bitmaps != NULL) {
@@ -1424,8 +1427,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         Qcow2Bitmap *bm;
 
         if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
-            bdrv_dirty_bitmap_readonly(bitmap))
-        {
+            bdrv_dirty_bitmap_readonly(bitmap) ||
+            bdrv_dirty_bitmap_inconsistent(bitmap)) {
             continue;
         }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[Eric Blake]

On 3/1/19 1:15 PM, John Snow wrote:
> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
> that have been marked as "in use".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/block-core.json         | 13 +++++++++----
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 19 +++++++++++++++++++
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6e543594b3..e639ef6d1c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -467,15 +467,20 @@
>  #        and cannot be modified via QMP or used by another operation.
>  #        Replaces `locked` and `frozen` statuses. (since 4.0)
>  #
> -# @persistent: true if the bitmap will eventually be flushed to persistent
> -#              storage (since 4.0)
> +# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
> +#              on disk, or both. (since 4.0)
> +#
> +# @inconsistent: true if this is a persistent bitmap that was improperly
> +#                stored. Implies @persistent to be true; @recording and
> +#                @busy to be false. This bitmap cannot be used. To remove
> +#                it, use @block-dirty-bitmap-remove. (Since 4.0)
>  #

I know we waffled on word-smithing this, but this turned out nicely.


>  
> +/* Called with BQL taken. */
> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
> +{
> +    qemu_mutex_lock(bitmap->mutex);
> +    bitmap->inconsistent = true;
> +    bitmap->disabled = true;
> +    qemu_mutex_unlock(bitmap->mutex);

Worth an assert that persistent is true?  Either way,

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Eric Blake]

On 3/1/19 1:15 PM, John Snow wrote:
> Instead of checking against busy, inconsistent, or read only directly,
> use a check function with permissions bits that let us streamline the
> checks without reproducing them in many places.
> 
> Included in this patch are permissions changes that simply add the
> inconsistent check to existing permissions call spots, without
> addressing existing bugs.
> 
> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
> which checks against all three conditions. busy-only checks become
> BDRV_BITMAP_ALLOW_RO.
> 
> Notably, remove allows inconsistent bitmaps, so it doesn't follow the pattern.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  include/block/dirty-bitmap.h   | 13 ++++++++-
>  block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
>  blockdev.c                     | 49 +++++++---------------------------
>  migration/block-dirty-bitmap.c | 12 +++------
>  nbd/server.c                   |  3 +--
>  5 files changed, 54 insertions(+), 61 deletions(-)
> 

Diffstat proves its a win, even with the extra documentation for the new
function. Nice.

> +int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
> +                            Error **errp)
> +{
> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
> +                   " operation and cannot be used", bitmap->name);

Split before space,

> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> +                   bitmap->name);
> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
> +                   bitmap->name);
> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
> +                          "this bitmap from disk");

split after space.  Looks inconsistent within a single function (pardon
the pun :)

That's minor,
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 4/7] block/dirty-bitmaps: prohibit readonly bitmaps for backups

[Eric Blake]

On 3/1/19 1:15 PM, John Snow wrote:
> drive and blockdev backup cannot use readonly bitmaps, because the
> sync=incremental mechanism actually edits the bitmaps on success.
> 
> If you really want to do this operation, use a copied bitmap.

In fact, that's what I ended up doing in my libvirt patches, always
running sync=incremental on a temporary copy.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 5/7] block/dirty-bitmaps: prohibit removing readonly bitmaps

[Eric Blake]

On 3/1/19 1:15 PM, John Snow wrote:
> Remove is an inherently RW operation, so this will fail anyway, but
> we can fail it very quickly instead of trying and failing, so do so.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/blockdev.c b/blockdev.c
> index c8255dda0b..a9a059c570 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2881,7 +2881,8 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>          return;
>      }
>  
> -    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
> +                                errp)) {
>          return;
>      }
>  
> 

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

Re: [Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[John Snow]

On 3/1/19 2:32 PM, Eric Blake wrote:
> On 3/1/19 1:15 PM, John Snow wrote:
>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>> that have been marked as "in use".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qapi/block-core.json         | 13 +++++++++----
>>  include/block/dirty-bitmap.h |  2 ++
>>  block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>  3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6e543594b3..e639ef6d1c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -467,15 +467,20 @@
>>  #        and cannot be modified via QMP or used by another operation.
>>  #        Replaces `locked` and `frozen` statuses. (since 4.0)
>>  #
>> -# @persistent: true if the bitmap will eventually be flushed to persistent
>> -#              storage (since 4.0)
>> +# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
>> +#              on disk, or both. (since 4.0)
>> +#
>> +# @inconsistent: true if this is a persistent bitmap that was improperly
>> +#                stored. Implies @persistent to be true; @recording and
>> +#                @busy to be false. This bitmap cannot be used. To remove
>> +#                it, use @block-dirty-bitmap-remove. (Since 4.0)
>>  #
> 
> I know we waffled on word-smithing this, but this turned out nicely.
> 

Yes, I think so too.

> 
>>  
>> +/* Called with BQL taken. */
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    bitmap->inconsistent = true;
>> +    bitmap->disabled = true;
>> +    qemu_mutex_unlock(bitmap->mutex);
> 
> Worth an assert that persistent is true?  Either way,
> 

Won't hurt.

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[Eric Blake]

On 3/1/19 1:15 PM, John Snow wrote:
> We didn't do any state checking on source bitmaps at all,
> so this adds inconsistent and busy checks. readonly is
> allowed, so you can still copy a readonly bitmap to a new
> destination to use it for operations like drive-backup.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

I understand forbidding inconsistent sources (because if the source is
potentially missing bits, then the merge destination will also be
missing bits and thus be inconsistent), but why forbid busy?  If I've
associated a bitmap with an NBD server (making it busy), it is still
readable, and so I should still be able to merge its bits into another copy.

> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 769668ccdc..8403c9981d 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>          goto out;
>      }
>  
> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {

Thus, I think this should be BDRV_BITMAP_INCONSISTENT.

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

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[John Snow]

On 3/1/19 2:44 PM, Eric Blake wrote:
> On 3/1/19 1:15 PM, John Snow wrote:
>> We didn't do any state checking on source bitmaps at all,
>> so this adds inconsistent and busy checks. readonly is
>> allowed, so you can still copy a readonly bitmap to a new
>> destination to use it for operations like drive-backup.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/dirty-bitmap.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
> 
> I understand forbidding inconsistent sources (because if the source is
> potentially missing bits, then the merge destination will also be
> missing bits and thus be inconsistent), but why forbid busy?  If I've
> associated a bitmap with an NBD server (making it busy), it is still
> readable, and so I should still be able to merge its bits into another copy.
> 

True, do you rely on this, though?

I was working from a space of "busy" meant "actively in-use by an
operation, and COULD change" so I was forbidding it out of good hygiene.

Clearly the ones in-use by NBD are actually static and unchanging, so
it's safer -- but that might not be true for push backups, where you
might not actually be getting what you think you are, because of the
bifurcated nature of those bitmaps.

If this causes a problem for you in the short-term I will simply roll
this back, but it stands out to me.

(I can't stop myself from trying to protect the user from themselves.
It's clearly a recurring theme in my design and reviews.)

>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 769668ccdc..8403c9981d 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>          goto out;
>>      }
>>  
>> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
> 
> Thus, I think this should be BDRV_BITMAP_INCONSISTENT.
> 

Re: [Qemu-devel] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[Eric Blake]

On 3/1/19 1:15 PM, John Snow wrote:
> Set the inconsistent bit on load instead of rejecting such bitmaps.
> There is no way to un-set it; the only option is to delete it.
> 
> Obvervations:
> - bitmap loading does not need to update the header for in_use bitmaps.
> - inconsistent bitmaps don't need to have their data loaded; they're
>   glorified corruption sentinels.
> - bitmap saving does not need to save inconsistent bitmaps back to disk.
> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>   bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>   being eventually flushed back to disk.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2-bitmap.c | 103 ++++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 50 deletions(-)
> 

> @@ -962,35 +963,39 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      }
>  
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {

> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> +        if (bitmap == NULL) {
> +            goto fail;
>          }

> +        if (bm->flags & BME_FLAG_IN_USE) {
> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);

If you take my suggestion of an assertion in 1/7, then this line...

>          } else {
> -            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
> -                            (gpointer)true);
> +            /* NB: updated flags only get written if can_write(bs) is true. */
> +            bm->flags |= BME_FLAG_IN_USE;
> +            needs_update = true;
>          }
> +        if (!(bm->flags & BME_FLAG_AUTO)) {
> +            bdrv_disable_dirty_bitmap(bitmap);
> +        }
> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);

...and this line need to swap order.

Also, can we have a preliminary patch to s/persistance/persistence/ and
fix our typo?

> @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>      }
>  
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
> -            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> -            if (bitmap == NULL) {
> -                continue;
> -            }
> -
> -            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> -                error_setg(errp, "Bitmap %s is not readonly but not marked"
> -                                 "'IN_USE' in the image. Something went wrong,"
> -                                 "all the bitmaps may be corrupted", bm->name);

Nice - you're fixingthemissing spaces.

> -                ret = -EINVAL;
> -                goto out;
> -            }
> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> +        if (bitmap == NULL) {
> +            continue;
> +        }
>  
> -            bm->flags |= BME_FLAG_IN_USE;
> -            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
> +        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> +            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
> +                       "not marked as readonly. This is a bug, something went "
> +                       "wrong. All of the bitmaps may be corrupted", bm->name);

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[John Snow]

On 3/1/19 2:36 PM, Eric Blake wrote:
> On 3/1/19 1:15 PM, John Snow wrote:
>> Instead of checking against busy, inconsistent, or read only directly,
>> use a check function with permissions bits that let us streamline the
>> checks without reproducing them in many places.
>>
>> Included in this patch are permissions changes that simply add the
>> inconsistent check to existing permissions call spots, without
>> addressing existing bugs.
>>
>> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
>> which checks against all three conditions. busy-only checks become
>> BDRV_BITMAP_ALLOW_RO.
>>
>> Notably, remove allows inconsistent bitmaps, so it doesn't follow the pattern.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  include/block/dirty-bitmap.h   | 13 ++++++++-
>>  block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
>>  blockdev.c                     | 49 +++++++---------------------------
>>  migration/block-dirty-bitmap.c | 12 +++------
>>  nbd/server.c                   |  3 +--
>>  5 files changed, 54 insertions(+), 61 deletions(-)
>>
> 
> Diffstat proves its a win, even with the extra documentation for the new
> function. Nice.
> 

It would have been even more obvious if I had added the individual
"inconsistent" checks before conversion, so that it's even close to
breaking even seems like a win.

>> +int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>> +                            Error **errp)
>> +{
>> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
>> +                   " operation and cannot be used", bitmap->name);
> 
> Split before space,
> 
>> +        return -1;
>> +    }
>> +
>> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>> +                   bitmap->name);
>> +        return -1;
>> +    }
>> +
>> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
>> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>> +                   bitmap->name);
>> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>> +                          "this bitmap from disk");
> 
> split after space.  Looks inconsistent within a single function (pardon
> the pun :)
> 

Ah... I've never known how to split strings. In fact, does anyone?
I'll address this either in staging or as a follow-up, as I assume
Vladimir will have some comments for me.

--js

> That's minor,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[Eric Blake]

On 3/1/19 1:48 PM, John Snow wrote:

>> I understand forbidding inconsistent sources (because if the source is
>> potentially missing bits, then the merge destination will also be
>> missing bits and thus be inconsistent), but why forbid busy?  If I've
>> associated a bitmap with an NBD server (making it busy), it is still
>> readable, and so I should still be able to merge its bits into another copy.
>>
> 
> True, do you rely on this, though?

Not in my current libvirt code (as I create a temporary bitmap to hand
to NBD, since it may be the merge of one or more disabled bitmaps in a
differential backup case), so being tighter for now and relaxing later
if we DO come up with a use is acceptable.

> 
> I was working from a space of "busy" meant "actively in-use by an
> operation, and COULD change" so I was forbidding it out of good hygiene.
> 
> Clearly the ones in-use by NBD are actually static and unchanging, so
> it's safer -- but that might not be true for push backups, where you
> might not actually be getting what you think you are, because of the
> bifurcated nature of those bitmaps.

Oh, good point, especially after you worked so hard to merge
locked/frozen into a single status - you WILL miss the bits from the
successor (unless we teach the merge algorithm to pull in the busy
bitmap's bits AND all the bits of its successors - but that feels like a
lot of work if we don't have a client needing it now).  Okay, with the
extra justification mentioned in the commit message,

> 
> If this causes a problem for you in the short-term I will simply roll
> this back, but it stands out to me.
> 
> (I can't stop myself from trying to protect the user from themselves.
> It's clearly a recurring theme in my design and reviews.)
> 
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 769668ccdc..8403c9981d 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>          goto out;
>>>      }
>>>  
>>> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
>>
>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT.

then I retract my complaint, and the code is acceptable for now.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Eric Blake]

On 3/1/19 1:57 PM, John Snow wrote:

>>> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>>> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
>>> +                   " operation and cannot be used", bitmap->name);
>>
>> Split before space,
>>

>>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>>> +                   bitmap->name);
>>> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>>> +                          "this bitmap from disk");
>>
>> split after space.  Looks inconsistent within a single function (pardon
>> the pun :)
>>
> 
> Ah... I've never known how to split strings. In fact, does anyone?
> I'll address this either in staging or as a follow-up, as I assume
> Vladimir will have some comments for me.

I don't care which way you go. git says we have both styles with enough
frequency that I wouldn't rule out the other style in HACKING.  But it
also says split after space seems more common, if you trust my regex:

$ git grep '"[^"]* "$' | wc
  11597  120619 1261416
$ git grep '^[[:space:]]*" [^"]*"' | wc
   4070   19423  271036

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

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[John Snow]

On 3/1/19 2:57 PM, Eric Blake wrote:
> On 3/1/19 1:48 PM, John Snow wrote:
> 
>>> I understand forbidding inconsistent sources (because if the source is
>>> potentially missing bits, then the merge destination will also be
>>> missing bits and thus be inconsistent), but why forbid busy?  If I've
>>> associated a bitmap with an NBD server (making it busy), it is still
>>> readable, and so I should still be able to merge its bits into another copy.
>>>
>>
>> True, do you rely on this, though?
> 
> Not in my current libvirt code (as I create a temporary bitmap to hand
> to NBD, since it may be the merge of one or more disabled bitmaps in a
> differential backup case), so being tighter for now and relaxing later
> if we DO come up with a use is acceptable.
> 
>>
>> I was working from a space of "busy" meant "actively in-use by an
>> operation, and COULD change" so I was forbidding it out of good hygiene.
>>
>> Clearly the ones in-use by NBD are actually static and unchanging, so
>> it's safer -- but that might not be true for push backups, where you
>> might not actually be getting what you think you are, because of the
>> bifurcated nature of those bitmaps.
> 
> Oh, good point, especially after you worked so hard to merge
> locked/frozen into a single status - you WILL miss the bits from the
> successor (unless we teach the merge algorithm to pull in the busy
> bitmap's bits AND all the bits of its successors - but that feels like a
> lot of work if we don't have a client needing it now).  Okay, with the
> extra justification mentioned in the commit message,
> 

(Though I am being a little fast and loose here: when we split a bitmap,
the top-level one that retains the name actually stays unchanging and
the child bitmap is the one that starts accruing writes from a blank
canvas, but that STILL may not be what you expect when you choose it as
a merge source, however.)

>>
>> If this causes a problem for you in the short-term I will simply roll
>> this back, but it stands out to me.
>>
>> (I can't stop myself from trying to protect the user from themselves.
>> It's clearly a recurring theme in my design and reviews.)
>>
>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>> index 769668ccdc..8403c9981d 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>>          goto out;
>>>>      }
>>>>  
>>>> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>
>>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT.
> 
> then I retract my complaint, and the code is acceptable for now.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

We could always split it back out later, but in basic terms for
permissions and user perspective, "in use" seems robust enough of a
resolution. (It might be safe to read, it might not be, who knows --
it's in use.)

If it really comes to a point, we can always re-add a new status bit to
let the end-user know if they're working with a bifurcated (I have to
use weird vocabulary words sometimes) bitmap but at the moment it seems
very safely an implementation detail.

You can also check that for "enabled" bitmap as reported back to user
via QAPI I check to see if the parent OR child is enabled and report
that cumulatively as "enabled", because together they are "effectively"
enabled.

--js

Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Eric Blake]

On 3/1/19 2:03 PM, Eric Blake wrote:
> On 3/1/19 1:57 PM, John Snow wrote:
> 
>>>> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>>>> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
>>>> +                   " operation and cannot be used", bitmap->name);
>>>
>>> Split before space,
>>>
> 
>>>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>>>> +                   bitmap->name);
>>>> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>>>> +                          "this bitmap from disk");
>>>
>>> split after space.  Looks inconsistent within a single function (pardon
>>> the pun :)
>>>
>>
>> Ah... I've never known how to split strings. In fact, does anyone?
>> I'll address this either in staging or as a follow-up, as I assume
>> Vladimir will have some comments for me.
> 
> I don't care which way you go. git says we have both styles with enough
> frequency that I wouldn't rule out the other style in HACKING.  But it
> also says split after space seems more common, if you trust my regex:
> 

Shoot, I was in the wrong directory when I counted. Trying again, this
time on qemu.git:

$ git grep '"[^"]* "$' | wc
   1566   13019  135245
$ git grep '^[[:space:]]*" [^"]*"' | wc
   1714   11772  130881

and now the numbers favor split before space.

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

Re: [Qemu-devel] [PATCH v3 0/7] bitmaps: add inconsistent bit

[John Snow]

On 3/1/19 2:15 PM, John Snow wrote:
> Allow QEMU to read in bitmaps that have the in-use bit set, for the
> purposes of allowing users to delete those bitmaps.
> 
> This is chosen in preference to a hard error on load to minimize
> impact for a non-critical error, but to force the user or management
> utility to acknowledge that the bitmap is no longer viable.
> 
> 1. Changed wording of meaning of persistent bit, inconsistent bit
>    Declining to optimize to avoid allocations for this revision.
> 
> 2. Add Reviewed-by from Eric.
> 
> 3. Split into several patches that are more single-purpose, which
>    highlights the individual fixes more clearly;
> 
>    - Prohibit BUSY or INCONSISTENT bitmaps from being merge sources.
> 
> 4. Declining feedback to prohibit disabling or enabling readonly bitmaps,
>    on the basis that users may wish to enable/disable them prior to
>    remounting their backing storage RW.
> 
>    Decided to prohibit attempting to remove readonly bitmaps, so the
>    failure happens earlier.
> 
>    Prohibit sync=incremental backups using readonly bitmaps, because
>    they're not capable of clearing the bitmap on success.
>    sync=differential would be acceptable here. (Good spot, Vladimir.)
> 
> John Snow (7):
>   block/dirty-bitmaps: add inconsistent bit
>   block/dirty-bitmap: add inconsistent status
>   block/dirty-bitmaps: add block_dirty_bitmap_check function
>   block/dirty-bitmaps: prohibit readonly bitmaps for backups
>   block/dirty-bitmaps: prohibit removing readonly bitmaps
>   block/dirty-bitmaps: disallow busy bitmaps as merge source
>   block/dirty-bitmaps: implement inconsistent bit
> 
>  qapi/block-core.json           |  20 +++++--
>  include/block/dirty-bitmap.h   |  15 ++++-
>  block/dirty-bitmap.c           |  63 +++++++++++++++++---
>  block/qcow2-bitmap.c           | 103 +++++++++++++++++----------------
>  blockdev.c                     |  50 ++++------------
>  migration/block-dirty-bitmap.c |  12 +---
>  nbd/server.c                   |   3 +-
>  7 files changed, 151 insertions(+), 115 deletions(-)
> 

Thanks for the reviews Eric, I have staged this on my bitmaps branch. I
realize this is hasty, but it's time to get patches in for release, and
it will make reviewing the next series earlier.

Vladimir, I realize I didn't take a few of your suggestions here so I am
delaying sending the PR for this a little bit to give you a chance to
object.

--js

Re: [Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
> that have been marked as "in use".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   qapi/block-core.json         | 13 +++++++++----
>   include/block/dirty-bitmap.h |  2 ++
>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>   3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6e543594b3..e639ef6d1c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -467,15 +467,20 @@
>   #        and cannot be modified via QMP or used by another operation.
>   #        Replaces `locked` and `frozen` statuses. (since 4.0)
>   #
> -# @persistent: true if the bitmap will eventually be flushed to persistent
> -#              storage (since 4.0)
> +# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
> +#              on disk, or both. (since 4.0)
> +#
> +# @inconsistent: true if this is a persistent bitmap that was improperly
> +#                stored. Implies @persistent to be true; @recording and
> +#                @busy to be false. This bitmap cannot be used. To remove
> +#                it, use @block-dirty-bitmap-remove. (Since 4.0)
>   #
>   # Since: 1.3
>   ##
>   { 'struct': 'BlockDirtyInfo',
>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> -           'recording': 'bool', 'busy': 'bool',
> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>   
>   ##
>   # @Qcow2BitmapInfoFlags:
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index ba8477b73f..bd1b6479df 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>                                          bool persistent);
> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                                HBitmap **backup, Error **errp);
> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 980cae4fa3..9e8630e1ac 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>                                      and this bitmap must remain unchanged while
>                                      this flag is set. */
>       bool persistent;            /* bitmap must be saved to owner disk image */
> +    bool inconsistent;          /* bitmap is persistent, but inconsistent.
> +                                 * It cannot be used at all in any way, except
> +                                 * a QMP user can remove it. */
>       bool migration;             /* Bitmap is selected for migration, it should
>                                      not be stored on the next inactivation
>                                      (persistent flag doesn't matter until next
> @@ -464,6 +467,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>           info->recording = bdrv_dirty_bitmap_recording(bm);
>           info->busy = bdrv_dirty_bitmap_busy(bm);
>           info->persistent = bm->persistent;
> +        info->has_inconsistent = bm->inconsistent;
> +        info->inconsistent = bm->inconsistent;
>           entry->value = info;
>           *plist = entry;
>           plist = &entry->next;
> @@ -711,6 +716,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>       qemu_mutex_unlock(bitmap->mutex);
>   }
>   
> +/* Called with BQL taken. */
> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
> +{
> +    qemu_mutex_lock(bitmap->mutex);
> +    bitmap->inconsistent = true;
> +    bitmap->disabled = true;

Agree, that it worth to assert persistance

> +    qemu_mutex_unlock(bitmap->mutex);
> +}
> +
>   /* Called with BQL taken. */
>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>   {
> @@ -724,6 +738,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>       return bitmap->persistent && !bitmap->migration;
>   }
>   
> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
> +{
> +    return bitmap->inconsistent;
> +}
> +
>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>   {
>       BdrvDirtyBitmap *bm;
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Sorry for a delay, I'm very busy with our rebase to 2.12 :(

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 2/7] block/dirty-bitmap: add inconsistent status

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> Even though the status field is deprecated, we still have to support
> it for a few more releases. Since this is a very new kind of bitmap
> state, it makes sense for it to have its own status field.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   qapi/block-core.json | 7 ++++++-
>   block/dirty-bitmap.c | 7 ++++++-
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e639ef6d1c..ae55cd0704 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -442,10 +442,15 @@
>   #          recording new writes. If the bitmap was @disabled, it is not
>   #          recording new writes. (Since 2.12)
>   #
> +# @inconsistent: This is a persistent dirty bitmap that was marked in-use on
> +#                disk, and is unusable by QEMU. It can only be deleted.
> +#                Please rely on the inconsistent field in @BlockDirtyInfo
> +#                instead, as the status field is deprecated. (Since 4.0)
> +#
>   # Since: 2.4
>   ##
>   { 'enum': 'DirtyBitmapStatus',
> -  'data': ['active', 'disabled', 'frozen', 'locked'] }
> +  'data': ['active', 'disabled', 'frozen', 'locked', 'inconsistent'] }
>   
>   ##
>   # @BlockDirtyInfo:
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 9e8630e1ac..71e0098396 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -209,10 +209,15 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>    *               or it can be Disabled and not recording writes.
>    * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
>    *               in any way from the monitor.
> + * (5) Inconsistent: This is a persistent bitmap whose "in use" bit is set, and

"was", like in qapi is better than "is", as while qemu running other (not inconsistent)
bitmaps' IN_USE bit is also set..

but I don't really care about deprecated comment:), anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> + *                   is unusable by QEMU. It can be deleted to remove it from
> + *                   the qcow2.
>    */
>   DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
>   {
> -    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
> +    if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +        return DIRTY_BITMAP_STATUS_INCONSISTENT;
> +    } else if (bdrv_dirty_bitmap_has_successor(bitmap)) {
>           return DIRTY_BITMAP_STATUS_FROZEN;
>       } else if (bdrv_dirty_bitmap_busy(bitmap)) {
>           return DIRTY_BITMAP_STATUS_LOCKED;
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

06.03.2019 15:25, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 22:15, John Snow wrote:
>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>> that have been marked as "in use".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   qapi/block-core.json         | 13 +++++++++----
>>   include/block/dirty-bitmap.h |  2 ++
>>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>   3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6e543594b3..e639ef6d1c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -467,15 +467,20 @@
>>   #        and cannot be modified via QMP or used by another operation.
>>   #        Replaces `locked` and `frozen` statuses. (since 4.0)
>>   #
>> -# @persistent: true if the bitmap will eventually be flushed to persistent
>> -#              storage (since 4.0)
>> +# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
>> +#              on disk, or both. (since 4.0)
>> +#
>> +# @inconsistent: true if this is a persistent bitmap that was improperly
>> +#                stored. Implies @persistent to be true; @recording and
>> +#                @busy to be false. This bitmap cannot be used. To remove
>> +#                it, use @block-dirty-bitmap-remove. (Since 4.0)
>>   #
>>   # Since: 1.3
>>   ##
>>   { 'struct': 'BlockDirtyInfo',
>>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>> -           'recording': 'bool', 'busy': 'bool',
>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>   ##
>>   # @Qcow2BitmapInfoFlags:
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index ba8477b73f..bd1b6479df 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>                                          bool persistent);
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>                                HBitmap **backup, Error **errp);
>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 980cae4fa3..9e8630e1ac 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>                                      and this bitmap must remain unchanged while
>>                                      this flag is set. */
>>       bool persistent;            /* bitmap must be saved to owner disk image */
>> +    bool inconsistent;          /* bitmap is persistent, but inconsistent.
>> +                                 * It cannot be used at all in any way, except
>> +                                 * a QMP user can remove it. */
>>       bool migration;             /* Bitmap is selected for migration, it should
>>                                      not be stored on the next inactivation
>>                                      (persistent flag doesn't matter until next
>> @@ -464,6 +467,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>           info->recording = bdrv_dirty_bitmap_recording(bm);
>>           info->busy = bdrv_dirty_bitmap_busy(bm);
>>           info->persistent = bm->persistent;
>> +        info->has_inconsistent = bm->inconsistent;
>> +        info->inconsistent = bm->inconsistent;
>>           entry->value = info;
>>           *plist = entry;
>>           plist = &entry->next;
>> @@ -711,6 +716,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>       qemu_mutex_unlock(bitmap->mutex);
>>   }
>> +/* Called with BQL taken. */
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    bitmap->inconsistent = true;
>> +    bitmap->disabled = true;
> 
> Agree, that it worth to assert persistance

Hmm, didn't we want to make it readonly too?

> 
>> +    qemu_mutex_unlock(bitmap->mutex);
>> +}
>> +
>>   /* Called with BQL taken. */
>>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>   {
>> @@ -724,6 +738,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>       return bitmap->persistent && !bitmap->migration;
>>   }
>> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->inconsistent;
>> +}
>> +
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>   {
>>       BdrvDirtyBitmap *bm;
>>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Sorry for a delay, I'm very busy with our rebase to 2.12 :(
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

06.03.2019 16:06, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 15:25, Vladimir Sementsov-Ogievskiy wrote:
>> 01.03.2019 22:15, John Snow wrote:
>>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>>> that have been marked as "in use".
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   qapi/block-core.json         | 13 +++++++++----
>>>   include/block/dirty-bitmap.h |  2 ++
>>>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>>   3 files changed, 30 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 6e543594b3..e639ef6d1c 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -467,15 +467,20 @@
>>>   #        and cannot be modified via QMP or used by another operation.
>>>   #        Replaces `locked` and `frozen` statuses. (since 4.0)
>>>   #
>>> -# @persistent: true if the bitmap will eventually be flushed to persistent
>>> -#              storage (since 4.0)
>>> +# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
>>> +#              on disk, or both. (since 4.0)
>>> +#
>>> +# @inconsistent: true if this is a persistent bitmap that was improperly
>>> +#                stored. Implies @persistent to be true; @recording and
>>> +#                @busy to be false. This bitmap cannot be used. To remove
>>> +#                it, use @block-dirty-bitmap-remove. (Since 4.0)
>>>   #
>>>   # Since: 1.3
>>>   ##
>>>   { 'struct': 'BlockDirtyInfo',
>>>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>>> -           'recording': 'bool', 'busy': 'bool',
>>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>>   ##
>>>   # @Qcow2BitmapInfoFlags:
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index ba8477b73f..bd1b6479df 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>>                                          bool persistent);
>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>                                HBitmap **backup, Error **errp);
>>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>>> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 980cae4fa3..9e8630e1ac 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>>                                      and this bitmap must remain unchanged while
>>>                                      this flag is set. */
>>>       bool persistent;            /* bitmap must be saved to owner disk image */
>>> +    bool inconsistent;          /* bitmap is persistent, but inconsistent.
>>> +                                 * It cannot be used at all in any way, except
>>> +                                 * a QMP user can remove it. */
>>>       bool migration;             /* Bitmap is selected for migration, it should
>>>                                      not be stored on the next inactivation
>>>                                      (persistent flag doesn't matter until next
>>> @@ -464,6 +467,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>>           info->recording = bdrv_dirty_bitmap_recording(bm);
>>>           info->busy = bdrv_dirty_bitmap_busy(bm);
>>>           info->persistent = bm->persistent;
>>> +        info->has_inconsistent = bm->inconsistent;
>>> +        info->inconsistent = bm->inconsistent;
>>>           entry->value = info;
>>>           *plist = entry;
>>>           plist = &entry->next;
>>> @@ -711,6 +716,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>>       qemu_mutex_unlock(bitmap->mutex);
>>>   }
>>> +/* Called with BQL taken. */
>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    qemu_mutex_lock(bitmap->mutex);
>>> +    bitmap->inconsistent = true;
>>> +    bitmap->disabled = true;
>>
>> Agree, that it worth to assert persistance
> 
> Hmm, didn't we want to make it readonly too?

May be better not doing it to not interfere these two things.

> 
>>
>>> +    qemu_mutex_unlock(bitmap->mutex);
>>> +}
>>> +
>>>   /* Called with BQL taken. */
>>>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>>   {
>>> @@ -724,6 +738,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>>       return bitmap->persistent && !bitmap->migration;
>>>   }
>>> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    return bitmap->inconsistent;
>>> +}
>>> +
>>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>>   {
>>>       BdrvDirtyBitmap *bm;
>>>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Sorry for a delay, I'm very busy with our rebase to 2.12 :(
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> Instead of checking against busy, inconsistent, or read only directly,
> use a check function with permissions bits that let us streamline the
> checks without reproducing them in many places.
> 
> Included in this patch are permissions changes that simply add the
> inconsistent check to existing permissions call spots, without
> addressing existing bugs.
> 
> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
> which checks against all three conditions. busy-only checks become
> BDRV_BITMAP_ALLOW_RO.

so, it's a conversion + check on inconsistance. backup/disable/enable correctness
postponed.

you've left bdrv_dirty_bitmap_create_successor as is..

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
> Notably, remove allows inconsistent bitmaps, so it doesn't follow the pattern.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   include/block/dirty-bitmap.h   | 13 ++++++++-
>   block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
>   blockdev.c                     | 49 +++++++---------------------------
>   migration/block-dirty-bitmap.c | 12 +++------
>   nbd/server.c                   |  3 +--
>   5 files changed, 54 insertions(+), 61 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index bd1b6479df..2a78243954 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -5,6 +5,16 @@
>   #include "qapi/qapi-types-block-core.h"
>   #include "qemu/hbitmap.h"
>   
> +typedef enum BitmapCheckFlags {
> +    BDRV_BITMAP_BUSY = 1,
> +    BDRV_BITMAP_RO = 2,
> +    BDRV_BITMAP_INCONSISTENT = 4,
> +} BitmapCheckFlags;
> +
> +#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO |        \
> +                             BDRV_BITMAP_INCONSISTENT)
> +#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
> +
>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                             uint32_t granularity,
>                                             const char *name,
> @@ -24,6 +34,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>   void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>                                           const char *name);
> +int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
> +                            Error **errp);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> @@ -93,7 +105,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>                                           BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 71e0098396..769668ccdc 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
>       return bitmap->successor;
>   }
>   
> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
> +static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap *bitmap) {
>       return bitmap->busy;
>   }
>   
> @@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
>                                    !bitmap->successor->disabled);
>   }
>   
> +int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
> +                            Error **errp)
> +{
> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
> +                   " operation and cannot be used", bitmap->name);
> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> +                   bitmap->name);
> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
> +                   bitmap->name);
> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
> +                          "this bitmap from disk");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>   /**
>    * Create a successor bitmap destined to replace this bitmap after an operation.
>    * Requires that the bitmap is not marked busy and has no successor.
> @@ -794,17 +821,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>   
>       qemu_mutex_lock(dest->mutex);
>   
> -    if (bdrv_dirty_bitmap_busy(dest)) {
> -        error_setg(errp, "Bitmap '%s' is currently in use by another"
> -        " operation and cannot be modified", dest->name);
> +    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>           goto out;
>       }
>   
> -    if (bdrv_dirty_bitmap_readonly(dest)) {
> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> -                   dest->name);
> -        goto out;
> -    }
>   
>       if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>           error_setg(errp, "Bitmaps are incompatible and can't be merged");
> diff --git a/blockdev.c b/blockdev.c
> index cbce44877d..5d74479ba7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2007,11 +2007,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
> -        error_setg(errp, "Cannot modify a bitmap in use by another operation");
> -        return;
> -    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
> -        error_setg(errp, "Cannot clear a readonly bitmap");
> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>           return;
>       }
>   
> @@ -2056,10 +2052,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be enabled", action->name);
> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }
>   
> @@ -2097,10 +2090,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be disabled", action->name);
> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }
>   
> @@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation and"
> -                   " cannot be removed", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
>           return;
>       }
>   
> @@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be cleared", name);
> -        return;
> -    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>           return;
>       }
>   
> @@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be enabled", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }
>   
> @@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be disabled", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }
>   
> @@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>               bdrv_unref(target_bs);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_busy(bmap)) {
> -            error_setg(errp,
> -                       "Bitmap '%s' is currently in use by another operation"
> -                       " and cannot be used for backup", backup->bitmap);
> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>               goto out;
>           }
>       }
> @@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
>               error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_busy(bmap)) {
> -            error_setg(errp,
> -                       "Bitmap '%s' is currently in use by another operation"
> -                       " and cannot be used for backup", backup->bitmap);
> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>               goto out;
>           }
>       }
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7057fff242..0fcf897f32 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
>       BdrvDirtyBitmap *bitmap;
>       DirtyBitmapMigBitmapState *dbms;
>       BdrvNextIterator it;
> +    Error *local_err = NULL;
>   
>       dirty_bitmap_mig_state.bulk_completed = false;
>       dirty_bitmap_mig_state.prev_bs = NULL;
> @@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
>                   goto fail;
>               }
>   
> -            if (bdrv_dirty_bitmap_busy(bitmap)) {
> -                error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
> -                             bdrv_dirty_bitmap_name(bitmap));
> -                goto fail;
> -            }
> -
> -            if (bdrv_dirty_bitmap_readonly(bitmap)) {
> -                error_report("Can't migrate read-only dirty bitmap: '%s",
> -                             bdrv_dirty_bitmap_name(bitmap));
> +            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
> +                error_report_err(local_err);
>                   goto fail;
>               }
>   
> diff --git a/nbd/server.c b/nbd/server.c
> index 02773e2836..9b87c7f243 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>               goto fail;
>           }
>   
> -        if (bdrv_dirty_bitmap_busy(bm)) {
> -            error_setg(errp, "Bitmap '%s' is in use", bitmap);
> +        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
>               goto fail;
>           }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 4/7] block/dirty-bitmaps: prohibit readonly bitmaps for backups

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> drive and blockdev backup cannot use readonly bitmaps, because the
> sync=incremental mechanism actually edits the bitmaps on success.
> 
> If you really want to do this operation, use a copied bitmap.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


> ---
>   blockdev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 5d74479ba7..c8255dda0b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3526,7 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>               bdrv_unref(target_bs);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
>               goto out;
>           }
>       }
> @@ -3636,7 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
>               error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
>               goto out;
>           }
>       }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 5/7] block/dirty-bitmaps: prohibit removing readonly bitmaps

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> Remove is an inherently RW operation, so this will fail anyway, but
> we can fail it very quickly instead of trying and failing, so do so.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


> ---
>   blockdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index c8255dda0b..a9a059c570 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2881,7 +2881,8 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
> +                                errp)) {
>           return;
>       }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[Vladimir Sementsov-Ogievskiy]

01.03.2019 23:04, John Snow wrote:
> 
> 
> On 3/1/19 2:57 PM, Eric Blake wrote:
>> On 3/1/19 1:48 PM, John Snow wrote:
>>
>>>> I understand forbidding inconsistent sources (because if the source is
>>>> potentially missing bits, then the merge destination will also be
>>>> missing bits and thus be inconsistent), but why forbid busy?  If I've
>>>> associated a bitmap with an NBD server (making it busy), it is still
>>>> readable, and so I should still be able to merge its bits into another copy.
>>>>
>>>
>>> True, do you rely on this, though?
>>
>> Not in my current libvirt code (as I create a temporary bitmap to hand
>> to NBD, since it may be the merge of one or more disabled bitmaps in a
>> differential backup case), so being tighter for now and relaxing later
>> if we DO come up with a use is acceptable.
>>
>>>
>>> I was working from a space of "busy" meant "actively in-use by an
>>> operation, and COULD change" so I was forbidding it out of good hygiene.
>>>
>>> Clearly the ones in-use by NBD are actually static and unchanging, so
>>> it's safer -- but that might not be true for push backups, where you
>>> might not actually be getting what you think you are, because of the
>>> bifurcated nature of those bitmaps.
>>
>> Oh, good point, especially after you worked so hard to merge
>> locked/frozen into a single status - you WILL miss the bits from the
>> successor (unless we teach the merge algorithm to pull in the busy
>> bitmap's bits AND all the bits of its successors - but that feels like a
>> lot of work if we don't have a client needing it now).  Okay, with the
>> extra justification mentioned in the commit message,
>>
> 
> (Though I am being a little fast and loose here: when we split a bitmap,
> the top-level one that retains the name actually stays unchanging and
> the child bitmap is the one that starts accruing writes from a blank
> canvas, but that STILL may not be what you expect when you choose it as
> a merge source, however.)
> 
>>>
>>> If this causes a problem for you in the short-term I will simply roll
>>> this back, but it stands out to me.
>>>
>>> (I can't stop myself from trying to protect the user from themselves.
>>> It's clearly a recurring theme in my design and reviews.)
>>>
>>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>>> index 769668ccdc..8403c9981d 100644
>>>>> --- a/block/dirty-bitmap.c
>>>>> +++ b/block/dirty-bitmap.c
>>>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>>>           goto out;
>>>>>       }
>>>>>   
>>>>> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>>
>>>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT.
>>
>> then I retract my complaint, and the code is acceptable for now.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> 
> We could always split it back out later, but in basic terms for
> permissions and user perspective, "in use" seems robust enough of a
> resolution. (It might be safe to read, it might not be, who knows --
> it's in use.)

I think, if we need some kind of sharing busy bitmaps, it should be two
busy states: one, which allows reads for other users and one that doesn't.

> 
> If it really comes to a point, we can always re-add a new status bit to
> let the end-user know if they're working with a bifurcated (I have to
> use weird vocabulary words sometimes) bitmap but at the moment it seems
> very safely an implementation detail.
> 
> You can also check that for "enabled" bitmap as reported back to user
> via QAPI I check to see if the parent OR child is enabled and report
> that cumulatively as "enabled", because together they are "effectively"
> enabled.
> 
> --js
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> We didn't do any state checking on source bitmaps at all,
> so this adds inconsistent and busy checks. readonly is
> allowed, so you can still copy a readonly bitmap to a new
> destination to use it for operations like drive-backup.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/dirty-bitmap.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 769668ccdc..8403c9981d 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>           goto out;
>       }
>   
> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
> +        goto out;
> +    }
>   
>       if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>           error_setg(errp, "Bitmaps are incompatible and can't be merged");
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, John Snow wrote:
> Set the inconsistent bit on load instead of rejecting such bitmaps.
> There is no way to un-set it; the only option is to delete it.

I think, s/delete it/delete the bitmap/, as "it" is the bit after "un-set it".

> 
> Obvervations:
> - bitmap loading does not need to update the header for in_use bitmaps.
> - inconsistent bitmaps don't need to have their data loaded; they're
>    glorified corruption sentinels.
> - bitmap saving does not need to save inconsistent bitmaps back to disk.
> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>    bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>    being eventually flushed back to disk.

hmm, and inconsitent bitmaps don't have this flag, isn't it?

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 103 ++++++++++++++++++++++---------------------
>   1 file changed, 53 insertions(+), 50 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3ee524da4b..c3b210ede1 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c

[..]

> @@ -962,35 +963,39 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       }
>   
>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> -            if (bitmap == NULL) {
> -                goto fail;
> -            }
> -
> -            if (!(bm->flags & BME_FLAG_AUTO)) {
> -                bdrv_disable_dirty_bitmap(bitmap);
> -            }
> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
> -            bm->flags |= BME_FLAG_IN_USE;
> -            created_dirty_bitmaps =
> -                    g_slist_append(created_dirty_bitmaps, bitmap);
> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> +        if (bitmap == NULL) {
> +            goto fail;
>           }
> -    }
>   
> -    if (created_dirty_bitmaps != NULL) {
> -        if (can_write(bs)) {
> -            /* in_use flags must be updated */
> -            int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> -            if (ret < 0) {
> -                error_setg_errno(errp, -ret, "Can't update bitmap directory");
> -                goto fail;
> -            }
> -            header_updated = true;
> +        if (bm->flags & BME_FLAG_IN_USE) {
> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
>           } else {
> -            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
> -                            (gpointer)true);
> +            /* NB: updated flags only get written if can_write(bs) is true. */
> +            bm->flags |= BME_FLAG_IN_USE;
> +            needs_update = true;
>           }
> +        if (!(bm->flags & BME_FLAG_AUTO)) {
> +            bdrv_disable_dirty_bitmap(bitmap);
> +        }
> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
> +        created_dirty_bitmaps =
> +            g_slist_append(created_dirty_bitmaps, bitmap);
> +    }
> +
> +    if (needs_update && can_write(bs)) {
> +        /* in_use flags must be updated */
> +        int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Can't update bitmap directory");
> +            goto fail;
> +        }
> +        header_updated = true;
> +    }
> +
> +    if (!can_write(bs)) {
> +        g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
> +                        (gpointer)true);

hmmm..

I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write,
as we are not going to write something related to inconsistent bitmaps.

And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly.

Readonly are bitmaps, which are not marked IN_USE on open for some reason..

I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them..
But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is
don't mark inconsistent bitmaps as readonly.

>       }
>   
>       g_slist_free(created_dirty_bitmaps);
> @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>       }
>   
>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
> -            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> -            if (bitmap == NULL) {
> -                continue;
> -            }
> -
> -            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> -                error_setg(errp, "Bitmap %s is not readonly but not marked"
> -                                 "'IN_USE' in the image. Something went wrong,"
> -                                 "all the bitmaps may be corrupted", bm->name);
> -                ret = -EINVAL;
> -                goto out;
> -            }
> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> +        if (bitmap == NULL) {
> +            continue;
> +        }
>   
> -            bm->flags |= BME_FLAG_IN_USE;
> -            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
> +        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> +            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
> +                       "not marked as readonly. This is a bug, something went "
> +                       "wrong. All of the bitmaps may be corrupted", bm->name);
> +            ret = -EINVAL;
> +            goto out;
>           }
> +
> +        bm->flags |= BME_FLAG_IN_USE;
> +        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>       }
>   
>       if (ro_dirty_bitmaps != NULL) {
> @@ -1424,8 +1427,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           Qcow2Bitmap *bm;
>   
>           if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
> -            bdrv_dirty_bitmap_readonly(bitmap))
> -        {
> +            bdrv_dirty_bitmap_readonly(bitmap) ||
> +            bdrv_dirty_bitmap_inconsistent(bitmap)) {
>               continue;
>           }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 2/7] block/dirty-bitmap: add inconsistent status

[John Snow]

On 3/6/19 8:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 22:15, John Snow wrote:
>> Even though the status field is deprecated, we still have to support
>> it for a few more releases. Since this is a very new kind of bitmap
>> state, it makes sense for it to have its own status field.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   qapi/block-core.json | 7 ++++++-
>>   block/dirty-bitmap.c | 7 ++++++-
>>   2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e639ef6d1c..ae55cd0704 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -442,10 +442,15 @@
>>   #          recording new writes. If the bitmap was @disabled, it is not
>>   #          recording new writes. (Since 2.12)
>>   #
>> +# @inconsistent: This is a persistent dirty bitmap that was marked in-use on
>> +#                disk, and is unusable by QEMU. It can only be deleted.
>> +#                Please rely on the inconsistent field in @BlockDirtyInfo
>> +#                instead, as the status field is deprecated. (Since 4.0)
>> +#
>>   # Since: 2.4
>>   ##
>>   { 'enum': 'DirtyBitmapStatus',
>> -  'data': ['active', 'disabled', 'frozen', 'locked'] }
>> +  'data': ['active', 'disabled', 'frozen', 'locked', 'inconsistent'] }
>>   
>>   ##
>>   # @BlockDirtyInfo:
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 9e8630e1ac..71e0098396 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -209,10 +209,15 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>>    *               or it can be Disabled and not recording writes.
>>    * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
>>    *               in any way from the monitor.
>> + * (5) Inconsistent: This is a persistent bitmap whose "in use" bit is set, and
> 
> "was", like in qapi is better than "is", as while qemu running other (not inconsistent)
> bitmaps' IN_USE bit is also set..
> 
> but I don't really care about deprecated comment:), anyway:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Hm, this is a weird English thing. "Was set" references some event in
the past but doesn't necessarily imply that it is still set. "is set"
implies that it is still currently set, which I think is correct.

But, I don't care much about the deprecated comment either :)

Re: [Qemu-devel] [PATCH v3 1/7] block/dirty-bitmaps: add inconsistent bit

[John Snow]

On 3/6/19 7:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 22:15, John Snow wrote:
>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>> that have been marked as "in use".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   qapi/block-core.json         | 13 +++++++++----
>>   include/block/dirty-bitmap.h |  2 ++
>>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>   3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6e543594b3..e639ef6d1c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -467,15 +467,20 @@
>>   #        and cannot be modified via QMP or used by another operation.
>>   #        Replaces `locked` and `frozen` statuses. (since 4.0)
>>   #
>> -# @persistent: true if the bitmap will eventually be flushed to persistent
>> -#              storage (since 4.0)
>> +# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
>> +#              on disk, or both. (since 4.0)
>> +#
>> +# @inconsistent: true if this is a persistent bitmap that was improperly
>> +#                stored. Implies @persistent to be true; @recording and
>> +#                @busy to be false. This bitmap cannot be used. To remove
>> +#                it, use @block-dirty-bitmap-remove. (Since 4.0)
>>   #
>>   # Since: 1.3
>>   ##
>>   { 'struct': 'BlockDirtyInfo',
>>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>> -           'recording': 'bool', 'busy': 'bool',
>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>   
>>   ##
>>   # @Qcow2BitmapInfoFlags:
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index ba8477b73f..bd1b6479df 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>                                          bool persistent);
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>                                HBitmap **backup, Error **errp);
>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 980cae4fa3..9e8630e1ac 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>                                      and this bitmap must remain unchanged while
>>                                      this flag is set. */
>>       bool persistent;            /* bitmap must be saved to owner disk image */
>> +    bool inconsistent;          /* bitmap is persistent, but inconsistent.
>> +                                 * It cannot be used at all in any way, except
>> +                                 * a QMP user can remove it. */
>>       bool migration;             /* Bitmap is selected for migration, it should
>>                                      not be stored on the next inactivation
>>                                      (persistent flag doesn't matter until next
>> @@ -464,6 +467,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>           info->recording = bdrv_dirty_bitmap_recording(bm);
>>           info->busy = bdrv_dirty_bitmap_busy(bm);
>>           info->persistent = bm->persistent;
>> +        info->has_inconsistent = bm->inconsistent;
>> +        info->inconsistent = bm->inconsistent;
>>           entry->value = info;
>>           *plist = entry;
>>           plist = &entry->next;
>> @@ -711,6 +716,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>       qemu_mutex_unlock(bitmap->mutex);
>>   }
>>   
>> +/* Called with BQL taken. */
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    bitmap->inconsistent = true;
>> +    bitmap->disabled = true;
> 
> Agree, that it worth to assert persistance
> 

OK, I will do so.

>> +    qemu_mutex_unlock(bitmap->mutex);
>> +}
>> +
>>   /* Called with BQL taken. */
>>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>   {
>> @@ -724,6 +738,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>       return bitmap->persistent && !bitmap->migration;
>>   }
>>   
>> +bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->inconsistent;
>> +}
>> +
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>   {
>>       BdrvDirtyBitmap *bm;
>>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Sorry for a delay, I'm very busy with our rebase to 2.12 :(
> 

I am very genuinely the last person you would ever have to apologize for
in being late. I'm only waiting for your reviews because you have
contributed so much to this feature and I want to make sure I don't
check in anything that you haven't had a chance to critique.

Thanks for the reviews!

Re: [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[John Snow]

On 3/6/19 8:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 22:15, John Snow wrote:
>> Instead of checking against busy, inconsistent, or read only directly,
>> use a check function with permissions bits that let us streamline the
>> checks without reproducing them in many places.
>>
>> Included in this patch are permissions changes that simply add the
>> inconsistent check to existing permissions call spots, without
>> addressing existing bugs.
>>
>> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
>> which checks against all three conditions. busy-only checks become
>> BDRV_BITMAP_ALLOW_RO.
> 
> so, it's a conversion + check on inconsistance. backup/disable/enable correctness
> postponed.
> 
> you've left bdrv_dirty_bitmap_create_successor as is..
> 

... That's an oversight! Will fix.

> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[John Snow]

On 3/6/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 23:04, John Snow wrote:
>>
>>
>> On 3/1/19 2:57 PM, Eric Blake wrote:
>>> On 3/1/19 1:48 PM, John Snow wrote:
>>>
>>>>> I understand forbidding inconsistent sources (because if the source is
>>>>> potentially missing bits, then the merge destination will also be
>>>>> missing bits and thus be inconsistent), but why forbid busy?  If I've
>>>>> associated a bitmap with an NBD server (making it busy), it is still
>>>>> readable, and so I should still be able to merge its bits into another copy.
>>>>>
>>>>
>>>> True, do you rely on this, though?
>>>
>>> Not in my current libvirt code (as I create a temporary bitmap to hand
>>> to NBD, since it may be the merge of one or more disabled bitmaps in a
>>> differential backup case), so being tighter for now and relaxing later
>>> if we DO come up with a use is acceptable.
>>>
>>>>
>>>> I was working from a space of "busy" meant "actively in-use by an
>>>> operation, and COULD change" so I was forbidding it out of good hygiene.
>>>>
>>>> Clearly the ones in-use by NBD are actually static and unchanging, so
>>>> it's safer -- but that might not be true for push backups, where you
>>>> might not actually be getting what you think you are, because of the
>>>> bifurcated nature of those bitmaps.
>>>
>>> Oh, good point, especially after you worked so hard to merge
>>> locked/frozen into a single status - you WILL miss the bits from the
>>> successor (unless we teach the merge algorithm to pull in the busy
>>> bitmap's bits AND all the bits of its successors - but that feels like a
>>> lot of work if we don't have a client needing it now).  Okay, with the
>>> extra justification mentioned in the commit message,
>>>
>>
>> (Though I am being a little fast and loose here: when we split a bitmap,
>> the top-level one that retains the name actually stays unchanging and
>> the child bitmap is the one that starts accruing writes from a blank
>> canvas, but that STILL may not be what you expect when you choose it as
>> a merge source, however.)
>>
>>>>
>>>> If this causes a problem for you in the short-term I will simply roll
>>>> this back, but it stands out to me.
>>>>
>>>> (I can't stop myself from trying to protect the user from themselves.
>>>> It's clearly a recurring theme in my design and reviews.)
>>>>
>>>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>>>> index 769668ccdc..8403c9981d 100644
>>>>>> --- a/block/dirty-bitmap.c
>>>>>> +++ b/block/dirty-bitmap.c
>>>>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>>>>           goto out;
>>>>>>       }
>>>>>>   
>>>>>> +    if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>>>
>>>>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT.
>>>
>>> then I retract my complaint, and the code is acceptable for now.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>
>> We could always split it back out later, but in basic terms for
>> permissions and user perspective, "in use" seems robust enough of a
>> resolution. (It might be safe to read, it might not be, who knows --
>> it's in use.)
> 
> I think, if we need some kind of sharing busy bitmaps, it should be two
> busy states: one, which allows reads for other users and one that doesn't.
> 

I think that's... hopefully premature. We don't need this NOW do we?

I think this is OK as-is.

Re: [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source

[Eric Blake]

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

On 3/6/19 9:24 AM, John Snow wrote:

>>>
>>> We could always split it back out later, but in basic terms for
>>> permissions and user perspective, "in use" seems robust enough of a
>>> resolution. (It might be safe to read, it might not be, who knows --
>>> it's in use.)
>>
>> I think, if we need some kind of sharing busy bitmaps, it should be two
>> busy states: one, which allows reads for other users and one that doesn't.
>>

Indeed, similar to flock having shared (multiple read-only users) and
exclusive (single writer) locks.  So it's nice to know we have a
technical plan for growth, if we have a reason to use that plan.

> 
> I think that's... hopefully premature. We don't need this NOW do we?

No, we don't need it now.

> 
> I think this is OK as-is.

It sounds like we are in violent agreement ;)

-- 
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 v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[John Snow]

On 3/6/19 9:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 22:15, John Snow wrote:
>> Set the inconsistent bit on load instead of rejecting such bitmaps.
>> There is no way to un-set it; the only option is to delete it.
> 
> I think, s/delete it/delete the bitmap/, as "it" is the bit after "un-set it".
> 

You are absolutely correct.

Don't let anyone fool you into thinking that Americans know English.

>>
>> Obvervations:
>> - bitmap loading does not need to update the header for in_use bitmaps.
>> - inconsistent bitmaps don't need to have their data loaded; they're
>>    glorified corruption sentinels.
>> - bitmap saving does not need to save inconsistent bitmaps back to disk.
>> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>>    bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>>    being eventually flushed back to disk.
> 
> hmm, and inconsitent bitmaps don't have this flag, isn't it?
> 

I didn't make any effort to treat them differently -- the readonly flag
only gets applied when the image is actually read only. I felt it was
conceptually simpler that way.

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 103 ++++++++++++++++++++++---------------------
>>   1 file changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524da4b..c3b210ede1 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
> 
> [..]
> 
>> @@ -962,35 +963,39 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>       }
>>   
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>> -            if (bitmap == NULL) {
>> -                goto fail;
>> -            }
>> -
>> -            if (!(bm->flags & BME_FLAG_AUTO)) {
>> -                bdrv_disable_dirty_bitmap(bitmap);
>> -            }
>> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> -            bm->flags |= BME_FLAG_IN_USE;
>> -            created_dirty_bitmaps =
>> -                    g_slist_append(created_dirty_bitmaps, bitmap);
>> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>> +        if (bitmap == NULL) {
>> +            goto fail;
>>           }
>> -    }
>>   
>> -    if (created_dirty_bitmaps != NULL) {
>> -        if (can_write(bs)) {
>> -            /* in_use flags must be updated */
>> -            int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> -            if (ret < 0) {
>> -                error_setg_errno(errp, -ret, "Can't update bitmap directory");
>> -                goto fail;
>> -            }
>> -            header_updated = true;
>> +        if (bm->flags & BME_FLAG_IN_USE) {
>> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
>>           } else {
>> -            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
>> -                            (gpointer)true);
>> +            /* NB: updated flags only get written if can_write(bs) is true. */
>> +            bm->flags |= BME_FLAG_IN_USE;
>> +            needs_update = true;
>>           }
>> +        if (!(bm->flags & BME_FLAG_AUTO)) {
>> +            bdrv_disable_dirty_bitmap(bitmap);
>> +        }
>> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> +        created_dirty_bitmaps =
>> +            g_slist_append(created_dirty_bitmaps, bitmap);
>> +    }
>> +
>> +    if (needs_update && can_write(bs)) {
>> +        /* in_use flags must be updated */
>> +        int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> +        if (ret < 0) {
>> +            error_setg_errno(errp, -ret, "Can't update bitmap directory");
>> +            goto fail;
>> +        }
>> +        header_updated = true;
>> +    }
>> +
>> +    if (!can_write(bs)) {
>> +        g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
>> +                        (gpointer)true);
> 
> hmmm..
> 
> I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write,
> as we are not going to write something related to inconsistent bitmaps.
> 

We'll never change metadata for inconsistent bitmaps, no. We might
delete them which causes a metadata change, though -- and right now the
readonly check stops that in blockdev.c before we attempt (and fail) the
operation. It does have at least a light usage.

It's not a crucial feature, but I think the error message is nicer.

> And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly.
> 

The way I think of it is as unrelated, which is why I set readonly and
inconsistent orthogonally.

> Readonly are bitmaps, which are not marked IN_USE on open for some reason..
>

I tend to think of readonly bitmaps as persistent bitmaps attached to
storage we are unable to write back to, so we must prevent their
modification.

I suppose the IN_USE viewpoint works too; "This is a bitmap that we have
not marked as IN_USE so it must not be used."

> I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them..
> But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is
> don't mark inconsistent bitmaps as readonly.
> 

Hm, so in your model; we just NEVER set readonly for persistent bitmaps,
which incurs some changes at load time, but allows the reload_rw
function to go completely unmodified.

That's not unreasonable in terms of SLOC, but semantically I am not sure
which approach is better. I'm leaning towards keeping this as written
for now, but... well. Any thoughts, Eric? Would you like to flip a coin?

--js

Re: [Qemu-devel] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[Eric Blake]

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

On 3/6/19 3:46 PM, John Snow wrote:

>> I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write,
>> as we are not going to write something related to inconsistent bitmaps.
>>
> 
> We'll never change metadata for inconsistent bitmaps, no. We might
> delete them which causes a metadata change, though -- and right now the
> readonly check stops that in blockdev.c before we attempt (and fail) the
> operation. It does have at least a light usage.
> 
> It's not a crucial feature, but I think the error message is nicer.
> 
>> And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly.
>>
> 
> The way I think of it is as unrelated, which is why I set readonly and
> inconsistent orthogonally.
> 
>> Readonly are bitmaps, which are not marked IN_USE on open for some reason..
>>
> 
> I tend to think of readonly bitmaps as persistent bitmaps attached to
> storage we are unable to write back to, so we must prevent their
> modification.

I like that wording (keeping read-only and inconsistent orthogonal),

> 
> I suppose the IN_USE viewpoint works too; "This is a bitmap that we have
> not marked as IN_USE so it must not be used."
> 

Yes, it does work, but it is a bit more of a stretch, so I'm not quite
as sure about using it.

>> I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them..
>> But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is
>> don't mark inconsistent bitmaps as readonly.
>>
> 
> Hm, so in your model; we just NEVER set readonly for persistent bitmaps,
> which incurs some changes at load time, but allows the reload_rw
> function to go completely unmodified.
> 
> That's not unreasonable in terms of SLOC, but semantically I am not sure
> which approach is better. I'm leaning towards keeping this as written
> for now, but... well. Any thoughts, Eric? Would you like to flip a coin?

No strong preferences, other than let's get it in before soft freeze, to
make sure we don't miss out on it entirely due to waiting for the coin
flip :)


-- 
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 v3 7/7] block/dirty-bitmaps: implement inconsistent bit

[John Snow]

On 3/7/19 11:37 AM, Eric Blake wrote:
> On 3/6/19 3:46 PM, John Snow wrote:
> 
>>> I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write,
>>> as we are not going to write something related to inconsistent bitmaps.
>>>
>>
>> We'll never change metadata for inconsistent bitmaps, no. We might
>> delete them which causes a metadata change, though -- and right now the
>> readonly check stops that in blockdev.c before we attempt (and fail) the
>> operation. It does have at least a light usage.
>>
>> It's not a crucial feature, but I think the error message is nicer.
>>
>>> And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly.
>>>
>>
>> The way I think of it is as unrelated, which is why I set readonly and
>> inconsistent orthogonally.
>>
>>> Readonly are bitmaps, which are not marked IN_USE on open for some reason..
>>>
>>
>> I tend to think of readonly bitmaps as persistent bitmaps attached to
>> storage we are unable to write back to, so we must prevent their
>> modification.
> 
> I like that wording (keeping read-only and inconsistent orthogonal),
> 
>>
>> I suppose the IN_USE viewpoint works too; "This is a bitmap that we have
>> not marked as IN_USE so it must not be used."
>>
> 
> Yes, it does work, but it is a bit more of a stretch, so I'm not quite
> as sure about using it.
> 
>>> I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them..
>>> But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is
>>> don't mark inconsistent bitmaps as readonly.
>>>
>>
>> Hm, so in your model; we just NEVER set readonly for persistent bitmaps,
>> which incurs some changes at load time, but allows the reload_rw
>> function to go completely unmodified.
>>
>> That's not unreasonable in terms of SLOC, but semantically I am not sure
>> which approach is better. I'm leaning towards keeping this as written
>> for now, but... well. Any thoughts, Eric? Would you like to flip a coin?
> 
> No strong preferences, other than let's get it in before soft freeze, to
> make sure we don't miss out on it entirely due to waiting for the coin
> flip :)
> 
> 

OK. I have a slight preference for keeping the flag meanings orthogonal
and logically consistent, because I believe it keeps the code less
surprising to anyone who isn't Eric, Vlad, or myself.

So I think I will stage this as-is, and if it poses a problem I will
accept counter-proposals during freeze to shore it up if necessary.

--js