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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

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.

> +/**
> + * 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/

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Fam Zheng]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2743 bytes --]

On Tue, Jul 29, 2014 at 09:00:43AM +0800, Fam Zheng wrote:
> 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:

I disagree.  If it was an absolute sector number then the memset() a few
lines above would be incorrect:

  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));

Look at the code you pasted again:

> @@ -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.

The last parameter is (extent_relative_sector_num %
extent->cluster_sectors) + (extent->cluster_sectors - index_in_cluster).
Those are definitely sector counts (like nb_sectors) and not absolute
sector numbers (like sector_num).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Fam Zheng]

On Tue, 07/29 13:51, Stefan Hajnoczi wrote:
> On Tue, Jul 29, 2014 at 09:00:43AM +0800, Fam Zheng wrote:
> > 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:
> 
> I disagree.

You are right, I totally misread when replying. Will respin to fix the
assertion and also the spellings.

Thanks for reviewing and explaining my mistake :)

Fam