diff mbox series

[v2,3/9] test-write-threshold: rewrite test_threshold_(not_)trigger tests

Message ID 20210504082553.20377-4-vsementsov@virtuozzo.com
State New
Headers show
Series block: refactor write threshold | expand

Commit Message

Vladimir Sementsov-Ogievskiy May 4, 2021, 8:25 a.m. UTC 
These tests use bdrv_write_threshold_exceeded() API, which is used only
for test. Better is testing real API, which is used in block.c as well.

So, let's call bdrv_write_threshold_check_write(), and check is
bs->write_threshold_offset cleared or not (it's cleared iff threshold
triggered).

Also we get rid of BdrvTrackedRequest use here. Tracked requests are
unrelated to write-threshold since we get rid of write notifiers.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/unit/test-write-threshold.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

Comments

Max Reitz May 5, 2021, 2:28 p.m. UTC | #1 
On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> These tests use bdrv_write_threshold_exceeded() API, which is used only
> for test.

Well, now.  That used to be different before patch 1.

> Better is testing real API, which is used in block.c as well.
> 
> So, let's call bdrv_write_threshold_check_write(), and check is
> bs->write_threshold_offset cleared or not (it's cleared iff threshold
> triggered).
> 
> Also we get rid of BdrvTrackedRequest use here. Tracked requests are
> unrelated to write-threshold since we get rid of write notifiers.

The purpose behind the BdrvTrackedRequest was clearly because this is 
the object bdrv_write_threshold_exceeded() expected.  This reads like 
there was some other purpose (i.e. actually tracked requests), but there 
wasn’t.

The question that comes to my mind is why we had the 
bdrv_check_request() calls there, and why this patch removes them.  They 
seem unrelated to the write threshold, but someone must have thought 
something when adding them.

Looking into git blame, that someone is you :) (8b1170012b1)
Looks like you added those calls because BdrvTrackedRequest is a block 
layer structure, so getting rid of it means the reason for 
bdrv_check_request() disappears.  OK.

Reviewed-by: Max Reitz <mreitz@redhat.com>

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/unit/test-write-threshold.c | 22 ++++------------------
>   1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
> index fc1c45a2eb..fd40a815b8 100644
> --- a/tests/unit/test-write-threshold.c
> +++ b/tests/unit/test-write-threshold.c
> @@ -55,41 +55,27 @@ static void test_threshold_multi_set_get(void)
>   
>   static void test_threshold_not_trigger(void)
>   {
> -    uint64_t amount = 0;
>       uint64_t threshold = 4 * 1024 * 1024;
>       BlockDriverState bs;
> -    BdrvTrackedRequest req;
>   
>       memset(&bs, 0, sizeof(bs));
> -    memset(&req, 0, sizeof(req));
> -    req.offset = 1024;
> -    req.bytes = 1024;
> -
> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>   
>       bdrv_write_threshold_set(&bs, threshold);
> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
> -    g_assert_cmpuint(amount, ==, 0);
> +    bdrv_write_threshold_check_write(&bs, 1024, 1024);
> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
>   }
>   
>   
>   static void test_threshold_trigger(void)
>   {
> -    uint64_t amount = 0;
>       uint64_t threshold = 4 * 1024 * 1024;
>       BlockDriverState bs;
> -    BdrvTrackedRequest req;
>   
>       memset(&bs, 0, sizeof(bs));
> -    memset(&req, 0, sizeof(req));
> -    req.offset = (4 * 1024 * 1024) - 1024;
> -    req.bytes = 2 * 1024;
> -
> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>   
>       bdrv_write_threshold_set(&bs, threshold);
> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
> -    g_assert_cmpuint(amount, >=, 1024);
> +    bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
>   }
>   
>   typedef struct TestStruct {
>
Vladimir Sementsov-Ogievskiy May 5, 2021, 3:20 p.m. UTC | #2 
05.05.2021 17:28, Max Reitz wrote:
> On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
>> These tests use bdrv_write_threshold_exceeded() API, which is used only
>> for test.
> 
> Well, now.  That used to be different before patch 1.
> 
>> Better is testing real API, which is used in block.c as well.
>>
>> So, let's call bdrv_write_threshold_check_write(), and check is
>> bs->write_threshold_offset cleared or not (it's cleared iff threshold
>> triggered).
>>
>> Also we get rid of BdrvTrackedRequest use here. Tracked requests are
>> unrelated to write-threshold since we get rid of write notifiers.
> 
> The purpose behind the BdrvTrackedRequest was clearly because this is the object bdrv_write_threshold_exceeded() expected.  This reads like there was some other purpose (i.e. actually tracked requests), but there wasn’t.
> 
> The question that comes to my mind is why we had the bdrv_check_request() calls there, and why this patch removes them.  They seem unrelated to the write threshold, but someone must have thought something when adding them.

I wanted to add a note for it but forget. Something like:

   "Such small requests are obviously good, no reason to check them" :)

> 
> Looking into git blame, that someone is you :) (8b1170012b1)

Oops:) When I read your "but someone must have thought something", I really doubt in it :) But that was me, and I really thought something. Respect for your punctuality!

> Looks like you added those calls because BdrvTrackedRequest is a block layer structure, so getting rid of it means the reason for bdrv_check_request() disappears.  OK.

Yes, I was worried about the fact that BdrvTrackedRequest is a public structure and is a potential backdoor for not-checked offset/bytes things. At some moment we'd better close it (hide structure and add some interfaces).

> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/unit/test-write-threshold.c | 22 ++++------------------
>>   1 file changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
>> index fc1c45a2eb..fd40a815b8 100644
>> --- a/tests/unit/test-write-threshold.c
>> +++ b/tests/unit/test-write-threshold.c
>> @@ -55,41 +55,27 @@ static void test_threshold_multi_set_get(void)
>>   static void test_threshold_not_trigger(void)
>>   {
>> -    uint64_t amount = 0;
>>       uint64_t threshold = 4 * 1024 * 1024;
>>       BlockDriverState bs;
>> -    BdrvTrackedRequest req;
>>       memset(&bs, 0, sizeof(bs));
>> -    memset(&req, 0, sizeof(req));
>> -    req.offset = 1024;
>> -    req.bytes = 1024;
>> -
>> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>>       bdrv_write_threshold_set(&bs, threshold);
>> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
>> -    g_assert_cmpuint(amount, ==, 0);
>> +    bdrv_write_threshold_check_write(&bs, 1024, 1024);
>> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
>>   }
>>   static void test_threshold_trigger(void)
>>   {
>> -    uint64_t amount = 0;
>>       uint64_t threshold = 4 * 1024 * 1024;
>>       BlockDriverState bs;
>> -    BdrvTrackedRequest req;
>>       memset(&bs, 0, sizeof(bs));
>> -    memset(&req, 0, sizeof(req));
>> -    req.offset = (4 * 1024 * 1024) - 1024;
>> -    req.bytes = 2 * 1024;
>> -
>> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>>       bdrv_write_threshold_set(&bs, threshold);
>> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
>> -    g_assert_cmpuint(amount, >=, 1024);
>> +    bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
>> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
>>   }
>>   typedef struct TestStruct {
>>
>
diff mbox series

Patch

diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index fc1c45a2eb..fd40a815b8 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -55,41 +55,27 @@  static void test_threshold_multi_set_get(void)
 
 static void test_threshold_not_trigger(void)
 {
-    uint64_t amount = 0;
     uint64_t threshold = 4 * 1024 * 1024;
     BlockDriverState bs;
-    BdrvTrackedRequest req;
 
     memset(&bs, 0, sizeof(bs));
-    memset(&req, 0, sizeof(req));
-    req.offset = 1024;
-    req.bytes = 1024;
-
-    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
-    amount = bdrv_write_threshold_exceeded(&bs, &req);
-    g_assert_cmpuint(amount, ==, 0);
+    bdrv_write_threshold_check_write(&bs, 1024, 1024);
+    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
 }
 
 
 static void test_threshold_trigger(void)
 {
-    uint64_t amount = 0;
     uint64_t threshold = 4 * 1024 * 1024;
     BlockDriverState bs;
-    BdrvTrackedRequest req;
 
     memset(&bs, 0, sizeof(bs));
-    memset(&req, 0, sizeof(req));
-    req.offset = (4 * 1024 * 1024) - 1024;
-    req.bytes = 2 * 1024;
-
-    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
-    amount = bdrv_write_threshold_exceeded(&bs, &req);
-    g_assert_cmpuint(amount, >=, 1024);
+    bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
+    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
 }
 
 typedef struct TestStruct {