[PATCH RFC 0/3] block: drop inherits_from

[PATCH RFC 0/3] block: drop inherits_from

[Vladimir Sementsov-Ogievskiy]

Hi all!

I now work on v3 for "block: update graph permissions update", and I'm at "[PATCH v2 28/36] block: add bdrv_set_backing_noperm() transaction action".

So, the problem is we should handle inherits_from carefully, and most probably it should be updated in bdrv_replace_child_noperm().. And then, bdrv_replace_child_noperm will become a transaction action, which should store old inherits_from to the transaction state for possible rollback.. Or something like this, I didn't try yet. I just thought, may be we can just drop inherits_from?

I decided to learn the thing a bit, and found that the only usage of inherits_from is to limit reopen process. When adding bs to reopen_queue we do add its children recursively, but only those which inherits from the bs.

That works so starting from

commit 67251a311371c4d22e803f151f47fe817175b6c3
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Thu Apr 9 18:54:04 2015 +0200

    block: Fix reopen flag inheritance


The commit made two things:

1. reopen recursively all* children, not only .file. That's OK.

2. * : not all, but only that inherits_from bs.

[2] Means that we don't reopen some implicitely created children.. And, I want to ask, why?

For me it seems that if we have reopen process.. And bs involved. And it has a child.. And child role defines how that child should inherit options.. Why not to just inherit them?


I decided to check iotests with dropped inherits_from feature.

For ./check -qcow2 on tmpfs only three failed: 30, 245, 258, not bad!

30 and 258 are easily fixed by assuming that if filter driver don't have .bdrv_reopen_prepare handler, it default to noop.

For 245 behavior changes in some places but it seems correct to me. And we have to fix commit job to keep reference to its filter node, otherwise we crash when remove the backing link from node to commit-top-filter of underlying commit job, which is allowed now.


I started this text as a letter, but finally I've fixed problems in 245 and decided to send and RFC series. Probably I miss some core sense of inherits_from, so that's an RFC.. So, what do you think?


Vladimir Sementsov-Ogievskiy (3):
  block/commit: keep reference on commit_top_bs
  block: allow filters to be reopened without .bdrv_reopen_prepare
  block: drop inherits_from logic

 include/block/block_int.h  |  4 --
 block.c                    | 95 ++++++--------------------------------
 block/commit.c             |  8 ++--
 tests/qemu-iotests/245     | 36 +++++++++------
 tests/qemu-iotests/245.out |  8 +++-
 5 files changed, 47 insertions(+), 104 deletions(-)

-- 
2.29.2

[PATCH 1/3] block/commit: keep reference on commit_top_bs

[Vladimir Sementsov-Ogievskiy]

Nothing prevent removing of this node during the job. Job needs this
node to be present, so it must keep the reference.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/commit.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index dd9ba87349..57f054ad5d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -71,9 +71,8 @@ static void commit_abort(Job *job)
         bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
     }
 
-    /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
+    /* Make sure top stay around until bdrv_replace_node() */
     bdrv_ref(top_bs);
-    bdrv_ref(s->commit_top_bs);
 
     if (s->base) {
         blk_unref(s->base);
@@ -93,7 +92,6 @@ static void commit_abort(Job *job)
     bdrv_replace_node(s->commit_top_bs, s->commit_top_bs->backing->bs,
                       &error_abort);
 
-    bdrv_unref(s->commit_top_bs);
     bdrv_unref(top_bs);
 }
 
@@ -109,6 +107,7 @@ static void commit_clean(Job *job)
     }
 
     g_free(s->backing_file_str);
+    bdrv_unref(s->commit_top_bs);
     blk_unref(s->top);
 }
 
@@ -317,6 +316,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
+    bdrv_ref(commit_top_bs);
+
     s->commit_top_bs = commit_top_bs;
 
     /*
@@ -416,6 +417,7 @@ fail:
      * otherwise this would fail because of lack of permissions. */
     if (commit_top_bs) {
         bdrv_replace_node(commit_top_bs, top, &error_abort);
+        bdrv_unref(commit_top_bs);
     }
 }
 
-- 
2.29.2

[PATCH 2/3] block: allow filters to be reopened without .bdrv_reopen_prepare

[Vladimir Sementsov-Ogievskiy]

We are going to drop inherits_from logic, so every child will be
recursively reopened including filters. Let's assume that by default
filters do nothing on reopen prepare.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                | 23 +++++++++++++----------
 tests/qemu-iotests/245 |  5 ++---
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 2daff6d29a..815396f460 100644
--- a/block.c
+++ b/block.c
@@ -4189,7 +4189,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
-    if (drv->bdrv_reopen_prepare) {
+    if (drv->bdrv_reopen_prepare || drv->is_filter) {
         /*
          * If a driver-specific option is missing, it means that we
          * should reset it to its default value.
@@ -4201,16 +4201,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             goto error;
         }
 
-        ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
-        if (ret) {
-            if (local_err != NULL) {
-                error_propagate(errp, local_err);
-            } else {
-                bdrv_refresh_filename(reopen_state->bs);
-                error_setg(errp, "failed while preparing to reopen image '%s'",
-                           reopen_state->bs->filename);
+        if (drv->bdrv_reopen_prepare) {
+            ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
+            if (ret) {
+                if (local_err != NULL) {
+                    error_propagate(errp, local_err);
+                } else {
+                    bdrv_refresh_filename(reopen_state->bs);
+                    error_setg(errp,
+                               "failed while preparing to reopen image '%s'",
+                               reopen_state->bs->filename);
+                }
+                goto error;
             }
-            goto error;
         }
     } else {
         /* It is currently mandatory to have a bdrv_reopen_prepare()
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 11104b9208..a7c70213dd 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -524,9 +524,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         result = self.vm.qmp('blockdev-add', conv_keys = False, **bvopts)
         self.assert_qmp(result, 'return', {})
 
-        # blkverify doesn't currently allow reopening. TODO: implement this
-        self.reopen(bvopts, {}, "Block format 'blkverify' used by node 'bv'" +
-                    " does not support reopening files")
+        # blkverify allows reopening
+        self.reopen(bvopts, {})
 
         # Illegal: hd0 is a child of the blkverify node
         self.reopen(opts[0], {'backing': 'bv'},
-- 
2.29.2

[PATCH 3/3] block: drop inherits_from logic

[Vladimir Sementsov-Ogievskiy]

inherits_from is used only to restrict recursively adding of block
children to reopen queue. Still, we have public block graph and we are
going to "blockdev" era, when user controls every nobe of block graph.

So, what's wrong in just reopening every child? Moreover, is it correct
to silently skip children hidden by filters from reopen process? We
have BdrvChildClass::inherit_options() to specify how child inherits
options. Why some dynamically created children should not inherit what
they want?

Dropping inherits_from simplifies things a bit, clearing the way for
updating fixing and refactoring of block graph permission update.

Also, inherits_from has some problems:

1. bdrv_unset_inherits_from() is called only from bdrv_unref_child(),
   when it probably should be called from bdrv_replace_child_noperm()
   to cover more cases.

2. bdrv_unset_inherits_from() tries to check that root has two children
   with same child->bs, but it doesn't check that there may be several
   long paths from root to bs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h  |  4 ---
 block.c                    | 72 --------------------------------------
 tests/qemu-iotests/245     | 31 ++++++++++------
 tests/qemu-iotests/245.out |  8 +++--
 4 files changed, 27 insertions(+), 88 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 88e4111939..ae8db04a0a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -935,10 +935,6 @@ struct BlockDriverState {
     /* operation blockers */
     QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
 
-    /* The node that this node inherited default options from (and a reopen on
-     * which can affect this node by changing these defaults). This is always a
-     * parent node of this node. */
-    BlockDriverState *inherits_from;
     QLIST_HEAD(, BdrvChild) children;
     QLIST_HEAD(, BdrvChild) parents;
 
diff --git a/block.c b/block.c
index 815396f460..af4f6095ca 100644
--- a/block.c
+++ b/block.c
@@ -2769,34 +2769,6 @@ void bdrv_root_unref_child(BdrvChild *child)
     bdrv_unref(child_bs);
 }
 
-/**
- * Clear all inherits_from pointers from children and grandchildren of
- * @root that point to @root, where necessary.
- */
-static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child)
-{
-    BdrvChild *c;
-
-    if (child->bs->inherits_from == root) {
-        /*
-         * Remove inherits_from only when the last reference between root and
-         * child->bs goes away.
-         */
-        QLIST_FOREACH(c, &root->children, next) {
-            if (c != child && c->bs == child->bs) {
-                break;
-            }
-        }
-        if (c == NULL) {
-            child->bs->inherits_from = NULL;
-        }
-    }
-
-    QLIST_FOREACH(c, &child->bs->children, next) {
-        bdrv_unset_inherits_from(root, c);
-    }
-}
-
 /* Callers must ensure that child->frozen is false. */
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
 {
@@ -2804,7 +2776,6 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
         return;
     }
 
-    bdrv_unset_inherits_from(parent, child);
     bdrv_root_unref_child(child);
 }
 
@@ -2819,18 +2790,6 @@ 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;
-}
-
 /*
  * Return the BdrvChildRole for @bs's backing child.  bs->backing is
  * mostly used for COW backing children (role = COW), but also for
@@ -2853,8 +2812,6 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                         Error **errp)
 {
     int ret = 0;
-    bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
-        bdrv_inherits_from_recursive(backing_hd, bs);
 
     if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
         return -EPERM;
@@ -2881,13 +2838,6 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
         goto out;
     }
 
-    /* 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;
-    }
-
 out:
     bdrv_refresh_limits(bs, NULL);
 
@@ -3283,7 +3233,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
             parent_is_format = true;
         }
 
-        bs->inherits_from = parent;
         child_class->inherit_options(child_role, parent_is_format,
                                      &flags, options,
                                      parent->open_flags, parent->options);
@@ -3717,13 +3666,6 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
         QDict *new_child_options = NULL;
         bool child_keep_old = keep_old_opts;
 
-        /* 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) {
-            continue;
-        }
-
         /* Check if the options contain a child reference */
         if (qdict_haskey(options, child->name)) {
             const char *childref = qdict_get_try_str(options, child->name);
@@ -4933,8 +4875,6 @@ void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base)
 int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
                            const char *backing_file_str)
 {
-    BlockDriverState *explicit_top = top;
-    bool update_inherits_from;
     BdrvChild *c;
     Error *local_err = NULL;
     int ret = -EIO;
@@ -4953,14 +4893,6 @@ 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. */
-    explicit_top = bdrv_skip_implicit_filters(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. */
@@ -5001,10 +4933,6 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         }
     }
 
-    if (update_inherits_from) {
-        base->inherits_from = explicit_top->inherits_from;
-    }
-
     ret = 0;
 exit:
     bdrv_subtree_drained_end(top);
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a7c70213dd..9fcb1b144e 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -307,8 +307,9 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(hd_opts(0), {'read-only': True})
         self.check_node_graph(original_graph)
 
-        # The backing file (hd0) is now a reference, we cannot change backing.* anymore
-        self.reopen(opts, {}, "Cannot change the option 'backing.driver'")
+        # The backing file (hd0) is now a reference, buf we can change
+        # backing.* anyway
+        self.reopen(opts, {})
 
         # We can't remove 'hd0' while it's a backing image of 'hd1'
         result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
@@ -899,7 +900,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         # We can't reopen with the original options because there is a filter
         # inserted by stream job above hd1.
         self.reopen(opts, {},
-                    "Cannot change the option 'backing.backing.file.node-name'")
+                    "Option 'bottom' cannot be reset to its default value")
 
         # We can't reopen hd1 to read-only, as block-stream requires it to be
         # read-write
@@ -949,7 +950,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.wait_until_completed(drive = 'commit0')
 
     # Reopen the chain during a block-commit job (from hd1 to hd2)
-    def test_block_commit_2(self):
+    def do_test_block_commit_2(self, detach_hd1):
         # hd2 <- hd1 <- hd0
         opts = hd_opts(0)
         opts['backing'] = hd_opts(1)
@@ -959,23 +960,33 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
         result = self.vm.qmp('block-commit', conv_keys = True, job_id = 'commit0',
                              device = 'hd0', top_node = 'hd1',
-                             auto_finalize = False)
+                             auto_finalize = False, filter_node_name = 'fl')
         self.assert_qmp(result, 'return', {})
 
         # We can't remove hd2 while the commit job is ongoing
         opts['backing']['backing'] = None
-        self.reopen(opts, {}, "Cannot change the option 'backing.driver'")
+        self.reopen(opts, {}, "Cannot change 'backing' link from 'fl' to 'hd1'")
 
-        # We can't remove hd1 while the commit job is ongoing
-        opts['backing'] = None
-        self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file")
+        if (detach_hd1):
+            # But nothing wrong in detaching hd1 together with the commit job
+            opts['backing'] = None
+            self.reopen(opts, {})
 
         # hd2 <- hd0
         self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True)
 
         self.assert_qmp(self.get_node('hd0'), 'ro', False)
         self.assertEqual(self.get_node('hd1'), None)
-        self.assert_qmp(self.get_node('hd2'), 'ro', True)
+        if detach_hd1:
+            self.assertEqual(self.get_node('hd2'), None)
+        else:
+            self.assert_qmp(self.get_node('hd2'), 'ro', True)
+
+    def test_block_commit_2(self):
+        self.do_test_block_commit_2(False)
+
+    def test_block_commit_3(self):
+        self.do_test_block_commit_2(True)
 
     def run_test_iothreads(self, iothread_a, iothread_b, errmsg = None):
         opts = hd_opts(0)
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 4b33dcaf5c..c1fda88f1a 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -2,6 +2,10 @@
 {"return": {}}
 {"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"execute": "job-finalize", "arguments": {"id": "stream0"}}
 {"return": {}}
 {"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
@@ -10,8 +14,8 @@
 {"return": {}}
 {"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-.....................
+......................
 ----------------------------------------------------------------------
-Ran 21 tests
+Ran 22 tests
 
 OK
-- 
2.29.2

Re: [PATCH RFC 0/3] block: drop inherits_from

[Kevin Wolf]

Am 11.03.2021 um 16:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> I now work on v3 for "block: update graph permissions update", and I'm
> at "[PATCH v2 28/36] block: add bdrv_set_backing_noperm() transaction
> action".
> 
> So, the problem is we should handle inherits_from carefully, and most
> probably it should be updated in bdrv_replace_child_noperm().. And
> then, bdrv_replace_child_noperm will become a transaction action,
> which should store old inherits_from to the transaction state for
> possible rollback.. Or something like this, I didn't try yet. I just
> thought, may be we can just drop inherits_from?
> 
> I decided to learn the thing a bit, and found that the only usage of
> inherits_from is to limit reopen process. When adding bs to
> reopen_queue we do add its children recursively, but only those which
> inherits from the bs.
> 
> That works so starting from
> 
> commit 67251a311371c4d22e803f151f47fe817175b6c3
> Author: Kevin Wolf <kwolf@redhat.com>
> Date:   Thu Apr 9 18:54:04 2015 +0200
> 
>     block: Fix reopen flag inheritance
> 
> 
> The commit made two things:
> 
> 1. reopen recursively all* children, not only .file. That's OK.
> 
> 2. * : not all, but only that inherits_from bs.
> 
> [2] Means that we don't reopen some implicitely created children..
> And, I want to ask, why?

The reason is the difference between

    -drive if=none,file=test.qcow2

and something like

    -blockdev file,filename=backing.img,node-name=backing
    -blockdev file,filename=test.qcow2,node-name=file
    -blockdev qcow2,file=file,backing=backing

The former means that bs->file and bs->backing inherit options from the
qcow2 layer. If you reopen the qcow2 layer to set cache.direct=on, both
children inherit the same update and both the file itself and the
backing file will use O_DIRECT - this is the same as would happen if you
had set cache.direct=on in the -drive option from the start.

In the -blockdev case, the nodes were defined explicitly without
inheriting from the qcow2 layer. Setting cache.direct=on on the qcow2
layer (which is actually created last) doesn't influence the two file
layers. So a reopen of the qcow2 layer shouldn't change the two file
nodes either: If they didn't inherit the option during bdrv_open(), they
certainly shouldn't inherit it during bdrv_reopen() either.

> For me it seems that if we have reopen process.. And bs involved. And
> it has a child.. And child role defines how that child should inherit
> options.. Why not to just inherit them?

The -blockdev behaviour makes things a lot more predictable for a
management tool for which we know that it can handle things on the node
level.

So what we really want is not inheriting at all. But compatibility with
-drive doesn't let us. (And actually -blockdev with inline declaration
of children behaves the same as -drive, which may have been a mistake.)

Kevin

Re: [PATCH RFC 0/3] block: drop inherits_from

[Vladimir Sementsov-Ogievskiy]

11.03.2021 20:09, Kevin Wolf wrote:
> Am 11.03.2021 um 16:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi all!
>>
>> I now work on v3 for "block: update graph permissions update", and I'm
>> at "[PATCH v2 28/36] block: add bdrv_set_backing_noperm() transaction
>> action".
>>
>> So, the problem is we should handle inherits_from carefully, and most
>> probably it should be updated in bdrv_replace_child_noperm().. And
>> then, bdrv_replace_child_noperm will become a transaction action,
>> which should store old inherits_from to the transaction state for
>> possible rollback.. Or something like this, I didn't try yet. I just
>> thought, may be we can just drop inherits_from?
>>
>> I decided to learn the thing a bit, and found that the only usage of
>> inherits_from is to limit reopen process. When adding bs to
>> reopen_queue we do add its children recursively, but only those which
>> inherits from the bs.
>>
>> That works so starting from
>>
>> commit 67251a311371c4d22e803f151f47fe817175b6c3
>> Author: Kevin Wolf <kwolf@redhat.com>
>> Date:   Thu Apr 9 18:54:04 2015 +0200
>>
>>      block: Fix reopen flag inheritance
>>
>>
>> The commit made two things:
>>
>> 1. reopen recursively all* children, not only .file. That's OK.
>>
>> 2. * : not all, but only that inherits_from bs.
>>
>> [2] Means that we don't reopen some implicitely created children..
>> And, I want to ask, why?
> 
> The reason is the difference between
> 
>      -drive if=none,file=test.qcow2
> 
> and something like
> 
>      -blockdev file,filename=backing.img,node-name=backing
>      -blockdev file,filename=test.qcow2,node-name=file
>      -blockdev qcow2,file=file,backing=backing
> 
> The former means that bs->file and bs->backing inherit options from the
> qcow2 layer. If you reopen the qcow2 layer to set cache.direct=on, both
> children inherit the same update and both the file itself and the
> backing file will use O_DIRECT - this is the same as would happen if you
> had set cache.direct=on in the -drive option from the start.
> 
> In the -blockdev case, the nodes were defined explicitly without
> inheriting from the qcow2 layer. Setting cache.direct=on on the qcow2
> layer (which is actually created last) doesn't influence the two file
> layers. So a reopen of the qcow2 layer shouldn't change the two file
> nodes either: If they didn't inherit the option during bdrv_open(), they
> certainly shouldn't inherit it during bdrv_reopen() either.

Hmm. Understand. Than I was wrong. So modern blockdev behaviour is NOT inherit options. That makes sense.

> 
>> For me it seems that if we have reopen process.. And bs involved. And
>> it has a child.. And child role defines how that child should inherit
>> options.. Why not to just inherit them?
> 
> The -blockdev behaviour makes things a lot more predictable for a
> management tool for which we know that it can handle things on the node
> level.
> 
> So what we really want is not inheriting at all. But compatibility with
> -drive doesn't let us. (And actually -blockdev with inline declaration
> of children behaves the same as -drive, which may have been a mistake.)
> 

So, inherits_from will disappear together with -drive some good future day. OK, thanks for explanation


-- 
Best regards,
Vladimir