commit: Support multiple roots above top node

There is no good reason for bdrv_drop_intermediate() to know the active layer above the subchain it is operating on - even more so, because the assumption that there is a single active layer above it is not generally true.

In order to prepare removal of the active parameter, use a BdrvChildRole callback to update the backing file string in the overlay image instead of directly calling bdrv_change_backing_file().

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block_int.h |  6 ++++++
 block.c                   | 33 ++++++++++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)

This changes the commit block job to support operation in a graph where there is more than a single active layer that references the top node.

This involves inserting the commit filter node not only on the path between the given active node and the top node, but between the top node and all of its parents.

On completion, bdrv_drop_intermediate() must consider all parents for updating the backing file link. These parents may be backing files themselves and such read-only; reopen them temporarily if necessary.

Previously this was achieved by the bdrv_reopen() calls in the commit block job that made overlay_bs read-write for the whole duration of the block job, even though write access is only needed on completion.

Now that we consider all parents, overlay_bs is meaningless. It is left in place in this commit, but we'll remove it soon.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c        | 68 ++++++++++++++++++++++++++++++++++------------------------
 block/commit.c |  2 +-
 2 files changed, 41 insertions(+), 29 deletions(-)

     /* success - we can delete the intermediate states, and link top->base */

block.c +4 -7

-    if (new_top_bs->backing->role->update_filename) {
-        backing_file_str = backing_file_str ? backing_file_str : base->filename;
-        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
-                                                         base, backing_file_str,
-                                                         &local_err);
-        if (ret < 0) {
-            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
+    backing_file_str = backing_file_str ? backing_file_str : base->filename;
+
+    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
+        /* Check whether we are allowed to switch c from top to base */

Quoted 7 lines

This changes the commit block job to support operation in a graph where there is more than a single active layer that references the top node.

This involves inserting the commit filter node not only on the path between the given active node and the top node, but between the top node and all of its parents.

Quoted 3 lines

On completion, bdrv_drop_intermediate() must consider all parents for updating the backing file link. These parents may be backing files themselves and such read-only; reopen them temporarily if necessary.

s/such/as such/

Quoted 3 lines

Previously this was achieved by the bdrv_reopen() calls in the commit block job that made overlay_bs read-write for the whole duration of the block job, even though write access is only needed on completion.

Do we know, at the start of the operation, enough to reject attempts that will eventually fail because we cannot obtain write access at completion, without having to wait for all the intermediate work before the completion phase is reached? If not, that might be a bit of a regression (where a command used to fail up front, we now waste a lot of effort, and possibly modify the backing chain irreversably, before finding out we still fail because we lack write access).

Quoted 22 lines

Now that we consider all parents, overlay_bs is meaningless. It is left in place in this commit, but we'll remove it soon.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c        | 68 ++++++++++++++++++++++++++++++++++------------------------
 block/commit.c |  2 +-
 2 files changed, 41 insertions(+), 29 deletions(-)

     /* success - we can delete the intermediate states, and link top->base */

block.c +4 -7

-    if (new_top_bs->backing->role->update_filename) {
-        backing_file_str = backing_file_str ? backing_file_str : base->filename;
-        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
-                                                         base, backing_file_str,
-                                                         &local_err);
-        if (ret < 0) {
-            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
+    backing_file_str = backing_file_str ? backing_file_str : base->filename;
+
+    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
+        /* Check whether we are allowed to switch c from top to base */

block.c int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, · line 3525

Quoted 10 lines

block.c +7 -1

+        GSList *ignore_children = g_slist_prepend(NULL, c);
+        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
+                               ignore_children, &local_err);
+        if (local_err) {
+            ret = -EPERM;
+            error_report_err(local_err);
             goto exit;
         }
-    }
+        g_slist_free(ignore_children);

And it looks like you DO check up front that permissions are sufficient for later on.

QMP responses to certain commands can become quite long, which doesn't only make reading them hard, but also means that the maximum line length in patch emails can be exceeded. Allow tests to switch to QMP pretty printing, which results in more, but shorter lines.

We also need to make sure to keep indentation in the response for this to work as expected.

Signed-off-by Kevin Wolf <[email protected]>

---
 tests/qemu-iotests/common.qemu | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Signed-off-by Kevin Wolf <[email protected]>

---
 tests/qemu-iotests/191     | 152 +++++++++
 tests/qemu-iotests/191.out | 827 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 980 insertions(+)
 create mode 100755 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

+++ b/tests/qemu-iotests/191.out

tests/qemu-iotests/191.out new file +4 -0

+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP

Review

Quoted 2 lines

(quoted diff) +2 -0

+                        "data": {
+                            "compat": "1.1",

You should make the test specifically exclude compat=0.10 images, or else have further filtering in place if we can still support this on old images.

We don't need to make any assumptions about the graph layout above the top node of the commit operation any more. Remove the use of bdrv_find_overlay() and related variables from the commit job code.

bdrv_drop_intermediate() doesn't use the 'active' parameter any more, so we can just drop it.

The overlay node was previously added to the block job to get a BLK_PERM_GRAPH_MOD. We really need to respect those permissions in bdrv_drop_intermediate() now, but as long as we haven't figured out yet how BLK_PERM_GRAPH_MOD is actually supposed to work, just leave a TODO comment there.

With this change, it is now possible to perform another block job on an overlay node without conflicts. qemu-iotests 030 is changed accordingly.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block.h  |  3 +--
 block.c                |  6 +++--
 block/commit.c         | 62 ++++++++++++--------------------------------------
 tests/qemu-iotests/030 |  4 ----
 4 files changed, 20 insertions(+), 55 deletions(-)

This is a step towards making the commit job flexible enough that it can work with any kind of block graph. Currently, it requires that not only the top and base node of the commit operation are specified, but also the active layer of the backing file chain. Of course, the assumption that a single active layer exists is invalid.

This series makes the commit job consider other roots as well so that all parent nodes of the top node get their backing file updated and stay valid after the commit job completes.

With this, we should have all of the prerequisites for a follow-up series that adds a new and clean blockdev-commit QMP command which doesn't require an option for the active layer and which accepts node names instead of file names for base and top.

Kevin Wolf (5):
block: Introduce BdrvChildRole.update_filename
commit: Support multiple roots above top node
qemu-iotests: Allow QMP pretty printing in common.qemu
qemu-iotests: Test commit block job where top has two parents
commit: Remove overlay_bs

 include/block/block.h          |   3 +-
 include/block/block_int.h      |   6 +
 block.c                        |  91 +++--
 block/commit.c                 |  64 +---
 tests/qemu-iotests/030         |   4 -
 tests/qemu-iotests/191         | 152 ++++++++
 tests/qemu-iotests/191.out     | 827 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.qemu |  14 +-
 tests/qemu-iotests/group       |   1 +
 9 files changed, 1078 insertions(+), 84 deletions(-)
 create mode 100755 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

Quoted 15 lines

This is a step towards making the commit job flexible enough that it can work with any kind of block graph. Currently, it requires that not only the top and base node of the commit operation are specified, but also the active layer of the backing file chain. Of course, the assumption that a single active layer exists is invalid.

This series makes the commit job consider other roots as well so that all parent nodes of the top node get their backing file updated and stay valid after the commit job completes.

With this, we should have all of the prerequisites for a follow-up series that adds a new and clean blockdev-commit QMP command which doesn't require an option for the active layer and which accepts node names instead of file names for base and top.

Quoted 6 lines

Kevin Wolf (5):
block: Introduce BdrvChildRole.update_filename
commit: Support multiple roots above top node
qemu-iotests: Allow QMP pretty printing in common.qemu
qemu-iotests: Test commit block job where top has two parents
commit: Remove overlay_bs

Does this depend on another series?