From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bL554-0001Xw-J2 for qemu-devel@nongnu.org; Thu, 07 Jul 2016 04:54:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bL553-0005Vk-Nb for qemu-devel@nongnu.org; Thu, 07 Jul 2016 04:54:46 -0400 Date: Thu, 7 Jul 2016 16:54:38 +0800 From: Fam Zheng Message-ID: <20160707085438.GB12042@ad.usersys.redhat.com> References: <20160707084249.29084-1-fullmanet@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160707084249.29084-1-fullmanet@gmail.com> Subject: Re: [Qemu-devel] [PATCH v2] vmdk: fix metadata write regression List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Reda Sallahi Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Kevin Wolf , Max Reitz On Thu, 07/07 10:42, Reda Sallahi wrote: > Commit "cdeaf1f vmdk: add bdrv_co_write_zeroes" causes a regression on > writes. It writes metadata after every write instead of doing it only once > for each cluster. > > vmdk_pwritev() writes metadata whenever m_data is set as valid so this patch > sets m_data as valid only when we have a new cluster which hasn't been > allocated before or a zero grain. > > Signed-off-by: Reda Sallahi > --- > v2: Corrected the commit id referenced in the commit message. > > block/vmdk.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/block/vmdk.c b/block/vmdk.c > index d73f431..1cbd487 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -1202,13 +1202,6 @@ static int get_cluster_offset(BlockDriverState *bs, > l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size; > cluster_sector = 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->l2_offset = l2_offset; > - m_data->l2_cache_entry = &l2_table[l2_index]; > - } > if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) { > zeroed = true; > } > @@ -1231,6 +1224,13 @@ static int get_cluster_offset(BlockDriverState *bs, > if (ret) { > return ret; > } > + if (m_data) { > + m_data->valid = 1; > + m_data->l1_index = l1_index; > + m_data->l2_index = l2_index; > + m_data->l2_offset = l2_offset; > + m_data->l2_cache_entry = &l2_table[l2_index]; > + } > } > *cluster_offset = cluster_sector << BDRV_SECTOR_BITS; > return VMDK_OK; > -- > 2.9.0 > Looks good, nice catch! Reviewed-by: Fam Zheng