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().
OK.
Still, if job is already in job_do_yield() in job_pause_point() we
should not enter it.
Agreed.
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 <vsement...@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)
{
job->pause_count++;
+ 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 <mre...@redhat.com>