From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37803) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5vlN-0001E9-Ul for qemu-devel@nongnu.org; Thu, 26 May 2016 09:55:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5vlM-0006FR-MR for qemu-devel@nongnu.org; Thu, 26 May 2016 09:55:49 -0400 References: <1464234529-13018-1-git-send-email-eblake@redhat.com> <1464234529-13018-6-git-send-email-eblake@redhat.com> From: "Denis V. Lunev" Message-ID: <5746FCFC.2060708@openvz.org> Date: Thu, 26 May 2016 16:41:16 +0300 MIME-Version: 1.0 In-Reply-To: <1464234529-13018-6-git-send-email-eblake@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Catch more unaligned write_zero into zero cluster List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, kwolf@redhat.com, Max Reitz On 05/26/2016 06:48 AM, Eric Blake wrote: > is_zero_cluster() and is_zero_cluster_top_locked() are used only > by qcow2_co_write_zeroes(). The former is too broad (we don't > care if the sectors we are about to overwrite are non-zero, only > that all other sectors in the cluster are zero), so it needs to > be called up to twice but with smaller limits - rename it along > with adding the neeeded parameter. The latter can be inlined for > more compact code. > > The testsuite change shows that we now have a sparser top file > when an unaligned write_zeroes overwrites the only portion of > the backing file with data. > > Based on a patch proposal by Denis V. Lunev. > > CC: Denis V. Lunev > Signed-off-by: Eric Blake > --- > block/qcow2.c | 47 +++++++++++++++++++++++----------------------- > tests/qemu-iotests/154.out | 8 ++++---- > 2 files changed, 27 insertions(+), 28 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 105fd5e..ecac399 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2406,26 +2406,19 @@ finish: > } > > > -static bool is_zero_cluster(BlockDriverState *bs, int64_t start) > +static bool is_zero_sectors(BlockDriverState *bs, int64_t start, > + uint32_t count) > { > - BDRVQcow2State *s = bs->opaque; > int nr; > BlockDriverState *file; > - int64_t res = bdrv_get_block_status_above(bs, NULL, start, > - s->cluster_sectors, &nr, &file); > - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == s->cluster_sectors; > -} > + int64_t res; > > -static bool is_zero_cluster_top_locked(BlockDriverState *bs, int64_t start) > -{ > - BDRVQcow2State *s = bs->opaque; > - int nr = s->cluster_sectors; > - uint64_t off; > - int ret; > - > - ret = qcow2_get_cluster_offset(bs, start << BDRV_SECTOR_BITS, &nr, &off); > - assert(nr == s->cluster_sectors); > - return ret == QCOW2_CLUSTER_UNALLOCATED || ret == QCOW2_CLUSTER_ZERO; > + if (!count) { > + return true; > + } > + res = bdrv_get_block_status_above(bs, NULL, start, count, > + &nr, &file); > + return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; > } > > static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs, > @@ -2434,27 +2427,33 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs, > int ret; > BDRVQcow2State *s = bs->opaque; > > - int head = sector_num % s->cluster_sectors; > - int tail = (sector_num + nb_sectors) % s->cluster_sectors; > + uint32_t head = sector_num % s->cluster_sectors; > + uint32_t tail = (sector_num + nb_sectors) % s->cluster_sectors; > > trace_qcow2_write_zeroes_start_req(qemu_coroutine_self(), sector_num, > nb_sectors); > > - if (head != 0 || tail != 0) { > + if (head || tail) { > int64_t cl_start = sector_num - head; > + uint64_t off; > + int nr; > > assert(cl_start + s->cluster_sectors >= sector_num + nb_sectors); > > - sector_num = cl_start; > - nb_sectors = s->cluster_sectors; > - > - if (!is_zero_cluster(bs, sector_num)) { > + /* check whether remainder of cluster already reads as zero */ > + if (!(is_zero_sectors(bs, cl_start, head) && > + is_zero_sectors(bs, sector_num + nb_sectors, > + -tail & (s->cluster_sectors - 1)))) { can we have cluster_sectors != 2^n? In this case this bits logic will be broken.