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

[Max Reitz]

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