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

[Vladimir Sementsov-Ogievskiy]

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.

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).

I'll work on a patch for it.

--
Best regards,
Vladimir