From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60550) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVM66-0004zY-GG for qemu-devel@nongnu.org; Thu, 25 Apr 2013 09:20:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVM61-0004ev-Lm for qemu-devel@nongnu.org; Thu, 25 Apr 2013 09:20:26 -0400 Received: from mail-wg0-x230.google.com ([2a00:1450:400c:c00::230]:44828) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVM61-0004en-Ft for qemu-devel@nongnu.org; Thu, 25 Apr 2013 09:20:21 -0400 Received: by mail-wg0-f48.google.com with SMTP id f11so1449668wgh.27 for ; Thu, 25 Apr 2013 06:20:20 -0700 (PDT) Date: Thu, 25 Apr 2013 15:20:11 +0200 From: Stefan Hajnoczi Message-ID: <20130425132011.GA10799@stefanha-thinkpad.redhat.com> References: <1366807475-26350-1-git-send-email-famz@redhat.com> <1366807475-26350-7-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1366807475-26350-7-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 6/6] vmdk: add bdrv_co_write_zeroes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com 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.