Re: [PATCH] kvm tools, qcow: Add the support for copy-on-write clusters

[Kevin Wolf <kwolf@redhat.com>]

Am 21.11.2011 08:12, schrieb Lan Tianyu:
> When meeting request to write the cluster without copied flag,
> allocate a new cluster and write original data with modification
> to the new cluster. This also adds support for the writing operation
> of the qcow2 compressed image. After testing, image file can pass
> through "qemu-img check". The performance is needed to be improved.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  tools/kvm/disk/qcow.c        |  411 +++++++++++++++++++++++++++++-------------
>  tools/kvm/include/kvm/qcow.h |    2 +
>  2 files changed, 285 insertions(+), 128 deletions(-)

> @@ -766,122 +872,166 @@ static ssize_t qcow_write_cluster(struct qcow *q, u64 offset, void *buf, u32 src
>  	if (l2t_idx >= l2t_size)
>  		return -1;
>  
> -	clust_off = get_cluster_offset(q, offset);
> -	if (clust_off >= clust_sz)
> -		return -1;
> -
> -	len = clust_sz - clust_off;
> -	if (len > src_len)
> -		len = src_len;
> -
> -	mutex_lock(&q->mutex);
> -
>  	l2t_offset = be64_to_cpu(l1t->l1_table[l1t_idx]);
> -	if (l2t_offset & QCOW2_OFLAG_COMPRESSED) {
> -		pr_warning("compressed clusters are not supported");
> -		goto error;
> -	}
> -	if (!(l2t_offset & QCOW2_OFLAG_COPIED)) {
> -		pr_warning("L2 copy-on-write clusters are not supported");
> -		goto error;
> -	}
> -
> -	l2t_offset &= QCOW2_OFFSET_MASK;
> -	if (l2t_offset) {
> -		/* read and cache l2 table */
> +	if (l2t_offset & QCOW2_OFLAG_COPIED) {
> +		l2t_offset &= ~QCOW2_OFLAG_COPIED;
>  		l2t = qcow_read_l2_table(q, l2t_offset);
>  		if (!l2t)
>  			goto error;
>  	} else {
> -		l2t = new_cache_table(q, l2t_offset);
> -		if (!l2t)
> +		l2t_new_offset = qcow_alloc_clusters(q, l2t_size*sizeof(u64));
> +		if (l2t_new_offset < 0)
>  			goto error;
>  
> -		/* Capture the state of the consistent QCOW image */
> -		f_sz = file_size(q->fd);
> -		if (!f_sz)
> -			goto free_cache;
> +		l2t = new_cache_table(q, l2t_new_offset);
> +		if (!l2t)
> +			goto free_cluster;
>  
> -		/* Write the l2 table of 0's at the end of the file */
> -		l2t_offset = qcow_write_l2_table(q, l2t->table);
> -		if (!l2t_offset)
> +		if (l2t_offset) {
> +			l2t = qcow_read_l2_table(q, l2t_offset);
> +			if (!l2t)
> +				goto free_cache;
> +		} else
> +			memset(l2t->table, 0x00, l2t_size * sizeof(u64));
> +
> +		/*write l2 table*/
> +		l2t->dirty = 1;
> +		if (qcow_l2_cache_write(q, l2t) < 0)
>  			goto free_cache;
>  
> -		if (cache_table(q, l2t) < 0) {
> -			if (ftruncate(q->fd, f_sz) < 0)
> -				goto free_cache;
> +		/*cache l2 table*/
> +		cache_table(q, l2t);

You're ignoring the return value here.

Otherwise I didn't find any obvious problem in a quick scan.

Kevin