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

[Kevin Wolf]

Am 10.03.2021 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 03.02.2021 21:38, Kevin Wolf wrote:
> > Am 28.01.2021 um 19:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 28.01.2021 20:13, Kevin Wolf wrote:
> > > > 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().
> > > 
> > > You mean parent BdrvChild objects? Then this question applies as well
> > > to pre-patch code.
> > 
> > I don't think so. The pre-patch code doesn't rely on the permissions
> > already being set in the BdrvChild object, but it gets them passed in
> > parameters. Changing the graph first and relying on the information in
> > BdrvChild is the new approach that you're introducing.
> 
> New code still pass permissions as parameters for root node. And only
> inside subtree we rely on updated parents. But that's correct due to
> topological order of updating.
> 
> 
> OK, that's incorrect for the following case: when one of the parents (X)
> of inner node in bs subtree IS NOT in the bs subtree but IS in reopen queue.
> And we'll use wrong permission of X. Still:
> 
> 1. It's preexisting. bdrv_check_update_perm() doesn't take reopen queue
> in mind when calculate cumulative permissions (and ignore_children doesn't
> help for the described case
> 
> 2. We can hope that on next permission update, started from node X, 
> permissions
> will become more correct
> 
> 3. At the end of series permission calculation in bdrv_reopen_multiple is
> rewritten anyway.

Yes, I think 3. is the strongest argument in favour of the patch.

Kevin