Re: [Qemu-devel] [PATCH v2] vmdk: Optimize cluster allocation

[Fam Zheng <famz@redhat.com>]

On Wed, 05/07 10:20, Kevin Wolf wrote:
> Am 07.05.2014 um 03:45 hat Fam Zheng geschrieben:
> > On Tue, 05/06 10:32, Fam Zheng wrote:
> > > On mounted NFS filesystem, ftruncate is much much slower than doing a
> > > zero write. Changing this significantly speeds up cluster allocation.
> > > 
> > > Comparing by converting a cirros image (296M) to VMDK on an NFS mount
> > > point, over 1Gbe LAN:
> > > 
> > >     $ time qemu-img convert cirros-0.3.1.img /mnt/a.raw -O vmdk
> > > 
> > > Before:
> > >     real    0m26.464s
> > >     user    0m0.133s
> > >     sys     0m0.527s
> > > 
> > > After:
> > >     real    0m2.120s
> > >     user    0m0.080s
> > >     sys     0m0.197s
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > 
> > > ---
> > > V2: Fix cluster_offset check. (Kevin)
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  block/vmdk.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/block/vmdk.c b/block/vmdk.c
> > > index 06a1f9f..98d2d56 100644
> > > --- a/block/vmdk.c
> > > +++ b/block/vmdk.c
> > > @@ -1037,6 +1037,7 @@ static int get_cluster_offset(BlockDriverState *bs,
> > >      int min_index, i, j;
> > >      uint32_t min_count, *l2_table;
> > >      bool zeroed = false;
> > > +    int64_t ret;
> > >  
> > >      if (m_data) {
> > >          m_data->valid = 0;
> > > @@ -1110,12 +1111,20 @@ static int get_cluster_offset(BlockDriverState *bs,
> > >          }
> > >  
> > >          /* Avoid the L2 tables update for the images that have snapshots. */
> > > -        *cluster_offset = bdrv_getlength(extent->file);
> > > +        ret = bdrv_getlength(extent->file);
> > > +        if (ret < 0 ||
> > > +            ret & ((extent->cluster_sectors << BDRV_SECTOR_BITS) - 1)) {
> > > +            return VMDK_ERROR;
> > > +        }
> > > +        *cluster_offset = ret;
> > >          if (!extent->compressed) {
> > > -            bdrv_truncate(
> > > -                extent->file,
> > > -                *cluster_offset + (extent->cluster_sectors << 9)
> > > -            );
> > > +            ret = bdrv_write_zeroes(extent->file,
> > > +                                    *cluster_offset >> BDRV_SECTOR_BITS,
> > > +                                    extent->cluster_sectors,
> > > +                                    0);
> > 
> > Hi Stefan,
> > 
> > By considering a bdrv_write_zeroes as a pre-write, it in general doubles the
> > write for the whole image, so it's not a good solution.
> > 
> > A better way would be removing the bdrv_truncate and require the caller to do
> > full cluster write (with a bounce buffer if necessary).
> 
> Doesn't get_whole_cluster() already ensure that you already write a full
> cluster to the image file?

That one is actually called get_backing_cluster(), if you look at the code it
has. :)

> 
> However, it might be better to not use bdrv_getlength() each time you
> need a new cluster, but instead use a field in VmdkExtent to keep the
> next free cluster offset (which is rounded up in vmdk_open).

Yes, indeed. We should do that.

Thanks,
Fam