Re: [PATCH v8 2/7] copy-on-read: add filter append/drop functions

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3603 bytes --]

On 23.09.20 16:38, Vladimir Sementsov-Ogievskiy wrote:
> 17.09.2020 19:09, Andrey Shinkevich wrote:
>> On 04.09.2020 14:22, Max Reitz wrote:
>>> On 28.08.20 18:52, Andrey Shinkevich wrote:
>>>> Provide API for the COR-filter insertion/removal.
>> ...
>>>> Also, drop the filter child permissions for an inactive state when the
>>>> filter node is being removed.
>>> Do we need .active for that?  Shouldn’t it be sufficient to just not
>>> require BLK_PERM_WRITE_UNCHANGED when no permissions are taken on the
>>> node (i.e. perm == 0 in cor_child_perm())?
>>>
>>> Of course, using an .active field works, too.  But Vladimir wanted a
>>> similar field in the preallocate filter, and there already is in
>>> backup-top.  I feel like there shouldn’t be a need for this.
>>>
>>> .bdrv_child_perm() should generally be able to decide what permissions
>>> it needs based on the information it gets.  If every driver needs to
>>> keep track of whether it has any parents and feed that information into
>>> .bdrv_child_perm() as external state, then something feels wrong.
>>>
>>> If perm == 0 is not sufficient to rule out any parents, we should just
>>> explicitly add a boolean that tells .bdrv_child_perm() whether there are
>>> any parents or not – shouldn’t be too difficult.
>>
>>
>> The issue is that we fail in the bdrv_check_update_perm() with
>> ""Conflicts with use by..." if the *nperm = 0 and the *nshared =
>> BLK_PERM_ALL are not set before the call to the bdrv_replace_node().
>> The filter is still in the backing chain by that moment and has a
>> parent with child->perm != 0.
>>
>> The implementation of  the .bdrv_child_perm()in the given patch is
>> similar to one in the block/mirror.c.
>>
>> How could we resolve the issue at the generic layer?
>>
>>
> 
> The problem is that when we update permissions in bdrv_replace_node, we
> consider new placement for "to" node, but old placement for "from" node.
> So, during update, we may consider stricter permission requirements for
> "from" than needed and they conflict with new "to" permissions.
> 
> We should consider all graph changes for permission update
> simultaneously, in same transaction. For this, we need refactor
> permission update system to work on queue of nodes instead of simple DFS
> recursion. And in the queue all the nodes to update should  be
> toplogically sorted. In this way, when we update node N, all its parents
> are already updated. And of course, we should make no-perm graph update
> before permission update, and rollback no-perm movement if permission
> update failed.

OK, you’ve convinced me, .active is good enough for now. O:)

> And we need topological sort anyway. Consider the following example:
> 
> A -
> |  \
> |  v
> |  B
> |  |
> v  /
> C<-
> 
> A is parent for B and C, B is parent for C.
> 
> Obviously, to update permissions, we should go in order A B C, so, when
> we update C, all it's parents permission already updated. But with
> current approach (simple recursion) we can update in sequence A C B C (C
> is updated twice). On first update of C, we consider old B permissions,
> so doing wrong thing. If it succeed, all is OK, on second C update we
> will finish with correct graph. But if the wrong thing failed, we break
> the whole process for no reason (it's possible that updated B permission
> will be less strict, but we will never check it).

True.

> I'll work on a patch for it.

Sounds great, though I fear for the complexity.  I’ll just wait and see
for now.

Max


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