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

[Alberto Garcia <berto@igalia.com>]

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.

>>>   (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).

>>> Reopen
>>> ------
>>>
>>> How should permissions be handled while the reopen is under way?
>>> Maybe we should take the union of @perm before and after, and the
>>> intersection of @shared before and after?
>> 
>> Do you have an example of a case in which you're reopening a node and
>> the change of permissions causes a problem?
>
> I do not.  So currently we switch from the permissions before to the
> ones after, right?  Which should be safe because that switch is a
> transaction that is integrated into reopen.
>
> However, I suppose it's possible the protocol layer gives us some
> issues again.  It cannot switch the locks before commit, but
> committing may fail.  What to do then?
>
> It would be safer if we took the union/intersection in
> bdrv_reopen_prepare() and then released the unnecessary flags in
> bdrv_reopen_commit().  We can still get errors (as discussed), but
> those can be safely ignored.  (They're just annoying, but not
> harmful.)

Ok, I suppose you're right.

Berto