On Mon, Apr 22, 2013 at 10:07:59AM +0800, Fam Zheng wrote:
> Use special offset to write zeroes efficiently, when zeroed-grain GTE is
> available. If zero-write an allocated cluster, cluster is leaked because
> its offset pointer is overwritten by "0x1".
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> block/vmdk.c | 82
> ++++++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 632689b..7475090 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -814,6 +814,7 @@ static int get_whole_cluster(BlockDriverState *bs,
> static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
> {
> /* update L2 table */
> + m_data->l2_offset = extent->l1_table[m_data->l1_index];
This is weird. vmdk_L2update() should not modify m_data. Please avoid
side-effects like this.
Is the VmdkMetaData.l2_offset field even needed? Perhaps you can drop
it from the struct and use a local in vmdk_L2update().
> if (bdrv_pwrite_sync(
> extent->file,
> ((int64_t)m_data->l2_offset * 512)
> @@ -905,6 +906,12 @@ 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 = cpu_to_le32(*cluster_offset);
It's risky to mix host endian and le32 fields in the same struct - the
chance is high that someone will make a mistake which field needs to be
byteswapped.
Instead, put the cpu_to_le32() inside vmdk_L2update() and use a local
variable there. Please do this in a separate patch so it's easy to
review in isolation.
> + }
> if (extent->has_zero_grain && *cluster_offset == VMDK_GTE_ZEROED) {
> zeroed = true;
> }
> @@ -939,10 +946,6 @@ static int get_cluster_offset(BlockDriverState *bs,
>
> if (m_data) {
> m_data->offset = tmp;
> - m_data->l1_index = l1_index;
> - m_data->l2_index = l2_index;
> - m_data->l2_offset = l2_offset;
> - m_data->valid = 1;
> }
> }
> *cluster_offset <<= 9;
> @@ -1165,8 +1168,16 @@ static coroutine_fn int vmdk_co_read(BlockDriverState
> *bs, int64_t sector_num,
> return ret;
> }
>
> +/**
> + * params:
> + * - zeroed: buf is ignored (data is zero), use zeroed_grain GTE
> + * feature if possible, otherwise return -ENOTSUP.
> + * - zero_dry_run: used for zeroed == true only, don't update L2 table, just
> + * try if it's supported
> + */
Thanks for documenting the function! Please use gtkdoc style, for
example:
/**
* object_get_class:
* @obj: A derivative of #Object
*
* Returns: The #ObjectClass of the type associated with @obj.
*/
ObjectClass *object_get_class(Object *obj);
(from include/qom/object.h)
https://developer.gnome.org/gtk-doc-manual/unstable/documenting_syntax.html.en
Previously there was no standard but it helps if we all use the same doc
comment style.
> @@ -1290,7 +1333,7 @@ static int vmdk_create_extent(const char *filename,
> int64_t filesize,
> header.version = zeroed_grain ? 2 : 1;
> header.flags = VMDK4_FLAG_RGD | VMDK4_FLAG_NL_DETECT
> | (compress ? VMDK4_FLAG_COMPRESS | VMDK4_FLAG_MARKER : 0)
> - | (zeroed_grain ? VMDK4_FLAG_ZG : 0);
> + | (zeroed_grain ? VMDK4_FLAG_ZERO_GRAIN : 0);
Please fix the patch series so the old VMDK4_FLAG_ZG is never
introduced. The best way of ensuring this is to buildtest the series at
each patch:
git rebase -i master -x make
This will run make on every commit from master..HEAD. If the build
fails, it stops and lets you git commit --amend.
Stefan
