[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse
Date: Tue, 16 Nov 2021 11:20:49 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0

15.11.2021 16:56, Hanna Reitz wrote:
On 12.11.21 17:25, Vladimir Sementsov-Ogievskiy wrote:
11.11.2021 15:08, Hanna Reitz wrote:
See the comment for why this is necessary.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
  tests/qemu-iotests/030 | 11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 5fb65b4bef..567bf1da67 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -251,7 +251,16 @@ class TestParallelOps(iotests.QMPTestCase):
              self.assert_qmp(result, 'return', {})
  -        for job in pending_jobs:
+        # Do this in reverse: After unthrottling them, some jobs may finish
+        # before we have unthrottled all of them.  This will drain their
+        # subgraph, and this will make jobs above them advance (despite those
+        # jobs on top being throttled).  In the worst case, all jobs below the
+        # top one are finished before we can unthrottle it, and this makes it
+        # advance so far that it completes before we can unthrottle it - which
+        # results in an error.
+        # Starting from the top (i.e. in reverse) does not have this problem:
+        # When a job finishes, the ones below it are not advanced.

Hmm, interesting why only jobs above the finished job may advance in the 

Looks like something may change and this workaround will stop working.

Isn't it better just handle the error, and don't care if job was just finished?

Something like

if result['return'] != {}:
   # Job was finished during drain caused by finish of already unthrottled job
   self.assert_qmp(result, 'error/class', 'DeviceNotActive')

Well.  My explanation (excuse) is that I felt like this was the hack-ish 
solution that I could have gone for from the start without understanding what 
the issue is (and in fact it was the solution I used while debugging the other 
problems).  I went with `reversed()`, because that really addresses the problem.

You’re right in that it only addresses the problem for now and there’s a chance 
it might reappear.  If we want to go with ignoring DeviceNotActive errors, then 
I think we should at least query all block jobs before the unthrottle loop and 
see that at least at one point they were all running simultaneously.

I don’t really have a strong opinion.  We can exchange this patch now (though 
I’d rather not hold up the rest of the series for it), or have a patch on top 
later, or, well, just keep it for now.  I think the least stressful option 
would be to just fix it up later.

OK, I agree

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]