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.