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

[Denis V. Lunev]

On 1/31/23 18:44, Vladimir Sementsov-Ogievskiy wrote:
> + Den
>
> Den, I remember we thought about that, and probably had a solution?
>
> Another possible approach to get benefits from both modes is to switch 
> to blocking mode after first loop of copying. [*]
>
> Hmm. Thinking about proposed solution it seems, that [*] is better. 
> The main reason of "write-blocking" mode is to force convergence of 
> the mirror job. But you lose this thing if you postpone switching to 
> the moment when mirror becomes READY: we may never reach it.
>
>
>
> Or, may be we can selectively skip or block guest writes, to keep some 
> specified level of convergence?
>
> Or, we can start in "background" mode, track the speed of convergence 
> (like dirty-delta per minute), and switch to blocking if speed becomes 
> less than some threshold.

That is absolutely correct. It would be very kind to start with
asynchronous mirror and switch to the synchronous one after
specific amount of loops. This should be somehow measured.

Originally we have thought about switching after the one loop
(when basic background sending is done), but may be 2 iterations
(initial + first one) would be better.

Talking about this specific approach and our past experience
I should note that reaching completion point in the
asynchronous mode was a real pain and slow down for the guest.
We have intentionally switched at that time to the sync mode
for that purpose. Thus this patch is a "worst half-way" for
us.

Frankly speaking I would say that this switch could be considered
NOT QEMU job and we should just send a notification (event) for the
completion of the each iteration and management software should
take a decision to switch from async mode to the sync one.

Under such a condition we should have a command to perform mode
switch. This seems more QEMU/QAPI way :)

> On 07.12.22 16:27, Fiona Ebner wrote:
> > The new copy mode starts out in 'background' mode and switches to
> > 'write-blocking' mode once the job transitions to ready.
> >
> > Before switching to active mode and indicating that the drives are
> > actively synced, it is necessary to have seen and handled all guest
> > I/O. This is done by checking the dirty bitmap inside a drained
> > section. Transitioning to ready is also only done at the same time.
> >
> > The new mode is useful for management applications using drive-mirror
> > in combination with migration. Currently, migration doesn't check on
> > mirror jobs before inactivating the blockdrives, so it's necessary to
> > either:
> > 1) use the 'pause-before-switchover' migration capability and complete
> >     mirror jobs before actually switching over.
> > 2) use 'write-blocking' copy mode for the drive mirrors.
> >
> > The downside with 1) is longer downtime for the guest, while the
> > downside with 2) is that guest write speed is limited by the
> > synchronous writes to the mirror target. The newly introduced copy
> > mode reduces the time that limit is in effect.
>
> Do you mean that in (2) we don't use pause-before-switchover? So, we 
> have to wait for mirror-ready before starting the migration? What's 
> the benefit of it? Or what I miss?
you will have zero dirty data from block level and thus you need
to send only dirty memory (without disk). That is pretty much
correct.


> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> > ---
> >
> > See [0] for a bit more context. While the new copy mode doesn't
> > fundamentally improve the downside of 2) (especially when multiple
> > drives are mirrored), it would still improve things a little. And I
> > guess when trying to keep downtime short, guest write speed needs to
> > be limited at /some/ point (always in the context of migration with
> > drive-mirror of course). Ideally, that could go hand-in-hand with
> > migration convergence, but that would require some larger changes to
> > implement and introduce more coupling.
> >
> > [0] 
> > https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg04886.html
> >
> >   block/mirror.c       | 29 +++++++++++++++++++++++++++--
> >   qapi/block-core.json |  5 ++++-
> >   2 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 251adc5ae0..e21b4e5e77 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -60,6 +60,7 @@ typedef struct MirrorBlockJob {
> >       /* Set when the target is synced (dirty bitmap is clean, nothing
> >        * in flight) and the job is running in active mode */
> >       bool actively_synced;
> > +    bool in_active_mode;
> >       bool should_complete;
> >       int64_t granularity;
> >       size_t buf_size;
> > @@ -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?

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.

That seems much more natural.

> 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).
>
> > +
> >                   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
>
> > +#
> >   # Since: 3.0
> >   ##
> >   { 'enum': 'MirrorCopyMode',
> > -  'data': ['background', 'write-blocking'] }
> > +  'data': ['background', 'write-blocking', 
> > 'write-blocking-after-ready'] }
> >     ##
> >   # @BlockJobInfo: