[Qemu-devel] [PATCH 0/2] Update the inherits_from pointer after stream and commit

[Qemu-devel] [PATCH 0/2] Update the inherits_from pointer after stream and commit

[Alberto Garcia]

Hi all,

when you open an image [A] with a few more images on the backing chain
you get something like this:

    [E] <- [D] <- [C] <- [B] <- [A]

Here you can go from [A] to [E] by following the bs->backing
pointer. At the same time each one of the backing files has an
'inherits_from' attribute pointing to their parent, so you can go from
[E] to [A] following the inherits_from pointer.

'inherits_from' is used on bdrv_reopen_queue_child() to decide if a
node's children must be reopened together with the parent and inherit
its options.

If some the intermediate nodes are removed (either by block-stream or
by block-commit) you end up with something like this:

   [E] <- [A]

In this case we would expect [E] to inherit from [A], however its
inherits_from pointer is NULL and trying to change its options by
reopening [A] with backing.option=value fails.

This patch series fixes this. See each individual patch for more
details.

Regards,

Berto

Alberto Garcia (2):
  block: Update BlockDriverState.inherits_from on bdrv_set_backing_hd()
  block: Update BlockDriverState.inherits_from on
    bdrv_drop_intermediate()

 block.c                    |  37 ++++++++++++
 tests/qemu-iotests/161     | 137 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/161.out |  39 +++++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 214 insertions(+)
 create mode 100755 tests/qemu-iotests/161
 create mode 100644 tests/qemu-iotests/161.out

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] block: Update BlockDriverState.inherits_from on bdrv_set_backing_hd()

[Alberto Garcia]

When a BlockDriverState's child is opened (be it a backing file, the
protocol layer, or any other) inherits_from is set to point to the
parent node. Children opened separately and then attached to a parent
don't have this pointer set.

bdrv_reopen_queue_child() uses this to determine whether a node's
children must also be reopened inheriting the options from the parent
or not. If inherits_from points to the parent then the child is
reopened and its options can be changed, like in this example:

   $ qemu-img create -f qcow2 hd0.qcow2 1M
   $ qemu-img create -f qcow2 hd1.qcow2 1M
   $ $QEMU -drive if=none,node-name=hd0,file=hd0.qcow2,\
                  backing.driver=qcow2,backing.file.filename=hd1.qcow2
   (qemu) qemu-io hd0 "reopen -o backing.l2-cache-size=2M"

If the child does not inherit from the parent then it does not get
reopened and its options cannot be changed:

   $ $QEMU -drive if=none,node-name=hd1,file=hd1.qcow2
           -drive if=none,node-name=hd0,file=hd0.qcow2,backing=hd1
   (qemu) qemu-io hd0 "reopen -o backing.l2-cache-size=2M"
   Cannot change the option 'backing.l2-cache-size'

If a disk image has a chain of backing files then all of them are also
connected through their inherits_from pointers (i.e. it's possible to
walk the chain in reverse order from base to top).

However this is broken if the intermediate nodes are removed using
e.g. block-stream because the inherits_from pointer from the base node
becomes NULL:

   $ qemu-img create -f qcow2 hd0.qcow2 1M
   $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
   $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2
   $ $QEMU -drive if=none,file=hd2.qcow2
   (qemu) qemu-io none0 "reopen -o backing.l2-cache-size=2M"
   (qemu) block_stream none0 0 hd0.qcow2
   (qemu) qemu-io none0 "reopen -o backing.l2-cache-size=2M"
   Cannot change the option 'backing.l2-cache-size'

This patch updates the inherits_from pointer if the intermediate nodes
of a backing chain are removed using bdrv_set_backing_hd(), and adds a
test case for this scenario.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c                    |  21 +++++++++
 tests/qemu-iotests/161     | 104 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/161.out |  23 ++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 149 insertions(+)
 create mode 100755 tests/qemu-iotests/161
 create mode 100644 tests/qemu-iotests/161.out

diff --git a/block.c b/block.c
index 08d64cdc61..b565ba805f 100644
--- a/block.c
+++ b/block.c
@@ -2228,6 +2228,18 @@ static void bdrv_parent_cb_change_media(BlockDriverState *bs, bool load)
     }
 }
 
+/* Return true if you can reach parent going through child->inherits_from
+ * recursively. If parent or child are NULL, return false */
+static bool bdrv_inherits_from_recursive(BlockDriverState *child,
+                                         BlockDriverState *parent)
+{
+    while (child && child != parent) {
+        child = child->inherits_from;
+    }
+
+    return child != NULL;
+}
+
 /*
  * Sets the backing file link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
@@ -2235,6 +2247,9 @@ static void bdrv_parent_cb_change_media(BlockDriverState *bs, bool load)
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                          Error **errp)
 {
+    bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
+        bdrv_inherits_from_recursive(backing_hd, bs);
+
     if (backing_hd) {
         bdrv_ref(backing_hd);
     }
@@ -2250,6 +2265,12 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
 
     bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
                                     errp);
+    /* If backing_hd was already part of bs's backing chain, and
+     * inherits_from pointed recursively to bs then let's update it to
+     * point directly to bs (else it will become NULL). */
+    if (update_inherits_from) {
+        backing_hd->inherits_from = bs;
+    }
     if (!bs->backing) {
         bdrv_unref(backing_hd);
     }
diff --git a/tests/qemu-iotests/161 b/tests/qemu-iotests/161
new file mode 100755
index 0000000000..8d0c6afb47
--- /dev/null
+++ b/tests/qemu-iotests/161
@@ -0,0 +1,104 @@
+#!/bin/bash
+#
+# Test reopening a backing image after block-stream
+#
+# Copyright (C) 2018 Igalia, S.L.
+#
+# 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=berto@igalia.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.base"
+    rm -f "$TEST_IMG.int"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+# Any format implementing BlockDriver.bdrv_change_backing_file
+_supported_fmt qcow2 qed
+_supported_proto file
+_supported_os Linux
+
+IMG_SIZE=1M
+
+# Create the images
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE | _filter_imgfmt
+TEST_IMG="$TEST_IMG.int" _make_test_img -b "$TEST_IMG.base" | _filter_imgfmt
+_make_test_img -b "$TEST_IMG.int" | _filter_imgfmt
+
+# First test: reopen $TEST.IMG changing the detect-zeroes option on
+# its backing file ($TEST_IMG.int).
+echo
+echo "*** Change an option on the backing file"
+echo
+_launch_qemu -drive if=none,file="${TEST_IMG}"
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io none0 \"reopen -o backing.detect-zeroes=on\"' } }" \
+    "return"
+
+_cleanup_qemu
+
+# Second test: stream $TEST_IMG.base into $TEST_IMG.int and then
+# reopen $TEST.IMG changing the detect-zeroes option on its new
+# backing file ($TEST_IMG.base).
+echo
+echo "*** Stream and then change an option on the backing file"
+echo
+_launch_qemu -drive if=none,file="${TEST_IMG}"
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'block-stream', \
+       'arguments': { 'device': 'none0',
+                      'base': '${TEST_IMG}.base' } }" \
+    'return'
+
+# Wait for block-stream to finish
+sleep 0.5
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io none0 \"reopen -o backing.detect-zeroes=on\"' } }" \
+    "return"
+
+_cleanup_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/161.out b/tests/qemu-iotests/161.out
new file mode 100644
index 0000000000..a3474717a2
--- /dev/null
+++ b/tests/qemu-iotests/161.out
@@ -0,0 +1,23 @@
+QA output created by 161
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.int
+
+*** Change an option on the backing file
+
+{"return": {}}
+{"return": ""}
+
+*** Stream and then change an option on the backing file
+
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "none0"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "none0", "len": 1048576, "offset": 1048576, "speed": 0, "type": "stream"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "none0"}}
+{"return": ""}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 31f6e77dcb..aecbd46087 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -167,6 +167,7 @@
 158 rw auto quick
 159 rw auto quick
 160 rw auto quick
+161 rw auto quick
 162 auto quick
 163 rw auto
 165 rw auto quick
-- 
2.11.0

[Qemu-devel] [PATCH 2/2] block: Update BlockDriverState.inherits_from on bdrv_drop_intermediate()

[Alberto Garcia]

The previous patch fixed the inherits_from pointer after block-stream,
and this one does the same for block-commit.

When block-commit finishes and the 'top' node is not the topmost one
from the backing chain then all nodes above 'base' up to and including
'top' are removed from the chain.

The bdrv_drop_intermediate() call converts a chain like this one:

    base <- intermediate <- top <- active

into this one:

    base <- active

In a simple scenario each backing file from the first chain has the
inherits_from attribute pointing to its parent. This means that
reopening 'active' will recursively reopen all its children, whose
options can be changed in the process.

However after the 'block-commit' call base.inherits_from is NULL and
the chain is broken, so 'base' does not inherit from 'active' and will
not be reopened automatically:

   $ qemu-img create -f qcow2 hd0.qcow2 1M
   $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
   $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2
   $ $QEMU -drive if=none,file=hd2.qcow2

   { 'execute': 'block-commit',
     'arguments': {
       'device': 'none0',
       'top': 'hd1.qcow2' } }

   { 'execute': 'human-monitor-command',
     'arguments': {
        'command-line':
          'qemu-io none0 "reopen -o backing.l2-cache-size=2M"' } }

   { "return": "Cannot change the option 'backing.l2-cache-size'\r\n"}

This patch updates base.inherits_from in this scenario, and adds a
test case.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c                    | 16 ++++++++++++++++
 tests/qemu-iotests/161     | 35 ++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/161.out | 16 ++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index b565ba805f..f2db213f4c 100644
--- a/block.c
+++ b/block.c
@@ -3809,6 +3809,8 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
 int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
                            const char *backing_file_str)
 {
+    BlockDriverState *explicit_top = top;
+    bool update_inherits_from;
     BdrvChild *c, *next;
     Error *local_err = NULL;
     int ret = -EIO;
@@ -3824,6 +3826,16 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         goto exit;
     }
 
+    /* If 'base' recursively inherits from 'top' then we should set
+     * base->inherits_from to top->inherits_from after 'top' and all
+     * other intermediate nodes have been dropped.
+     * If 'top' is an implicit node (e.g. "commit_top") we should skip
+     * it because no one inherits from it. We use explicit_top for that. */
+    while (explicit_top && explicit_top->implicit) {
+        explicit_top = backing_bs(explicit_top);
+    }
+    update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top);
+
     /* success - we can delete the intermediate states, and link top->base */
     /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once
      * we've figured out how they should work. */
@@ -3859,6 +3871,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         bdrv_unref(top);
     }
 
+    if (update_inherits_from) {
+        base->inherits_from = explicit_top->inherits_from;
+    }
+
     ret = 0;
 exit:
     bdrv_unref(top);
diff --git a/tests/qemu-iotests/161 b/tests/qemu-iotests/161
index 8d0c6afb47..180df17ad6 100755
--- a/tests/qemu-iotests/161
+++ b/tests/qemu-iotests/161
@@ -1,6 +1,6 @@
 #!/bin/bash
 #
-# Test reopening a backing image after block-stream
+# Test reopening a backing image after block-stream and block-commit
 #
 # Copyright (C) 2018 Igalia, S.L.
 #
@@ -98,6 +98,39 @@ _send_qemu_cmd $QEMU_HANDLE \
 
 _cleanup_qemu
 
+# Third test: commit $TEST_IMG.int into $TEST_IMG.base and then reopen
+# $TEST.IMG changing the detect-zeroes option on its new backing file
+# ($TEST_IMG.base).
+echo
+echo "*** Commit and then change an option on the backing file"
+echo
+# Create the images again
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE | _filter_imgfmt
+TEST_IMG="$TEST_IMG.int" _make_test_img -b "$TEST_IMG.base" | _filter_imgfmt
+_make_test_img -b "$TEST_IMG.int" | _filter_imgfmt
+
+_launch_qemu -drive if=none,file="${TEST_IMG}"
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'block-commit', \
+       'arguments': { 'device': 'none0',
+                      'top': '${TEST_IMG}.int' } }" \
+    'return'
+
+# Wait for block-commit to finish
+sleep 0.5
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io none0 \"reopen -o backing.detect-zeroes=on\"' } }" \
+    "return"
+
+_cleanup_qemu
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/161.out b/tests/qemu-iotests/161.out
index a3474717a2..39951993ee 100644
--- a/tests/qemu-iotests/161.out
+++ b/tests/qemu-iotests/161.out
@@ -20,4 +20,20 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "none0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "none0"}}
 {"return": ""}
+
+*** Commit and then change an option on the backing file
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.int
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "none0"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "none0", "len": 1048576, "offset": 1048576, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "none0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "none0"}}
+{"return": ""}
 *** done
-- 
2.11.0

Re: [Qemu-devel] [PATCH 0/2] Update the inherits_from pointer after stream and commit

[Alberto Garcia]

ping

> Hi all,
>
> when you open an image [A] with a few more images on the backing chain
> you get something like this:
>
>     [E] <- [D] <- [C] <- [B] <- [A]
>
> Here you can go from [A] to [E] by following the bs->backing
> pointer. At the same time each one of the backing files has an
> 'inherits_from' attribute pointing to their parent, so you can go from
> [E] to [A] following the inherits_from pointer.
>
> 'inherits_from' is used on bdrv_reopen_queue_child() to decide if a
> node's children must be reopened together with the parent and inherit
> its options.
>
> If some the intermediate nodes are removed (either by block-stream or
> by block-commit) you end up with something like this:
>
>    [E] <- [A]
>
> In this case we would expect [E] to inherit from [A], however its
> inherits_from pointer is NULL and trying to change its options by
> reopening [A] with backing.option=value fails.
>
> This patch series fixes this. See each individual patch for more
> details.

Re: [Qemu-devel] [PATCH 0/2] Update the inherits_from pointer after stream and commit

[Kevin Wolf]

Am 31.10.2018 um 17:16 hat Alberto Garcia geschrieben:
> Hi all,
> 
> when you open an image [A] with a few more images on the backing chain
> you get something like this:
> 
>     [E] <- [D] <- [C] <- [B] <- [A]
> 
> Here you can go from [A] to [E] by following the bs->backing
> pointer. At the same time each one of the backing files has an
> 'inherits_from' attribute pointing to their parent, so you can go from
> [E] to [A] following the inherits_from pointer.
> 
> 'inherits_from' is used on bdrv_reopen_queue_child() to decide if a
> node's children must be reopened together with the parent and inherit
> its options.
> 
> If some the intermediate nodes are removed (either by block-stream or
> by block-commit) you end up with something like this:
> 
>    [E] <- [A]
> 
> In this case we would expect [E] to inherit from [A], however its
> inherits_from pointer is NULL and trying to change its options by
> reopening [A] with backing.option=value fails.
> 
> This patch series fixes this. See each individual patch for more
> details.

Thanks, applied to the block branch.

Not a problem with the series, but I tried to run the test case without
the fix, and this is what I got:

-{"return": ""}
+{"return": "Cannot change the option 'backing.detect-zeroes'rn"}

Where does that final "rn" come from? Looks like we have a bug somewhere
in the error reporting code?

Kevin

Re: [Qemu-devel] [PATCH 0/2] Update the inherits_from pointer after stream and commit

[Alberto Garcia]

On Thu 22 Nov 2018 06:52:00 PM CET, Kevin Wolf <kwolf@redhat.com> wrote:
> Not a problem with the series, but I tried to run the test case without
> the fix, and this is what I got:
>
> -{"return": ""}
> +{"return": "Cannot change the option 'backing.detect-zeroes'rn"}
>
> Where does that final "rn" come from? Looks like we have a bug
> somewhere in the error reporting code?

It looks like a \r\n that hasn't been properly interpreted as CRLF.

Berto