Re: [PATCH for-6.1? v2 6/7] mirror: Check job_is_cancelled() earlier

[Max Reitz <mreitz@redhat.com>]

On 04.08.21 11:48, Kevin Wolf wrote:
> Am 04.08.2021 um 10:25 hat Max Reitz geschrieben:
>> On 03.08.21 16:34, Kevin Wolf wrote:
>>> Am 26.07.2021 um 16:46 hat Max Reitz geschrieben:
>>>> We must check whether the job is force-cancelled early in our main loop,
>>>> most importantly before any `continue` statement.  For example, we used
>>>> to have `continue`s before our current checking location that are
>>>> triggered by `mirror_flush()` failing.  So, if `mirror_flush()` kept
>>>> failing, force-cancelling the job would not terminate it.
>>>>
>>>> A job being force-cancelled should be treated the same as the job having
>>>> failed, so put the check in the same place where we check `s->ret < 0`.
>>>>
>>>> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>    block/mirror.c | 7 +------
>>>>    1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>> index 72e02fa34e..46d1a1e5a2 100644
>>>> --- a/block/mirror.c
>>>> +++ b/block/mirror.c
>>>> @@ -993,7 +993,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>>>>                mirror_wait_for_any_operation(s, true);
>>>>            }
>>>> -        if (s->ret < 0) {
>>>> +        if (s->ret < 0 || job_is_cancelled(&s->common.job)) {
>>>>                ret = s->ret;
>>>>                goto immediate_exit;
>>>>            }
>>>> @@ -1078,8 +1078,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>>>>                break;
>>>>            }
>>>> -        ret = 0;
>>>> -
>>>>            if (job_is_ready(&s->common.job) && !should_complete) {
>>>>                delay_ns = (s->in_flight == 0 &&
>>>>                            cnt == 0 ? BLOCK_JOB_SLICE_TIME : 0);
>>>> @@ -1087,9 +1085,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>>>>            trace_mirror_before_sleep(s, cnt, job_is_ready(&s->common.job),
>>>>                                      delay_ns);
>>>>            job_sleep_ns(&s->common.job, delay_ns);
>>>> -        if (job_is_cancelled(&s->common.job)) {
>>>> -            break;
>>>> -        }
>>> I think it was intentional that the check is here because it means
>>> skipping the job_sleep_ns() and instead cancelling immediately, and we
>>> probably still want that. Between your check above and here, the
>>> coroutine can yield, so cancellation could have been newly requested.
>> I’m afraid I don’t quite understand.
> Hm, I don't either. Somehow I thought job_sleep_ns() was after the
> check, while quoting the exact hunk that shows that it comes before
> it...
>
> I'm still not sure if sleeping before exiting is really useful, but it
> seems we never cared about that.

Jobs that are (force-)cancelled cannot yield or sleep anyway 
(job_sleep_ns(), job_yield(), and job_pause_point() will all return 
immediately when called on a cancelled job).

So I thought you meant that a job can only be cancelled while it is 
yielding, so we should prefer to put the is_cancelled check after a 
yield point (like job_pause_point()) than before it.

But I mean, if you’re happy, I’ll be happy, too. :)

Max