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

[Max Reitz]

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 <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)
>   {
>       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:))

Max