From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52348) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5wih-0000G0-NC for qemu-devel@nongnu.org; Thu, 26 May 2016 10:57:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5wic-0005Id-NS for qemu-devel@nongnu.org; Thu, 26 May 2016 10:57:06 -0400 Received: from mail-db5eur01on0139.outbound.protection.outlook.com ([104.47.2.139]:48974 helo=EUR01-DB5-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5wic-0005IM-6z for qemu-devel@nongnu.org; Thu, 26 May 2016 10:57:02 -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: <57470EB6.8010207@openvz.org> Date: Thu, 26 May 2016 17:56:54 +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)))) { > return -ENOTSUP; > } > > qemu_co_mutex_lock(&s->lock); > /* We can have new write after previous check */ > - if (!is_zero_cluster_top_locked(bs, sector_num)) { > + sector_num = cl_start; > + nb_sectors = nr = s->cluster_sectors; > + ret = qcow2_get_cluster_offset(bs, cl_start << BDRV_SECTOR_BITS, > + &nr, &off); > + if (ret != QCOW2_CLUSTER_UNALLOCATED && ret != QCOW2_CLUSTER_ZERO) { > qemu_co_mutex_unlock(&s->lock); > return -ENOTSUP; > } > diff --git a/tests/qemu-iotests/154.out b/tests/qemu-iotests/154.out > index 531fd8c..da9eabd 100644 > --- a/tests/qemu-iotests/154.out > +++ b/tests/qemu-iotests/154.out > @@ -102,13 +102,13 @@ wrote 2048/2048 bytes at offset 29696 > read 4096/4096 bytes at offset 28672 > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > [{ "start": 0, "length": 4096, "depth": 1, "zero": true, "data": false}, > -{ "start": 4096, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": 20480}, > +{ "start": 4096, "length": 4096, "depth": 0, "zero": true, "data": false}, > { "start": 8192, "length": 4096, "depth": 1, "zero": true, "data": false}, > -{ "start": 12288, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": 24576}, > +{ "start": 12288, "length": 4096, "depth": 0, "zero": true, "data": false}, > { "start": 16384, "length": 4096, "depth": 1, "zero": true, "data": false}, > -{ "start": 20480, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": 28672}, > +{ "start": 20480, "length": 4096, "depth": 0, "zero": true, "data": false}, > { "start": 24576, "length": 4096, "depth": 1, "zero": true, "data": false}, > -{ "start": 28672, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": 32768}, > +{ "start": 28672, "length": 4096, "depth": 0, "zero": true, "data": false}, > { "start": 32768, "length": 134184960, "depth": 1, "zero": true, "data": false}] > > == spanning two clusters, non-zero before request == Reviewed-by: Denis V. Lunev Thank you very much for pushing this!