| Message ID | 20210806093859.706464-12-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mirror: Handle errors after READY cancel | expand |
On Fri, Aug 06, 2021 at 11:38:58AM +0200, Max Reitz wrote: > Clearing .cancelled before leaving the main loop when the job has been > soft-cancelled is no longer necessary since job_is_cancelled() only > returns true for jobs that have been force-cancelled. > > Therefore, this only makes a differences in places that call > job_cancel_requested(). In block/mirror.c, this is done only before > .cancelled was cleared. > > In job.c, there are two callers: > - job_completed_txn_abort() asserts that .cancelled is true, so keeping > it true will not affect this place. > > - job_complete() refuses to let a job complete that has .cancelled set. > It is correct to refuse to let the user invoke job-complete on mirror > jobs that have already been soft-cancelled. > > With this change, there are no places that reset .cancelled to false and > so we can be sure that .force_cancel can only be true of .cancelled is s/of/if/ > true as well. Assert this in job_is_cancelled(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/mirror.c | 2 -- > job.c | 4 +++- > 2 files changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
06.08.2021 12:38, Max Reitz wrote: > Clearing .cancelled before leaving the main loop when the job has been > soft-cancelled is no longer necessary since job_is_cancelled() only > returns true for jobs that have been force-cancelled. > > Therefore, this only makes a differences in places that call > job_cancel_requested(). In block/mirror.c, this is done only before > .cancelled was cleared. > > In job.c, there are two callers: > - job_completed_txn_abort() asserts that .cancelled is true, so keeping > it true will not affect this place. > > - job_complete() refuses to let a job complete that has .cancelled set. > It is correct to refuse to let the user invoke job-complete on mirror > jobs that have already been soft-cancelled. > > With this change, there are no places that reset .cancelled to false and > so we can be sure that .force_cancel can only be true of .cancelled is > true as well. Assert this in job_is_cancelled(). > > Signed-off-by: Max Reitz<mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/block/mirror.c b/block/mirror.c index af89c1716a..f94aa52fae 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -939,7 +939,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) while (!job_cancel_requested(&s->common.job) && !s->should_complete) { job_yield(&s->common.job); } - s->common.job.cancelled = false; goto immediate_exit; } @@ -1078,7 +1077,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) * completion. */ assert(QLIST_EMPTY(&bs->tracked_requests)); - s->common.job.cancelled = false; need_drain = false; break; } diff --git a/job.c b/job.c index 2bd3c946a7..2ce6865ab2 100644 --- a/job.c +++ b/job.c @@ -217,7 +217,9 @@ const char *job_type_str(const Job *job) bool job_is_cancelled(Job *job) { - return job->cancelled && job->force_cancel; + /* force_cancel may be true only if cancelled is true, too */ + assert(job->cancelled || !job->force_cancel); + return job->force_cancel; } bool job_cancel_requested(Job *job)
Clearing .cancelled before leaving the main loop when the job has been soft-cancelled is no longer necessary since job_is_cancelled() only returns true for jobs that have been force-cancelled. Therefore, this only makes a differences in places that call job_cancel_requested(). In block/mirror.c, this is done only before .cancelled was cleared. In job.c, there are two callers: - job_completed_txn_abort() asserts that .cancelled is true, so keeping it true will not affect this place. - job_complete() refuses to let a job complete that has .cancelled set. It is correct to refuse to let the user invoke job-complete on mirror jobs that have already been soft-cancelled. With this change, there are no places that reset .cancelled to false and so we can be sure that .force_cancel can only be true of .cancelled is true as well. Assert this in job_is_cancelled(). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 2 -- job.c | 4 +++- 2 files changed, 3 insertions(+), 3 deletions(-)