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: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.



-- 
Best regards,
Vladimir