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

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

[Fam Zheng]

v3: Fix test case. [Max]

v2: Use update_flags_from_options. [Kevin]

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 (2):
  block: Fix flags in reopen queue
  iotests: Add regression test for commit base locking

 block.c                    |  7 +++++++
 tests/qemu-iotests/153     | 12 ++++++++++++
 tests/qemu-iotests/153.out |  5 +++++
 3 files changed, 24 insertions(+)

-- 
2.14.3

[Qemu-devel] [PATCH v3 1/2] block: Fix flags in reopen queue

[Fam Zheng]

Reopen flags are not synchronized according to the
bdrv_reopen_queue_child precedence until bdrv_reopen_prepare. It is a
bit too late: we already check the consistency in bdrv_check_perm before
that.

This fixes the bug that when bdrv_reopen a RO node as RW, the flags for
backing child are wrong. Before, we could recurse with flags.rw=1; now,
role->inherit_options + update_flags_from_options will make sure to
clear the bit when necessary.  Note that this will not clear an
explicitly set bit, as in the case of parallel block jobs (e.g.
test_stream_parallel in 030), because the explicit options include
'read-only=false' (for an intermediate node used by a different job).

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

diff --git a/block.c b/block.c
index 75a9fd49de..a121d2ebcc 100644
--- a/block.c
+++ b/block.c
@@ -2883,8 +2883,15 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
     /* Inherit from parent node */
     if (parent_options) {
+        QemuOpts *opts;
+        QDict *options_copy;
         assert(!flags);
         role->inherit_options(&flags, options, parent_flags, parent_options);
+        options_copy = qdict_clone_shallow(options);
+        opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+        qemu_opts_absorb_qdict(opts, options_copy, NULL);
+        update_flags_from_options(&flags, opts);
+        qemu_opts_del(opts);
     }
 
     /* Old values are used for options that aren't set yet */
-- 
2.14.3

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

[Fam Zheng]

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

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index adfd02695b..a0fd815483 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -178,6 +178,18 @@ 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,if=none \
+             -device virtio-blk,drive=drive0
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+_run_cmd $QEMU_IMG commit -b "${TEST_IMG}.b" "${TEST_IMG}.c"
+
+_cleanup_qemu
+
 _launch_qemu
 
 _send_qemu_cmd $QEMU_HANDLE \
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 34309cfb20..bb721cb747 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -372,6 +372,11 @@ Is another process using the image?
 == Symbolic link ==
 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 ==
+{"return": {}}
+
+_qemu_img_wrapper commit -b TEST_DIR/t.qcow2.b TEST_DIR/t.qcow2.c
 {"return": {}}
 Adding drive
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 0/2] block: Fix permission during reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On 2018-03-13 15:20, Fam Zheng wrote:
> v3: Fix test case. [Max]
> 
> v2: Use update_flags_from_options. [Kevin]
> 
> 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 (2):
>   block: Fix flags in reopen queue
>   iotests: Add regression test for commit base locking

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v3 0/2] block: Fix permission during reopen

[Kevin Wolf]

Am 13.03.2018 um 15:20 hat Fam Zheng geschrieben:
> v3: Fix test case. [Max]
> 
> v2: Use update_flags_from_options. [Kevin]
> 
> 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.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v3 1/2] block: Fix flags in reopen queue

[Kevin Wolf]

Am 13.03.2018 um 15:20 hat Fam Zheng geschrieben:
> Reopen flags are not synchronized according to the
> bdrv_reopen_queue_child precedence until bdrv_reopen_prepare. It is a
> bit too late: we already check the consistency in bdrv_check_perm before
> that.
> 
> This fixes the bug that when bdrv_reopen a RO node as RW, the flags for
> backing child are wrong. Before, we could recurse with flags.rw=1; now,
> role->inherit_options + update_flags_from_options will make sure to
> clear the bit when necessary.  Note that this will not clear an
> explicitly set bit, as in the case of parallel block jobs (e.g.
> test_stream_parallel in 030), because the explicit options include
> 'read-only=false' (for an intermediate node used by a different job).
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 75a9fd49de..a121d2ebcc 100644
> --- a/block.c
> +++ b/block.c
> @@ -2883,8 +2883,15 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>  
>      /* Inherit from parent node */
>      if (parent_options) {
> +        QemuOpts *opts;
> +        QDict *options_copy;
>          assert(!flags);
>          role->inherit_options(&flags, options, parent_flags, parent_options);
> +        options_copy = qdict_clone_shallow(options);
> +        opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
> +        qemu_opts_absorb_qdict(opts, options_copy, NULL);
> +        update_flags_from_options(&flags, opts);
> +        qemu_opts_del(opts);

Squashed in a line here after Fam and Max agreed on IRC:

+        QDECREF(options_copy);

>      }
>  
>      /* Old values are used for options that aren't set yet */

Kevin