[PATCH v5 04/45] test-bdrv-graph-mod: update test_parallel_perm_update test case

Vladimir Sementsov-Ogievskiy posted 45 patches 3 years, 8 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Ari Sundholm <ari@tuxera.com>, Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>, "Denis V. Lunev" <den@openvz.org>, Wen Congyang <wencongyang2@huawei.com>, Xie Changlong <xiechanglong.d@gmail.com>, Stefan Weil <sw@weilnetz.de>, Jeff Cody <codyprime@gmail.com>, Fam Zheng <fam@euphon.net>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>
[PATCH v5 04/45] test-bdrv-graph-mod: update test_parallel_perm_update test case
Posted by Vladimir Sementsov-Ogievskiy 3 years, 8 months ago
test_parallel_perm_update() does two things that we are going to
restrict in the near future:

1. It updates bs->file field by hand. bs->file will be managed
   automatically by generic code (together with bs->children list).

   Let's better refactor our "tricky" bds to have own state where one
   of children is linked as "selected".
   This also looks less "tricky", so avoid using this word.

2. It create FILTERED children that are not PRIMARY. Except for tests
   all FILTERED children in the Qemu block layer are always PRIMARY as
   well.  We are going to formalize this rule, so let's better use DATA
   children here.

While being here, update the picture to better correspond to the test
code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 tests/unit/test-bdrv-graph-mod.c | 70 ++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index a6e3bb79be..40795d3c04 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c
@@ -241,13 +241,26 @@ 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)
+/*
+ * write-to-selected node may have several DATA children, one of them may be
+ * "selected". Exclusive write permission is taken on selected child.
+ *
+ * We don't realize write handler itself, as we need only to test how permission
+ * update works.
+ */
+typedef struct BDRVWriteToSelectedState {
+    BdrvChild *selected;
+} BDRVWriteToSelectedState;
+
+static void write_to_selected_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) {
+    BDRVWriteToSelectedState *s = bs->opaque;
+
+    if (s->selected && c == s->selected) {
         *nperm = BLK_PERM_WRITE;
         *nshared = BLK_PERM_ALL & ~BLK_PERM_WRITE;
     } else {
@@ -256,9 +269,10 @@ static void write_to_file_perms(BlockDriverState *bs, BdrvChild *c,
     }
 }
 
-static BlockDriver bdrv_write_to_file = {
-    .format_name = "tricky-perm",
-    .bdrv_child_perm = write_to_file_perms,
+static BlockDriver bdrv_write_to_selected = {
+    .format_name = "write-to-selected",
+    .instance_size = sizeof(BDRVWriteToSelectedState),
+    .bdrv_child_perm = write_to_selected_perms,
 };
 
 
@@ -266,15 +280,18 @@ static BlockDriver bdrv_write_to_file = {
  * The following test shows that topological-sort order is required for
  * permission update, simple DFS is not enough.
  *
- * Consider the block driver which has two filter children: one active
- * with exclusive write access and one inactive with no specific
- * permissions.
+ * Consider the block driver (write-to-selected) which has two children: one is
+ * selected so we have exclusive write access to it and for the other one we
+ * don't need any specific permissions.
  *
  * And, these two children has a common base child, like this:
+ *   (additional "top" on top is used in test just because the only public
+ *    function to update permission should get a specific child to update.
+ *    Making bdrv_refresh_perms() public just for this test doesn't worth it)
  *
- * ┌─────┐     ┌──────┐
- * │ fl2 │ ◀── │ top  │
- * └─────┘     └──────┘
+ * ┌─────┐     ┌───────────────────┐     ┌─────┐
+ * │ fl2 │ ◀── │ write-to-selected │ ◀── │ top │
+ * └─────┘     └───────────────────┘     └─────┘
  *   │           │
  *   │           │ w
  *   │           ▼
@@ -290,7 +307,7 @@ static BlockDriver bdrv_write_to_file = {
  *
  * So, exclusive write is propagated.
  *
- * Assume, we want to make fl2 active instead of fl1.
+ * Assume, we want to select fl2  instead of fl1.
  * So, we set some option for top driver and do permission update.
  *
  * With simple DFS, if permission update goes first through
@@ -306,9 +323,10 @@ static BlockDriver bdrv_write_to_file = {
 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,
+    BlockDriverState *ws =
+            bdrv_new_open_driver(&bdrv_write_to_selected, "ws", BDRV_O_RDWR,
                                  &error_abort);
+    BDRVWriteToSelectedState *s = ws->opaque;
     BlockDriverState *base = no_perm_node("base");
     BlockDriverState *fl1 = pass_through_node("fl1");
     BlockDriverState *fl2 = pass_through_node("fl2");
@@ -320,33 +338,33 @@ static void test_parallel_perm_update(void)
      */
     bdrv_ref(base);
 
-    bdrv_attach_child(top, tricky, "file", &child_of_bds, BDRV_CHILD_DATA,
+    bdrv_attach_child(top, ws, "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);
+    c_fl1 = bdrv_attach_child(ws, fl1, "first", &child_of_bds,
+                              BDRV_CHILD_DATA, &error_abort);
+    c_fl2 = bdrv_attach_child(ws, fl2, "second", &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);
 
     /* Select fl1 as first child to be active */
-    tricky->file = c_fl1;
+    s->selected = c_fl1;
     bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort);
 
     assert(c_fl1->perm & BLK_PERM_WRITE);
     assert(!(c_fl2->perm & BLK_PERM_WRITE));
 
     /* Now, try to switch active child and update permissions */
-    tricky->file = c_fl2;
+    s->selected = c_fl2;
     bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort);
 
     assert(c_fl2->perm & BLK_PERM_WRITE);
     assert(!(c_fl1->perm & BLK_PERM_WRITE));
 
     /* Switch once more, to not care about real child order in the list */
-    tricky->file = c_fl1;
+    s->selected = c_fl1;
     bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort);
 
     assert(c_fl1->perm & BLK_PERM_WRITE);
-- 
2.35.1


Re: [PATCH v5 04/45] test-bdrv-graph-mod: update test_parallel_perm_update test case
Posted by Hanna Reitz 3 years, 6 months ago
On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> test_parallel_perm_update() does two things that we are going to
> restrict in the near future:
>
> 1. It updates bs->file field by hand. bs->file will be managed
>     automatically by generic code (together with bs->children list).
>
>     Let's better refactor our "tricky" bds to have own state where one
>     of children is linked as "selected".
>     This also looks less "tricky", so avoid using this word.
>
> 2. It create FILTERED children that are not PRIMARY. Except for tests
>     all FILTERED children in the Qemu block layer are always PRIMARY as
>     well.  We are going to formalize this rule, so let's better use DATA
>     children here.

Another thing is that any node may have at most one FILTERED child at a 
time, which was already formalized in BDRV_CHILD_FILTERED’s description.

> While being here, update the picture to better correspond to the test
> code.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---

The change looks good, I’m just a bit confused when it comes to the 
comment describing what’s going on.

>   tests/unit/test-bdrv-graph-mod.c | 70 ++++++++++++++++++++------------
>   1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
> index a6e3bb79be..40795d3c04 100644
> --- a/tests/unit/test-bdrv-graph-mod.c
> +++ b/tests/unit/test-bdrv-graph-mod.c

[...]

> @@ -266,15 +280,18 @@ static BlockDriver bdrv_write_to_file = {
>    * The following test shows that topological-sort order is required for
>    * permission update, simple DFS is not enough.
>    *
> - * Consider the block driver which has two filter children: one active
> - * with exclusive write access and one inactive with no specific
> - * permissions.
> + * Consider the block driver (write-to-selected) which has two children: one is
> + * selected so we have exclusive write access to it and for the other one we
> + * don't need any specific permissions.
>    *
>    * And, these two children has a common base child, like this:
> + *   (additional "top" on top is used in test just because the only public
> + *    function to update permission should get a specific child to update.
> + *    Making bdrv_refresh_perms() public just for this test doesn't worth it)

s/doesn't/isn't/

>    *
> - * ┌─────┐     ┌──────┐
> - * │ fl2 │ ◀── │ top  │
> - * └─────┘     └──────┘
> + * ┌─────┐     ┌───────────────────┐     ┌─────┐
> + * │ fl2 │ ◀── │ write-to-selected │ ◀── │ top │
> + * └─────┘     └───────────────────┘     └─────┘
>    *   │           │
>    *   │           │ w
>    *   │           ▼
> @@ -290,7 +307,7 @@ static BlockDriver bdrv_write_to_file = {
>    *
>    * So, exclusive write is propagated.
>    *
> - * Assume, we want to make fl2 active instead of fl1.
> + * Assume, we want to select fl2  instead of fl1.

There’s a double space after “fl2”.

>    * So, we set some option for top driver and do permission update.

Here and in the rest of the comment, it’s now unclear what node “top” 
refers to.  I think it’s still the now-renamed “write-to-selected” node, 
right?  But “top” is now a different node, so I’m not 100% sure.

(On the other hand, even before this patch, there was a “top” node that 
was distinct from the former “tricky” node...  So it seems like this 
comment was already not quite right before?)

>    *
>    * With simple DFS, if permission update goes first through


Re: [PATCH v5 04/45] test-bdrv-graph-mod: update test_parallel_perm_update test case
Posted by Vladimir Sementsov-Ogievskiy 3 years, 6 months ago
On 6/7/22 13:53, Hanna Reitz wrote:
> On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
>> test_parallel_perm_update() does two things that we are going to
>> restrict in the near future:
>>
>> 1. It updates bs->file field by hand. bs->file will be managed
>>     automatically by generic code (together with bs->children list).
>>
>>     Let's better refactor our "tricky" bds to have own state where one
>>     of children is linked as "selected".
>>     This also looks less "tricky", so avoid using this word.
>>
>> 2. It create FILTERED children that are not PRIMARY. Except for tests
>>     all FILTERED children in the Qemu block layer are always PRIMARY as
>>     well.  We are going to formalize this rule, so let's better use DATA
>>     children here.
> 
> Another thing is that any node may have at most one FILTERED child at a time, which was already formalized in BDRV_CHILD_FILTERED’s description.

Right, will add

> 
>> While being here, update the picture to better correspond to the test
>> code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
> 
> The change looks good, I’m just a bit confused when it comes to the comment describing what’s going on.
> 
>>   tests/unit/test-bdrv-graph-mod.c | 70 ++++++++++++++++++++------------
>>   1 file changed, 44 insertions(+), 26 deletions(-)
>>
>> diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
>> index a6e3bb79be..40795d3c04 100644
>> --- a/tests/unit/test-bdrv-graph-mod.c
>> +++ b/tests/unit/test-bdrv-graph-mod.c
> 
> [...]
> 
>> @@ -266,15 +280,18 @@ static BlockDriver bdrv_write_to_file = {
>>    * The following test shows that topological-sort order is required for
>>    * permission update, simple DFS is not enough.
>>    *
>> - * Consider the block driver which has two filter children: one active
>> - * with exclusive write access and one inactive with no specific
>> - * permissions.
>> + * Consider the block driver (write-to-selected) which has two children: one is
>> + * selected so we have exclusive write access to it and for the other one we
>> + * don't need any specific permissions.
>>    *
>>    * And, these two children has a common base child, like this:
>> + *   (additional "top" on top is used in test just because the only public
>> + *    function to update permission should get a specific child to update.
>> + *    Making bdrv_refresh_perms() public just for this test doesn't worth it)
> 
> s/doesn't/isn't/
> 
>>    *
>> - * ┌─────┐     ┌──────┐
>> - * │ fl2 │ ◀── │ top  │
>> - * └─────┘     └──────┘
>> + * ┌─────┐     ┌───────────────────┐     ┌─────┐
>> + * │ fl2 │ ◀── │ write-to-selected │ ◀── │ top │
>> + * └─────┘     └───────────────────┘     └─────┘
>>    *   │           │
>>    *   │           │ w
>>    *   │           ▼
>> @@ -290,7 +307,7 @@ static BlockDriver bdrv_write_to_file = {
>>    *
>>    * So, exclusive write is propagated.
>>    *
>> - * Assume, we want to make fl2 active instead of fl1.
>> + * Assume, we want to select fl2  instead of fl1.
> 
> There’s a double space after “fl2”.
> 
>>    * So, we set some option for top driver and do permission update.
> 
> Here and in the rest of the comment, it’s now unclear what node “top” refers to.  I think it’s still the now-renamed “write-to-selected” node, right?  But “top” is now a different node, so I’m not 100% sure.

Right, will fix.

> 
> (On the other hand, even before this patch, there was a “top” node that was distinct from the former “tricky” node...  So it seems like this comment was already not quite right before?)

Hmm yes. Obviously I tried to make this more obvious, but didn't update the whole comment.

> 
>>    *
>>    * With simple DFS, if permission update goes first through
> 
> 


-- 
Best regards,
Vladimir