From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
John Snow <jsnow@redhat.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH for-6.1? v2 6/7] mirror: Check job_is_cancelled() earlier
Date: Wed, 4 Aug 2021 10:25:08 +0200 [thread overview]
Message-ID: <cbf2f3c8-6e7a-b277-f90a-483699828b6f@redhat.com> (raw)
In-Reply-To: <YQlT4rO9OoykGl/b@redhat.com>
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. If cancel is requested in
job_sleep_ns(), then we will go back to the top of the loop, wait for
in-flight active requests and then break. Waiting for the in-flight
requests seems unnecessary, but does it really make a difference in
practice? We don’t start new requests, so it should be legal to wait
for existing ones to settle, and also I believe someone will have to
wait for those in-flight requests anyway (when the mirror top node is
removed). (The only thing we could do is to cancel the in-flight
requests, but that is what mirror_cancel() does.)
Looking more at the whole loop, there are a couple of places that can
yield. Of course we can check whether the job has been cancelled after
every single one of them, but that would be a bit strange. We only
really need to check before we initiate new requests or want to change
the state. I believe the right place to do the check would be after the
job_pause_point().
And perhaps the active write functions (bdrv_mirror_top_do_write() and
bdrv_mirror_top_pwritev()) should stop copying to the target if the job
has been cancelled.
Max
> So have the check in both places, I guess? And a comment to explain why
> neither is redundant.
>
>> s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> }
> Kevin
>
next prev parent reply other threads:[~2021-08-04 8:27 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-26 14:46 [PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel Max Reitz
2021-07-26 14:46 ` [PATCH for-6.1? v2 1/7] mirror: Keep s->synced on error Max Reitz
2021-07-26 14:46 ` [PATCH for-6.1? v2 2/7] mirror: Drop s->synced Max Reitz
2021-07-26 15:26 ` Eric Blake
2021-07-27 11:41 ` Vladimir Sementsov-Ogievskiy
2021-07-26 14:46 ` [PATCH for-6.1? v2 3/7] job: @force parameter for job_cancel_sync{, _all}() Max Reitz
2021-08-03 14:00 ` [PATCH for-6.1? v2 3/7] job: @force parameter for job_cancel_sync{,_all}() Kevin Wolf
2021-07-26 14:46 ` [PATCH for-6.1? v2 4/7] jobs: Give Job.force_cancel more meaning Max Reitz
2021-07-26 21:38 ` Eric Blake
2021-07-27 12:40 ` Vladimir Sementsov-Ogievskiy
2021-07-26 14:46 ` [PATCH for-6.1? v2 5/7] job: Add job_cancel_requested() Max Reitz
2021-07-27 13:04 ` Vladimir Sementsov-Ogievskiy
2021-07-27 15:39 ` Max Reitz
2021-07-27 15:47 ` Vladimir Sementsov-Ogievskiy
2021-08-02 10:23 ` Max Reitz
2021-08-03 12:35 ` Vladimir Sementsov-Ogievskiy
2021-08-03 14:25 ` Kevin Wolf
2021-08-04 8:07 ` Max Reitz
2021-08-04 10:34 ` Kevin Wolf
2021-08-04 11:00 ` Peter Krempa
2021-08-04 14:15 ` Max Reitz
2021-07-26 14:46 ` [PATCH for-6.1? v2 6/7] mirror: Check job_is_cancelled() earlier Max Reitz
2021-07-27 13:13 ` Vladimir Sementsov-Ogievskiy
2021-07-27 15:40 ` Max Reitz
2021-08-03 14:34 ` Kevin Wolf
2021-08-04 8:25 ` Max Reitz [this message]
2021-08-04 9:48 ` Kevin Wolf
2021-08-04 10:12 ` Max Reitz
2021-07-26 14:46 ` [PATCH for-6.1? v2 7/7] iotests: Add mirror-ready-cancel-error test Max Reitz
2021-07-27 13:24 ` Vladimir Sementsov-Ogievskiy
2021-08-03 14:36 ` [PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel Kevin Wolf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cbf2f3c8-6e7a-b277-f90a-483699828b6f@redhat.com \
--to=mreitz@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.