[Top][All Lists]

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

Re: [PATCH v4 09/23] job: call job_enter from job_pause

From: Max Reitz
Subject: Re: [PATCH v4 09/23] job: call job_enter from job_pause
Date: Mon, 18 Jan 2021 15:20:33 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0

On 18.01.21 14:45, Max Reitz wrote:
On 16.01.21 22:46, Vladimir Sementsov-Ogievskiy wrote:
If main job coroutine called job_yield (while some background process
is in progress), we should give it a chance to call job_pause_point().
It will be used in backup, when moved on async block-copy.

Note, that job_user_pause is not enough: we want to handle
child_job_drained_begin() as well, which call job_pause().


Still, if job is already in job_do_yield() in job_pause_point() we
should not enter it.


iotest 109 output is modified: on stop we do bdrv_drain_all() which now
triggers job pause immediately (and pause after ready is standby).

Sounds like a good thing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
  job.c                      |  3 +++
  tests/qemu-iotests/109.out | 24 ++++++++++++++++++++++++
  2 files changed, 27 insertions(+)

diff --git a/job.c b/job.c
index 8fecf38960..3aaaebafe2 100644
--- a/job.c
+++ b/job.c
@@ -553,6 +553,9 @@ static bool job_timer_not_pending(Job *job)
  void job_pause(Job *job)
+    if (!job->paused) {
+        job_enter(job);
+    }

I see job_pause is also called from block_job_error_action() – should we reenter the job there, too?

(It looks to me like e.g. mirror would basically just continue to run, then, until it needs to yield because of some other issue.  I don’t know whether that’s a problem, because I suppose we don’t guarantee to stop immediately on an error, though I suspect users would expect us to do that as early as possible (i.e., not to launch new requests).

[Quite some time later]

I’ve now tested a mirror job that stops due to a target error, and it actually does not make any progress; or at least it doesn’t report any.  So it looks like my concern is unjustified.  I don’t know why it’s unjustified, though, so perhaps you can explain it before I give my R-b O:))

Oh, I guess because job_enter_cond() doesn’t enter if the job is busy already. That would make a lot of sense, so I’m going to assume that’s what’s preventing the job_enter() to do anything if the job is already running (which it has to be to invoke block_job_error_action()).

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

reply via email to

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