Re: [libvirt] [PATCH v2 3/6] block/dirty-bitmap: change semantics of enabled predicate
On 2/18/19 11:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:23, 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() 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. To accommodate this, create_successor now ensures that
>> any bitmap with a successor is explicitly disabled.
>>
>
> 5-8 are examples of "this is how this predicate was already used"
>
>> 5. qcow2_store_persistent_dirty_bitmaps: 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,
>
> No, as you explicitly disable bitmap in create_successor, so bitmaps with
> successor
> will be disabled anyway.
>
Well, yeah. There's no way it happens in practice currently. It's just
"theoretically" from the viewpoint of the API call itself. There's
nothing stopping a developer from making that call, and this is a
potential change in behavior that we don't expect to observe. Just
noting it down.
> Hmm, and this shows, that actually, you don't need this big description for
> all calls,
> as actually nothing changed and all calls may be described like (4.). Except
> (2. and 3.),
> as these calls are removed (so, is it worth to split them into separate
> previous patch?)
>
I could, to at least have its own justification in a commit message
apart from these -- but at this point it's primarily a benefit for Eric,
You, and myself.
> but this cannot happen
>> in practice because jobs must be finished before we close the disk.
>>
>> 6. block_dirty_bitmap_enable_prepare only ever cared 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.
>
>
> hmmm
> 9. nbd_export_new, which too checks bdrv_dirty_bitmap_user_locked but _after_
> call to bdrv_dirty_bitmap_enabled. Anyway it's not changed as described
> in (4.),
> I think it is better to check _user_locked first.
>
You're right, and Eric left a similar feedback elsewhere. user_locked is
the more obvious disqualifier. I think this ought to be its own small
patch because it has nothing much to do with this one.
>
>>
>> Signed-off-by: John Snow
>> Reviewed-by: Eric Blake
>> ---
>> block/dirty-bitmap.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 639ebc0645..bb2e19e0d8 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;
>
> at this point comment to the function
> "The merged parent will not be user_locked, nor expl
Re: [libvirt] [PATCH v2 3/6] block/dirty-bitmap: change semantics of enabled predicate
14.02.2019 2:23, 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() 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. To accommodate this, create_successor now ensures that
> any bitmap with a successor is explicitly disabled.
>
5-8 are examples of "this is how this predicate was already used"
> 5. qcow2_store_persistent_dirty_bitmaps: 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,
No, as you explicitly disable bitmap in create_successor, so bitmaps with
successor
will be disabled anyway.
Hmm, and this shows, that actually, you don't need this big description for all
calls,
as actually nothing changed and all calls may be described like (4.). Except
(2. and 3.),
as these calls are removed (so, is it worth to split them into separate
previous patch?)
but this cannot happen
> in practice because jobs must be finished before we close the disk.
>
> 6. block_dirty_bitmap_enable_prepare only ever cared 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.
hmmm
9. nbd_export_new, which too checks bdrv_dirty_bitmap_user_locked but _after_
call to bdrv_dirty_bitmap_enabled. Anyway it's not changed as described in
(4.),
I think it is better to check _user_locked first.
>
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
> ---
> block/dirty-bitmap.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 639ebc0645..bb2e19e0d8 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;
at this point comment to the function
"The merged parent will not be user_locked, nor explicitly re-enabled."
becomes really weird. Need to reword it somehow..
> 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_reado
