dirty-bitmaps: fix QMP command permissions
v4 cover dirty-bitmaps: fix QMP command permissions
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(-)
Signature · 1 line
2.14.4
PATCH v4 1/6 block/dirty-bitmaps: add user_locked status checker
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.
--- 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(-)
block/dirty-bitmap.c +6 -0
@@ -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);
blockdev.c +8 -21
@@ -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,
include/block/dirty-bitmap.h +1 -0
@@ -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);
migration/block-dirty-bitmap.c +2 -8
@@ -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; }
Signature · 1 line
2.14.4
Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker
Quoted 9 lines · John Snow
Instead of both frozen and
qmp_lockedchecks, wrap it into one check. frozen implies the bitmap is split in two (for backup), and shouldn't be modified.qmp_lockedimplies 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_lockedwith the new check.
Quoted 8 lines
Signed-off-by John Snow <[email protected]>--- 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(-)
Signature · 3 lines
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
In reply to John Snow03.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.
--- 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(-)
block/dirty-bitmap.c +4 -0
@@ -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);[...]
migration/block-dirty-bitmap.c +2 -8
@@ -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,
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker
Quoted 26 lines · Vladimir Sementsov-Ogievskiy
Quoted 25 lines · John Snow
Instead of both frozen and
qmp_lockedchecks, wrap it into one check. frozen implies the bitmap is split in two (for backup), and shouldn't be modified.qmp_lockedimplies 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_lockedwith the new check.Signed-off-by John Snow <[email protected]>--- 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(-)block/dirty-bitmap.c +0
-0@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)return bitmap->successor; }
Quoted 6 lines
block/dirty-bitmap.c bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) · line 182
Quoted 4 lines
block/dirty-bitmap.c +4
-0+/* 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.
Quoted 15 lines
Quoted 6 lines
block/dirty-bitmap.c +2
-0+} +void bdrv_dirty_bitmap_set_qmp_locked (BdrvDirtyBitmap *bitmap,bool qmp_locked) {qemu_mutex_lock (bitmap->mutex);[...]
Quoted 6 lines
migration/block-dirty-bitmap.c +0
-0@@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void) goto fail; }
Quoted 13 lines
migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 305
Quoted 10 lines
migration/block-dirty-bitmap.c +2
-8-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.
Quoted 11 lines
migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 302
Quoted 3 lines
migration/block-dirty-bitmap.c +0
-0bdrv_dirty_bitmap_name(bitmap)); goto fail; }anyway,
Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>Signature · 2 lines
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
Quoted 3 lines · John Snow
Thanks, I'll just make these edits and trust that Eric is fine with it as well.
Yes, works for me
Quoted 3 lines
--js
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 18 lines · 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: adduser_lockedstatus checkerblock/dirty-bitmaps: fix merge permissionsblock/dirty-bitmaps: allow clear on disabled bitmapsblock/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in use bitmaps
nbd: forbid use of frozen bitmaps
Quoted 7 lines
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
Quoted 10 lines · John Snow
Quoted 8 lines
John Snow (6):
block/dirty-bitmaps: adduser_lockedstatus checkerblock/dirty-bitmaps: fix merge permissionsblock/dirty-bitmaps: allow clear on disabled bitmapsblock/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in use bitmaps
nbd: forbid use of frozen bitmaps
Quoted 2 lines
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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v4 2/6 block/dirty-bitmaps: fix merge permissions
In reply to John SnowIn 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.
--- block/dirty-bitmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
block/dirty-bitmap.c +3 -3
@@ -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; }
Signature · 1 line
2.14.4
Re: [Qemu-devel] [PATCH v4 2/6] block/dirty-bitmaps: fix merge permissions
Quoted 4 lines · 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.
Quoted 8 lines
Use the new
user_lockedfunction to check.Reported-by Eric Blake <[email protected]>Signed-off-by John Snow <[email protected]>--- block/dirty-bitmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Signature · 3 lines
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
In reply to John SnowQuoted 8 lines · 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_lockedfunction to check.Reported-by Eric Blake <[email protected]>Signed-off-by John Snow <[email protected]>
Quoted 21 lines
--- block/dirty-bitmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)block/dirty-bitmap.c +3
-3@@ -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; }
Signature · 2 lines
Best regards, Vladimir
PATCH v4 3/6 block/dirty-bitmaps: allow clear on disabled bitmaps
In reply to John SnowSimilarly 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.
--- block/dirty-bitmap.c | 1 - blockdev.c | 8 -------- 2 files changed, 9 deletions(-)
block/dirty-bitmap.c +0 -1
@@ -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) {
blockdev.c +0 -8
@@ -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 ;
Signature · 1 line
2.14.4
Re: [Qemu-devel] [PATCH v4 3/6] block/dirty-bitmaps: allow clear on disabled bitmaps
Quoted 10 lines · 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 <[email protected]>--- block/dirty-bitmap.c | 1 - blockdev.c | 8 -------- 2 files changed, 9 deletions(-)
Signature · 3 lines
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
In reply to John SnowQuoted 5 lines · 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 <[email protected]>
Quoted 43 lines
--- block/dirty-bitmap.c | 1 - blockdev.c | 8 -------- 2 files changed, 9 deletions(-)block/dirty-bitmap.c +0
-1@@ -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) {blockdev.c +0
-8@@ -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 ;
Signature · 2 lines
Best regards, Vladimir
PATCH v4 4/6 block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
In reply to John SnowWe'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.
--- blockdev.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
blockdev.c +20 -6
@@ -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 ; }
Signature · 1 line
2.14.4
Re: [Qemu-devel] [PATCH v4 4/6] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
Quoted 8 lines · 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 <[email protected]>--- blockdev.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
Signature · 3 lines
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
In reply to John SnowQuoted 4 lines · 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 <[email protected]>
Quoted 64 lines
--- blockdev.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)blockdev.c +20
-6@@ -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 ; }
Signature · 2 lines
Best regards, Vladimir
PATCH v4 5/6 block/backup: prohibit backup from using in use bitmaps
In reply to John SnowIf the bitmap is frozen, we shouldn't touch it.
--- blockdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
blockdev.c +6 -6
@@ -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; } }
Signature · 1 line
2.14.4
Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps
Quoted 13 lines · John Snow
If the bitmap is frozen, we shouldn't touch it.
Signed-off-by John Snow <[email protected]>--- blockdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)blockdev.c +0
-0@@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,bdrv_unref (target_bs);
blockdev.c static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, · line 3519
Quoted 10 lines
blockdev.c +3
-3goto 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 <[email protected]>
Signature · 3 lines
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
Quoted 32 lines · Eric Blake
Quoted 20 lines · John Snow
If the bitmap is frozen, we shouldn't touch it.
Signed-off-by John Snow <[email protected]>---
blockdev.c| 12 ++++++------1 file changed, 6 insertions(+), 6 deletions(-)blockdev.c +1
-2@@ -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 cannotQuoted 7 lines
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
Quoted 8 lines
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.
Quoted 3 lines
Weak Reviewed-by: Eric Blake <[email protected]>
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps
In reply to Eric BlakeQuoted 40 lines · Eric Blake
Quoted 20 lines · John Snow
If the bitmap is frozen, we shouldn't touch it.
Signed-off-by John Snow <[email protected]>---
blockdev.c| 12 ++++++------1 file changed, 6 insertions(+), 6 deletions(-)blockdev.c +1
-2@@ -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 beQuoted 7 lines
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.
Quoted 2 lines
Weak Reviewed-by: Eric Blake <[email protected]>
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 5/6] block/backup: prohibit backup from using in use bitmaps
In reply to John SnowQuoted 3 lines · John Snow
If the bitmap is frozen, we shouldn't touch it.
Signed-off-by John Snow <[email protected]>
Quoted 36 lines
--- blockdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)blockdev.c +6
-6@@ -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; } }
Signature · 2 lines
Best regards, Vladimir
PATCH v4 6/6 nbd: forbid use of frozen bitmaps
In reply to John SnowWhether it's "locked" or "frozen", it's in use and should not be allowed for the purposes of this operation.
--- nbd/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
nbd/server.c +2 -2
@@ -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 ; }
Signature · 1 line
2.14.4
Re: [Qemu-devel] [PATCH v4 6/6] nbd: forbid use of frozen bitmaps
Quoted 13 lines · 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 <[email protected]>--- nbd/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)nbd/server.c +0
-0
nbd/server.c void nbd_export_bitmap(NBDExport *exp, const char *bitmap, · line 2482
Quoted 7 lines
nbd/server.c +2
-2@@ -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 <[email protected]>
Signature · 3 lines
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
In reply to John SnowQuoted 23 lines · 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 <[email protected]>--- nbd/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)nbd/server.c +2
-2@@ -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 ; }
Signature · 2 lines
Best regards, Vladimir
John Snow
AppliedQuoted 18 lines · 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: adduser_lockedstatus checkerblock/dirty-bitmaps: fix merge permissionsblock/dirty-bitmaps: allow clear on disabled bitmapsblock/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in use bitmaps
nbd: forbid use of frozen bitmaps
Quoted 7 lines
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
Quoted 10 lines · 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.
Quoted 7 lines
John Snow (6):
block/dirty-bitmaps: adduser_lockedstatus checkerblock/dirty-bitmaps: fix merge permissionsblock/dirty-bitmaps: allow clear on disabled bitmapsblock/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)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 43 lines · Eric Blake
Quoted 10 lines · 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.Quoted 7 lines
John Snow (6):
block/dirty-bitmaps: adduser_lockedstatus checkerblock/dirty-bitmaps: fix merge permissionsblock/dirty-bitmaps: allow clear on disabled bitmapsblock/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in use bitmaps
nbd: forbid use of frozen bitmapsJust 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.
Quoted 7 lines
...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