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