On Wed, Nov 09, 2016 at 05:01:31PM -0500, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
> > From: "Jeff Cody" <jc...@redhat.com>
> > To: "Paolo Bonzini" <pbonz...@redhat.com>
> > Cc: qemu-devel@nongnu.org, qemu-bl...@nongnu.org, js...@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 <pbonz...@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
Very well :)
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc.git block
-Jeff
