[PATCH 0/7] iotests/129: Fix it

[PATCH 0/7] iotests/129: Fix it

[Max Reitz]

Hi,

There are some problems with iotests 129 (perhaps more than these, but
these are the ones I know of):

1. It checks @busy to see whether a block job is still running; however,
   block jobs tend to unset @busy all the time (when they yield).
   [Fixed by patch 3]

2. It uses blockdev throttling, which quite some time ago has been moved
   to the BB level; since then, such throttling will no longer affect
   block jobs.  We can get throttling to work by using a throttle filter
   node.
   [Fixed by patch 4]

3. The mirror job has a large buffer size by default.  A simple drain
   may lead to it making significant process, which is kind of
   dangerous, because we don’t want the job to complete.
   To get around this, we can simply limit its buffer size.  (And we
   should make the commit job an actual commit job instead of an active
   commit (which is just mirror), because the commit interface does not
   allow setting a buffer size.)
   [Fixed by patches 5 and 6]

This series fixes those things, and now 129 seems to reliably pass for
me.


Apart from the major issues above, there are also minor flaws:

- It doesn’t remove the test images.
  [Fixed by patches 1 and 2]

- pylint and mypy complain.
  (Running mypy with the options given in 297.)
  [Patch 4 removes one pylint complaint; patch 7 the rest.]


Max Reitz (7):
  iotests: Move try_remove to iotests.py
  iotests/129: Remove test images in tearDown()
  iotests/129: Do not check @busy
  iotests/129: Use throttle node
  iotests/129: Actually test a commit job
  iotests/129: Limit mirror job's buffer size
  iotests/129: Clean up pylint and mypy complaints

 tests/qemu-iotests/124        |  8 +---
 tests/qemu-iotests/129        | 76 ++++++++++++++++++++++-------------
 tests/qemu-iotests/iotests.py | 11 +++--
 3 files changed, 55 insertions(+), 40 deletions(-)

-- 
2.29.2

[PATCH 1/7] iotests: Move try_remove to iotests.py

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/124        |  8 +-------
 tests/qemu-iotests/iotests.py | 11 +++++++----
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3705cbb6b3..e40eeb50b9 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -22,6 +22,7 @@
 
 import os
 import iotests
+from iotests import try_remove
 
 
 def io_write_patterns(img, patterns):
@@ -29,13 +30,6 @@ def io_write_patterns(img, patterns):
         iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
 
 
-def try_remove(img):
-    try:
-        os.remove(img)
-    except OSError:
-        pass
-
-
 def transaction_action(action, **kwargs):
     return {
         'type': action,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dcdcd0387f..15704ca466 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -515,12 +515,15 @@ class FilePath:
         return False
 
 
+def try_remove(img):
+    try:
+        os.remove(img)
+    except OSError:
+        pass
+
 def file_path_remover():
     for path in reversed(file_path_remover.paths):
-        try:
-            os.remove(path)
-        except OSError:
-            pass
+        try_remove(path)
 
 
 def file_path(*names, base_dir=test_dir):
-- 
2.29.2

Re: [PATCH 1/7] iotests: Move try_remove to iotests.py

[Eric Blake]

On 1/13/21 8:06 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/124        |  8 +-------
>  tests/qemu-iotests/iotests.py | 11 +++++++----
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 2/7] iotests/129: Remove test images in tearDown()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..2fc65ada6a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
                              **params)
         self.vm.shutdown()
+        for img in (self.test_img, self.target_img, self.base_img):
+            iotests.try_remove(img)
 
     def do_test_stop(self, cmd, **args):
         """Test 'stop' while block job is running on a throttled drive.
-- 
2.29.2

Re: [PATCH 2/7] iotests/129: Remove test images in tearDown()

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:06, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/129 | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 0e13244d85..2fc65ada6a 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>           result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>                                **params)
>           self.vm.shutdown()
> +        for img in (self.test_img, self.target_img, self.base_img):
> +            iotests.try_remove(img)
>   
>       def do_test_stop(self, cmd, **args):
>           """Test 'stop' while block job is running on a throttled drive.
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/7] iotests/129: Remove test images in tearDown()

[Eric Blake]

On 1/13/21 8:06 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129 | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 0e13244d85..2fc65ada6a 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>          result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>                               **params)
>          self.vm.shutdown()
> +        for img in (self.test_img, self.target_img, self.base_img):
> +            iotests.try_remove(img)
>  
>      def do_test_stop(self, cmd, **args):
>          """Test 'stop' while block job is running on a throttled drive.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 3/7] iotests/129: Do not check @busy

[Max Reitz]

@busy is false when the job is paused, which happens all the time
because that is how jobs yield (e.g. for mirror at least since commit
565ac01f8d3).

Back when 129 was added (2015), perhaps there was no better way of
checking whether the job was still actually running.  Now we have the
@status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
that information.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 2fc65ada6a..dd23bb2e5a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         result = self.vm.qmp("stop")
         self.assert_qmp(result, 'return', {})
         result = self.vm.qmp("query-block-jobs")
-        self.assert_qmp(result, 'return[0]/busy', True)
+        self.assert_qmp(result, 'return[0]/status', 'running')
         self.assert_qmp(result, 'return[0]/ready', False)
 
     def test_drive_mirror(self):
-- 
2.29.2

Re: [PATCH 3/7] iotests/129: Do not check @busy

[Eric Blake]

On 1/13/21 8:06 AM, Max Reitz wrote:
> @busy is false when the job is paused, which happens all the time
> because that is how jobs yield (e.g. for mirror at least since commit
> 565ac01f8d3).
> 
> Back when 129 was added (2015), perhaps there was no better way of
> checking whether the job was still actually running.  Now we have the
> @status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
> that information.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 2fc65ada6a..dd23bb2e5a 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>          result = self.vm.qmp("stop")
>          self.assert_qmp(result, 'return', {})
>          result = self.vm.qmp("query-block-jobs")
> -        self.assert_qmp(result, 'return[0]/busy', True)
> +        self.assert_qmp(result, 'return[0]/status', 'running')
>          self.assert_qmp(result, 'return[0]/ready', False)
>  
>      def test_drive_mirror(self):
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/7] iotests/129: Do not check @busy

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:06, Max Reitz wrote:
> @busy is false when the job is paused, which happens all the time
> because that is how jobs yield (e.g. for mirror at least since commit
> 565ac01f8d3).
> 
> Back when 129 was added (2015), perhaps there was no better way of
> checking whether the job was still actually running.  Now we have the
> @status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
> that information.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/129 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 2fc65ada6a..dd23bb2e5a 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>           result = self.vm.qmp("stop")
>           self.assert_qmp(result, 'return', {})
>           result = self.vm.qmp("query-block-jobs")
> -        self.assert_qmp(result, 'return[0]/busy', True)
> +        self.assert_qmp(result, 'return[0]/status', 'running')
>           self.assert_qmp(result, 'return[0]/ready', False)
>   
>       def test_drive_mirror(self):
> 


-- 
Best regards,
Vladimir

[PATCH 4/7] iotests/129: Use throttle node

[Max Reitz]

Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index dd23bb2e5a..febc806398 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
                          "-b", self.base_img, '-F', iotests.imgfmt)
         iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
-        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm = iotests.VM()
+        self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
+
+        source_drive = 'driver=throttle,' + \
+                       'throttle-group=tg0,' + \
+                      f'file.driver={iotests.imgfmt},' + \
+                      f'file.file.filename={self.test_img}'
+
+        self.vm.add_drive(None, source_drive)
         self.vm.launch()
 
     def tearDown(self):
-        params = {"device": "drive0",
-                  "bps": 0,
-                  "bps_rd": 0,
-                  "bps_wr": 0,
-                  "iops": 0,
-                  "iops_rd": 0,
-                  "iops_wr": 0,
-                 }
-        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
-                             **params)
         self.vm.shutdown()
         for img in (self.test_img, self.target_img, self.base_img):
             iotests.try_remove(img)
@@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
     def do_test_stop(self, cmd, **args):
         """Test 'stop' while block job is running on a throttled drive.
         The 'stop' command shouldn't drain the job"""
-        params = {"device": "drive0",
-                  "bps": 1024,
-                  "bps_rd": 0,
-                  "bps_wr": 0,
-                  "iops": 0,
-                  "iops_rd": 0,
-                  "iops_wr": 0,
-                 }
-        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
-                             **params)
-        self.assert_qmp(result, 'return', {})
         result = self.vm.qmp(cmd, **args)
         self.assert_qmp(result, 'return', {})
+
         result = self.vm.qmp("stop")
         self.assert_qmp(result, 'return', {})
         result = self.vm.qmp("query-block-jobs")
+
         self.assert_qmp(result, 'return[0]/status', 'running')
         self.assert_qmp(result, 'return[0]/ready', False)
 
+        self.vm.qmp("block-job-cancel", device="drive0", force=True)
+
     def test_drive_mirror(self):
         self.do_test_stop("drive-mirror", device="drive0",
-                          target=self.target_img,
+                          target=self.target_img, format=iotests.imgfmt,
                           sync="full")
 
     def test_drive_backup(self):
         self.do_test_stop("drive-backup", device="drive0",
-                          target=self.target_img,
+                          target=self.target_img, format=iotests.imgfmt,
                           sync="full")
 
     def test_block_commit(self):
-- 
2.29.2

Re: [PATCH 4/7] iotests/129: Use throttle node

[Max Reitz]

On 13.01.21 15:06, Max Reitz wrote:
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>   1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index dd23bb2e5a..febc806398 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129

[...]

> @@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>       def do_test_stop(self, cmd, **args):
>           """Test 'stop' while block job is running on a throttled drive.
>           The 'stop' command shouldn't drain the job"""
> -        params = {"device": "drive0",
> -                  "bps": 1024,
> -                  "bps_rd": 0,
> -                  "bps_wr": 0,
> -                  "iops": 0,
> -                  "iops_rd": 0,
> -                  "iops_wr": 0,
> -                 }
> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> -                             **params)
> -        self.assert_qmp(result, 'return', {})
>           result = self.vm.qmp(cmd, **args)
>           self.assert_qmp(result, 'return', {})
> +
>           result = self.vm.qmp("stop")
>           self.assert_qmp(result, 'return', {})
>           result = self.vm.qmp("query-block-jobs")
> +
>           self.assert_qmp(result, 'return[0]/status', 'running')
>           self.assert_qmp(result, 'return[0]/ready', False)
>   
> +        self.vm.qmp("block-job-cancel", device="drive0", force=True)
> +

Sorry, somewhere along the way I forgot to remove this block-job-cancel. 
  It’s still from
https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html 
and I don’t know why I added it there.  It should probably go.

('stop' should have drained, that’s the point, so there shouldn’t be any 
further delay if we quit the VM without cancelling the job.)

Max

Re: [PATCH 4/7] iotests/129: Use throttle node

[Eric Blake]

On 1/13/21 8:10 AM, Max Reitz wrote:
> On 13.01.21 15:06, Max Reitz wrote:
>> Throttling on the BB has not affected block jobs in a while, so it is
>> possible that one of the jobs in 129 finishes before the VM is stopped.
>> We can fix that by running the job from a throttle node.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>>   1 file changed, 15 insertions(+), 24 deletions(-)
>>

>>           result = self.vm.qmp("stop")
>>           self.assert_qmp(result, 'return', {})
>>           result = self.vm.qmp("query-block-jobs")
>> +
>>           self.assert_qmp(result, 'return[0]/status', 'running')
>>           self.assert_qmp(result, 'return[0]/ready', False)
>>   +        self.vm.qmp("block-job-cancel", device="drive0", force=True)
>> +
> 
> Sorry, somewhere along the way I forgot to remove this block-job-cancel.
>  It’s still from
> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html
> and I don’t know why I added it there.  It should probably go.
> 
> ('stop' should have drained, that’s the point, so there shouldn’t be any
> further delay if we quit the VM without cancelling the job.)

Since the test still passes without the added block-job-cancel, I'm fine
with removing that line then adding:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 4/7] iotests/129: Use throttle node

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:10, Max Reitz wrote:
> On 13.01.21 15:06, Max Reitz wrote:
>> Throttling on the BB has not affected block jobs in a while, so it is
>> possible that one of the jobs in 129 finishes before the VM is stopped.
>> We can fix that by running the job from a throttle node.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>>   1 file changed, 15 insertions(+), 24 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>> index dd23bb2e5a..febc806398 100755
>> --- a/tests/qemu-iotests/129
>> +++ b/tests/qemu-iotests/129
> 
> [...]
> 
>> @@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>       def do_test_stop(self, cmd, **args):
>>           """Test 'stop' while block job is running on a throttled drive.
>>           The 'stop' command shouldn't drain the job"""
>> -        params = {"device": "drive0",
>> -                  "bps": 1024,
>> -                  "bps_rd": 0,
>> -                  "bps_wr": 0,
>> -                  "iops": 0,
>> -                  "iops_rd": 0,
>> -                  "iops_wr": 0,
>> -                 }
>> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>> -                             **params)
>> -        self.assert_qmp(result, 'return', {})
>>           result = self.vm.qmp(cmd, **args)
>>           self.assert_qmp(result, 'return', {})
>> +
>>           result = self.vm.qmp("stop")
>>           self.assert_qmp(result, 'return', {})
>>           result = self.vm.qmp("query-block-jobs")
>> +
>>           self.assert_qmp(result, 'return[0]/status', 'running')
>>           self.assert_qmp(result, 'return[0]/ready', False)
>> +        self.vm.qmp("block-job-cancel", device="drive0", force=True)
>> +
> 
> Sorry, somewhere along the way I forgot to remove this block-job-cancel.  It’s still from
> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html and I don’t know why I added it there.  It should probably go.
> 
> ('stop' should have drained, that’s the point, so there shouldn’t be any further delay if we quit the VM without cancelling the job.)
> 

what do you mean by 'stop' should have drained? As I understand, after "stop" jobs should continue and it's OK.. So, immediately after "stop" command job may generate new requests, why not?


-- 
Best regards,
Vladimir

Re: [PATCH 4/7] iotests/129: Use throttle node

[Max Reitz]

On 13.01.21 16:07, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 17:10, Max Reitz wrote:
>> On 13.01.21 15:06, Max Reitz wrote:
>>> Throttling on the BB has not affected block jobs in a while, so it is
>>> possible that one of the jobs in 129 finishes before the VM is stopped.
>>> We can fix that by running the job from a throttle node.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>>>   1 file changed, 15 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>>> index dd23bb2e5a..febc806398 100755
>>> --- a/tests/qemu-iotests/129
>>> +++ b/tests/qemu-iotests/129
>>
>> [...]
>>
>>> @@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>>       def do_test_stop(self, cmd, **args):
>>>           """Test 'stop' while block job is running on a throttled 
>>> drive.
>>>           The 'stop' command shouldn't drain the job"""
>>> -        params = {"device": "drive0",
>>> -                  "bps": 1024,
>>> -                  "bps_rd": 0,
>>> -                  "bps_wr": 0,
>>> -                  "iops": 0,
>>> -                  "iops_rd": 0,
>>> -                  "iops_wr": 0,
>>> -                 }
>>> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>>> -                             **params)
>>> -        self.assert_qmp(result, 'return', {})
>>>           result = self.vm.qmp(cmd, **args)
>>>           self.assert_qmp(result, 'return', {})
>>> +
>>>           result = self.vm.qmp("stop")
>>>           self.assert_qmp(result, 'return', {})
>>>           result = self.vm.qmp("query-block-jobs")
>>> +
>>>           self.assert_qmp(result, 'return[0]/status', 'running')
>>>           self.assert_qmp(result, 'return[0]/ready', False)
>>> +        self.vm.qmp("block-job-cancel", device="drive0", force=True)
>>> +
>>
>> Sorry, somewhere along the way I forgot to remove this 
>> block-job-cancel.  It’s still from
>> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html 
>> and I don’t know why I added it there.  It should probably go.
>>
>> ('stop' should have drained, that’s the point, so there shouldn’t be 
>> any further delay if we quit the VM without cancelling the job.)
>>
> 
> what do you mean by 'stop' should have drained? As I understand, after 
> "stop" jobs should continue and it's OK.. So, immediately after "stop" 
> command job may generate new requests, why not?

Hm, right.

Well, either way, the cancel shouldn’t make a difference.  If quitting 
the VM were to stall because of the job, then cancelling it beforehand 
only means that then cancel will stall.

Max

Re: [PATCH 4/7] iotests/129: Use throttle node

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:06, Max Reitz wrote:
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>   1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index dd23bb2e5a..febc806398 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>           iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
>                            "-b", self.base_img, '-F', iotests.imgfmt)
>           iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
> -        self.vm = iotests.VM().add_drive(self.test_img)
> +        self.vm = iotests.VM()
> +        self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
> +
> +        source_drive = 'driver=throttle,' + \
> +                       'throttle-group=tg0,' + \
> +                      f'file.driver={iotests.imgfmt},' + \
> +                      f'file.file.filename={self.test_img}'

python has a "c-like" string "concatenation", i.e., you may omit "+" operators (I don't know is it a good practice, but I do so:)

> +
> +        self.vm.add_drive(None, source_drive)
>           self.vm.launch()
>   
>       def tearDown(self):
> -        params = {"device": "drive0",
> -                  "bps": 0,
> -                  "bps_rd": 0,
> -                  "bps_wr": 0,
> -                  "iops": 0,
> -                  "iops_rd": 0,
> -                  "iops_wr": 0,
> -                 }
> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> -                             **params)
>           self.vm.shutdown()
>           for img in (self.test_img, self.target_img, self.base_img):
>               iotests.try_remove(img)
> @@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>       def do_test_stop(self, cmd, **args):
>           """Test 'stop' while block job is running on a throttled drive.
>           The 'stop' command shouldn't drain the job"""
> -        params = {"device": "drive0",
> -                  "bps": 1024,
> -                  "bps_rd": 0,
> -                  "bps_wr": 0,
> -                  "iops": 0,
> -                  "iops_rd": 0,
> -                  "iops_wr": 0,
> -                 }
> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> -                             **params)
> -        self.assert_qmp(result, 'return', {})
>           result = self.vm.qmp(cmd, **args)
>           self.assert_qmp(result, 'return', {})
> +
>           result = self.vm.qmp("stop")
>           self.assert_qmp(result, 'return', {})
>           result = self.vm.qmp("query-block-jobs")
> +
>           self.assert_qmp(result, 'return[0]/status', 'running')
>           self.assert_qmp(result, 'return[0]/ready', False)
>   
> +        self.vm.qmp("block-job-cancel", device="drive0", force=True)

won't hurt, but I'd drop it as unrelated to commit

> +
>       def test_drive_mirror(self):
>           self.do_test_stop("drive-mirror", device="drive0",
> -                          target=self.target_img,
> +                          target=self.target_img, format=iotests.imgfmt,
>                             sync="full")

this doesn't seem need/related too..

>   
>       def test_drive_backup(self):
>           self.do_test_stop("drive-backup", device="drive0",
> -                          target=self.target_img,
> +                          target=self.target_img, format=iotests.imgfmt,
>                             sync="full")
>   
>       def test_block_commit(self):
> 


-- 
Best regards,
Vladimir

Re: [PATCH 4/7] iotests/129: Use throttle node

[Max Reitz]

On 13.01.21 17:46, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 17:06, Max Reitz wrote:
>> Throttling on the BB has not affected block jobs in a while, so it is
>> possible that one of the jobs in 129 finishes before the VM is stopped.
>> We can fix that by running the job from a throttle node.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>>   1 file changed, 15 insertions(+), 24 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>> index dd23bb2e5a..febc806398 100755
>> --- a/tests/qemu-iotests/129
>> +++ b/tests/qemu-iotests/129
>> @@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>           iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
>>                            "-b", self.base_img, '-F', iotests.imgfmt)
>>           iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 
>> 128M', self.test_img)
>> -        self.vm = iotests.VM().add_drive(self.test_img)
>> +        self.vm = iotests.VM()
>> +        self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
>> +
>> +        source_drive = 'driver=throttle,' + \
>> +                       'throttle-group=tg0,' + \
>> +                      f'file.driver={iotests.imgfmt},' + \
>> +                      f'file.file.filename={self.test_img}'
> 
> python has a "c-like" string "concatenation", i.e., you may omit "+" 
> operators (I don't know is it a good practice, but I do so:)

OK, why not.

>> +
>> +        self.vm.add_drive(None, source_drive)
>>           self.vm.launch()
>>       def tearDown(self):
>> -        params = {"device": "drive0",
>> -                  "bps": 0,
>> -                  "bps_rd": 0,
>> -                  "bps_wr": 0,
>> -                  "iops": 0,
>> -                  "iops_rd": 0,
>> -                  "iops_wr": 0,
>> -                 }
>> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>> -                             **params)
>>           self.vm.shutdown()
>>           for img in (self.test_img, self.target_img, self.base_img):
>>               iotests.try_remove(img)
>> @@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>       def do_test_stop(self, cmd, **args):
>>           """Test 'stop' while block job is running on a throttled drive.
>>           The 'stop' command shouldn't drain the job"""
>> -        params = {"device": "drive0",
>> -                  "bps": 1024,
>> -                  "bps_rd": 0,
>> -                  "bps_wr": 0,
>> -                  "iops": 0,
>> -                  "iops_rd": 0,
>> -                  "iops_wr": 0,
>> -                 }
>> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>> -                             **params)
>> -        self.assert_qmp(result, 'return', {})
>>           result = self.vm.qmp(cmd, **args)
>>           self.assert_qmp(result, 'return', {})
>> +
>>           result = self.vm.qmp("stop")
>>           self.assert_qmp(result, 'return', {})
>>           result = self.vm.qmp("query-block-jobs")
>> +
>>           self.assert_qmp(result, 'return[0]/status', 'running')
>>           self.assert_qmp(result, 'return[0]/ready', False)
>> +        self.vm.qmp("block-job-cancel", device="drive0", force=True)
> 
> won't hurt, but I'd drop it as unrelated to commit
> 
>> +
>>       def test_drive_mirror(self):
>>           self.do_test_stop("drive-mirror", device="drive0",
>> -                          target=self.target_img,
>> +                          target=self.target_img, format=iotests.imgfmt,
>>                             sync="full")
> 
> this doesn't seem need/related too..

It is, because without a @format parameter, the job uses the driver of 
@device as the output format.  That driver is now 'throttle', which 
doesn’t work as a format.

Max

Re: [PATCH 4/7] iotests/129: Use throttle node

[Vladimir Sementsov-Ogievskiy]

13.01.2021 20:02, Max Reitz wrote:
> On 13.01.21 17:46, Vladimir Sementsov-Ogievskiy wrote:
>> 13.01.2021 17:06, Max Reitz wrote:
>>> Throttling on the BB has not affected block jobs in a while, so it is
>>> possible that one of the jobs in 129 finishes before the VM is stopped.
>>> We can fix that by running the job from a throttle node.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/129 | 39 +++++++++++++++------------------------
>>>   1 file changed, 15 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>>> index dd23bb2e5a..febc806398 100755
>>> --- a/tests/qemu-iotests/129
>>> +++ b/tests/qemu-iotests/129
>>> @@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>>           iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
>>>                            "-b", self.base_img, '-F', iotests.imgfmt)
>>>           iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
>>> -        self.vm = iotests.VM().add_drive(self.test_img)
>>> +        self.vm = iotests.VM()
>>> +        self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
>>> +
>>> +        source_drive = 'driver=throttle,' + \
>>> +                       'throttle-group=tg0,' + \
>>> +                      f'file.driver={iotests.imgfmt},' + \
>>> +                      f'file.file.filename={self.test_img}'
>>
>> python has a "c-like" string "concatenation", i.e., you may omit "+" operators (I don't know is it a good practice, but I do so:)
> 
> OK, why not.
> 
>>> +
>>> +        self.vm.add_drive(None, source_drive)
>>>           self.vm.launch()
>>>       def tearDown(self):
>>> -        params = {"device": "drive0",
>>> -                  "bps": 0,
>>> -                  "bps_rd": 0,
>>> -                  "bps_wr": 0,
>>> -                  "iops": 0,
>>> -                  "iops_rd": 0,
>>> -                  "iops_wr": 0,
>>> -                 }
>>> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>>> -                             **params)
>>>           self.vm.shutdown()
>>>           for img in (self.test_img, self.target_img, self.base_img):
>>>               iotests.try_remove(img)
>>> @@ -53,33 +51,26 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>>>       def do_test_stop(self, cmd, **args):
>>>           """Test 'stop' while block job is running on a throttled drive.
>>>           The 'stop' command shouldn't drain the job"""
>>> -        params = {"device": "drive0",
>>> -                  "bps": 1024,
>>> -                  "bps_rd": 0,
>>> -                  "bps_wr": 0,
>>> -                  "iops": 0,
>>> -                  "iops_rd": 0,
>>> -                  "iops_wr": 0,
>>> -                 }
>>> -        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>>> -                             **params)
>>> -        self.assert_qmp(result, 'return', {})
>>>           result = self.vm.qmp(cmd, **args)
>>>           self.assert_qmp(result, 'return', {})
>>> +
>>>           result = self.vm.qmp("stop")
>>>           self.assert_qmp(result, 'return', {})
>>>           result = self.vm.qmp("query-block-jobs")
>>> +
>>>           self.assert_qmp(result, 'return[0]/status', 'running')
>>>           self.assert_qmp(result, 'return[0]/ready', False)
>>> +        self.vm.qmp("block-job-cancel", device="drive0", force=True)
>>
>> won't hurt, but I'd drop it as unrelated to commit
>>
>>> +
>>>       def test_drive_mirror(self):
>>>           self.do_test_stop("drive-mirror", device="drive0",
>>> -                          target=self.target_img,
>>> +                          target=self.target_img, format=iotests.imgfmt,
>>>                             sync="full")
>>
>> this doesn't seem need/related too..
> 
> It is, because without a @format parameter, the job uses the driver of @device as the output format.  That driver is now 'throttle', which doesn’t work as a format.
> 

Ah, understand now, OK.


-- 
Best regards,
Vladimir

[PATCH 5/7] iotests/129: Actually test a commit job

[Max Reitz]

Before this patch, test_block_commit() performs an active commit, which
under the hood is a mirror job.  If we want to test various different
block jobs, we should perhaps run an actual commit job instead.

Doing so requires adding an overlay above the source node before the
commit is done (and then specifying the source node as the top node for
the commit job).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index febc806398..e6753c90ef 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -26,6 +26,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
     test_img = os.path.join(iotests.test_dir, 'test.img')
     target_img = os.path.join(iotests.test_dir, 'target.img')
     base_img = os.path.join(iotests.test_dir, 'base.img')
+    overlay_img = os.path.join(iotests.test_dir, 'overlay.img')
 
     def setUp(self):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
@@ -36,6 +37,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
 
         source_drive = 'driver=throttle,' + \
+                       'node-name=source,' + \
                        'throttle-group=tg0,' + \
                       f'file.driver={iotests.imgfmt},' + \
                       f'file.file.filename={self.test_img}'
@@ -45,7 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 
     def tearDown(self):
         self.vm.shutdown()
-        for img in (self.test_img, self.target_img, self.base_img):
+        for img in (self.test_img, self.target_img, self.base_img,
+                    self.overlay_img):
             iotests.try_remove(img)
 
     def do_test_stop(self, cmd, **args):
@@ -74,7 +77,27 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
                           sync="full")
 
     def test_block_commit(self):
-        self.do_test_stop("block-commit", device="drive0")
+        # Add overlay above the source node so that we actually use a
+        # commit job instead of a mirror job
+
+        iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
+                         '1G')
+
+        result = self.vm.qmp('blockdev-add', **{
+                                 'node-name': 'overlay',
+                                 'driver': iotests.imgfmt,
+                                 'file': {
+                                     'driver': 'file',
+                                     'filename': self.overlay_img
+                                 }
+                             })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-snapshot',
+                             node='source', overlay='overlay')
+        self.assert_qmp(result, 'return', {})
+
+        self.do_test_stop('block-commit', device='drive0', top_node='source')
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=["qcow2"],
-- 
2.29.2

Re: [PATCH 5/7] iotests/129: Actually test a commit job

[Eric Blake]

On 1/13/21 8:06 AM, Max Reitz wrote:
> Before this patch, test_block_commit() performs an active commit, which
> under the hood is a mirror job.  If we want to test various different
> block jobs, we should perhaps run an actual commit job instead.
> 
> Doing so requires adding an overlay above the source node before the
> commit is done (and then specifying the source node as the top node for
> the commit job).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129 | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 5/7] iotests/129: Actually test a commit job

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:06, Max Reitz wrote:
> Before this patch, test_block_commit() performs an active commit, which
> under the hood is a mirror job.  If we want to test various different
> block jobs, we should perhaps run an actual commit job instead.
> 
> Doing so requires adding an overlay above the source node before the
> commit is done (and then specifying the source node as the top node for
> the commit job).
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 6/7] iotests/129: Limit mirror job's buffer size

[Max Reitz]

Issuing 'stop' on the VM drains all nodes.  If the mirror job has many
large requests in flight, this may lead to significant I/O that looks a
bit like the job is being drained.

We can limit the I/O in flight by limiting the buffer size, so mirror
will make very little progress during the 'stop' drain.

(We do not need to do anything about commit, which has a buffer size of
512 kB by default; or backup, which goes cluster by cluster.  Once we
have asynchronous requests for backup, that will change, but then we can
fine-tune the backup job to only perform a single request on a very
small chunk, too.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index e6753c90ef..c3ad584ba2 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
     def test_drive_mirror(self):
         self.do_test_stop("drive-mirror", device="drive0",
                           target=self.target_img, format=iotests.imgfmt,
-                          sync="full")
+                          sync="full", buf_size=65536)
 
     def test_drive_backup(self):
         self.do_test_stop("drive-backup", device="drive0",
-- 
2.29.2

Re: [PATCH 6/7] iotests/129: Limit mirror job's buffer size

[Eric Blake]

On 1/13/21 8:06 AM, Max Reitz wrote:
> Issuing 'stop' on the VM drains all nodes.  If the mirror job has many
> large requests in flight, this may lead to significant I/O that looks a
> bit like the job is being drained.
> 
> We can limit the I/O in flight by limiting the buffer size, so mirror
> will make very little progress during the 'stop' drain.
> 
> (We do not need to do anything about commit, which has a buffer size of
> 512 kB by default; or backup, which goes cluster by cluster.  Once we
> have asynchronous requests for backup, that will change, but then we can
> fine-tune the backup job to only perform a single request on a very
> small chunk, too.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index e6753c90ef..c3ad584ba2 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>      def test_drive_mirror(self):
>          self.do_test_stop("drive-mirror", device="drive0",
>                            target=self.target_img, format=iotests.imgfmt,
> -                          sync="full")
> +                          sync="full", buf_size=65536)
>  
>      def test_drive_backup(self):
>          self.do_test_stop("drive-backup", device="drive0",
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 6/7] iotests/129: Limit mirror job's buffer size

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:06, Max Reitz wrote:
> Issuing 'stop' on the VM drains all nodes.  If the mirror job has many
> large requests in flight, this may lead to significant I/O that looks a
> bit like the job is being drained.

what do you mean by "looks like the job is being drained"? If "drain"
in Qemu means "flush all in-flight requests, and don't create more during
the drain", then job is exactly "drained" during the drain..

do you mean, that it looks like "job trying to finish copying the whole drive"? Or "job hangs", or "stop hangs"...

> 
> We can limit the I/O in flight by limiting the buffer size, so mirror
> will make very little progress during the 'stop' drain.
> 
> (We do not need to do anything about commit, which has a buffer size of
> 512 kB by default; or backup, which goes cluster by cluster.  Once we
> have asynchronous requests for backup, that will change, but then we can
> fine-tune the backup job to only perform a single request on a very
> small chunk, too.)

OK

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

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/129 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index e6753c90ef..c3ad584ba2 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>       def test_drive_mirror(self):
>           self.do_test_stop("drive-mirror", device="drive0",
>                             target=self.target_img, format=iotests.imgfmt,
> -                          sync="full")
> +                          sync="full", buf_size=65536)
>   
>       def test_drive_backup(self):
>           self.do_test_stop("drive-backup", device="drive0",
> 


-- 
Best regards,
Vladimir

[PATCH 7/7] iotests/129: Clean up pylint and mypy complaints

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index c3ad584ba2..ec303069e9 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -20,9 +20,10 @@
 
 import os
 import iotests
-import time
 
 class TestStopWithBlockJob(iotests.QMPTestCase):
+    assert iotests.test_dir is not None
+
     test_img = os.path.join(iotests.test_dir, 'test.img')
     target_img = os.path.join(iotests.test_dir, 'target.img')
     base_img = os.path.join(iotests.test_dir, 'base.img')
@@ -32,7 +33,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
                          "-b", self.base_img, '-F', iotests.imgfmt)
-        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
+        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
+                        self.test_img)
         self.vm = iotests.VM()
         self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
 
-- 
2.29.2

Re: [PATCH 7/7] iotests/129: Clean up pylint and mypy complaints

[Eric Blake]

On 1/13/21 8:06 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/7] iotests/129: Fix it

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:06, Max Reitz wrote:
> Hi,
> 
> There are some problems with iotests 129 (perhaps more than these, but
> these are the ones I know of):
> 
> 1. It checks @busy to see whether a block job is still running; however,
>     block jobs tend to unset @busy all the time (when they yield).
>     [Fixed by patch 3]
> 
> 2. It uses blockdev throttling, which quite some time ago has been moved
>     to the BB level; since then, such throttling will no longer affect
>     block jobs.  We can get throttling to work by using a throttle filter
>     node.
>     [Fixed by patch 4]
> 
> 3. The mirror job has a large buffer size by default.  A simple drain
>     may lead to it making significant process, which is kind of
>     dangerous, because we don’t want the job to complete.

Not quite clear to me. iotest 129 wants to mirror 128M of data. Mirror by
default will have 1M chunk size and maximum of 16 parallel requests. So with
throttling (even if throttling can't correctly handle parallel requests)
we will not exceed 16M of progress.. Why we need limiting buffer size?

>     To get around this, we can simply limit its buffer size.  (And we
>     should make the commit job an actual commit job instead of an active
>     commit (which is just mirror), because the commit interface does not
>     allow setting a buffer size.)
>     [Fixed by patches 5 and 6]
> 
> This series fixes those things, and now 129 seems to reliably pass for
> me.
> 
> 
> Apart from the major issues above, there are also minor flaws:
> 
> - It doesn’t remove the test images.
>    [Fixed by patches 1 and 2]
> 
> - pylint and mypy complain.
>    (Running mypy with the options given in 297.)
>    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> 
> 
> Max Reitz (7):
>    iotests: Move try_remove to iotests.py
>    iotests/129: Remove test images in tearDown()
>    iotests/129: Do not check @busy
>    iotests/129: Use throttle node
>    iotests/129: Actually test a commit job
>    iotests/129: Limit mirror job's buffer size
>    iotests/129: Clean up pylint and mypy complaints
> 
>   tests/qemu-iotests/124        |  8 +---
>   tests/qemu-iotests/129        | 76 ++++++++++++++++++++++-------------
>   tests/qemu-iotests/iotests.py | 11 +++--
>   3 files changed, 55 insertions(+), 40 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/7] iotests/129: Fix it

[Max Reitz]

On 13.01.21 15:31, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 17:06, Max Reitz wrote:
>> Hi,
>>
>> There are some problems with iotests 129 (perhaps more than these, but
>> these are the ones I know of):
>>
>> 1. It checks @busy to see whether a block job is still running; however,
>>     block jobs tend to unset @busy all the time (when they yield).
>>     [Fixed by patch 3]
>>
>> 2. It uses blockdev throttling, which quite some time ago has been moved
>>     to the BB level; since then, such throttling will no longer affect
>>     block jobs.  We can get throttling to work by using a throttle filter
>>     node.
>>     [Fixed by patch 4]
>>
>> 3. The mirror job has a large buffer size by default.  A simple drain
>>     may lead to it making significant process, which is kind of
>>     dangerous, because we don’t want the job to complete.
> 
> Not quite clear to me. iotest 129 wants to mirror 128M of data. Mirror by
> default will have 1M chunk size and maximum of 16 parallel requests. So 
> with
> throttling (even if throttling can't correctly handle parallel requests)
> we will not exceed 16M of progress.. Why we need limiting buffer size?

It does exceed 16M of progress; without the limit, I generally see 
something between 16M and 32M.

Now, that still is below 128M, but it’s kind of in the same magnitude. 
I don’t feel comfortable with that, especially given it’s so easy to 
limit it to much less (buf_size=64k makes the job proceed to 128k).

Also, maybe the default is increased in the future.  Increasing the 
chunk size by 4 would mean that it might be possible to reach 128M.

I find not relying on the default better.

Max

Re: [PATCH 0/7] iotests/129: Fix it

[Vladimir Sementsov-Ogievskiy]

13.01.2021 18:19, Max Reitz wrote:
> On 13.01.21 15:31, Vladimir Sementsov-Ogievskiy wrote:
>> 13.01.2021 17:06, Max Reitz wrote:
>>> Hi,
>>>
>>> There are some problems with iotests 129 (perhaps more than these, but
>>> these are the ones I know of):
>>>
>>> 1. It checks @busy to see whether a block job is still running; however,
>>>     block jobs tend to unset @busy all the time (when they yield).
>>>     [Fixed by patch 3]
>>>
>>> 2. It uses blockdev throttling, which quite some time ago has been moved
>>>     to the BB level; since then, such throttling will no longer affect
>>>     block jobs.  We can get throttling to work by using a throttle filter
>>>     node.
>>>     [Fixed by patch 4]
>>>
>>> 3. The mirror job has a large buffer size by default.  A simple drain
>>>     may lead to it making significant process, which is kind of
>>>     dangerous, because we don’t want the job to complete.
>>
>> Not quite clear to me. iotest 129 wants to mirror 128M of data. Mirror by
>> default will have 1M chunk size and maximum of 16 parallel requests. So with
>> throttling (even if throttling can't correctly handle parallel requests)
>> we will not exceed 16M of progress.. Why we need limiting buffer size?
> 
> It does exceed 16M of progress; without the limit, I generally see something between 16M and 32M.
> 
> Now, that still is below 128M, but it’s kind of in the same magnitude. I don’t feel comfortable with that, especially given it’s so easy to limit it to much less (buf_size=64k makes the job proceed to 128k).
> 
> Also, maybe the default is increased in the future.  Increasing the chunk size by 4 would mean that it might be possible to reach 128M.
> 
> I find not relying on the default better.

Hmm, OK, agreed


-- 
Best regards,
Vladimir

Re: [PATCH 0/7] iotests/129: Fix it

[Kevin Wolf]

Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
> - pylint and mypy complain.
>   (Running mypy with the options given in 297.)
>   [Patch 4 removes one pylint complaint; patch 7 the rest.]

Should we add it to 297 then to make sure we won't regress?

At some point, I guess we'll want to cover all Python tests, but I
assume it was too much to change in the same series as for the
iotests.py cleanup.

Kevin

Re: [PATCH 0/7] iotests/129: Fix it

[Vladimir Sementsov-Ogievskiy]

13.01.2021 17:38, Kevin Wolf wrote:
> Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
>> - pylint and mypy complain.
>>    (Running mypy with the options given in 297.)
>>    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> 
> Should we add it to 297 then to make sure we won't regress?
> 
> At some point, I guess we'll want to cover all Python tests, but I
> assume it was too much to change in the same series as for the
> iotests.py cleanup.
> 

Related:

anyone knows, what's the difference between pylint and pylint-3?

I know I can do pip3 install pylint, which brings pylint binary.. Also there is pylint fedora package which provides pylint binary and python3-pylint fedora package which provides pylint-3 binary, both package names looks so that they are... the same?

Also. Interesting, but pylint don't check PEP8 code style (or at least not everything of it). For example, pycodestyle has a lot of complains about iotests.py (most of them "E302 expected 2 blank lines, found 1").. So, what about adding pycodestyle (or may be better flake8 which is pycodestyle + some additional things) to 127 iotest?

-- 
Best regards,
Vladimir

Re: [PATCH 0/7] iotests/129: Fix it

[Max Reitz]

On 13.01.21 15:38, Kevin Wolf wrote:
> Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
>> - pylint and mypy complain.
>>    (Running mypy with the options given in 297.)
>>    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> 
> Should we add it to 297 then to make sure we won't regress?

Sounds like a good precedent to set indeed.

> At some point, I guess we'll want to cover all Python tests, but I
> assume it was too much to change in the same series as for the
> iotests.py cleanup.
Originally, I hadn’t intended to write patch 7 at all; I just wanted to 
see what pylint/mypy said to my changes in this series, but then they 
made me aware of pre-existing litter.  I thought adding patch 7 would be 
the right thing to do, so I did.

(That’s to say so far I had no intentions of cleaning up all Python 
tests.  It’s just coincidence that I did so for 129.)

Max

Re: [PATCH 0/7] iotests/129: Fix it

[Kevin Wolf]

Am 13.01.2021 um 16:26 hat Max Reitz geschrieben:
> On 13.01.21 15:38, Kevin Wolf wrote:
> > Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
> > > - pylint and mypy complain.
> > >    (Running mypy with the options given in 297.)
> > >    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> > 
> > Should we add it to 297 then to make sure we won't regress?
> 
> Sounds like a good precedent to set indeed.
> 
> > At some point, I guess we'll want to cover all Python tests, but I
> > assume it was too much to change in the same series as for the
> > iotests.py cleanup.
> Originally, I hadn’t intended to write patch 7 at all; I just wanted to see
> what pylint/mypy said to my changes in this series, but then they made me
> aware of pre-existing litter.  I thought adding patch 7 would be the right
> thing to do, so I did.
> 
> (That’s to say so far I had no intentions of cleaning up all Python tests.
> It’s just coincidence that I did so for 129.)

Indeed, and I wasn't trying to imply that you should, but just making a
guess why we cover iotests.py and nothing else so far.

Cleaning up 129 is a nice side effect of this series. If more such side
effects accumulate and the rest becomes small enough, I might eventually
just convert the rest. Or I might not, who knows.

Kevin