30.05.2017 09:50, Vladimir Sementsov-Ogievskiy wrote:
Thank you for this scenario. Hmm.

So, as I need guarantee that image and bitmap are unchanged, bdrv_set_dirty should return error and fail the whole write. Ok?

No, bad idea. As bdrv_set_dirty is called after write completed.


29.05.2017 21:35, Max Reitz wrote:
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 <vsement...@virtuozzo.com>
---
  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



--
Best regards,
Vladimir


Reply via email to