Re: [PATCH] block/mirror: add 'write-blocking-after-ready' copy mode

[Fiona Ebner]

Am 02.02.23 um 12:09 schrieb Denis V. Lunev:
> On 2/2/23 11:19, Fiona Ebner wrote:
>> Am 31.01.23 um 19:18 schrieb Denis V. Lunev:
>>> Frankly speaking I do not like this. I'd better would not
>>> rely on the enable/disable of the whole bitmap but encode
>>> mode into MirrorOp and mark blocks dirty only for those
>>> operations for which in_active_mode was set at the
>>> operation start.
>>>
>> Do you mean "for which in_active_mode was *not* set at the operation
>> start"? Also, the dirty bitmap gets set by things like the
>> bdrv_co_pwritev() call in bdrv_mirror_top_do_write(), so how would we
>> prevent setting it? The dirty bitmap does get reset in
>> do_sync_target_write(), so maybe that already takes care of it, but I
>> didn't check in detail.
>>
> I thought about something like this
> 
> iris ~/src/qemu $ git diff
> diff --git a/block/mirror.c b/block/mirror.c
> index 634815d78d..9cf5f884ee 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -230,7 +230,9 @@ static void coroutine_fn
> mirror_write_complete(MirrorOp *op, int ret)
>      if (ret < 0) {
>          BlockErrorAction action;
> 
> -        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
> +        if (op->need_dirty) {
> +            bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
> +        }

I might be missing something, but what do we gain by making this
conditional? Doesn't this even mess up e.g. the case where the error
action is 'ignore'? AFAIU, the bitmap is reset in mirror_iteration() and
if there is a write error to the target, we need to set the bitmap here
to make sure we try again later.

What is the issue with disabling the bitmap when entering synchronous
mode? From that point on, we don't care about recording new writes to
the source in the bitmap, because they will be synced to the target
right away. We still need to set the bitmap here in the error case of
course, but that happens with the code as-is.

Best Regards,
Fiona

>          action = mirror_error_action(s, false, -ret);
>          if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
>              s->ret = ret;
> @@ -437,6 +439,7 @@ static unsigned mirror_perform(MirrorBlockJob *s,
> int64_t offset,
>          .offset         = offset,
>          .bytes          = bytes,
>          .bytes_handled  = &bytes_handled,
> +        .need_dirty     = s->job->copy_mode !=
> MIRROR_COPY_MODE_WRITE_BLOCKING;
>      };
>      qemu_co_queue_init(&op->waiting_requests);
> 
> iris ~/src/qemu $
> 
> Den
> 
>