Re: [PATCH v3 01/36] tests/test-bdrv-graph-mod: add test_parallel_exclusive_write

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

23.04.2021 15:59, Kevin Wolf wrote:
> Am 23.04.2021 um 14:46 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 23.04.2021 15:25, Kevin Wolf wrote:
>>> Am 17.03.2021 um 15:34 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Add the test that shows that concept of ignore_children is incomplete.
>>>> Actually, when we want to update something, ignoring permission of some
>>>> existing BdrvChild, we should ignore also the propagated effect of this
>>>> child to the other children. But that's not done. Better approach
>>>> (update permissions on already updated graph) will be implemented
>>>> later.
>>>>
>>>> Now the test fails, so it's added with -d argument to not break make
>>>> check.
>>>>
>>>> Test fails with
>>>>
>>>>    "Conflicts with use by fl1 as 'backing', which does not allow 'write' on base"
>>>>
>>>> because when updating permissions we can ignore original top->fl1
>>>> BdrvChild. But we don't ignore exclusive write permission in fl1->base
>>>> BdrvChild, which is propagated. Correct thing to do is make graph
>>>> change first and then do permission update from the top node.
>>>>
>>>> To run test do
>>>>
>>>>     ./test-bdrv-graph-mod -d -p /bdrv-graph-mod/parallel-exclusive-write
>>>>
>>>> from <build-directory>/tests.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    tests/unit/test-bdrv-graph-mod.c | 70 +++++++++++++++++++++++++++++++-
>>>>    1 file changed, 69 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
>>>> index c4f7d16039..4e4e83674a 100644
>>>> --- a/tests/unit/test-bdrv-graph-mod.c
>>>> +++ b/tests/unit/test-bdrv-graph-mod.c
>>>> @@ -1,7 +1,7 @@
>>>>    /*
>>>>     * Block node graph modifications tests
>>>>     *
>>>> - * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>>>> + * Copyright (c) 2019-2021 Virtuozzo International GmbH. All rights reserved.
>>>>     *
>>>>     * This program is free software; you can redistribute it and/or modify
>>>>     * it under the terms of the GNU General Public License as published by
>>>> @@ -44,6 +44,21 @@ static BlockDriver bdrv_no_perm = {
>>>>        .bdrv_child_perm = no_perm_default_perms,
>>>>    };
>>>> +static void exclusive_write_perms(BlockDriverState *bs, BdrvChild *c,
>>>> +                                  BdrvChildRole role,
>>>> +                                  BlockReopenQueue *reopen_queue,
>>>> +                                  uint64_t perm, uint64_t shared,
>>>> +                                  uint64_t *nperm, uint64_t *nshared)
>>>> +{
>>>> +    *nperm = BLK_PERM_WRITE;
>>>> +    *nshared = BLK_PERM_ALL & ~BLK_PERM_WRITE;
>>>> +}
>>>> +
>>>> +static BlockDriver bdrv_exclusive_writer = {
>>>> +    .format_name = "exclusive-writer",
>>>> +    .bdrv_child_perm = exclusive_write_perms,
>>>> +};
>>>> +
>>>>    static BlockDriverState *no_perm_node(const char *name)
>>>>    {
>>>>        return bdrv_new_open_driver(&bdrv_no_perm, name, BDRV_O_RDWR, &error_abort);
>>>> @@ -55,6 +70,12 @@ static BlockDriverState *pass_through_node(const char *name)
>>>>                                    BDRV_O_RDWR, &error_abort);
>>>>    }
>>>> +static BlockDriverState *exclusive_writer_node(const char *name)
>>>> +{
>>>> +    return bdrv_new_open_driver(&bdrv_exclusive_writer, name,
>>>> +                                BDRV_O_RDWR, &error_abort);
>>>> +}
>>>> +
>>>>    /*
>>>>     * test_update_perm_tree
>>>>     *
>>>> @@ -185,8 +206,50 @@ static void test_should_update_child(void)
>>>>        blk_unref(root);
>>>>    }
>>>> +/*
>>>> + * test_parallel_exclusive_write
>>>> + *
>>>> + * Check that when we replace node, old permissions of the node being removed
>>>> + * doesn't break the replacement.
>>>> + */
>>>> +static void test_parallel_exclusive_write(void)
>>>> +{
>>>> +    BlockDriverState *top = exclusive_writer_node("top");
>>>> +    BlockDriverState *base = no_perm_node("base");
>>>> +    BlockDriverState *fl1 = pass_through_node("fl1");
>>>> +    BlockDriverState *fl2 = pass_through_node("fl2");
>>>> +
>>>> +    /*
>>>> +     * bdrv_attach_child() eats child bs reference, so we need two @base
>>>> +     * references for two filters:
>>>> +     */
>>>> +    bdrv_ref(base);
>>>> +
>>>> +    bdrv_attach_child(top, fl1, "backing", &child_of_bds, BDRV_CHILD_DATA,
>>>> +                      &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_replace_node(fl1, fl2, &error_abort);
>>>> +
>>>> +    bdrv_unref(fl2); /* second reference was created by bdrv_replace_node() */
>>>
>>> This line is new and I don't understand it.
>>>
>>> Why does bdrv_replace_node() create new references? Shouldn't it just
>>> move all parents of fl2 to fl1, and when the refcount of fl2 drops to
>>> zero, it would be deleted?
>>
>> fl2 is second argument of bdrv_replace_node, it's @to. So all parents
>> of fl1 moved to fl2. So, fl2 referenced by top. But our first
>> reference that comes from pass_through_node() is still here as well.
> 
> Oh, right. I assumed that fl2 was attached to top, but it isn't. So we
> indeed still own that reference.
> 
> I feel the comment is misleading, it made me think that we unref a
> reference that was created by bdrv_replace_node(). What you probably
> meant is that bdrv_replace_node() only took an additional reference (by
> attaching it to top), but did not take ownership of the reference that
> the test function owns.
> 
> Maybe it would be better without the comment.

Agree

-- 
Best regards,
Vladimir