06.05.2020 18:14, Eric Blake wrote:
On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
Currently this field only set by qed and qcow2.
Well, only after patches 1-6 (prior to then, it was also set in protocol
drivers). I think you might be able to hoist part of this patch earlier in the
series, to make the changes to the protocol drivers easier to review, by
rewording this sentence:
Currently, the only format drivers that set this field are qed and qcow2.
But in fact, all
backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
this semantics: on unallocated blocks, if there is no backing file they
s/this/these/
just memset the buffer with zeroes.
So, document this behavior for .supports_backing and drop
.unallocated_blocks_are_zero
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
include/block/block.h | 6 ------
include/block/block_int.h | 13 ++++++++++++-
block.c | 15 ---------------
block/io.c | 8 ++++----
block/qcow2.c | 1 -
block/qed.c | 1 -
6 files changed, 16 insertions(+), 28 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8b62429aa4..db1cb503ec 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
/* offset at which the VM state can be saved (0 if not possible) */
int64_t vm_state_offset;
bool is_dirty;
- /*
- * True if unallocated blocks read back as zeroes. This is equivalent
- * to the LBPRZ flag in the SCSI logical block provisioning page.
- */
- bool unallocated_blocks_are_zero;
You can't delete this field until all protocol drivers are cleaned up, so
deferring this part of the change to the end of the series makes sense.
/*
* True if this block driver only supports compressed writes
*/
@@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
int64_t bytes);
int bdrv_has_zero_init_1(BlockDriverState *bs);
int bdrv_has_zero_init(BlockDriverState *bs);
int bdrv_has_zero_init_truncate(BlockDriverState *bs);
-bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
Doing this cleanup makes sense: there is only one caller of this function
pre-patch, and 0 callers post-patch - but whether the cleanup should be at the
same time as you fix the one caller, or deferred to when you also clean up the
field, is less important.
If I were writing the series:
1 - fix qemu-img.c to not use the field
2 - fix block_status to not use the function
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.
Hmm, yes.
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
OK
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.
I understand the idea.. Ha, it looks like an optimization issue :) I use greedy
algorithm, trying to make each patch to be a simple small step to the result.
But greedy algorithm now always optimal as we know. Actually, here, making
first step larger and more complicated, we achieve less total
review-complexity. Good new experience for me, thanks, I'll try the proposed
way.
--
Best regards,
Vladimir
