Re: [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced

[Paolo Bonzini <pbonzini@redhat.com>]

----- Original Message -----
> From: "Jeff Cody" <jcody@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, jsnow@redhat.com
> Sent: Wednesday, November 9, 2016 7:38:26 PM
> Subject: Re: [PATCH for-2.8] mirror: do not flush every time the disks are synced
> 
> On Wed, Nov 09, 2016 at 05:20:08PM +0100, Paolo Bonzini wrote:
> > This puts a huge strain on the disks when there are many concurrent
> > migrations.  With this patch we only flush twice: just before issuing
> > the event, and just before pivoting to the destination.  If management
> > will complete the job close to the BLOCK_JOB_READY event, the cost of
> > the second flush should be small anyway.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block/mirror.c | 40 +++++++++++++++++++++++++---------------
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index b2c1fb8..3ec281c 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -615,6 +615,20 @@ static int coroutine_fn
> > mirror_dirty_init(MirrorBlockJob *s)
> >      return 0;
> >  }
> >  
> > +/* Called when going out of the streaming phase to flush the bulk of the
> > + * data to the medium, or just before completing.
> > + */
> > +static int mirror_flush(MirrorBlockJob *s)
> > +{
> > +    int ret = blk_flush(s->target);
> > +    if (ret < 0) {
> > +        if (mirror_error_action(s, false, -ret) ==
> > BLOCK_ERROR_ACTION_REPORT) {
> > +            s->ret = ret;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
> >  static void coroutine_fn mirror_run(void *opaque)
> >  {
> >      MirrorBlockJob *s = opaque;
> > @@ -727,27 +741,23 @@ static void coroutine_fn mirror_run(void *opaque)
> >          should_complete = false;
> >          if (s->in_flight == 0 && cnt == 0) {
> >              trace_mirror_before_flush(s);
> > -            ret = blk_flush(s->target);
> > -            if (ret < 0) {
> > -                if (mirror_error_action(s, false, -ret) ==
> > -                    BLOCK_ERROR_ACTION_REPORT) {
> > -                    goto immediate_exit;
> > +            if (!s->synced) {
> > +                if (mirror_flush(s) < 0) {
> > +                    /* Go check s->ret.  */
> > +                    continue;
> 
> I think this would be easier to follow, if rather than popping back up to
> the top of the loop to do error checking, to just do the error cleanup like
> normal, e.g.:
> 
> > @@ -765,7 +775,7 @@ static void coroutine_fn mirror_run(void *opaque)
> >  
> >              bdrv_drained_begin(bs);
> >              cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> > -            if (cnt > 0) {
> > +            if (cnt > 0 || mirror_flush(s) < 0) {
> >                  bdrv_drained_end(bs);
> >                  continue;
> 
> Bah, that continue paradigm is used here from before this patch.  Could I
> convince you to change this too?

I tried but I could not really write it in a way that looked nice, so I at
least made both calls to mirror_flush look the same. :(

The problem is that the cnt > 0 really needs to be a bdrv_drained_end+continue,
while the mirror_flush case would be a bdrv_drained_end+goto immediate_exit.

Paolo
Paolo