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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
