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

[Vladimir Sementsov-Ogievskiy]

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