Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Max Reitz]

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
> It will be needed in following commits for persistent bitmaps.
> If bitmap is loaded from read-only storage (and we can't mark it
> "in use" in this storage) corresponding BdrvDirtyBitmap should be
> read-only.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/dirty-bitmap.c         | 16 ++++++++++++++++
>  include/block/dirty-bitmap.h |  3 +++
>  2 files changed, 19 insertions(+)

Revisiting this again after the whole series: So you never really make
sure that the read-only bitmaps are not written to (except for these
assertions). The idea is that you only set it for read-only BDS and
read-only BDS are never written to. But that assumption is not true,
generally, and can be broken e.g. using a commit job:

$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
    cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
Formatting 'top.qcow2', fmt=qcow2 size=67108864
    backing_file=backing.qcow2 encryption=off cluster_size=65536
    lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
"package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
{'execute': 'qmp_capabilities'}
{"return": {}}
{'execute': 'blockdev-add',
 'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
               'file': {'driver': 'file', 'filename': 'backing.qcow2'}}}
{"return": {}}
{'execute': 'block-dirty-bitmap-add',
 'arguments': {'node': 'backing-node', 'name': 'foo',
               'persistent': true, 'autoload': true}}
{"return": {}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
{"return": {}}
{'execute': 'blockdev-add',
 'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
               'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
{"return": {}}
{'execute': 'human-monitor-command',
 'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
{"return": ""}
{'execute': 'block-commit',
 'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
{"return": {}}
qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
`!bdrv_dirty_bitmap_readonly(bitmap)' failed.
[1]    10872 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -qmp
stdio

So there needs to be something else than just assertions to make sure
that read-only bitmaps are never written to.

Max

signature.asc
Description: OpenPGP digital signature