31.05.2017 16:43, Max Reitz wrote:
On 2017-05-30 08: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?
I don't know. That would mean that you couldn't commit to an image that
has a persistent auto-loading bitmap, which doesn't seem very nice to me.
I'm not quite sure what to do myself. So first I'd definitely want the
commit operation to succeed. That means we'd have to automatically make
the bitmap non-readonly once we write to it. The "readonly" flag would
then be an "unchanged" flag, rather, to signify that the bitmap has not
been changed since it was loaded, which means that it does not need to
be written back to the image file.
Now the issue remains that if you modify a persistent bitmap that is
stored in an image file that is opened RO when it's closed, you won't be
able to write the modifications back.
So in addition, I guess we'd need to "flush" all persistent bitmaps
(that is, write all modifications back to the file and set the
"unchanged" flag (you could also call it "dirty" and then mean the
opposite) for each bitmap) not only when the image is closed or
invalidated, but also when it is reopened read-only.
(block-commit reopens the backing BDS R/W, then writes to them, thus
modifying the dirty bitmaps, and finally reopens the BDS as read-only;
before that happens, we will have to flush the modified bitmap data.)
Ok, understand.
We need to consider also setting in_use flag in the image. We _must not_
write to image with dirty bitmap,
if in_use flag of this dirty bitmap is not set, as in case of something
fail we will have image with wrong bitmap with
unset in_use flag (which looks ok).
I see two ways to handle it:
variant 1:
1. readonly field stays as is (see v19, with normal errors, not only
asserts)
2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e.
set in_use in the image and set BdrvDirtyBitmap.readonly = false
3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
save them into the image and set BdrvDirtyBitmap.readonly = true
variant 2:
1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all
bitmaps on create
2. before write/discard check this field in all related bitmaps, and if
dirty=0 (and persistent=1), write IN_USE flag into the image first, set
dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the
whole write)
3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
save them into the image and set BdrvDirtyBitmap.dirty = 0
Max
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
