From patchwork Mon Jul 3 22:14:49 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 783676 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3x1hWm225jz9s06 for ; Tue, 4 Jul 2017 08:24:36 +1000 (AEST) Received: from localhost ([::1]:37523 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dS9lf-0006kY-GX for incoming@patchwork.ozlabs.org; Mon, 03 Jul 2017 18:24:33 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55942) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dS9cx-0007fQ-K9 for qemu-devel@nongnu.org; Mon, 03 Jul 2017 18:15:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dS9cw-0004d1-8G for qemu-devel@nongnu.org; Mon, 03 Jul 2017 18:15:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44180) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dS9cs-0004Xe-SR; Mon, 03 Jul 2017 18:15:27 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D097A3B74D; Mon, 3 Jul 2017 22:15:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D097A3B74D Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=eblake@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D097A3B74D Received: from red.redhat.com (ovpn-120-36.rdu2.redhat.com [10.10.120.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5D7201719B; Mon, 3 Jul 2017 22:15:24 +0000 (UTC) From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 3 Jul 2017 17:14:49 -0500 Message-Id: <20170703221456.30817-9-eblake@redhat.com> In-Reply-To: <20170703221456.30817-1-eblake@redhat.com> References: <20170703221456.30817-1-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 03 Jul 2017 22:15:26 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v2 08/15] block: Convert bdrv_get_block_status() to bytes X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, Fam Zheng , qemu-block@nongnu.org, el13635@mail.ntua.gr, Max Reitz , Stefan Hajnoczi , jsnow@redhat.com Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" We are gradually moving away from sector-based interfaces, towards byte-based. In the common case, allocation is unlikely to ever use values that are not naturally sector-aligned, but it is possible that byte-based values will let us be more precise about allocation at the end of an unaligned file that can do byte-based access. Changing the name of the function from bdrv_get_block_status() to bdrv_block_status() ensures that the compiler enforces that all callers are updated. For now, the io.c layer still assert()s that all callers are sector-aligned, but that can be relaxed when a later patch implements byte-based block status in the drivers. Note that we have an inherent limitation in the BDRV_BLOCK_* return values: BDRV_BLOCK_OFFSET_VALID can only return the start of a sector, even if we later relax the interface to query for the status starting at an intermediate byte; document the obvious interpretation that valid offsets are always sector-relative. Therefore, for the most part this patch is just the addition of scaling at the callers followed by inverse scaling at bdrv_block_status(). But some code, particularly bdrv_is_allocated(), gets a lot simpler because it no longer has to mess with sectors. For ease of review, bdrv_get_block_status_above() will be tackled separately. Signed-off-by: Eric Blake Reviewed-by: Fam Zheng --- v2: rebase to earlier changes --- include/block/block.h | 12 +++++++----- block/io.c | 31 +++++++++++++++++++------------ block/qcow2-cluster.c | 2 +- qemu-img.c | 20 +++++++++++--------- 4 files changed, 38 insertions(+), 27 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index cc82a0d..e3e6582 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -138,8 +138,10 @@ typedef struct HDGeometry { * * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) * represent the offset in the returned BDS that is allocated for the - * corresponding raw data; however, whether that offset actually contains - * data also depends on BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, as follows: + * corresponding raw data. Individual bytes are at the same sector-relative + * locations (and thus, this bit cannot be set for mappings which are + * not equivalent modulo 512). However, whether that offset actually + * contains data also depends on BDRV_BLOCK_DATA, as follows: * * DATA ZERO OFFSET_VALID * t t t sectors read as zero, returned file is zero at offset @@ -419,9 +421,9 @@ int bdrv_has_zero_init_1(BlockDriverState *bs); int bdrv_has_zero_init(BlockDriverState *bs); bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs); -int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum, - BlockDriverState **file); +int64_t bdrv_block_status(BlockDriverState *bs, int64_t offset, + int64_t bytes, int64_t *pnum, + BlockDriverState **file); int64_t bdrv_get_block_status_above(BlockDriverState *bs, BlockDriverState *base, int64_t sector_num, diff --git a/block/io.c b/block/io.c index b3ba9af..2662f37 100644 --- a/block/io.c +++ b/block/io.c @@ -671,7 +671,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) { int64_t target_size, ret, bytes, offset = 0; BlockDriverState *bs = child->bs; - int n; /* sectors */ target_size = bdrv_getlength(bs); if (target_size < 0) { @@ -683,24 +682,23 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) if (bytes <= 0) { return 0; } - ret = bdrv_get_block_status(bs, offset >> BDRV_SECTOR_BITS, - bytes >> BDRV_SECTOR_BITS, &n, NULL); + ret = bdrv_block_status(bs, offset, bytes, &bytes, NULL); if (ret < 0) { error_report("error getting block status at offset %" PRId64 ": %s", offset, strerror(-ret)); return ret; } if (ret & BDRV_BLOCK_ZERO) { - offset += n * BDRV_SECTOR_BITS; + offset += bytes; continue; } - ret = bdrv_pwrite_zeroes(child, offset, n * BDRV_SECTOR_SIZE, flags); + ret = bdrv_pwrite_zeroes(child, offset, bytes, flags); if (ret < 0) { error_report("error writing zeroes at offset %" PRId64 ": %s", offset, strerror(-ret)); return ret; } - offset += n * BDRV_SECTOR_SIZE; + offset += bytes; } } @@ -1956,13 +1954,22 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs, nb_sectors, pnum, file); } -int64_t bdrv_get_block_status(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, int *pnum, - BlockDriverState **file) +int64_t bdrv_block_status(BlockDriverState *bs, + int64_t offset, int64_t bytes, int64_t *pnum, + BlockDriverState **file) { - return bdrv_get_block_status_above(bs, backing_bs(bs), - sector_num, nb_sectors, pnum, file); + int64_t ret; + int n; + + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); + assert(bytes <= BDRV_REQUEST_MAX_BYTES); + ret = bdrv_get_block_status_above(bs, backing_bs(bs), + offset >> BDRV_SECTOR_BITS, + bytes >> BDRV_SECTOR_BITS, &n, file); + if (pnum) { + *pnum = n * BDRV_SECTOR_SIZE; + } + return ret; } int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index f06c08f..0185986 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1567,7 +1567,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset, * cluster is already marked as zero, or if it's unallocated and we * don't have a backing file. * - * TODO We might want to use bdrv_get_block_status(bs) here, but we're + * TODO We might want to use bdrv_block_status(bs) here, but we're * holding s->lock, so that doesn't work today. * * If full_discard is true, the sector should not read back as zeroes, diff --git a/qemu-img.c b/qemu-img.c index 7b7f992..2b1422a 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1594,9 +1594,14 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) if (s->sector_next_status <= sector_num) { if (s->target_has_backing) { - ret = bdrv_get_block_status(blk_bs(s->src[src_cur]), - sector_num - src_cur_offset, - n, &n, NULL); + int64_t count = n * BDRV_SECTOR_SIZE; + + ret = bdrv_block_status(blk_bs(s->src[src_cur]), + (sector_num - src_cur_offset) * + BDRV_SECTOR_SIZE, + count, &count, NULL); + assert(ret < 0 || QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE)); + n = count >> BDRV_SECTOR_BITS; } else { ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL, sector_num - src_cur_offset, @@ -2681,9 +2686,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, int depth; BlockDriverState *file; bool has_offset; - int nb_sectors = bytes >> BDRV_SECTOR_BITS; - assert(bytes < INT_MAX); /* As an optimization, we could cache the current range of unallocated * clusters in each file of the chain, and avoid querying the same * range repeatedly. @@ -2691,12 +2694,11 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, depth = 0; for (;;) { - ret = bdrv_get_block_status(bs, offset >> BDRV_SECTOR_BITS, nb_sectors, - &nb_sectors, &file); + ret = bdrv_block_status(bs, offset, bytes, &bytes, &file); if (ret < 0) { return ret; } - assert(nb_sectors); + assert(bytes); if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) { break; } @@ -2713,7 +2715,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, *e = (MapEntry) { .start = offset, - .length = nb_sectors * BDRV_SECTOR_SIZE, + .length = bytes, .data = !!(ret & BDRV_BLOCK_DATA), .zero = !!(ret & BDRV_BLOCK_ZERO), .offset = ret & BDRV_BLOCK_OFFSET_MASK,