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 <ebl...@redhat.com>
>> Signed-off-by: John Snow <js...@redhat.com>
>> ---
>> 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 <ebl...@redhat.com>
>
It looks like in addition to this we need to also check the locked
property and disallow those, so, NACK.
--js
