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 <ebl...@redhat.com> --- v3: clamp bytes to 32-bits, rather than asserting 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 bb3b95d491..7a9a8db588 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 @@ -421,9 +423,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 638b3890b7..1ed46bcece 100644 --- a/block/io.c +++ b/block/io.c @@ -695,7 +695,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) { @@ -707,24 +706,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; } } @@ -1983,13 +1981,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)); + bytes = MIN(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 0d4824993c..d837b3980d 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1584,7 +1584,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 54f7682069..897f80abb3 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1598,9 +1598,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, @@ -2677,9 +2682,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. @@ -2687,12 +2690,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; } @@ -2709,7 +2711,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, -- 2.13.5