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.