Re: [Qemu-devel] KVM Forum block no[td]es

[Max Reitz <mreitz@redhat.com>]

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

On 15.11.18 15:28, Alberto Garcia wrote:
> On Wed 14 Nov 2018 06:24:10 PM CET, Max Reitz wrote:
>>>> Permission system
>>>> =================
>>>>
>>>> GRAPH_MOD
>>>> ---------
>>>>
>>>> We need some way for the commit job to prevent graph changes on its
>>>> chain while it is running.  Our current blocker doesn’t do the job,
>>>> however.  What to do?
>>>>
>>>> - We have no idea how to make a *permission* work.  Maybe the biggest
>>>>   problem is that it just doesn’t work as a permission, because the
>>>>   commit job doesn’t own the BdrvChildren that would need to be
>>>>   blocked (namely the @backing BdrvChild).
>>>
>>> What I'm doing at the moment in my blockdev-reopen series is check
>>> whether all parents of the node I want to change share the GRAPH_MOD
>>> permission. If any of them doesn't then the operation fails.
>>>
>>> This can be checked by calling bdrv_get_cumulative_perm() or simply
>>> bdrv_check_update_perm(..., BLK_PERM_GRAPH_MOD, ...).
>>
>> Sure.
>>
>>> It solves the problem because e.g. the stream block job only allows
>>> BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED, so no graph
>>> modifications allowed.
>>
>> But the problem is that the commit job needs to unshare the permission
>> on all backing links between base and top, so none of the backing
>> links can be broken.  But it cannot do that because those backing
>> links do not belong to it (but to the respective overlay nodes).
> 
> I'm not sure if I'm following you. The commit block job has references
> to all intermediate nodes (with block_job_add_bdrv()) and only shares
> BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE. Anyone trying to obtain
> BLK_PERM_GRAPH_MOD on an intermediate node would see that the block job
> doesn't allow it.

But first of all, why would anyone try to obtain it?  You only obtain a
permission when you attach a new parent (or when an existing parent
tries to change the ones it has).

For instance, in the commit case, if you just remove a backing link,
then the chain is clearly broken and the graph has been modified, but
noone will have taken any permission to do so.

But even beyond that, commit doesn't care about all of the graph.  All
it cares about are the backing links.  So it should not prevent anyone
else from changing any links outside of the backing chain (which a
permission would clearly do).

Let me give a concrete example:

top (T) -> intermediate (I) -> base (B)

Now you commit from top to base.  Clearly you don't want the backing
chain between top and base to change.  So say you unshare the GRAPH_MOD
permission (implying that whenever a parent doesn't care, it just shares
it).  But as said above, if someone just drops the backing link from I
to B, the permission system won't catch that.

The next thing is that how would you even try to catch it in the first
place?  The normal case would be for everything to both share and claim
the GRAPH_MOD permission, the latter so it can see if something unshared
it.  But that would mean that everything is holding that permission all
the time and it's impossible to unshare it.

(You'd take the GRAPH_MOD permission for the backing link from I to B,
and then commit couldn't unshare it because, well, it's taken already.)

So the solution might be to take it first, and release it after having
attached the child.  And maybe you'd say that for the removal case,
you'd have to take the GRAPH_MOD permission before you remove any child.
 But I think that's weird, and also, there's another issue still.


What if you try to attach a read-only NBD server to B?  That should be
OK in my mind.  But it is a graph change, so it wouldn't be allowed if
GRAPH_MOD is a permission, even though commit doesn't care at all.

The fact is that commit clearly only cares about edges it is not
involved in as either parent or child (the backing links), and therefore
has not control over the permissions taken.  It does not care about any
other edges, which is different from what permissions do, because they
always apply to a whole set of edges.


I don't think anything needs a way to generally block graph changes
around some node.  We only need to prevent changes to very specific sets
of edges.  This is something that the permission system just cannot do.

>>>>   (We never quite knew what “taking the GRAPH_PERMISSION” or
>>>>   “unsharing the GRPAH_MOD permission” was supposed to mean.
>>>>   Figuring that out always took like half an our in any face-to-face
>>>>   meeting, and then we decided it was pretty much useless for any
>>>>   case we had at hand.)
>>>
>>> Yeah it's a bit unclear. It can mean "none of this node's children
>>> can be replaced / removed", which is what I'm using it for at the
>>> moment.
>>
>> Yes, but that's just not useful.  I don't think we have any case where
>> something would be annoyed by having its immediate child replaced
>> (other than the visible data changing, potentially).  Usually when we
>> say something (e.g. a block job) wants to prevent graph modification,
>> it's about changing edges that exist independently of the job (such as
>> a backing chain).
> 
> Isn't that what I just said? You cannot change other edges <=> you
> cannot replace a node's children (or parents).

Hm, true, but I think it's (1) hard to understand, (2) would require
non-trivial changes to the current model anyway (take GRAPH_MOD before
any modification, release it afterwards (which is quite different from
any other permission anyway)), and (3) would be too restrictive.

Max


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