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

[Fam Zheng]

On Wed, 05/07 11:06, Kevin Wolf wrote:
> Am 07.05.2014 um 10:57 hat Fam Zheng geschrieben:
> > 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 <address@hidden>
> > > > > 
> > > > > ---
> > > > > V2: Fix cluster_offset check. (Kevin)
> > > > > 
> > > > > Signed-off-by: Fam Zheng <address@hidden>
> > > > > ---
> > > > >  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. :)
> 
> Right, it doesn't do anything without a backing file. This is different
> from qcow2, whose mechanism I assumed without reading the code in
> detail. :-)
> 
> I think it would make sense to rewrite get_whole_cluster() to write the
> cluster for both image with a backing file and standalone images; just
> that without a backing file it would use memset() to fill the buffer
> instead of bdrv_read().
> 
> Not sure how easy it would be, but it might be an opportunity to also
> change it to write only those parts of the cluster that aren't written
> to anyway by the cluster.
> 

I think that shouldn't be hard . I'll make the change and send another patch
later.

Thanks,
Fam