21.06.2018 20:17, Kevin Wolf wrote:
Am 21.06.2018 um 17:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
21.06.2018 17:25, Kevin Wolf wrote:
Am 15.06.2018 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
14.06.2018 13:46, Kevin Wolf wrote:
Am 12.06.2018 um 20:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
Hi all!
I've faced the following problem:
1. create image with dirty bitmap, a.qcow2 (start qemu and run qmp
command block-dirty-bitmap-add)
2. run the following commands:
qemu-img create -f qcow2 -b a.qcow2 b.qcow2 10M
qemu-io -c 'write 0 512' b.qcow2
qemu-img commit b.qcow2
3. last command fails with the following output:
Formatting 'b.qcow2', fmt=qcow2 size=68719476736 backing_file=a.qcow2
cluster_size=65536 lazy_refcounts=off refcount_bits=16
wrote 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0953 sec (5.243 KiB/sec and 10.4867 ops/sec)
qemu-img: #block397: Failed to make dirty bitmaps writable: Can't update
bitmap directory: Operation not permitted
qemu-img: Block job failed: Operation not permitted
And problem is that children are reopened _after_ parent. But qcow2 reopen
needs write access to its file, to write IN_USE flag to dirty-bitmaps
extension.
I was aware of a different instance of this problem: Assume a qcow2
image with an unknown autoclear flag (so it will be cleared on r/w
open), which is first opened r/o and then reopened r/w. This will fail
because .bdrv_reopen_prepare doesn't have the permissions yet.
Hm.. If I understand correctly qcow2_reopen_prepare doesn't deal with
autoclear flags, as it doesn't call qcow2_do_open.
Hm, right, not sure what I really meant back then when I added it to my
to-do list... Maybe I confused reopen and invalidate_cache.
Simply changing the order won't fix this because in the r/w -> r/o, the
driver will legitimately flush its caches in .bdrv_reopen_prepare, and
for this it still needs to be able to write.
We may need to have a way for nodes to access both the old and the new
state of their children. I'm not completely sure how to achieve this
best, though.
When I thought only of permissions, the obvious and simple thing to do
was to just get combined permissions for the old and new state, i.e.
'old_perm | new_perm' and 'old_shared & new_shared'. But I don't think
this is actually enough when the child node switches between a r/w and
a r/o file descriptor because even though QEMU's permission system would
allow the write, you still can't successfully write to a r/o file
descriptor.
Kevin
Maybe we want two .bdrv_reopen_prepare: .bdrv_reopen_prepare_before_children
and .bdrv_reopen_prepare_after_children. But to write something in
reopen_prepare, we need to move bdrv_set_perm from reopen_commit to
.. Is it possible?
Getting the permission problems out of the way can be solved by changing
permissions twice, like I said above: First to the combined permissions
of old and new, and finally to only the new permissions.
The problem I see with .bdrv_reopen_prepare_after_children is that I
don't see how it actually buys you anything: Even if the children
already prepared the reopen, any access of the child node still refers
to the old file descriptor because the new one only becomes valid with
.bdrv_reopen_commit.
Now, I've found the following workaround, what do you think about something
like this as a temporary fix:
I honestly don't understand why this workaround makes any difference.
with this patch, commit for children will be called earlier than for parent,
so, when reopening bitmaps rw (which is done in commit) bs->file will be
already completely reopened rw, and all works.
.bdrv_reopen_commit() can't do any I/O because it must not fail.
Therefore the order in which nodes are committed should not matter.
Any I/O that needs to be done has to be in .bdrv_reopen_prepare() (and
possibly be kept in a temporary buffer) and .bdrv_reopen_commit() can
only apply what is already in memory.
I don't see the code for reopening bitmaps in master. Is this a pending
patch?
it is in block.c, in bdrv_reopen_commit()
...
if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
Error *local_err = NULL;
if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
...
Kevin
--
Best regards,
Vladimir
