From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Lan, Tianyu" <tianyu.lan@intel.com>
Subject: RE: [RFC v2 PATCH] kvm tools, qcow: Add the support for
 copy-on-write clusters
Date: Sat, 19 Nov 2011 10:09:05 +0800
Message-ID: <625BA99ED14B2D499DC4E29D8138F1506860C491FF@shsmsx502.ccr.corp.intel.com>
References: <1321606038-29989-1-git-send-email-tianyu.lan@intel.com>
 <4EC62F14.6090505@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
Cc: "penberg@kernel.org" <penberg@kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"asias.hejun@gmail.com" <asias.hejun@gmail.com>,
	"levinsasha928@gmail.com" <levinsasha928@gmail.com>,
	"prasadjoshi124@gmail.com" <prasadjoshi124@gmail.com>
To: Kevin Wolf <kwolf@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga02.intel.com ([134.134.136.20]:29497 "EHLO mga02.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751749Ab1KSCKG convert rfc822-to-8bit (ORCPT
	<rfc822;kvm@vger.kernel.org>); Fri, 18 Nov 2011 21:10:06 -0500
In-Reply-To: <4EC62F14.6090505@redhat.com>
Content-Language: en-US
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Hi Kevin:
	Thanks for your review. The following means that there should be a fsync after updating 
metadata(refcunt block, l1 table and l2 table).

Thanks 
Tianyu Lan

-----Original Message-----
> +		/*write l2 table*/
> +		l2t->dirty = 1;
> +		if (qcow_l2_cache_write(q, l2t) < 0)
>  			goto free_cache;

You need to make sure that the refcount update is written first (e.g.
with fsync), otherwise you risk corruption when the host crashes in the middle.

>  
> -		if (cache_table(q, l2t) < 0) {
> -			if (ftruncate(q->fd, f_sz) < 0)
> -				goto free_cache;
> +		/* Update the l1 talble */
> +		l1t->l1_table[l1t_idx] = cpu_to_be64(l2t_new_offset
> +			| QCOW2_OFLAG_COPIED);
>  
> -			goto free_cache;
> -		}
> +		if (pwrite_in_full(q->fd, l1t->l1_table,
> +			l1t->table_size * sizeof(u64),
> +			header->l1_table_offset) < 0)
> +			goto error;

Likewise, the L1 table write must be ordered against the L2 write.

goto error is using the wrong label.

>  
> -		/* Update the in-core entry */
> -		l1t->l1_table[l1t_idx] = cpu_to_be64(l2t_offset);
> +		/*cache l2 table*/
> +		cache_table(q, l2t);

After so many explicit comments, you can probably guess what's wrong here...