Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > Add test to show that simple DFS recursion order is not correct for > permission update. Correct order is topological-sort order, which will > be introduced later. > > Consider the block driver which has two filter children: one active > with exclusive write access and one inactive with no specific > permissions. > > And, these two children has a common base child, like this: > > ┌─────┐ ┌──────┐ > │ fl2 │ ◀── │ top │ > └─────┘ └──────┘ > │ │ > │ │ w > │ ▼ > │ ┌──────┐ > │ │ fl1 │ > │ └──────┘ > │ │ > │ │ w > │ ▼ > │ ┌──────┐ > └───────▶ │ base │ > └──────┘ > > So, exclusive write is propagated. > > Assume, we want to make fl2 active instead of fl1. > So, we set some option for top driver and do permission update. > > If permission update (remember, it's DFS) goes first through > top->fl1->base branch it will succeed: it firstly drop exclusive write > permissions and than apply them for another BdrvChildren. > But if permission update goes first through top->fl2->base branch it > will fail, as when we try to update fl2->base child, old not yet > updated fl1->base child will be in conflict. > > Now test fails, so it runs only with -d flag. To run do > > ./test-bdrv-graph-mod -d -p /bdrv-graph-mod/parallel-perm-update > > from <build-directory>/tests. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> > --- > tests/test-bdrv-graph-mod.c | 64 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c > index 3b9e6f242f..27e3361a60 100644 > --- a/tests/test-bdrv-graph-mod.c > +++ b/tests/test-bdrv-graph-mod.c > @@ -232,6 +232,68 @@ static void test_parallel_exclusive_write(void) > bdrv_unref(top); > } > > +static void write_to_file_perms(BlockDriverState *bs, BdrvChild *c, > + BdrvChildRole role, > + BlockReopenQueue *reopen_queue, > + uint64_t perm, uint64_t shared, > + uint64_t *nperm, uint64_t *nshared) > +{ > + if (bs->file && c == bs->file) { > + *nperm = BLK_PERM_WRITE; > + *nshared = BLK_PERM_ALL & ~BLK_PERM_WRITE; > + } else { > + *nperm = 0; > + *nshared = BLK_PERM_ALL; > + } > +} > + > +static BlockDriver bdrv_write_to_file = { > + .format_name = "tricky-perm", > + .bdrv_child_perm = write_to_file_perms, > +}; > + > +static void test_parallel_perm_update(void) > +{ > + BlockDriverState *top = no_perm_node("top"); > + BlockDriverState *tricky = > + bdrv_new_open_driver(&bdrv_write_to_file, "tricky", BDRV_O_RDWR, > + &error_abort); > + BlockDriverState *base = no_perm_node("base"); > + BlockDriverState *fl1 = pass_through_node("fl1"); > + BlockDriverState *fl2 = pass_through_node("fl2"); > + BdrvChild *c_fl1, *c_fl2; > + > + bdrv_attach_child(top, tricky, "file", &child_of_bds, BDRV_CHILD_DATA, > + &error_abort); > + c_fl1 = bdrv_attach_child(tricky, fl1, "first", &child_of_bds, > + BDRV_CHILD_FILTERED, &error_abort); > + c_fl2 = bdrv_attach_child(tricky, fl2, "second", &child_of_bds, > + BDRV_CHILD_FILTERED, &error_abort); > + bdrv_attach_child(fl1, base, "backing", &child_of_bds, > BDRV_CHILD_FILTERED, > + &error_abort); > + bdrv_attach_child(fl2, base, "backing", &child_of_bds, > BDRV_CHILD_FILTERED, > + &error_abort); > + bdrv_ref(base); > + > + /* Select fl1 as first child to be active */ > + tricky->file = c_fl1; > + bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort); > + > + assert(c_fl1->perm & BLK_PERM_WRITE); > + > + /* Now, try to switch active child and update permissions */ > + tricky->file = c_fl2; > + bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort); > + > + assert(c_fl2->perm & BLK_PERM_WRITE); > + > + /* Switch once more, to not care about real child order in the list */ > + tricky->file = c_fl1; > + bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort); > + > + assert(c_fl1->perm & BLK_PERM_WRITE);
Should we also assert in each case that the we don't hole the write permission for the inactive child? Kevin
