diff mbox series

[3/5] block/dirty-bitmap: change semantics of enabled predicate

Message ID 20190212010248.11056-4-jsnow@redhat.com
State New
Headers show
Series dirty-bitmaps: deprecate @status field | expand

Commit Message

John Snow Feb. 12, 2019, 1:02 a.m. UTC 
Currently, enabled means something like "the status of the bitmap
is ACTIVE." After this patch, it should mean exclusively: "This
bitmap is recording guest writes, and is allowed to do so."

In many places, this is how this predicate was already used.
We'll allow users to call user_locked if they're really curious about
finding out if the bitmap is in use by an operation.

To accommodate this, modify the create_successor routine to now
explicitly disable the parent bitmap at creation time.


Justifications:

1. bdrv_dirty_bitmap_status suffers no change from the lack of
   1:1 parity with the new predicates because of the order in which
   the predicates are checked. This is now only for compatibility.

2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
   disabled bitmaps -- all of these writes are internal usages.
   Therefore, we should allow writes even in the disabled state.
   The condition is removed.

3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
   mirror and migration. In these contexts it is always enabled anyway,
   but our API does not need to enforce this.

4. bdrv_set_dirty will skip recording writes from the guest here if
   we are disabled OR if we had a successor, which now changes.
   Accommodate the change by explicitly disabling bitmaps with successors.

5. qcow2/dirty-bitmap: This only ever wanted to check if the bitmap
   was enabled or not. Theoretically if we save during an operation,
   this now gets set as enabled instead of disabled.

6. block_dirty_bitmap_enable_prepare only ever cared if about the
   literal bit, and already checked for user_locked beforehand.

7. block_dirty_bitmap_disable_prepare ditto as above.

8. init_dirty_bitmap_migration also already checks user_locked,
   so this call can be a simple enabled/disabled check.
---
 block/dirty-bitmap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Eric Blake Feb. 12, 2019, 6:58 p.m. UTC | #1 
On 2/11/19 7:02 PM, John Snow wrote:
> Currently, enabled means something like "the status of the bitmap
> is ACTIVE." After this patch, it should mean exclusively: "This
> bitmap is recording guest writes, and is allowed to do so."
> 
> In many places, this is how this predicate was already used.
> We'll allow users to call user_locked if they're really curious about
> finding out if the bitmap is in use by an operation.
> 
> To accommodate this, modify the create_successor routine to now
> explicitly disable the parent bitmap at creation time.
> 
> 
> Justifications:
> 
> 1. bdrv_dirty_bitmap_status suffers no change from the lack of
>    1:1 parity with the new predicates because of the order in which
>    the predicates are checked. This is now only for compatibility.
> 
> 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
>    disabled bitmaps -- all of these writes are internal usages.
>    Therefore, we should allow writes even in the disabled state.
>    The condition is removed.
> 
> 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
>    mirror and migration. In these contexts it is always enabled anyway,
>    but our API does not need to enforce this.
> 
> 4. bdrv_set_dirty will skip recording writes from the guest here if
>    we are disabled OR if we had a successor, which now changes.
>    Accommodate the change by explicitly disabling bitmaps with successors.

I didn't quite follow this wording.  My try:

The code in bdrv_set_dirty() is unchanged: pre-patch, it was skipping
bitmaps that were disabled or had a successor, while post-patch it is
only skipping bitmaps that are disabled. But we have the same behavior
because the change to create_successor now ensures that any bitmap with
a successor is disabled.

> 
> 5. qcow2/dirty-bitmap: This only ever wanted to check if the bitmap

Did you mean qcow2_store_persistent_dirty_bitmaps()?

>    was enabled or not. Theoretically if we save during an operation,
>    this now gets set as enabled instead of disabled.

I'm not sure I see the theoretical change in behavior (let alone whether
you could write an iotest to expose it).  Pre-patch, persistent bitmaps
that were disabled or which had a successor did not have the AUTO bit
set (although since we currently only write persistent bitmaps out to
file at exit, when there should be no ongoing jobs and thus no
successors); post-patch, only disabled bitmaps do not have the AUTO bit
(but a bitmap with a successor is disabled because of the change to
create_successor).  But I agree that this code did not need a change due
to the new semantics of bdrv_dirty_bitmap_enabled.

> 
> 6. block_dirty_bitmap_enable_prepare only ever cared if about the

s/if //

>    literal bit, and already checked for user_locked beforehand.

That is, the check for user_locked already ruled out the has_successor
clause.

> 
> 7. block_dirty_bitmap_disable_prepare ditto as above.
> 
> 8. init_dirty_bitmap_migration also already checks user_locked,
>    so this call can be a simple enabled/disabled check.

Looks like correct conversions to me.

> ---
>  block/dirty-bitmap.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
John Snow Feb. 12, 2019, 7:03 p.m. UTC | #2 
On 2/12/19 1:58 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> Currently, enabled means something like "the status of the bitmap
>> is ACTIVE." After this patch, it should mean exclusively: "This
>> bitmap is recording guest writes, and is allowed to do so."
>>
>> In many places, this is how this predicate was already used.
>> We'll allow users to call user_locked if they're really curious about
>> finding out if the bitmap is in use by an operation.
>>
>> To accommodate this, modify the create_successor routine to now
>> explicitly disable the parent bitmap at creation time.
>>
>>
>> Justifications:
>>
>> 1. bdrv_dirty_bitmap_status suffers no change from the lack of
>>    1:1 parity with the new predicates because of the order in which
>>    the predicates are checked. This is now only for compatibility.
>>
>> 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
>>    disabled bitmaps -- all of these writes are internal usages.
>>    Therefore, we should allow writes even in the disabled state.
>>    The condition is removed.
>>
>> 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
>>    mirror and migration. In these contexts it is always enabled anyway,
>>    but our API does not need to enforce this.
>>
>> 4. bdrv_set_dirty will skip recording writes from the guest here if
>>    we are disabled OR if we had a successor, which now changes.
>>    Accommodate the change by explicitly disabling bitmaps with successors.
> 
> I didn't quite follow this wording.  My try:
> 
> The code in bdrv_set_dirty() is unchanged: pre-patch, it was skipping
> bitmaps that were disabled or had a successor, while post-patch it is
> only skipping bitmaps that are disabled. But we have the same behavior
> because the change to create_successor now ensures that any bitmap with
> a successor is disabled.
> 

Yes, sorry, the final sentence there is doing a lot of heavy lifting.

>>
>> 5. qcow2/dirty-bitmap: This only ever wanted to check if the bitmap
> 
> Did you mean qcow2_store_persistent_dirty_bitmaps()?
> 

Ah, yeah, I skipped the function name. Yes, that function. It's the only
use in this file.

>>    was enabled or not. Theoretically if we save during an operation,
>>    this now gets set as enabled instead of disabled.
> 
> I'm not sure I see the theoretical change in behavior (let alone whether
> you could write an iotest to expose it).  Pre-patch, persistent bitmaps
> that were disabled or which had a successor did not have the AUTO bit
> set (although since we currently only write persistent bitmaps out to
> file at exit, when there should be no ongoing jobs and thus no
> successors); post-patch, only disabled bitmaps do not have the AUTO bit
> (but a bitmap with a successor is disabled because of the change to
> create_successor).  But I agree that this code did not need a change due
> to the new semantics of bdrv_dirty_bitmap_enabled.
> 

Right, the change is extremely subtle and likely should be impossible to
see. If you CAN see it, that's a bug!

>>
>> 6. block_dirty_bitmap_enable_prepare only ever cared if about the
> 
> s/if //
> 
>>    literal bit, and already checked for user_locked beforehand.
> 
> That is, the check for user_locked already ruled out the has_successor
> clause.
> 

Yes.

>>
>> 7. block_dirty_bitmap_disable_prepare ditto as above.
>>
>> 8. init_dirty_bitmap_migration also already checks user_locked,
>>    so this call can be a simple enabled/disabled check.
> 
> Looks like correct conversions to me.
> 
>> ---
>>  block/dirty-bitmap.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!
diff mbox series

Patch

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index dbe2d97d3f..1a433130ff 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,7 +209,7 @@  bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
-    return !(bitmap->disabled || bitmap->successor);
+    return !bitmap->disabled;
 }
 
 /* Called with BQL taken.  */
@@ -264,6 +264,7 @@  int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Successor will be on or off based on our current state. */
     child->disabled = bitmap->disabled;
+    bitmap->disabled = true;
 
     /* Install the successor and freeze the parent */
     bitmap->successor = child;
@@ -346,6 +347,8 @@  BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
         error_setg(errp, "Merging of parent and successor bitmap failed");
         return NULL;
     }
+
+    parent->disabled = successor->disabled;
     bdrv_release_dirty_bitmap_locked(successor);
     parent->successor = NULL;
 
@@ -542,7 +545,6 @@  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                   int64_t offset, int64_t bytes)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_set(bitmap->bitmap, offset, bytes);
 }
@@ -559,7 +561,6 @@  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                     int64_t offset, int64_t bytes)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_reset(bitmap->bitmap, offset, bytes);
 }