migration/block: Avoid invoking blk_drain too frequently

[Qemu-devel] [PATCH v2] migration/block: Avoid invoking blk_drain too frequently

Posted by Lidong Chen 7 years, 1 month ago

Increase bmds->cur_dirty after submit io, so reduce the frequency
involve into blk_drain, and improve the performance obviously
when block migration.

The performance test result of this patch:

During the block dirty save phase, this patch improve guest os IOPS
from 4.0K to 9.5K. and improve the migration speed from
505856 rsec/s to 855756 rsec/s.

Signed-off-by: Lidong Chen <jemmy858585@gmail.com>
---
 migration/block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/block.c b/migration/block.c
index 6741228..7734ff7 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -576,6 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
             }
 
             bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
+            sector += nr_sectors;
+            bmds->cur_dirty = sector;
+
             break;
         }
         sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH v2] migration/block: Avoid invoking blk_drain too frequently

Posted by Kevin Wolf 7 years, 1 month ago

Am 15.03.2017 um 04:37 hat Lidong Chen geschrieben:
> Increase bmds->cur_dirty after submit io, so reduce the frequency
> involve into blk_drain, and improve the performance obviously
> when block migration.
> 
> The performance test result of this patch:
> 
> During the block dirty save phase, this patch improve guest os IOPS
> from 4.0K to 9.5K. and improve the migration speed from
> 505856 rsec/s to 855756 rsec/s.
> 
> Signed-off-by: Lidong Chen <jemmy858585@gmail.com>
> ---
>  migration/block.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 6741228..7734ff7 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -576,6 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>              }
>  
>              bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
> +            sector += nr_sectors;

sector += BDRV_SECTORS_PER_DIRTY_CHUNK would be acceptable here...

> +            bmds->cur_dirty = sector;
> +
>              break;
>          }
>          sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
>          bmds->cur_dirty = sector;
>      }

...which would make it the same code as the rest of the loop. It could
be nicer to reuse the same code and just set a bool when a request is
submitted so that the loop terminates after that.

But that's just a matter of taste, so I'll still give:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v2] migration/block: Avoid invoking blk_drain too frequently

Posted by Fam Zheng 7 years, 1 month ago

On Wed, 03/15 11:37, Lidong Chen wrote:
> Increase bmds->cur_dirty after submit io, so reduce the frequency
> involve into blk_drain, and improve the performance obviously
> when block migration.
> 
> The performance test result of this patch:
> 
> During the block dirty save phase, this patch improve guest os IOPS
> from 4.0K to 9.5K. and improve the migration speed from
> 505856 rsec/s to 855756 rsec/s.
> 
> Signed-off-by: Lidong Chen <jemmy858585@gmail.com>
> ---
>  migration/block.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 6741228..7734ff7 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -576,6 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>              }
>  
>              bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
> +            sector += nr_sectors;
> +            bmds->cur_dirty = sector;
> +
>              break;
>          }
>          sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> -- 
> 1.8.3.1
> 

Nice catch above all, thank you!

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2] migration/block: Avoid invoking blk_drain too frequently

Posted by Dr. David Alan Gilbert 7 years, 1 month ago

* Fam Zheng (famz@redhat.com) wrote:
> On Wed, 03/15 11:37, Lidong Chen wrote:
> > Increase bmds->cur_dirty after submit io, so reduce the frequency
> > involve into blk_drain, and improve the performance obviously
> > when block migration.
> > 
> > The performance test result of this patch:
> > 
> > During the block dirty save phase, this patch improve guest os IOPS
> > from 4.0K to 9.5K. and improve the migration speed from
> > 505856 rsec/s to 855756 rsec/s.
> > 
> > Signed-off-by: Lidong Chen <jemmy858585@gmail.com>
> > ---
> >  migration/block.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/migration/block.c b/migration/block.c
> > index 6741228..7734ff7 100644
> > --- a/migration/block.c
> > +++ b/migration/block.c
> > @@ -576,6 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> >              }
> >  
> >              bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
> > +            sector += nr_sectors;
> > +            bmds->cur_dirty = sector;
> > +
> >              break;
> >          }
> >          sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> > -- 
> > 1.8.3.1
> > 
> 
> Nice catch above all, thank you!
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Are you taking that via a block pull?

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] migration/block: Avoid invoking blk_drain too frequently

Posted by Fam Zheng 7 years, 1 month ago

On Wed, 03/15 17:31, Dr. David Alan Gilbert wrote:
> * Fam Zheng (famz@redhat.com) wrote:
> > On Wed, 03/15 11:37, Lidong Chen wrote:
> > > Increase bmds->cur_dirty after submit io, so reduce the frequency
> > > involve into blk_drain, and improve the performance obviously
> > > when block migration.
> > > 
> > > The performance test result of this patch:
> > > 
> > > During the block dirty save phase, this patch improve guest os IOPS
> > > from 4.0K to 9.5K. and improve the migration speed from
> > > 505856 rsec/s to 855756 rsec/s.
> > > 
> > > Signed-off-by: Lidong Chen <jemmy858585@gmail.com>
> > > ---
> > >  migration/block.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/migration/block.c b/migration/block.c
> > > index 6741228..7734ff7 100644
> > > --- a/migration/block.c
> > > +++ b/migration/block.c
> > > @@ -576,6 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> > >              }
> > >  
> > >              bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
> > > +            sector += nr_sectors;
> > > +            bmds->cur_dirty = sector;
> > > +
> > >              break;
> > >          }
> > >          sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > Nice catch above all, thank you!
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> Are you taking that via a block pull?

I can do that, but I'm not sure whether it should go to 2.9. This is a
performance improvement, which usually doesn't qualify as bug fixes. But this
also looks like a mistake in original code.

Fam