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

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

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 {
>>
> 


-- 
Best regards,
Vladimir