Re: [PATCH v2 02/36] tests/test-bdrv-graph-mod: add test_parallel_perm_update

[Kevin Wolf]

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 <vsementsov@virtuozzo.com>
> ---
>  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