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
