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? Kevin
