Re: [Qemu-devel] [PATCH v3 6/6] vmdk: add bdrv_co_write_zeroes

[Stefan Hajnoczi]

On Wed, Apr 24, 2013 at 08:44:35PM +0800, Fam Zheng wrote:
> @@ -905,6 +905,13 @@ static int get_cluster_offset(BlockDriverState *bs,
>      l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
>      *cluster_offset = le32_to_cpu(l2_table[l2_index]);
>  
> +    if (m_data) {
> +        m_data->valid = 1;
> +        m_data->l1_index = l1_index;
> +        m_data->l2_index = l2_index;
> +        m_data->offset = *cluster_offset;
> +        m_data->l2_offset = extent->l1_table[m_data->l1_index];

This line can simply be:

m_data->l2_offset = l2_offset;

> +    }

Filling in m_data up here means that only the ->offset field needs to be
filled in when we allocate a cluster further down.  Right now the code
is duplicated, but it just overwrites the fields with the same value
again.

> @@ -1222,17 +1238,34 @@ static int vmdk_write(BlockDriverState *bs, int64_t 
> sector_num,
>          if (n > nb_sectors) {
>              n = nb_sectors;
>          }
> -
> -        ret = vmdk_write_extent(extent,
> -                        cluster_offset, index_in_cluster * 512,
> -                        buf, n, sector_num);
> -        if (ret) {
> -            return ret;
> -        }
> -        if (m_data.valid) {
> -            /* update L2 tables */
> -            if (vmdk_L2update(extent, &m_data) == -1) {
> -                return -EIO;
> +        if (zeroed) {
> +            /* Do zeroed write, buf is ignored */
> +            if (extent->has_zero_grain &&
> +                    index_in_cluster == 0 &&
> +                    n >= extent->cluster_sectors) {
> +                n = extent->cluster_sectors;
> +                if (!zero_dry_run) {
> +                    m_data.offset = cpu_to_le32(VMDK_GTE_ZEROED);

offset is host endian now!

> +                    /* update L2 tables */
> +                    if (vmdk_L2update(extent, &m_data) != VMDK_OK) {

Zeroing cluster-by-cluster is slow - vmdk_L2update() uses sync to flush
the L2 update.  The vmdk.c code isn't great for buffering up metadata
changes and flushing them in a single operation though, so this is fine
for now.

> +                        return -EIO;
> +                    }

l2_cache[] has not been updated with the new VMDK_GTE_ZEROED offset.