[Qemu-devel] [PATCH 0/4] block: Fix permission during reopen

[Qemu-devel] [PATCH 0/4] block: Fix permission during reopen

[Fam Zheng]

We write open the whole backing chain during reopen. It is not necessary and
will cause image locking problems if the backing image is shared.

Fam Zheng (4):
  block: Pass "current_flags" in BdrvChildRole.inherit_options
  block: Fix write flags in bdrv_backing_options
  block: Use the actual current_flags during reopen
  iotests: Add regression test for commit base locking

 block.c                    | 52 +++++++++++++++++++++++++++++-----------------
 block/block-backend.c      |  3 ++-
 block/vvfat.c              |  3 ++-
 include/block/block_int.h  |  3 ++-
 tests/qemu-iotests/153     |  6 ++++++
 tests/qemu-iotests/153.out |  4 ++++
 6 files changed, 49 insertions(+), 22 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/4] block: Pass "current_flags" in BdrvChildRole.inherit_options

[Fam Zheng]

So that the current flags can influence the returned flags in the coming
patches.

For now, 0 is passed everywhere and the parameter is not used.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 20 +++++++++++++-------
 block/block-backend.c     |  3 ++-
 block/vvfat.c             |  3 ++-
 include/block/block_int.h |  3 ++-
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 4f76714f6b..bedbe208e6 100644
--- a/block.c
+++ b/block.c
@@ -869,7 +869,8 @@ static void bdrv_temp_snapshot_options(int *child_flags, QDict *child_options,
  * is expected, based on the given options and flags for the parent BDS
  */
 static void bdrv_inherited_options(int *child_flags, QDict *child_options,
-                                   int parent_flags, QDict *parent_options)
+                                   int parent_flags, QDict *parent_options,
+                                   int current_flags)
 {
     int flags = parent_flags;
 
@@ -913,10 +914,12 @@ const BdrvChildRole child_file = {
  * flags for the parent BDS
  */
 static void bdrv_inherited_fmt_options(int *child_flags, QDict *child_options,
-                                       int parent_flags, QDict *parent_options)
+                                       int parent_flags, QDict *parent_options,
+                                       int current_flags)
 {
     child_file.inherit_options(child_flags, child_options,
-                               parent_flags, parent_options);
+                               parent_flags, parent_options,
+                               current_flags);
 
     *child_flags &= ~(BDRV_O_PROTOCOL | BDRV_O_NO_IO);
 }
@@ -991,7 +994,8 @@ static void bdrv_backing_detach(BdrvChild *c)
  * given options and flags for the parent BDS
  */
 static void bdrv_backing_options(int *child_flags, QDict *child_options,
-                                 int parent_flags, QDict *parent_options)
+                                 int parent_flags, QDict *parent_options,
+                                 int current_flags)
 {
     int flags = parent_flags;
 
@@ -2548,7 +2552,8 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     if (child_role) {
         bs->inherits_from = parent;
         child_role->inherit_options(&flags, options,
-                                    parent->open_flags, parent->options);
+                                    parent->open_flags, parent->options,
+                                    0);
     }
 
     ret = bdrv_fill_options(&options, filename, &flags, &local_err);
@@ -2576,7 +2581,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
                                    flags, options);
         /* Let bdrv_backing_options() override "read-only" */
         qdict_del(options, BDRV_OPT_READ_ONLY);
-        bdrv_backing_options(&flags, options, flags, options);
+        bdrv_backing_options(&flags, options, flags, options, 0);
     }
 
     bs->open_flags = flags;
@@ -2837,7 +2842,8 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     /* Inherit from parent node */
     if (parent_options) {
         assert(!flags);
-        role->inherit_options(&flags, options, parent_flags, parent_options);
+        role->inherit_options(&flags, options, parent_flags, parent_options,
+                              0);
     }
 
     /* Old values are used for options that aren't set yet */
diff --git a/block/block-backend.c b/block/block-backend.c
index b3c790e2bd..a9a72f5aa8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -107,7 +107,8 @@ static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
     QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
 
 static void blk_root_inherit_options(int *child_flags, QDict *child_options,
-                                     int parent_flags, QDict *parent_options)
+                                     int parent_flags, QDict *parent_options,
+                                     int current_flags)
 {
     /* We're not supposed to call this function for root nodes */
     abort();
diff --git a/block/vvfat.c b/block/vvfat.c
index 4a17a49e12..ae90aa0af0 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3126,7 +3126,8 @@ static BlockDriver vvfat_write_target = {
 };
 
 static void vvfat_qcow_options(int *child_flags, QDict *child_options,
-                               int parent_flags, QDict *parent_options)
+                               int parent_flags, QDict *parent_options,
+                               int current_flags)
 {
     qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "off");
     *child_flags = BDRV_O_NO_FLUSH;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 64a5700f2b..fcda7220a2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -535,7 +535,8 @@ struct BdrvChildRole {
     bool stay_at_node;
 
     void (*inherit_options)(int *child_flags, QDict *child_options,
-                            int parent_flags, QDict *parent_options);
+                            int parent_flags, QDict *parent_options,
+                            int current_flags);
 
     void (*change_media)(BdrvChild *child, bool load);
     void (*resize)(BdrvChild *child);
-- 
2.14.3

[Qemu-devel] [PATCH 2/4] block: Fix write flags in bdrv_backing_options

[Fam Zheng]

When the child in question already has write access, we shouldn't change
that just because the parent being open/reopen doesn't need it. But if
we are inherenting the flags from a writable parent, we shouldn't copy
the BDRV_O_RDWR flag, which is consistent with the BDRV_OPT_READ_ONLY
option we set.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index bedbe208e6..937f9fe159 100644
--- a/block.c
+++ b/block.c
@@ -997,7 +997,10 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
                                  int parent_flags, QDict *parent_options,
                                  int current_flags)
 {
-    int flags = parent_flags;
+    /* write flag should be preserved if the backing file already has it. This
+     * is important during reopen, to keep "parallel block jobs" work. */
+    int write_flag = current_flags & BDRV_O_RDWR;
+    int flags = parent_flags | write_flag;
 
     /* The cache mode is inherited unmodified for backing files; except WCE,
      * which is only applied on the top level (BlockBackend) */
@@ -1005,9 +1008,13 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
     qdict_copy_default(child_options, parent_options, BDRV_OPT_FORCE_SHARE);
 
-    /* backing files always opened read-only */
-    qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
-    flags &= ~BDRV_O_COPY_ON_READ;
+
+    if (!write_flag) {
+        flags &= ~BDRV_O_COPY_ON_READ;
+        /* backing files always opened read-only */
+        qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
+        flags &= ~BDRV_O_RDWR;
+    }
 
     /* snapshot=on is handled on the top layer */
     flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_TEMPORARY);
-- 
2.14.3

[Qemu-devel] [PATCH 3/4] block: Use the actual current_flags during reopen

[Fam Zheng]

This complies to the function contract and allows bdrv_backing_options
to make the right decision.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 937f9fe159..1e2b0d822d 100644
--- a/block.c
+++ b/block.c
@@ -2785,14 +2785,14 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
                                                  BlockDriverState *bs,
                                                  QDict *options,
                                                  int flags,
-                                                 const BdrvChildRole *role,
+                                                 BdrvChild *child,
                                                  QDict *parent_options,
                                                  int parent_flags)
 {
     assert(bs != NULL);
 
     BlockReopenQueueEntry *bs_entry;
-    BdrvChild *child;
+    BdrvChild *c;
     QDict *old_options, *explicit_options;
 
     /* Make sure that the caller remembered to use a drained section. This is
@@ -2847,10 +2847,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     explicit_options = qdict_clone_shallow(options);
 
     /* Inherit from parent node */
+    assert(!!child == !!parent_options);
     if (parent_options) {
-        assert(!flags);
-        role->inherit_options(&flags, options, parent_flags, parent_options,
-                              0);
+        child->role->inherit_options(&flags, options,
+                                     parent_flags, parent_options,
+                                     child->bs->open_flags);
     }
 
     /* Old values are used for options that aren't set yet */
@@ -2881,23 +2882,23 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     bs_entry->state.perm = UINT64_MAX;
     bs_entry->state.shared_perm = 0;
 
-    QLIST_FOREACH(child, &bs->children, next) {
+    QLIST_FOREACH(c, &bs->children, next) {
         QDict *new_child_options;
         char *child_key_dot;
 
         /* reopen can only change the options of block devices that were
          * implicitly created and inherited options. For other (referenced)
          * block devices, a syntax like "backing.foo" results in an error. */
-        if (child->bs->inherits_from != bs) {
+        if (c->bs->inherits_from != bs) {
             continue;
         }
 
-        child_key_dot = g_strdup_printf("%s.", child->name);
+        child_key_dot = g_strdup_printf("%s.", c->name);
         qdict_extract_subqdict(options, &new_child_options, child_key_dot);
         g_free(child_key_dot);
 
-        bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
-                                child->role, options, flags);
+        bdrv_reopen_queue_child(bs_queue, c->bs, new_child_options, 0,
+                                c, options, flags);
     }
 
     return bs_queue;
-- 
2.14.3

[Qemu-devel] [PATCH 4/4] iotests: Add regression test for commit base locking

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/153     | 6 ++++++
 tests/qemu-iotests/153.out | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index fa25eb24bd..5a82acebd3 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -177,6 +177,12 @@ rm -f "${TEST_IMG}.lnk" &>/dev/null
 ln -s ${TEST_IMG} "${TEST_IMG}.lnk" || echo "Failed to create link"
 _run_qemu_with_images "${TEST_IMG}.lnk" "${TEST_IMG}"
 
+echo
+echo "== Active commit to intermediate layer should work when base in use =="
+_launch_qemu -drive format=$IMGFMT,file="${TEST_IMG}.a",id=drive0 \
+             -device virtio-blk,drive=drive0
+_run_cmd $QEMU_IMG commit -b "${TEST_IMG}.b" "${TEST_IMG}.c"
+
 echo
 echo "== Closing an image should unlock it =="
 _launch_qemu
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 5b917b177c..60d2d661f1 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -373,6 +373,10 @@ Is another process using the image?
 QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to get "write" lock
 Is another process using the image?
 
+== Active commit to intermediate layer should work when base in use ==
+
+_qemu_img_wrapper commit -b TEST_DIR/t.qcow2.b TEST_DIR/t.qcow2.c
+
 == Closing an image should unlock it ==
 {"return": {}}
 Adding drive
-- 
2.14.3