Re: [PATCH v2 15/36] block: use topological sort for permission update

[Kevin Wolf]

Am 28.01.2021 um 10:34 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 27.01.2021 21:38, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > -static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
> > > -                           uint64_t cumulative_perms,
> > > -                           uint64_t cumulative_shared_perms,
> > > -                           GSList *ignore_children, Error **errp)
> > > +static int bdrv_node_check_perm(BlockDriverState *bs, BlockReopenQueue 
> > > *q,
> > > +                                uint64_t cumulative_perms,
> > > +                                uint64_t cumulative_shared_perms,
> > > +                                GSList *ignore_children, Error **errp)
> > >   {
> > >       BlockDriver *drv = bs->drv;
> > >       BdrvChild *c;
> > > @@ -2166,21 +2193,43 @@ static int bdrv_check_perm(BlockDriverState *bs, 
> > > BlockReopenQueue *q,
> > >       /* Check all children */
> > >       QLIST_FOREACH(c, &bs->children, next) {
> > >           uint64_t cur_perm, cur_shared;
> > > -        GSList *cur_ignore_children;
> > >           bdrv_child_perm(bs, c->bs, c, c->role, q,
> > >                           cumulative_perms, cumulative_shared_perms,
> > >                           &cur_perm, &cur_shared);
> > > +        bdrv_child_set_perm_safe(c, cur_perm, cur_shared, NULL);
> > 
> > This "added" line is actually old code. What is removed here is the
> > recursive call of bdrv_check_update_perm(). This is what the code below
> > will have to replace.
> 
> yes, we'll use explicit loop instead of recursion
> 
> > 
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
> > > +                           uint64_t cumulative_perms,
> > > +                           uint64_t cumulative_shared_perms,
> > > +                           GSList *ignore_children, Error **errp)
> > > +{
> > > +    int ret;
> > > +    BlockDriverState *root = bs;
> > > +    g_autoptr(GSList) list = bdrv_topological_dfs(NULL, NULL, root);
> > > +
> > > +    for ( ; list; list = list->next) {
> > > +        bs = list->data;
> > > +
> > > +        if (bs != root) {
> > > +            if (!bdrv_check_parents_compliance(bs, ignore_children, 
> > > errp)) {
> > > +                return -EINVAL;
> > > +            }
> > 
> > At this point bs still had the old permissions, but we don't access
> > them. As we're going in topological order, the parents have already been
> > updated if they were a child covered in bdrv_node_check_perm(), so we're
> > checking the relevant values. Good.
> > 
> > What about the root node? If I understand correctly, the parents of the
> > root nodes wouldn't have been checked in the old code. In the new state,
> > the parent BdrvChild already has to contain the new permission.
> > 
> > In bdrv_refresh_perms(), we already check parent conflicts, so no change
> > for all callers going through it. Good.
> > 
> > bdrv_reopen_multiple() is less obvious. It passes permissions from the
> > BDRVReopenState, without applying the permissions first.
> 
> It will be changed in the series
> 
> > Do we check the
> > old parent permissions instead of the new state here?
> 
> We use given (new) cumulative permissions for bs, and recalculate
> permissions for bs subtree.

Where do we actually set them? I would expect a
bdrv_child_set_perm_safe() call somewhere, but I can't see it in the
call path from bdrv_reopen_multiple().

> It follows old behavior. The only thing is changed that pre-patch we
> do DFS recursion starting from bs (and probably visit some nodes
> several times), after-patch we first do topological sort of bs subtree
> and go through the list. The order of nodes is better and we visit
> each node once.

It's not the only thing that changes. Maybe this is what makes the patch
hard to understand, because it seems to do two steps at once:

1. Change the order in which nodes are processed

2. Replace bdrv_check_update_perm() with bdrv_check_parents_compliance()

In step 2, the point I mentioned above is important (new permissions
must already be set in the BdrvChild objects).

The switch to bdrv_check_parents_compliance() also means that error
messages become a bit worse because we don't know any more which of the
conflicting nodes is the new one, so we can't provide two different
messages any more. This is probably unavoidable, though.

> > 
> > > +            bdrv_get_cumulative_perm(bs, &cumulative_perms,
> > > +                                     &cumulative_shared_perms);
> > > +        }
> > > -        cur_ignore_children = 
> > > g_slist_prepend(g_slist_copy(ignore_children), c);
> > > -        ret = bdrv_check_update_perm(c->bs, q, cur_perm, cur_shared,
> > > -                                     cur_ignore_children, errp);
> > > -        g_slist_free(cur_ignore_children);
> > > +        ret = bdrv_node_check_perm(bs, q, cumulative_perms,
> > > +                                   cumulative_shared_perms,
> > > +                                   ignore_children, errp);
> > 
> > We use the original ignore_children for every node in the sorted list.
> > The old code extends it with all nodes in the path to each node.
> > 
> > For the bdrv_check_update_perm() call that is now replaced with
> > bdrv_check_parents_compliance(), I think this was necessary because
> > bdrv_check_update_perm() always assumes adding a new edge, so if you
> > update one instead of adding it, you have to ignore it so that it can't
> > conflict with itself. This isn't necessary any more now because we just
> > update and then check for consistency.
> > 
> > For passing to bdrv_node_check_perm() it doesn't make a difference
> > anyway because the parameter is now unused (and should probably be
> > removed).
> 
> ignore_children will be dropped in [27]. For now it is still needed
> for bdrv_replace_node_common

In bdrv_node_check_perm(), it's already unused after this patch. But
fair enough.

Kevin