3-n - fix protocol drivers that set the field to also return _ZERO
during block status (but not delete the field at that time)
n+1 - delete unused function and field (from ALL drivers)
bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
int bdrv_block_status(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum, int64_t *map,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 92335f33c7..c156b22c6b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -115,7 +115,18 @@ struct BlockDriver {
*/
bool bdrv_needs_filename;
- /* Set if a driver can support backing files */
+ /*
+ * Set if a driver can support backing files. This also implies the
+ * following semantics:
+ *
+ * - Return status 0 of .bdrv_co_block_status means that corresponding
+ * blocks are not allocated in this layer of backing-chain
+ * - For such (unallocated) blocks, read will:
+ * - read from backing file if there is one and big enough
s/and/and it is/
+ * - fill buffer with zeroes if there is no backing file
+ * - space after EOF of the backing file considered as zero
+ * (corresponding part of read-buffer must be zeroed by driver)
Does the driver actually have to do the zeroing? Looking at qcow2.c, I see:
static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
...
case QCOW2_CLUSTER_UNALLOCATED:
assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
return bdrv_co_preadv_part(bs->backing, offset, bytes,
qiov, qiov_offset, 0);
which just defers to the block layer to handle read-beyond-EOF, rather than an
explicit memset in the driver.
So maybe you can simplify to:
- For such (unallocated) blocks, read will:
- fill buffer with zeros if there is no backing file
- read from the backing file otherwise, where the block layer
takes care of reading zeros beyond EOF if backing file is short
But the effect is the same.
+++ b/block/io.c
@@ -2385,16 +2385,16 @@ static int coroutine_fn
bdrv_co_block_status(BlockDriverState *bs,
if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
ret |= BDRV_BLOCK_ALLOCATED;
- } else if (want_zero) {
- if (bdrv_unallocated_blocks_are_zero(bs)) {
- ret |= BDRV_BLOCK_ZERO;
- } else if (bs->backing) {
+ } else if (want_zero && bs->drv->supports_backing) {
+ if (bs->backing) {
BlockDriverState *bs2 = bs->backing->bs;
int64_t size2 = bdrv_getlength(bs2);
if (size2 >= 0 && offset >= size2) {
ret |= BDRV_BLOCK_ZERO;
}
+ } else {
+ ret |= BDRV_BLOCK_ZERO;
}
I like this part of the change. But if it is done first in the series, it
_does_ have a semantic impact on protocol drivers (previously, protocol drivers
that return 0 but set the field .unallocated_blocks_are_zero will be changed to
report _ZERO; after this patch, protocol drivers do not do that, because they
don't support backing files, and it is now only backing files that do the _ZERO
magic). So doing _just_ this change, along with a better analysis of how it
changes the semantics of 'qemu-io -c map' on protocol drivers while mentioning
why that is okay, would make a better start to the series, rather than here at
the end. Of course, if you defer it to the end, then none of the protocol
drivers set .unallocated_blocks_are_zero anyway, but that cost more review work
on each altered protocol driver.