On 24.09.20 16:51, Vladimir Sementsov-Ogievskiy wrote: > 24.09.2020 16:25, Max Reitz wrote: >> 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. >> > > If you are OK with .active for now, then I think, Andrey can resend with > .active and I'll dig into permissions in parallel. If Andrey's series > go first, I'll just drop .active later if my idea works.
Sure, that works for me. Max
signature.asc
Description: OpenPGP digital signature