[Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[John Snow]

based on: jsnow/bitmaps staging branch

This series builds on a previous standalone patch and adjusts
the permission for all (or most) of the QMP bitmap commands.

V4:
 - Replace "in-use" with "in use"
 - Replace "user_modifiable" version with "user_locked"
 - Remove more usages of frozen-and-or-locked in NBD.

John Snow (6):
  block/dirty-bitmaps: add user_locked status checker
  block/dirty-bitmaps: fix merge permissions
  block/dirty-bitmaps: allow clear on disabled bitmaps
  block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
  block/backup: prohibit backup from using in use bitmaps
  nbd: forbid use of frozen bitmaps

 block/dirty-bitmap.c           | 13 +++++---
 blockdev.c                     | 75 +++++++++++++++++++-----------------------
 include/block/dirty-bitmap.h   |  1 +
 migration/block-dirty-bitmap.c | 10 ++----
 nbd/server.c                   |  4 +--
 5 files changed, 48 insertions(+), 55 deletions(-)

-- 
2.14.4

[Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[John Snow]

Instead of both frozen and qmp_locked checks, wrap it into one check.
frozen implies the bitmap is split in two (for backup), and shouldn't
be modified. qmp_locked implies it's being used by another operation,
like being exported over NBD. In both cases it means we shouldn't allow
the user to modify it in any meaningful way.

Replace any usages where we check both frozen and qmp_locked with the
new check.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c           |  6 ++++++
 blockdev.c                     | 29 ++++++++---------------------
 include/block/dirty-bitmap.h   |  1 +
 migration/block-dirty-bitmap.c | 10 ++--------
 4 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ac933cf1c..85bc668f6a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
     return bitmap->successor;
 }
 
+/* Both conditions disallow user-modification via QMP. */
+bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
+    return (bdrv_dirty_bitmap_frozen(bitmap) ||
+            bdrv_dirty_bitmap_qmp_locked(bitmap));
+}
+
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
 {
     qemu_mutex_lock(bitmap->mutex);
diff --git a/blockdev.c b/blockdev.c
index 670ae5bbde..d775f228fe 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2009,11 +2009,8 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
-        error_setg(errp, "Cannot modify a frozen bitmap");
-        return;
-    } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
-        error_setg(errp, "Cannot modify a locked bitmap");
+    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+        error_setg(errp, "Cannot modify a bitmap in use by another operation");
         return;
     } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
         error_setg(errp, "Cannot clear a disabled bitmap");
@@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
         error_setg(errp,
-                   "Bitmap '%s' is currently frozen and cannot be removed",
-                   name);
-        return;
-    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently locked and cannot be removed",
-                   name);
+                   "Bitmap '%s' is currently in use by another operation and"
+                   " cannot be removed", name);
         return;
     }
 
@@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
         error_setg(errp,
-                   "Bitmap '%s' is currently frozen and cannot be modified",
-                   name);
-        return;
-    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently locked and cannot be modified",
-                   name);
+                   "Bitmap '%s' is currently in use by another operation"
+                   " and cannot be cleared", name);
         return;
     } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
         error_setg(errp,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 201ff7f20b..14639439a2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -94,6 +94,7 @@ 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_qmp_locked(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
                                         BdrvDirtyBitmap *bitmap);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 477826330c..d49c581023 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void)
                 goto fail;
             }
 
-            if (bdrv_dirty_bitmap_frozen(bitmap)) {
-                error_report("Can't migrate frozen dirty bitmap: '%s",
-                             bdrv_dirty_bitmap_name(bitmap));
-                goto fail;
-            }
-
-            if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-                error_report("Can't migrate locked dirty bitmap: '%s",
+            if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+                error_report("Can't migrate a bitmap that is in use: '%s'",
                              bdrv_dirty_bitmap_name(bitmap));
                 goto fail;
             }
-- 
2.14.4

[Qemu-devel] [PATCH v4 2/6] block/dirty-bitmaps: fix merge permissions

[John Snow]

In prior commits that made merge transactionable, we removed the
assertion that merge cannot operate on disabled bitmaps. In addition,
we want to make sure that we are prohibiting merges to "locked" bitmaps.

Use the new user_locked function to check.

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

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 85bc668f6a..8a6e07930f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
     qemu_mutex_lock(dest->mutex);
 
-    if (bdrv_dirty_bitmap_frozen(dest)) {
-        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
-                   dest->name);
+    if (bdrv_dirty_bitmap_user_locked(dest)) {
+        error_setg(errp, "Bitmap '%s' is currently in use by another"
+        " operation and cannot be modified", dest->name);
         goto out;
     }
 
-- 
2.14.4

[Qemu-devel] [PATCH v4 3/6] block/dirty-bitmaps: allow clear on disabled bitmaps

[John Snow]

Similarly to merge, it's OK to allow clear operations on disabled
bitmaps, as this condition only means that they are not recording
new writes. We are free to clear it if the user requests it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c | 1 -
 blockdev.c           | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8a6e07930f..035f97e5c2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -625,7 +625,6 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     bdrv_dirty_bitmap_lock(bitmap);
     if (!out) {
diff --git a/blockdev.c b/blockdev.c
index d775f228fe..cccd36b5e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2012,9 +2012,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
     if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
         error_setg(errp, "Cannot modify a bitmap in use by another operation");
         return;
-    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
-        error_setg(errp, "Cannot clear a disabled bitmap");
-        return;
     } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
         error_setg(errp, "Cannot clear a readonly bitmap");
         return;
@@ -2917,11 +2914,6 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
                    "Bitmap '%s' is currently in use by another operation"
                    " and cannot be cleared", name);
         return;
-    } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently disabled 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);
         return;
-- 
2.14.4

[Qemu-devel] [PATCH v4 4/6] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps

[John Snow]

We're not being consistent about this. If it's in use by an operation,
the user should not be able to change the behavior of that bitmap.

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

diff --git a/blockdev.c b/blockdev.c
index cccd36b5e7..d0febfca79 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2058,6 +2058,13 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
         return;
     }
 
+    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+        error_setg(errp,
+                   "Bitmap '%s' is currently in use by another operation"
+                   " and cannot be enabled", action->name);
+        return;
+    }
+
     state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
     bdrv_enable_dirty_bitmap(state->bitmap);
 }
@@ -2092,6 +2099,13 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
         return;
     }
 
+    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+        error_setg(errp,
+                   "Bitmap '%s' is currently in use by another operation"
+                   " and cannot be disabled", action->name);
+        return;
+    }
+
     state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
     bdrv_disable_dirty_bitmap(state->bitmap);
 }
@@ -2933,10 +2947,10 @@ void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
         error_setg(errp,
-                   "Bitmap '%s' is currently frozen and cannot be enabled",
-                   name);
+                   "Bitmap '%s' is currently in use by another operation"
+                   " and cannot be enabled", name);
         return;
     }
 
@@ -2954,10 +2968,10 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
         error_setg(errp,
-                   "Bitmap '%s' is currently frozen and cannot be disabled",
-                   name);
+                   "Bitmap '%s' is currently in use by another operation"
+                   " and cannot be disabled", name);
         return;
     }
 
-- 
2.14.4

[Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[John Snow]

If the bitmap is frozen, we shouldn't touch it.

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

diff --git a/blockdev.c b/blockdev.c
index d0febfca79..098d4c337f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
             bdrv_unref(target_bs);
             goto out;
         }
-        if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
+        if (bdrv_dirty_bitmap_user_locked(bmap)) {
             error_setg(errp,
-                       "Bitmap '%s' is currently locked and cannot be used for "
-                       "backup", backup->bitmap);
+                       "Bitmap '%s' is currently in use by another operation"
+                       " and cannot be used for backup", backup->bitmap);
             goto out;
         }
     }
@@ -3620,10 +3620,10 @@ 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_qmp_locked(bmap)) {
+        if (bdrv_dirty_bitmap_user_locked(bmap)) {
             error_setg(errp,
-                       "Bitmap '%s' is currently locked and cannot be used for "
-                       "backup", backup->bitmap);
+                       "Bitmap '%s' is currently in use by another operation"
+                       " and cannot be used for backup", backup->bitmap);
             goto out;
         }
     }
-- 
2.14.4

[Qemu-devel] [PATCH v4 6/6] nbd: forbid use of frozen bitmaps

[John Snow]

Whether it's "locked" or "frozen", it's in use and should
not be allowed for the purposes of this operation.

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

diff --git a/nbd/server.c b/nbd/server.c
index c3dd402b45..940b7aa0cd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2478,8 +2478,8 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
         return;
     }
 
-    if (bdrv_dirty_bitmap_qmp_locked(bm)) {
-        error_setg(errp, "Bitmap '%s' is locked", bitmap);
+    if (bdrv_dirty_bitmap_user_locked(bm)) {
+        error_setg(errp, "Bitmap '%s' is in use", bitmap);
         return;
     }
 
-- 
2.14.4

Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[John Snow]

On 10/02/2018 07:02 PM, John Snow wrote:
> based on: jsnow/bitmaps staging branch
> 
> This series builds on a previous standalone patch and adjusts
> the permission for all (or most) of the QMP bitmap commands.
> 
> V4:
>  - Replace "in-use" with "in use"
>  - Replace "user_modifiable" version with "user_locked"
>  - Remove more usages of frozen-and-or-locked in NBD.
> 
> John Snow (6):
>   block/dirty-bitmaps: add user_locked status checker
>   block/dirty-bitmaps: fix merge permissions
>   block/dirty-bitmaps: allow clear on disabled bitmaps
>   block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>   block/backup: prohibit backup from using in use bitmaps
>   nbd: forbid use of frozen bitmaps
> 
>  block/dirty-bitmap.c           | 13 +++++---
>  blockdev.c                     | 75 +++++++++++++++++++-----------------------
>  include/block/dirty-bitmap.h   |  1 +
>  migration/block-dirty-bitmap.c | 10 ++----
>  nbd/server.c                   |  4 +--
>  5 files changed, 48 insertions(+), 55 deletions(-)
> 

Planning on submitting tests for 3.1, but with the merge queue opened up
again I'm pretty keen on getting some stable commit IDs for these so far.

--js

Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> Instead of both frozen and qmp_locked checks, wrap it into one check.
> frozen implies the bitmap is split in two (for backup), and shouldn't
> be modified. qmp_locked implies it's being used by another operation,
> like being exported over NBD. In both cases it means we shouldn't allow
> the user to modify it in any meaningful way.
> 
> Replace any usages where we check both frozen and qmp_locked with the
> new check.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/dirty-bitmap.c           |  6 ++++++
>   blockdev.c                     | 29 ++++++++---------------------
>   include/block/dirty-bitmap.h   |  1 +
>   migration/block-dirty-bitmap.c | 10 ++--------
>   4 files changed, 17 insertions(+), 29 deletions(-)
> 

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

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

Re: [Qemu-devel] [PATCH v4 2/6] block/dirty-bitmaps: fix merge permissions

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> In prior commits that made merge transactionable, we removed the
> assertion that merge cannot operate on disabled bitmaps. In addition,
> we want to make sure that we are prohibiting merges to "locked" bitmaps.
> 
> Use the new user_locked function to check.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/dirty-bitmap.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

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

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

Re: [Qemu-devel] [PATCH v4 3/6] block/dirty-bitmaps: allow clear on disabled bitmaps

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> Similarly to merge, it's OK to allow clear operations on disabled
> bitmaps, as this condition only means that they are not recording
> new writes. We are free to clear it if the user requests it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/dirty-bitmap.c | 1 -
>   blockdev.c           | 8 --------
>   2 files changed, 9 deletions(-)
> 

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

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

Re: [Qemu-devel] [PATCH v4 4/6] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> We're not being consistent about this. If it's in use by an operation,
> the user should not be able to change the behavior of that bitmap.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
> 

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

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

Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> If the bitmap is frozen, we shouldn't touch it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index d0febfca79..098d4c337f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>               bdrv_unref(target_bs);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
> +        if (bdrv_dirty_bitmap_user_locked(bmap)) {
>               error_setg(errp,
> -                       "Bitmap '%s' is currently locked and cannot be used for "
> -                       "backup", backup->bitmap);
> +                       "Bitmap '%s' is currently in use by another operation"
> +                       " and cannot be used for backup", backup->bitmap);
>               goto out;

Is this right?  Why can we not have two parallel backups utilizing the 
same unchanging locked bitmap as its source?  Of course, to do that, 
we'd need the condition of being locked to be a ref-counter (increment 
for each backup that reuses the bitmap, decrement when the backup 
finishes, and it is unlocked when the counter is 0) rather than a bool. 
So, without that larger refactoring, this is a conservative approach 
that is a little too strict, but allows for a simpler implementation. 
And the user can always work around the limitation by cloning the locked 
bitmap into another temporary bitmap, and starting the second parallel 
backup with the second backup instead of the original.

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

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

Re: [Qemu-devel] [PATCH v4 6/6] nbd: forbid use of frozen bitmaps

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> Whether it's "locked" or "frozen", it's in use and should
> not be allowed for the purposes of this operation.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   nbd/server.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index c3dd402b45..940b7aa0cd 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2478,8 +2478,8 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_qmp_locked(bm)) {
> -        error_setg(errp, "Bitmap '%s' is locked", bitmap);
> +    if (bdrv_dirty_bitmap_user_locked(bm)) {
> +        error_setg(errp, "Bitmap '%s' is in use", bitmap);

Again, I'm not sure about this. A locked bitmap is unchanging.  A frozen 
bitmap is split, and might change when the split is resolved, so we are 
correct that we cannot tolerate a frozen bitmap. But if locking were 
changed to a refcount instead of a bool, I don't see why we have to 
forbid a locked bitmap.  Then again, as in 5/6, the user can work around 
this limitation by cloning a locked bitmap into an unlocked temporary 
one, where we are trading ease of maintenance for a bit more user effort.

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

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

Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[Vladimir Sementsov-Ogievskiy]

03.10.2018 02:02, John Snow wrote:

Instead of both frozen and qmp_locked checks, wrap it into one check.
frozen implies the bitmap is split in two (for backup), and shouldn't
be modified. qmp_locked implies it's being used by another operation,
like being exported over NBD. In both cases it means we shouldn't allow
the user to modify it in any meaningful way.

Replace any usages where we check both frozen and qmp_locked with the
new check.

Signed-off-by: John Snow <jsnow@redhat.com><mailto:jsnow@redhat.com>
---
 block/dirty-bitmap.c           |  6 ++++++
 blockdev.c                     | 29 ++++++++---------------------
 include/block/dirty-bitmap.h   |  1 +
 migration/block-dirty-bitmap.c | 10 ++--------
 4 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ac933cf1c..85bc668f6a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
     return bitmap->successor;
 }

+/* Both conditions disallow user-modification via QMP. */
+bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
+    return (bdrv_dirty_bitmap_frozen(bitmap) ||
+            bdrv_dirty_bitmap_qmp_locked(bitmap));

hmm, extra parentheses



+}
+
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
 {
     qemu_mutex_lock(bitmap->mutex);


[...]



--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void)
                 goto fail;
             }

-            if (bdrv_dirty_bitmap_frozen(bitmap)) {
-                error_report("Can't migrate frozen dirty bitmap: '%s",
-                             bdrv_dirty_bitmap_name(bitmap));
-                goto fail;
-            }
-
-            if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-                error_report("Can't migrate locked dirty bitmap: '%s",
+            if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+                error_report("Can't migrate a bitmap that is in use: '%s'",

"by another operation" like in other cases?



                              bdrv_dirty_bitmap_name(bitmap));
                 goto fail;
             }


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


--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 2/6] block/dirty-bitmaps: fix merge permissions

[Vladimir Sementsov-Ogievskiy]

03.10.2018 02:02, John Snow wrote:
> In prior commits that made merge transactionable, we removed the
> assertion that merge cannot operate on disabled bitmaps. In addition,
> we want to make sure that we are prohibiting merges to "locked" bitmaps.
>
> Use the new user_locked function to check.
>
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>

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

> ---
>   block/dirty-bitmap.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 85bc668f6a..8a6e07930f 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>   
>       qemu_mutex_lock(dest->mutex);
>   
> -    if (bdrv_dirty_bitmap_frozen(dest)) {
> -        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
> -                   dest->name);
> +    if (bdrv_dirty_bitmap_user_locked(dest)) {
> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
> +        " operation and cannot be modified", dest->name);
>           goto out;
>       }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 3/6] block/dirty-bitmaps: allow clear on disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

03.10.2018 02:02, John Snow wrote:
> Similarly to merge, it's OK to allow clear operations on disabled
> bitmaps, as this condition only means that they are not recording
> new writes. We are free to clear it if the user requests it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

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

> ---
>   block/dirty-bitmap.c | 1 -
>   blockdev.c           | 8 --------
>   2 files changed, 9 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 8a6e07930f..035f97e5c2 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -625,7 +625,6 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>   
>   void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>   {
> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
>       assert(!bdrv_dirty_bitmap_readonly(bitmap));
>       bdrv_dirty_bitmap_lock(bitmap);
>       if (!out) {
> diff --git a/blockdev.c b/blockdev.c
> index d775f228fe..cccd36b5e7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2012,9 +2012,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>       if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
>           error_setg(errp, "Cannot modify a bitmap in use by another operation");
>           return;
> -    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
> -        error_setg(errp, "Cannot clear a disabled bitmap");
> -        return;
>       } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
>           error_setg(errp, "Cannot clear a readonly bitmap");
>           return;
> @@ -2917,11 +2914,6 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>                      "Bitmap '%s' is currently in use by another operation"
>                      " and cannot be cleared", name);
>           return;
> -    } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently disabled 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);
>           return;


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 4/6] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps

[Vladimir Sementsov-Ogievskiy]

03.10.2018 02:02, John Snow wrote:
> We're not being consistent about this. If it's in use by an operation,
> the user should not be able to change the behavior of that bitmap.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

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

> ---
>   blockdev.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index cccd36b5e7..d0febfca79 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2058,6 +2058,13 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>           return;
>       }
>   
> +    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap '%s' is currently in use by another operation"
> +                   " and cannot be enabled", action->name);
> +        return;
> +    }
> +
>       state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>       bdrv_enable_dirty_bitmap(state->bitmap);
>   }
> @@ -2092,6 +2099,13 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>           return;
>       }
>   
> +    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap '%s' is currently in use by another operation"
> +                   " and cannot be disabled", action->name);
> +        return;
> +    }
> +
>       state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>       bdrv_disable_dirty_bitmap(state->bitmap);
>   }
> @@ -2933,10 +2947,10 @@ void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>           error_setg(errp,
> -                   "Bitmap '%s' is currently frozen and cannot be enabled",
> -                   name);
> +                   "Bitmap '%s' is currently in use by another operation"
> +                   " and cannot be enabled", name);
>           return;
>       }
>   
> @@ -2954,10 +2968,10 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>           error_setg(errp,
> -                   "Bitmap '%s' is currently frozen and cannot be disabled",
> -                   name);
> +                   "Bitmap '%s' is currently in use by another operation"
> +                   " and cannot be disabled", name);
>           return;
>       }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[Vladimir Sementsov-Ogievskiy]

03.10.2018 15:28, Eric Blake wrote:
> On 10/2/18 6:02 PM, John Snow wrote:
>> If the bitmap is frozen, we shouldn't touch it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index d0febfca79..098d4c337f 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup 
>> *backup, JobTxn *txn,
>>               bdrv_unref(target_bs);
>>               goto out;
>>           }
>> -        if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
>> +        if (bdrv_dirty_bitmap_user_locked(bmap)) {
>>               error_setg(errp,
>> -                       "Bitmap '%s' is currently locked and cannot 
>> be used for "
>> -                       "backup", backup->bitmap);
>> +                       "Bitmap '%s' is currently in use by another 
>> operation"
>> +                       " and cannot be used for backup", 
>> backup->bitmap);
>>               goto out;
>
> Is this right?  Why can we not have two parallel backups utilizing the 
> same unchanging locked bitmap as its source?  Of course, to do that, 
> we'd need the condition of being locked to be a ref-counter (increment 
> for each backup that reuses the bitmap, decrement when the backup 
> finishes, and it is unlocked when the counter is 0) rather than a 
> bool. So, without that larger refactoring, this is a conservative 
> approach that is a little too strict, but allows for a simpler 
> implementation. And the user can always work around the limitation by 
> cloning the locked bitmap into another temporary bitmap, and starting 
> the second parallel backup with the second backup instead of the 
> original.

hm, firstly, we can have only one job per bds (only one backup with this 
source), and now we don't search for backup bitmap anywhere else than 
source bds, so bitmap sharing is impossible. Secondly, when we support 
several jobs per bds, or allow using bitmaps of source backing chain 
(for example), I'm afraid that it is still a bad idea to share the 
bitmap, because backup should to abdicate/reclaim the bitmap at its 
finish, which definitely affect the second backup.

Maybe we'll finally come to not doing reclaim/abdicate automatically in 
backup and user will just enable/disable/merge bitmaps by hand as he 
wants. It is near to be possible now, we just need a flag for such 
behavior, as backup now use sync bitmap only on initialization phase, to 
initialize copy_bitmap. So, backup itself don't need disabling or 
locking of the bitmap and freeze/abdicate/reclaim is an additional logic.

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


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[Vladimir Sementsov-Ogievskiy]

03.10.2018 02:02, John Snow wrote:
> If the bitmap is frozen, we shouldn't touch it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

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

> ---
>   blockdev.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index d0febfca79..098d4c337f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>               bdrv_unref(target_bs);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
> +        if (bdrv_dirty_bitmap_user_locked(bmap)) {
>               error_setg(errp,
> -                       "Bitmap '%s' is currently locked and cannot be used for "
> -                       "backup", backup->bitmap);
> +                       "Bitmap '%s' is currently in use by another operation"
> +                       " and cannot be used for backup", backup->bitmap);
>               goto out;
>           }
>       }
> @@ -3620,10 +3620,10 @@ 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_qmp_locked(bmap)) {
> +        if (bdrv_dirty_bitmap_user_locked(bmap)) {
>               error_setg(errp,
> -                       "Bitmap '%s' is currently locked and cannot be used for "
> -                       "backup", backup->bitmap);
> +                       "Bitmap '%s' is currently in use by another operation"
> +                       " and cannot be used for backup", backup->bitmap);
>               goto out;
>           }
>       }


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 6/6] nbd: forbid use of frozen bitmaps

[Vladimir Sementsov-Ogievskiy]

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

03.10.2018 02:02, John Snow wrote:
> Whether it's "locked" or "frozen", it's in use and should
> not be allowed for the purposes of this operation.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   nbd/server.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index c3dd402b45..940b7aa0cd 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2478,8 +2478,8 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_qmp_locked(bm)) {
> -        error_setg(errp, "Bitmap '%s' is locked", bitmap);
> +    if (bdrv_dirty_bitmap_user_locked(bm)) {
> +        error_setg(errp, "Bitmap '%s' is in use", bitmap);
>           return;
>       }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[John Snow]

On 10/03/2018 08:28 AM, Eric Blake wrote:
> On 10/2/18 6:02 PM, John Snow wrote:
>> If the bitmap is frozen, we shouldn't touch it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index d0febfca79..098d4c337f 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup
>> *backup, JobTxn *txn,
>>               bdrv_unref(target_bs);
>>               goto out;
>>           }
>> -        if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
>> +        if (bdrv_dirty_bitmap_user_locked(bmap)) {
>>               error_setg(errp,
>> -                       "Bitmap '%s' is currently locked and cannot be
>> used for "
>> -                       "backup", backup->bitmap);
>> +                       "Bitmap '%s' is currently in use by another
>> operation"
>> +                       " and cannot be used for backup",
>> backup->bitmap);
>>               goto out;
> 
> Is this right?  Why can we not have two parallel backups utilizing the
> same unchanging locked bitmap as its source?  Of course, to do that,
> we'd need the condition of being locked to be a ref-counter (increment
> for each backup that reuses the bitmap, decrement when the backup
> finishes, and it is unlocked when the counter is 0) rather than a bool.
> So, without that larger refactoring, this is a conservative approach
> that is a little too strict, but allows for a simpler implementation.
> And the user can always work around the limitation by cloning the locked
> bitmap into another temporary bitmap, and starting the second parallel
> backup with the second backup instead of the original.
> 
> Weak Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Vladimir gave a good recounting of the reasons. My principal
justification here is that:

- FROZEN implies that the bitmap has been split; which means there is a
pending operating to re-suture them into one bitmap which may occur at
an indeterminate time in the future that we cannot account for in the
following job code, and

- QMP_LOCKED only implies that the bitmap is in use by, say, the NBD
fleecing operation with no further pending actions.

Here, in do_drive_backup, we check only that we are not qmp_locked, but
I argue we ought to check against frozen as well. It's likely to fail
because the BDS is already in use by another job, but this check is
strictly more correct.

In the opposite case, we don't want to split a bitmap that is being used
by someone else -- we're about to fork this bitmap (which means that the
bitmap referenced by this named handle will be CLEARED) which can alter
what the NBD process is doing, which is also bad.

For now, this is correct.

--js

Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[John Snow]

On 10/03/2018 08:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.10.2018 02:02, John Snow wrote:
>> Instead of both frozen and qmp_locked checks, wrap it into one check.
>> frozen implies the bitmap is split in two (for backup), and shouldn't
>> be modified. qmp_locked implies it's being used by another operation,
>> like being exported over NBD. In both cases it means we shouldn't allow
>> the user to modify it in any meaningful way.
>>
>> Replace any usages where we check both frozen and qmp_locked with the
>> new check.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/dirty-bitmap.c           |  6 ++++++
>>  blockdev.c                     | 29 ++++++++---------------------
>>  include/block/dirty-bitmap.h   |  1 +
>>  migration/block-dirty-bitmap.c | 10 ++--------
>>  4 files changed, 17 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 8ac933cf1c..85bc668f6a 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
>>      return bitmap->successor;
>>  }
>>  
>> +/* Both conditions disallow user-modification via QMP. */
>> +bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
>> +    return (bdrv_dirty_bitmap_frozen(bitmap) ||
>> +            bdrv_dirty_bitmap_qmp_locked(bitmap));
> 
> hmm, extra parentheses

I'm fond of them, but I'll take them out.

> 
>> +}
>> +
>>  void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
>>  {
>>      qemu_mutex_lock(bitmap->mutex);
> 
> [...]
> 
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void)
>>                  goto fail;
>>              }
>>  
>> -            if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> -                error_report("Can't migrate frozen dirty bitmap: '%s",
>> -                             bdrv_dirty_bitmap_name(bitmap));
>> -                goto fail;
>> -            }
>> -
>> -            if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
>> -                error_report("Can't migrate locked dirty bitmap: '%s",
>> +            if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>> +                error_report("Can't migrate a bitmap that is in use: '%s'",
> 
> "by another operation" like in other cases?
> 

Oh, sure. Can't hurt.

>>                               bdrv_dirty_bitmap_name(bitmap));
>>                  goto fail;
>>              }
> 
> anyway,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> -- 
> Best regards,
> Vladimir
> 

Thanks, I'll just make these edits and trust that Eric is fine with it
as well.

--js

Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[Eric Blake]

On 10/3/18 1:28 PM, John Snow wrote:

> 
> Thanks, I'll just make these edits and trust that Eric is fine with it
> as well.

Yes, works for me

> 
> --js
> 

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

Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[John Snow]

On 10/02/2018 07:02 PM, John Snow wrote:
> based on: jsnow/bitmaps staging branch
> 
> This series builds on a previous standalone patch and adjusts
> the permission for all (or most) of the QMP bitmap commands.
> 
> V4:
>  - Replace "in-use" with "in use"
>  - Replace "user_modifiable" version with "user_locked"
>  - Remove more usages of frozen-and-or-locked in NBD.
> 
> John Snow (6):
>   block/dirty-bitmaps: add user_locked status checker
>   block/dirty-bitmaps: fix merge permissions
>   block/dirty-bitmaps: allow clear on disabled bitmaps
>   block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>   block/backup: prohibit backup from using in use bitmaps
>   nbd: forbid use of frozen bitmaps
> 
>  block/dirty-bitmap.c           | 13 +++++---
>  blockdev.c                     | 75 +++++++++++++++++++-----------------------
>  include/block/dirty-bitmap.h   |  1 +
>  migration/block-dirty-bitmap.c | 10 ++----
>  nbd/server.c                   |  4 +--
>  5 files changed, 48 insertions(+), 55 deletions(-)
> 

Thanks, applied to my bitmaps tree: [with edits suggested by Vladimir];

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:
> based on: jsnow/bitmaps staging branch
> 
> This series builds on a previous standalone patch and adjusts
> the permission for all (or most) of the QMP bitmap commands.
> 
> V4:
>   - Replace "in-use" with "in use"
>   - Replace "user_modifiable" version with "user_locked"
>   - Remove more usages of frozen-and-or-locked in NBD.
> 
> John Snow (6):
>    block/dirty-bitmaps: add user_locked status checker
>    block/dirty-bitmaps: fix merge permissions
>    block/dirty-bitmaps: allow clear on disabled bitmaps
>    block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>    block/backup: prohibit backup from using in use bitmaps
>    nbd: forbid use of frozen bitmaps

Just now noticing that our docs are slightly out of sync with these changes:

# @DirtyBitmapStatus:
#
# An enumeration of possible states that a dirty bitmap can report to 
the user.
#
# @frozen: The bitmap is currently in-use by a backup operation or block 
job,
#          and is immutable.
#
# @disabled: The bitmap is currently in-use by an internal operation and is
#            read-only. It can still be deleted.

This state is also reachable when using x-block-dirty-bitmap-disable, 
which is not an internal operation.  We probably also ought to document 
that this state is a prereq to x-nbd-server-add-bitmap, since...

#
# @active: The bitmap is actively monitoring for new writes, and can be 
cleared,
#          deleted, or used for backup operations.

...active is not valid for that usage.

#
# @locked: The bitmap is currently in-use by some operation and can not be
#          cleared, deleted, or used for backup operations. (Since 2.12)


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

Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[John Snow]

On 10/17/2018 02:24 PM, Eric Blake wrote:
> On 10/2/18 6:02 PM, John Snow wrote:
>> based on: jsnow/bitmaps staging branch
>>
>> This series builds on a previous standalone patch and adjusts
>> the permission for all (or most) of the QMP bitmap commands.
>>
>> V4:
>>   - Replace "in-use" with "in use"
>>   - Replace "user_modifiable" version with "user_locked"
>>   - Remove more usages of frozen-and-or-locked in NBD.
>>
>> John Snow (6):
>>    block/dirty-bitmaps: add user_locked status checker
>>    block/dirty-bitmaps: fix merge permissions
>>    block/dirty-bitmaps: allow clear on disabled bitmaps
>>    block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>>    block/backup: prohibit backup from using in use bitmaps
>>    nbd: forbid use of frozen bitmaps
> 
> Just now noticing that our docs are slightly out of sync with these
> changes:
> 
> # @DirtyBitmapStatus:
> #
> # An enumeration of possible states that a dirty bitmap can report to
> the user.
> #
> # @frozen: The bitmap is currently in-use by a backup operation or block
> job,
> #          and is immutable.
> #
> # @disabled: The bitmap is currently in-use by an internal operation and is
> #            read-only. It can still be deleted.
> 
> This state is also reachable when using x-block-dirty-bitmap-disable,
> which is not an internal operation.  We probably also ought to document
> that this state is a prereq to x-nbd-server-add-bitmap, since...
> 
> #
> # @active: The bitmap is actively monitoring for new writes, and can be
> cleared,
> #          deleted, or used for backup operations.
> 
> ...active is not valid for that usage.
> 
> #
> # @locked: The bitmap is currently in-use by some operation and can not be
> #          cleared, deleted, or used for backup operations. (Since 2.12)
> 
> 

Ah, you're right. The rationale on disabled there is a bit wrong. I
think active is still correct, but the pull workflow has some extra
criteria.

I'll give a shot at touching this up.

--js

Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[Eric Blake]

On 10/2/18 6:08 PM, John Snow wrote:

>> John Snow (6):
>>    block/dirty-bitmaps: add user_locked status checker
>>    block/dirty-bitmaps: fix merge permissions
>>    block/dirty-bitmaps: allow clear on disabled bitmaps
>>    block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>>    block/backup: prohibit backup from using in use bitmaps
>>    nbd: forbid use of frozen bitmaps
>>

> 
> Planning on submitting tests for 3.1, but with the merge queue opened up
> again I'm pretty keen on getting some stable commit IDs for these so far.

Food for thought when writing tests:
I've just noticed with my progress on libvirt code that for 
error-recovery purposes, if we decide to make 
x-block-dirty-bitmap-disable fail on a disabled bitmap (or enable on an 
enabled map) that it would be nice to have a distinct error code; or 
even better to just be a silent no-op.  That's because when libvirt has 
the situation:

bitmap1 (disabled), bitmap2 (enabled)

and wants to delete the point in time between those two, it will call:

enable bitmap1
merge bitmap2 into bitmap1
remove bitmap2

Removing a bitmap is not part of 'transaction', and there is no pressing 
reason for it to be (other than convenience) - which means this is at 
least 2 commands (if the first two are a transaction) or 3 (if all are 
done independently).

But because it is not a transaction, libvirt can conceivably succeed at 
reenabling bitmap1 but then fail because the guest shut down before 
merging bitmap2 into 1.  When the guest is restarted, there is no real 
data loss (bitmap1 and 2 are now both active, which is a little bit less 
efficient for guest writes than having just one active), so the user can 
attempt to have libvirt retry the deletion sequence.  If asking for 
bitmap1 to be enabled fails (because it is already enabled), then 
libvirt will have to add workaround code (either to detect that the 
error iss different than other failures, or to pre-probe whether the 
bitmap is already enabled); whereas if the request is a no-op, things 
are easier from libvirt's point of view.

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