Re: [Qemu-devel] bug in reopen arch

[Vladimir Sementsov-Ogievskiy]

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.

> 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 
reopen_prepare.. Is it possible?

Now, I've found the following workaround, what do you think about 
something like this as a temporary fix:

diff --git a/include/block/block.h b/include/block/block.h
index e677080c4e..c21392491d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -266,7 +266,8 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver 
*drv, const char *node_name,
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs,
                                     QDict *options, int flags);
-int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, 
Error **errp);
+int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue,
+                         bool cheat_reopen_rw, Error **errp);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
diff --git a/block.c b/block.c
index 50887087f3..9b50828cd2 100644
--- a/block.c
+++ b/block.c
@@ -2988,7 +2988,8 @@ BlockReopenQueue 
*bdrv_reopen_queue(BlockReopenQueue *bs_queue,
  * All affected nodes must be drained between bdrv_reopen_queue() and
  * bdrv_reopen_multiple().
  */
-int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, 
Error **errp)
+int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue,
+                         bool cheat_reopen_rw, Error **errp)
 {
     int ret = -1;
     BlockReopenQueueEntry *bs_entry, *next;
@@ -3005,6 +3006,14 @@ int bdrv_reopen_multiple(AioContext *ctx, 
BlockReopenQueue *bs_queue, Error **er
         bs_entry->prepared = true;
     }

+    if (cheat_reopen_rw) {
+        /* reverse queue, to reopen children first */
+        QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+            QSIMPLEQ_REMOVE(bs_queue, bs_entry, BlockReopenQueueEntry, 
entry);
+            QSIMPLEQ_INSERT_HEAD(bs_queue, bs_entry, entry);
+        }
+    }
+
     /* If we reach this point, we have success and just need to apply the
      * changes
      */
@@ -3036,11 +3045,13 @@ int bdrv_reopen(BlockDriverState *bs, int 
bdrv_flags, Error **errp)
     int ret = -1;
     Error *local_err = NULL;
     BlockReopenQueue *queue;
+    bool cheat_reopen_rw = bdrv_is_read_only(bs) && (bdrv_flags & 
BDRV_O_RDWR);

     bdrv_subtree_drained_begin(bs);

     queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
-    ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, 
&local_err);
+    ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, 
cheat_reopen_rw,
+                               &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
     }
diff --git a/block/replication.c b/block/replication.c
index 826db7b304..e528969e2b 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -411,7 +411,7 @@ static void reopen_backing_file(BlockDriverState 
*bs, bool writable,

     if (reopen_queue) {
         bdrv_reopen_multiple(bdrv_get_aio_context(bs),
-                             reopen_queue, &local_err);
+                             reopen_queue, false, &local_err);
         error_propagate(errp, local_err);
     }

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5bf5f28178..e0c4de323c 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2051,7 +2051,7 @@ static int reopen_f(BlockBackend *blk, int argc, 
char **argv)

     bdrv_subtree_drained_begin(bs);
     brq = bdrv_reopen_queue(NULL, bs, opts, flags);
-    bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
+    bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, false, &local_err);
     bdrv_subtree_drained_end(bs);

     if (local_err) {


--
Best regards,
Vladimir