Re: [Qemu-devel] [PATCH] dirty-bitmaps: allow merging to disabled bitmaps

[John Snow]

On 09/19/2018 04:58 PM, Eric Blake wrote:
> On 9/19/18 2:58 PM, John Snow wrote:
>> We wish to prohibit merging to read-only bitmaps and frozen bitmaps,
>> but "disabled" bitmaps only preclude their recording of live, new
>> information. It does not prohibit them from manual writes at the behest
>> of the user, as is the case for merge operations.
> 
> In particular, if I have a linear sequence of bitmaps,
> bitmap1 (disabled) tracking sectors touched during times T1-T2
> bitmap2 (disabled) tracking sectors touched during T2-T3
> bitmap3 (enabled) tracking sectors touched during T3-present
> 
> and later decide that I no longer care about time T2, but may still want
> to create a differential backup against time T1, then the logical action
> is to merge bitmap1 and bitmap2 into a single bitmap tracking T1-T3.
> Whether I keep the name bitmap1 (b1 as dst, b2 as src, delete b2 on
> completion), bitmap2 (b1 as src, b2 as dst, delete b1 on completion)
> or pick yet another name (create new disabled map b12, merge b1 into
> b12, merge b2 into b12, delete b1 and b2) is less important - the point
> remains that I am trying to merge into a disabled bitmap.  If a bitmap
> has to be enabled to perform the merge, then any guest I/O during the
> window in time where it is temporarily enabled will perhaps spuriously
> set bits corresponding to sectors not actually touched during T1-T3.
> 
> Perhaps it can be worked around with a transaction that does
> bitmap-enable, bitmap-merge, bitmap-disable - but that seems like a lot
> of overhead (and does it actually prevent guest I/O from happening
> during the transaction?), compared to just allowing the merge.
> 
>>
>> Reported-by: Eric Blake <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   block/dirty-bitmap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index c9b8a6fd52..fa7e75e0af 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap
>> *dest, const BdrvDirtyBitmap *src,
>>         qemu_mutex_lock(dest->mutex);
>>   -    assert(bdrv_dirty_bitmap_enabled(dest));
>> +    assert(!bdrv_dirty_bitmap_frozen(dest));
>>       assert(!bdrv_dirty_bitmap_readonly(dest));
> 
> At any rate, the fact that the deleted assertion was triggerable by QMP
> actions is a good reason to change it.  Here's how I triggered it, if
> you want to beef up the commit message:
> 
> $ qemu-img create -f qcow2 file 64k
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio
> {'execute':'qmp_capabilities'}
> {'execute':'blockdev-add', 'arguments':{'driver':'qcow2',
>  'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}}
> {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp',
>  'name':'b0'}}
> {'execute':'transaction', 'arguments':{'actions':[
>  {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp',
>   'name':'b0'}},
>  {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp',
>   'name':'b1'}}]}}
> {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp',
>  'name':'tmp'}}
> {'execute':'x-block-dirty-bitmap-disable', 'arguments':{'node':'tmp',
>  'name':'tmp'}}
> {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp',
>  'src_name':'b0', 'dst_name':'tmp'}}
> qemu-system-x86_64: block/dirty-bitmap.c:801: bdrv_merge_dirty_bitmap:
> Assertion `bdrv_dirty_bitmap_enabled(dest)' failed.
> 
> However, are we sure that the remaining assertions are properly flagged
> by callers, and that we aren't leaving any other lingering QMP crashers?
>  Let's see if I can modify my example
> 
> $ qemu-img create -f qcow2 -b file -F qcow2 file2
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio
> {'execute':'qmp_capabilities'}
> {'execute':'blockdev-add', 'arguments':{'driver':'qcow2',
>  'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}}
> {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp',
>  'name':'b0'}}
> {'execute':'nbd-server-start', 'arguments':{'addr':{'type':'inet',
>  'data':{'host':'localhost', 'port':'10809'}}}}
> {'execute':'blockdev-add', 'arguments':{'driver':'qcow2',
>  'node-name':'fleece', 'file':{'driver':'file', 'filename':'file2'},
>   'backing':'tmp'}}
> {'execute':'transaction', 'arguments':{'actions':[
>  {'type':'blockdev-backup', 'data':{'job-id':'backup', 'device':'tmp',
>   'target':'fleece', 'sync':'none'}},
>  {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', 'name':'b1'}},
>  {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp',
>   'name':'b0'}}
>  ]}}
> {'execute':'nbd-server-add', 'arguments':{'device':'fleece'}}
> {'execute':'x-nbd-server-add-bitmap', 'arguments':{'name':'fleece',
>  'bitmap':'b0'}}
> {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp',
>  'src_name':'b1', 'dst_name':'b0'}}
> 
> Prior to your patch, that asserts; but after your patch, it silently
> succeeds. Shouldn't a bitmap be marked frozen while it is advertised
> over an NBD export? We already require such a bitmap to be disabled
> before you can attach it, and you REALLY don't want a read-only export
> to be seeing changes to block status information due to merges. And then
> we need to make sure that we give a proper error message rather than an
> assertion when using QMP to attempt to merge into a frozen bitmap. But
> that's probably an independent patch, so:
> 
> Reviewed-by: Eric Blake <address@hidden>
> 

It looks like in addition to this we need to also check the locked
property and disallow those, so, NACK.

--js