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

[Kevin Wolf]

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.

Kevin