* [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced @ 2016-11-09 16:20 Paolo Bonzini 2016-11-09 18:38 ` Jeff Cody 0 siblings, 1 reply; 4+ messages in thread From: Paolo Bonzini @ 2016-11-09 16:20 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-block, jsnow, jcody 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; } - } else { /* 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 * the target in a consistent state. */ - if (!s->synced) { - block_job_event_ready(&s->common); - s->synced = true; - } - - should_complete = s->should_complete || - block_job_is_cancelled(&s->common); - cnt = bdrv_get_dirty_count(s->dirty_bitmap); + block_job_event_ready(&s->common); + s->synced = true; } + + should_complete = s->should_complete || + block_job_is_cancelled(&s->common); + cnt = bdrv_get_dirty_count(s->dirty_bitmap); } if (cnt == 0 && should_complete) { @@ -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; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced 2016-11-09 16:20 [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced Paolo Bonzini @ 2016-11-09 18:38 ` Jeff Cody 2016-11-09 22:01 ` Paolo Bonzini 0 siblings, 1 reply; 4+ messages in thread From: Jeff Cody @ 2016-11-09 18:38 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, qemu-block, jsnow 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.: + ret = mirror_flush(s); + if (ret < 0 && s->ret < 0) { + goto immediate_exit; + } > } > - } else { > /* 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 > * the target in a consistent state. > */ > - if (!s->synced) { > - block_job_event_ready(&s->common); > - s->synced = true; > - } > - > - should_complete = s->should_complete || > - block_job_is_cancelled(&s->common); > - cnt = bdrv_get_dirty_count(s->dirty_bitmap); > + block_job_event_ready(&s->common); > + s->synced = true; > } > + > + should_complete = s->should_complete || > + block_job_is_cancelled(&s->common); > + cnt = bdrv_get_dirty_count(s->dirty_bitmap); > } > > if (cnt == 0 && should_complete) { > @@ -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? -Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced 2016-11-09 18:38 ` Jeff Cody @ 2016-11-09 22:01 ` Paolo Bonzini 2016-11-15 3:49 ` Jeff Cody 0 siblings, 1 reply; 4+ messages in thread From: Paolo Bonzini @ 2016-11-09 22:01 UTC (permalink / raw) To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow ----- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced 2016-11-09 22:01 ` Paolo Bonzini @ 2016-11-15 3:49 ` Jeff Cody 0 siblings, 0 replies; 4+ messages in thread From: Jeff Cody @ 2016-11-15 3:49 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, qemu-block, jsnow On Wed, Nov 09, 2016 at 05:01:31PM -0500, Paolo Bonzini wrote: > > > ----- 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 Very well :) Applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc.git block -Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-15 3:49 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-09 16:20 [Qemu-devel] [PATCH for-2.8] mirror: do not flush every time the disks are synced Paolo Bonzini 2016-11-09 18:38 ` Jeff Cody 2016-11-09 22:01 ` Paolo Bonzini 2016-11-15 3:49 ` Jeff Cody
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).