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

From: Max Reitz <mreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, wencongyang2@huawei.com,
	xiechanglong.d@gmail.com, qemu-devel@nongnu.org,
	armbru@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v4 09/23] job: call job_enter from job_pause
Date: Mon, 18 Jan 2021 15:20:33 +0100	[thread overview]
Message-ID: <d68bbac5-0c19-7ee5-fd79-1f4c7d688a9f@redhat.com> (raw)
In-Reply-To: <cbd92d66-c403-e37e-2bf9-2c301f9fd667@redhat.com>

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

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>