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 <ebl...@redhat.com>
>
Thanks!