[Qemu-devel] [PATCH for-2.10 0/5] block: bdrv_reopen() fixes

[Qemu-devel] [PATCH for-2.10 0/5] block: bdrv_reopen() fixes

[Kevin Wolf]

This is the first part of some fixes to bdrv_reopen(), which seems
reasonable enough to merge for 2.10.

There is much more wrong with bdrv_reopen() currently, especially with
respect to op blocker permissions (basically the required permissions
can change based on the options used in bdrv_reopen(), and both things
affect the whole tree and aren't integrated with each other at all), but
I have little hope to get this fixed before 2.10, so let's start with
what is ready now.

Kevin Wolf (5):
  block: Fix order in bdrv_replace_child()
  block: Allow reopen rw without BDRV_O_ALLOW_RDWR
  block: Set BDRV_O_ALLOW_RDWR during rw reopen
  qemu-io: Allow reopen read-write
  qemu-iotests: Test reopen between read-only and read-write

 include/block/block.h      |  3 +-
 block.c                    | 20 +++++++++-----
 qemu-io-cmds.c             | 19 +++++++++++--
 tests/qemu-iotests/187     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/187.out | 18 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 120 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

-- 
2.13.3

[Qemu-devel] [PATCH for-2.10 1/5] block: Fix order in bdrv_replace_child()

[Kevin Wolf]

Commit 8ee03995 refactored the code incorrectly and broke the release of
permissions on the old BDS. Instead of changing the permissions to the
new required values after removing the old BDS from the list of
children, it only re-obtains the permissions it already had.

Change the order of operations so that the old BDS is removed again
before calculating the new required permissions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index ce9cce7b3c..ab908cdc50 100644
--- a/block.c
+++ b/block.c
@@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
     BlockDriverState *old_bs = child->bs;
     uint64_t perm, shared_perm;
 
+    bdrv_replace_child_noperm(child, new_bs);
+
     if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
@@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
-    bdrv_replace_child_noperm(child, new_bs);
-
     if (new_bs) {
         bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
         bdrv_set_perm(new_bs, perm, shared_perm);
-- 
2.13.3

[Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Kevin Wolf]

BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
reopen a node read-write temporarily because the user requested
read-write for the top-level image, but qemu decided that read-only is
enough for this node (a backing file).

bdrv_reopen() is different, it is also used for cases where the user
changed their mind and wants to update the options. There is no reason
to forbid making a node read-write in that case.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  3 ++-
 block.c               | 11 +++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 34770bb33a..ab80195378 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
 
 bool bdrv_is_read_only(BlockDriverState *bs);
 bool bdrv_is_writable(BlockDriverState *bs);
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+                           bool ignore_allow_rdw, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ab908cdc50..2711c3dd3b 100644
--- a/block.c
+++ b/block.c
@@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
     return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
 }
 
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+                           bool ignore_allow_rdw, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
     if (bs->copy_on_read && read_only) {
@@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
     }
 
     /* Do not clear read_only if it is prohibited */
-    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
+        !ignore_allow_rdw)
+    {
         error_setg(errp, "Node '%s' is read only",
                    bdrv_get_device_or_node_name(bs));
         return -EPERM;
@@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     int ret = 0;
 
-    ret = bdrv_can_set_read_only(bs, read_only, errp);
+    ret = bdrv_can_set_read_only(bs, read_only, false, errp);
     if (ret < 0) {
         return ret;
     }
@@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
      * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
      * not set, or if the BDS still has copy_on_read enabled */
     read_only = !(reopen_state->flags & BDRV_O_RDWR);
-    ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
+    ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error;
-- 
2.13.3

[Qemu-devel] [PATCH for-2.10 3/5] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Kevin Wolf]

Reopening an image should be consistent with opening it, so we should
set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
bdrv_open_inherit().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 2711c3dd3b..3615a6809e 100644
--- a/block.c
+++ b/block.c
@@ -2729,8 +2729,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     bdrv_join_options(bs, options, old_options);
     QDECREF(old_options);
 
-    /* bdrv_open() masks this flag out */
+    /* bdrv_open_inherit() sets and clears some additional flags internally */
     flags &= ~BDRV_O_PROTOCOL;
+    if (flags & BDRV_O_RDWR) {
+        flags |= BDRV_O_ALLOW_RDWR;
+    }
 
     QLIST_FOREACH(child, &bs->children, next) {
         QDict *new_child_options;
-- 
2.13.3

[Qemu-devel] [PATCH for-2.10 4/5] qemu-io: Allow reopen read-write

[Kevin Wolf]

This allows qemu-iotests to test the switch between read-only and
read-write mode for block devices.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3eb42c6728..2811a89099 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1920,6 +1920,7 @@ static void reopen_help(void)
 " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 image\n"
 "\n"
 " -r, -- Reopen the image read-only\n"
+" -w, -- Reopen the image read-write\n"
 " -c, -- Change the cache mode to the given value\n"
 " -o, -- Changes block driver options (cf. 'open' command)\n"
 "\n");
@@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = {
        .argmin         = 0,
        .argmax         = -1,
        .cfunc          = reopen_f,
-       .args           = "[-r] [-c cache] [-o options]",
+       .args           = "[(-r|-w)] [-c cache] [-o options]",
        .oneline        = "reopens an image with new options",
        .help           = reopen_help,
 };
@@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
     int c;
     int flags = bs->open_flags;
     bool writethrough = !blk_enable_write_cache(blk);
+    bool has_rw_option = false;
 
     BlockReopenQueue *brq;
     Error *local_err = NULL;
 
-    while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+    while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
         switch (c) {
         case 'c':
             if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) {
@@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
             }
             break;
         case 'r':
+            if (has_rw_option) {
+                error_report("Only one -r/-w option may be given");
+                return 0;
+            }
             flags &= ~BDRV_O_RDWR;
+            has_rw_option = true;
+            break;
+        case 'w':
+            if (has_rw_option) {
+                error_report("Only one -r/-w option may be given");
+                return 0;
+            }
+            flags |= BDRV_O_RDWR;
+            has_rw_option = true;
             break;
         default:
             qemu_opts_reset(&reopen_opts);
-- 
2.13.3

[Qemu-devel] [PATCH for-2.10 5/5] qemu-iotests: Test reopen between read-only and read-write

[Kevin Wolf]

This serves as a regression test for the bugs that were just fixed for
bdrv_reopen() between read-only and read-write mode.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/187     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/187.out | 18 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187
new file mode 100755
index 0000000000..7bb783363c
--- /dev/null
+++ b/tests/qemu-iotests/187
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test switching between read-only and read-write
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+    rm -f "$TEST_IMG.2"
+    rm -f "$TEST_IMG.3"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+
+echo
+echo "Start from read-only"
+echo
+
+$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+
+echo
+echo "Start from read-write"
+echo
+
+$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
new file mode 100644
index 0000000000..68fb944cd5
--- /dev/null
+++ b/tests/qemu-iotests/187.out
@@ -0,0 +1,18 @@
+QA output created by 187
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+Start from read-only
+
+Block node is read-only
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Block node is read-only
+
+Start from read-write
+
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Operation not permitted
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 823811076d..1848077932 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -182,6 +182,7 @@
 183 rw auto migration
 185 rw auto
 186 rw auto
+187 rw auto
 188 rw auto quick
 189 rw auto
 190 rw auto quick
-- 
2.13.3

Re: [Qemu-devel] [PATCH for-2.10 1/5] block: Fix order in bdrv_replace_child()

[Eric Blake]

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

On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> Commit 8ee03995 refactored the code incorrectly and broke the release of
> permissions on the old BDS. Instead of changing the permissions to the
> new required values after removing the old BDS from the list of
> children, it only re-obtains the permissions it already had.
> 
> Change the order of operations so that the old BDS is removed again
> before calculating the new required permissions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Eric Blake]

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

On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> reopen a node read-write temporarily because the user requested
> read-write for the top-level image, but qemu decided that read-only is
> enough for this node (a backing file).
> 
> bdrv_reopen() is different, it is also used for cases where the user
> changed their mind and wants to update the options. There is no reason
> to forbid making a node read-write in that case.

Hmm, I wonder.  https://bugzilla.redhat.com/show_bug.cgi?id=1465320
details a failure when starting qemu with a read-write NBD disk, then
taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
intermediate commit (snap2 into nbd) works but live commit (snap3 into
nbd) fails with a message that nbd does not support reopening.  I'm
presuming that your series may help to address that; I'll give it a spin
and see what happens.  But first, the code review:

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  3 ++-
>  block.c               | 11 +++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.10 3/5] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Eric Blake]

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

On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> Reopening an image should be consistent with opening it, so we should
> set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
> bdrv_open_inherit().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.10 4/5] qemu-io: Allow reopen read-write

[Eric Blake]

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

On 08/03/2017 10:03 AM, Kevin Wolf wrote:
> This allows qemu-iotests to test the switch between read-only and
> read-write mode for block devices.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io-cmds.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.10 5/5] qemu-iotests: Test reopen between read-only and read-write

[Eric Blake]

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

On 08/03/2017 10:03 AM, Kevin Wolf wrote:
> This serves as a regression test for the bugs that were just fixed for
> bdrv_reopen() between read-only and read-write mode.

If I'm right that this also fixes the difference between intermediate
vs. live commit to an initial read-write image that can't be reopened,
can we add that to this test?

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/187     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/187.out | 18 ++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 88 insertions(+)
>  create mode 100755 tests/qemu-iotests/187
>  create mode 100644 tests/qemu-iotests/187.out
> 

> +
> +_cleanup()
> +{
> +	_cleanup_test_img
> +    rm -f "$TEST_IMG.2"
> +    rm -f "$TEST_IMG.3"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15

There are pending patches for 2.11 that will need to tweak this. But for
2.10, this is fine.


> +++ b/tests/qemu-iotests/187.out
> @@ -0,0 +1,18 @@
> +QA output created by 187
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +
> +Start from read-only
> +
> +Block node is read-only
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Block node is read-only
> +

Why the difference in error messages, when starting read-only,

> +Start from read-write
> +
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +write failed: Operation not permitted

vs. when reopened read-only?

I don't see it as a flaw in the test, so much as another odd difference
in code paths that we may later want to improve.

So, as written,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 1/5] block: Fix order in bdrv_replace_child()

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:02:57PM +0200, Kevin Wolf wrote:
> Commit 8ee03995 refactored the code incorrectly and broke the release of
> permissions on the old BDS. Instead of changing the permissions to the
> new required values after removing the old BDS from the list of
> children, it only re-obtains the permissions it already had.
> 
> Change the order of operations so that the old BDS is removed again
> before calculating the new required permissions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce9cce7b3c..ab908cdc50 100644
> --- a/block.c
> +++ b/block.c
> @@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>      BlockDriverState *old_bs = child->bs;
>      uint64_t perm, shared_perm;
>  
> +    bdrv_replace_child_noperm(child, new_bs);
> +
>      if (old_bs) {
>          /* Update permissions for old node. This is guaranteed to succeed
>           * because we're just taking a parent away, so we're loosening
> @@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>          bdrv_set_perm(old_bs, perm, shared_perm);
>      }
>  
> -    bdrv_replace_child_noperm(child, new_bs);
> -
>      if (new_bs) {
>          bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
>          bdrv_set_perm(new_bs, perm, shared_perm);
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:02:58PM +0200, Kevin Wolf wrote:
> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> reopen a node read-write temporarily because the user requested
> read-write for the top-level image, but qemu decided that read-only is
> enough for this node (a backing file).
> 
> bdrv_reopen() is different, it is also used for cases where the user
> changed their mind and wants to update the options. There is no reason
> to forbid making a node read-write in that case.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  3 ++-
>  block.c               | 11 +++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 34770bb33a..ab80195378 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>  
>  bool bdrv_is_read_only(BlockDriverState *bs);
>  bool bdrv_is_writable(BlockDriverState *bs);
> -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> +                           bool ignore_allow_rdw, Error **errp);
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
> diff --git a/block.c b/block.c
> index ab908cdc50..2711c3dd3b 100644
> --- a/block.c
> +++ b/block.c
> @@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
>      return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
>  }
>  
> -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> +                           bool ignore_allow_rdw, Error **errp)

I think 'override_allow_rdw' may be a bit clearer, but that is just
bikeshedding.

Reviewed-by: Jeff Cody <jcody@redhat.com>


>  {
>      /* Do not set read_only if copy_on_read is enabled */
>      if (bs->copy_on_read && read_only) {
> @@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>      }
>  
>      /* Do not clear read_only if it is prohibited */
> -    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
> +    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> +        !ignore_allow_rdw)
> +    {
>          error_setg(errp, "Node '%s' is read only",
>                     bdrv_get_device_or_node_name(bs));
>          return -EPERM;
> @@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
>      int ret = 0;
>  
> -    ret = bdrv_can_set_read_only(bs, read_only, errp);
> +    ret = bdrv_can_set_read_only(bs, read_only, false, errp);
>      if (ret < 0) {
>          return ret;
>      }
> @@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>       * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
>       * not set, or if the BDS still has copy_on_read enabled */
>      read_only = !(reopen_state->flags & BDRV_O_RDWR);
> -    ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
> +    ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto error;
> -- 
> 2.13.3
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 3/5] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:02:59PM +0200, Kevin Wolf wrote:
> Reopening an image should be consistent with opening it, so we should
> set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
> bdrv_open_inherit().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 2711c3dd3b..3615a6809e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2729,8 +2729,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      bdrv_join_options(bs, options, old_options);
>      QDECREF(old_options);
>  
> -    /* bdrv_open() masks this flag out */
> +    /* bdrv_open_inherit() sets and clears some additional flags internally */
>      flags &= ~BDRV_O_PROTOCOL;
> +    if (flags & BDRV_O_RDWR) {
> +        flags |= BDRV_O_ALLOW_RDWR;
> +    }
>  
>      QLIST_FOREACH(child, &bs->children, next) {
>          QDict *new_child_options;
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 4/5] qemu-io: Allow reopen read-write

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:03:00PM +0200, Kevin Wolf wrote:
> This allows qemu-iotests to test the switch between read-only and
> read-write mode for block devices.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io-cmds.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 3eb42c6728..2811a89099 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1920,6 +1920,7 @@ static void reopen_help(void)
>  " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 image\n"
>  "\n"
>  " -r, -- Reopen the image read-only\n"
> +" -w, -- Reopen the image read-write\n"
>  " -c, -- Change the cache mode to the given value\n"
>  " -o, -- Changes block driver options (cf. 'open' command)\n"
>  "\n");
> @@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = {
>         .argmin         = 0,
>         .argmax         = -1,
>         .cfunc          = reopen_f,
> -       .args           = "[-r] [-c cache] [-o options]",
> +       .args           = "[(-r|-w)] [-c cache] [-o options]",
>         .oneline        = "reopens an image with new options",
>         .help           = reopen_help,
>  };
> @@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
>      int c;
>      int flags = bs->open_flags;
>      bool writethrough = !blk_enable_write_cache(blk);
> +    bool has_rw_option = false;
>  
>      BlockReopenQueue *brq;
>      Error *local_err = NULL;
>  
> -    while ((c = getopt(argc, argv, "c:o:r")) != -1) {
> +    while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
>          switch (c) {
>          case 'c':
>              if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) {
> @@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
>              }
>              break;
>          case 'r':
> +            if (has_rw_option) {
> +                error_report("Only one -r/-w option may be given");
> +                return 0;
> +            }
>              flags &= ~BDRV_O_RDWR;
> +            has_rw_option = true;
> +            break;
> +        case 'w':
> +            if (has_rw_option) {
> +                error_report("Only one -r/-w option may be given");
> +                return 0;
> +            }
> +            flags |= BDRV_O_RDWR;
> +            has_rw_option = true;
>              break;
>          default:
>              qemu_opts_reset(&reopen_opts);
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 5/5] qemu-iotests: Test reopen between read-only and read-write

[Jeff Cody]

On Thu, Aug 03, 2017 at 05:03:01PM +0200, Kevin Wolf wrote:
> This serves as a regression test for the bugs that were just fixed for
> bdrv_reopen() between read-only and read-write mode.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/187     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/187.out | 18 ++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 88 insertions(+)
>  create mode 100755 tests/qemu-iotests/187
>  create mode 100644 tests/qemu-iotests/187.out
> 
> diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187
> new file mode 100755
> index 0000000000..7bb783363c
> --- /dev/null
> +++ b/tests/qemu-iotests/187
> @@ -0,0 +1,69 @@
> +#!/bin/bash
> +#
> +# Test switching between read-only and read-write
> +#
> +# Copyright (C) 2017 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +	_cleanup_test_img
> +    rm -f "$TEST_IMG.2"
> +    rm -f "$TEST_IMG.3"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +

As Eric noted, I'll need to make sure to add this to my series to strip out
the cleanup.



Reviewed-by: Jeff Cody <jcody@redhat.com>


> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +size=64M
> +_make_test_img $size
> +
> +echo
> +echo "Start from read-only"
> +echo
> +
> +$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +
> +echo
> +echo "Start from read-write"
> +echo
> +
> +$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
> +
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
> new file mode 100644
> index 0000000000..68fb944cd5
> --- /dev/null
> +++ b/tests/qemu-iotests/187.out
> @@ -0,0 +1,18 @@
> +QA output created by 187
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +
> +Start from read-only
> +
> +Block node is read-only
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Block node is read-only
> +
> +Start from read-write
> +
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +write failed: Operation not permitted
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 823811076d..1848077932 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -182,6 +182,7 @@
>  183 rw auto migration
>  185 rw auto
>  186 rw auto
> +187 rw auto
>  188 rw auto quick
>  189 rw auto
>  190 rw auto quick
> -- 
> 2.13.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Eric Blake]

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

On 08/03/2017 10:21 AM, Eric Blake wrote:
> On 08/03/2017 10:02 AM, Kevin Wolf wrote:
>> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
>> reopen a node read-write temporarily because the user requested
>> read-write for the top-level image, but qemu decided that read-only is
>> enough for this node (a backing file).
>>
>> bdrv_reopen() is different, it is also used for cases where the user
>> changed their mind and wants to update the options. There is no reason
>> to forbid making a node read-write in that case.
> 
> Hmm, I wonder.  https://bugzilla.redhat.com/show_bug.cgi?id=1465320
> details a failure when starting qemu with a read-write NBD disk, then
> taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
> intermediate commit (snap2 into nbd) works but live commit (snap3 into
> nbd) fails with a message that nbd does not support reopening.  I'm
> presuming that your series may help to address that; I'll give it a spin
> and see what happens.

Nope, even with your patches, I'm still getting:

{'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
{"return": {}}
{"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
2097152, "offset": 2097152, "speed": 0, "type": "commit"}}

{'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
{"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
node '#block048' does not support reopening files"}}

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Kevin Wolf]

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

Am 04.08.2017 um 03:49 hat Eric Blake geschrieben:
> On 08/03/2017 10:21 AM, Eric Blake wrote:
> > On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> >> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> >> reopen a node read-write temporarily because the user requested
> >> read-write for the top-level image, but qemu decided that read-only is
> >> enough for this node (a backing file).
> >>
> >> bdrv_reopen() is different, it is also used for cases where the user
> >> changed their mind and wants to update the options. There is no reason
> >> to forbid making a node read-write in that case.
> > 
> > Hmm, I wonder.  https://bugzilla.redhat.com/show_bug.cgi?id=1465320
> > details a failure when starting qemu with a read-write NBD disk, then
> > taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
> > intermediate commit (snap2 into nbd) works but live commit (snap3 into
> > nbd) fails with a message that nbd does not support reopening.  I'm
> > presuming that your series may help to address that; I'll give it a spin
> > and see what happens.
> 
> Nope, even with your patches, I'm still getting:
> 
> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
> {"return": {}}
> {"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
> 2097152, "offset": 2097152, "speed": 0, "type": "commit"}}
> 
> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
> {"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
> node '#block048' does not support reopening files"}}

That's simply NBD not implementing .bdrv_reopen_*. In other words, not a
bug, but just a missing feature.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 2/5] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Eric Blake]

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

On 08/04/2017 05:20 AM, Kevin Wolf wrote:
>>>
>>> Hmm, I wonder.  https://bugzilla.redhat.com/show_bug.cgi?id=1465320
>>> details a failure when starting qemu with a read-write NBD disk, then
>>> taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
>>> intermediate commit (snap2 into nbd) works but live commit (snap3 into
>>> nbd) fails with a message that nbd does not support reopening.  I'm
>>> presuming that your series may help to address that; I'll give it a spin
>>> and see what happens.
>>
>> Nope, even with your patches, I'm still getting:
>>
>> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
>> {"return": {}}
>> {"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
>> "BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
>> 2097152, "offset": 2097152, "speed": 0, "type": "commit"}}
>>
>> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
>> {"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
>> node '#block048' does not support reopening files"}}
> 
> That's simply NBD not implementing .bdrv_reopen_*. In other words, not a
> bug, but just a missing feature.

But why does intermediate commit work, while live commit fails?  Both
require that the image being committed into be read-write, and the NBD
image was opened read-write (even if it can be treated as read-only for
the duration of time that it is a backing image).  I understand missing
.bdrv_reopen making it impossible to change read-only to read-write, but
when missing it means you can never change read-write down to the
tighter read-only originally, then what is making live commit different
from intermediate in not being able to handle it?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 0/5] block: bdrv_reopen() fixes

[John Snow]

On 08/03/2017 11:02 AM, Kevin Wolf wrote:
> This is the first part of some fixes to bdrv_reopen(), which seems
> reasonable enough to merge for 2.10.
> 
> There is much more wrong with bdrv_reopen() currently, especially with
> respect to op blocker permissions (basically the required permissions
> can change based on the options used in bdrv_reopen(), and both things
> affect the whole tree and aren't integrated with each other at all), but
> I have little hope to get this fixed before 2.10, so let's start with
> what is ready now.
> 
> Kevin Wolf (5):
>   block: Fix order in bdrv_replace_child()
>   block: Allow reopen rw without BDRV_O_ALLOW_RDWR
>   block: Set BDRV_O_ALLOW_RDWR during rw reopen
>   qemu-io: Allow reopen read-write
>   qemu-iotests: Test reopen between read-only and read-write
> 
>  include/block/block.h      |  3 +-
>  block.c                    | 20 +++++++++-----
>  qemu-io-cmds.c             | 19 +++++++++++--
>  tests/qemu-iotests/187     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/187.out | 18 ++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 120 insertions(+), 10 deletions(-)
>  create mode 100755 tests/qemu-iotests/187
>  create mode 100644 tests/qemu-iotests/187.out
> 

Not that you need it, but I reviewed the series because I wanted to know
what this change was, so while I'm here:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH for-2.10 0/5] block: bdrv_reopen() fixes

[Kevin Wolf]

Am 03.08.2017 um 17:02 hat Kevin Wolf geschrieben:
> This is the first part of some fixes to bdrv_reopen(), which seems
> reasonable enough to merge for 2.10.
> 
> There is much more wrong with bdrv_reopen() currently, especially with
> respect to op blocker permissions (basically the required permissions
> can change based on the options used in bdrv_reopen(), and both things
> affect the whole tree and aren't integrated with each other at all), but
> I have little hope to get this fixed before 2.10, so let's start with
> what is ready now.

Applied to the block branch.

Kevin