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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]