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

[Eric Blake]

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>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org