Re: [Qemu-devel] [PATCH v5 2/2] vmdk: Optimize cluster allocation

[Fam Zheng <famz@redhat.com>]

On Mon, 07/28 16:11, Stefan Hajnoczi wrote:
> On Tue, May 27, 2014 at 04:49:22PM +0800, Fam Zheng wrote:
> > +    if (!bs->backing_hd) {
> > +        memset(whole_grain, 0,  skip_start_sector << BDRV_SECTOR_BITS);
> > +        memset(whole_grain + (skip_end_sector << BDRV_SECTOR_BITS), 0,
> > +               cluster_bytes - (skip_end_sector << BDRV_SECTOR_BITS));
> > +    }
> > +
> > +    assert(skip_end_sector <= sector_num + extent->cluster_sectors);
> 
> Does this assertion make sense?  skip_end_sector is a small number of
> sectors (relative to start of cluster), while sector_num +
> extent->cluster_sectors is a large absolute sector offset.

skip_end_sector is absolute sector number too. The caller hunk in this patch
is:

@@ -1406,12 +1468,17 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
         if (!extent) {
             return -EIO;
         }
-        ret = get_cluster_offset(
-                                bs,
-                                extent,
-                                &m_data,
-                                sector_num << 9, !extent->compressed,
-                                &cluster_offset);
+        extent_begin_sector = extent->end_sector - extent->sectors;
+        extent_relative_sector_num = sector_num - extent_begin_sector;
+        index_in_cluster = extent_relative_sector_num % extent->cluster_sectors;
+        n = extent->cluster_sectors - index_in_cluster;
+        if (n > nb_sectors) {
+            n = nb_sectors;
+        }
+        ret = get_cluster_offset(bs, extent, &m_data, sector_num << 9,
+                                 !(extent->compressed || zeroed),
+                                 &cluster_offset,
+                                 index_in_cluster, index_in_cluster + n);
         if (extent->compressed) {
             if (ret == VMDK_OK) {
                 /* Refuse write to allocated cluster for streamOptimized */

See the last parameter of get_cluster_offset.

> 
> > +/**
> > + * get_cluster_offset
> > + *
> > + * Look up cluster offset in extent file by sector number, and store in
> > + * @cluster_offset.
> > + *
> > + * For flat extent, the start offset as parsed from the description file is
> 
> s/extent/extents/
> 
> > + * returned.
> > + *
> > + * For sparse extent, look up in L1, L2 table. If allocate is true, return an
> 
> s/extent/extents/
> 
> > + * offset for a new cluster and update L2 cache. If there is a backing file,
> > + * COW is done before returning; otherwise, zeroes are written to the allocated
> > + * cluster. Both COW and zero writting skips the sector range
> 
> s/writting/writing/

Thanks,
Fam