Am 31.01.23 um 18:44 schrieb Vladimir Sementsov-Ogievskiy:
>> @@ -1035,10 +1036,31 @@ static int coroutine_fn mirror_run(Job *job,
>> Error **errp)
>> if (s->in_flight == 0 && cnt == 0) {
>> trace_mirror_before_flush(s);
>> if (!job_is_ready(&s->common.job)) {
>> + if (s->copy_mode ==
>> + MIRROR_COPY_MODE_WRITE_BLOCKING_AFTER_READY) {
>> + /*
>> + * Pause guest I/O to check if we can switch to
>> active mode.
>> + * To set actively_synced to true below, it is
>> necessary to
>> + * have seen and synced all guest I/O.
>> + */
>> + s->in_drain = true;
>> + bdrv_drained_begin(bs);
>> + if (bdrv_get_dirty_count(s->dirty_bitmap) > 0) {
>> + bdrv_drained_end(bs);
>> + s->in_drain = false;
>> + continue;
>> + }
>> + s->in_active_mode = true;
>> + bdrv_disable_dirty_bitmap(s->dirty_bitmap);
>> + bdrv_drained_end(bs);
>> + s->in_drain = false;
>> + }
>
> I doubt, do we really need the drained section?
>
> Why can't we simply set s->in_active_mode here and don't care?
>
> I think bdrv_get_dirty_count(s->dirty_bitmap) can't become > 0 here, as
> cnt is zero, we are in context of bs and we don't have yield point. (ok,
> we have one in drained_begin(), but what I think we can simply drop
> drained section here).
>
Thank you for the explanation! I wasn't sure if it's really needed and
wanted to take it safe (should've mentioned that in the original mail).
>> +
>> if (mirror_flush(s) < 0) {
>> /* Go check s->ret. */
>> continue;
>> }
>> +
>> /* We're out of the streaming phase. From now on,
>> if the job
>> * is cancelled we will actually complete all
>> pending I/O and
>> * report completion. This way, block-job-cancel
>> will leave
>> @@ -1443,7 +1465,7 @@ static int coroutine_fn
>> bdrv_mirror_top_do_write(BlockDriverState *bs,
>> if (s->job) {
>> copy_to_target = s->job->ret >= 0 &&
>> !job_is_cancelled(&s->job->common.job) &&
>> - s->job->copy_mode ==
>> MIRROR_COPY_MODE_WRITE_BLOCKING;
>> + s->job->in_active_mode;
>> }
>> if (copy_to_target) {
>> @@ -1494,7 +1516,7 @@ static int coroutine_fn
>> bdrv_mirror_top_pwritev(BlockDriverState *bs,
>> if (s->job) {
>> copy_to_target = s->job->ret >= 0 &&
>> !job_is_cancelled(&s->job->common.job) &&
>> - s->job->copy_mode ==
>> MIRROR_COPY_MODE_WRITE_BLOCKING;
>> + s->job->in_active_mode;
>> }
>> if (copy_to_target) {
>> @@ -1792,7 +1814,10 @@ static BlockJob *mirror_start_job(
>> goto fail;
>> }
>> if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
>> + s->in_active_mode = true;
>> bdrv_disable_dirty_bitmap(s->dirty_bitmap);
>> + } else {
>> + s->in_active_mode = false;
>> }
>> ret = block_job_add_bdrv(&s->common, "source", bs, 0,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 95ac4fa634..2a983ed78d 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1200,10 +1200,13 @@
>> # addition, data is copied in background just like in
>> # @background mode.
>> #
>> +# @write-blocking-after-ready: starts out in @background mode and
>> switches to
>> +# @write-blocking when transitioning to
>> ready.
>
> You should add also (Since: 8.0) label
>
I'll try to remember it next time.
>> +#
>> # Since: 3.0
>> ##
>> { 'enum': 'MirrorCopyMode',
>> - 'data': ['background', 'write-blocking'] }
>> + 'data': ['background', 'write-blocking',
>> 'write-blocking-after-ready'] }
>> ##
>> # @BlockJobInfo:
>
