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
signature.asc
Description: OpenPGP digital signature
