* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.