* [PATCH v2 0/9] drop unallocated_blocks_are_zero
@ 2020-05-07 8:47 Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 1/9] qemu-img: convert: don't use unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (10 more replies)
0 siblings, 11 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
Hi all!
v2 (by Eric's review):
01: moved to the start of the series, add Eric's r-b
02: new
03-04: improve commit message
05: add Eric's r-b
06-08: improve commit message a bit, add Eric's r-b
09: typos and wording, rebase on 02
This is first step to block-status refactoring, and solves most simple
problem mentioned in my investigation of block-status described in
the thread "backing chain & block status & filters":
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04706.html
unallocated_blocks_are_zero doesn't simplify all the logic about
block-status, and happily it's not needed, as shown by the following
patches. So, let's get rid of it.
Vladimir Sementsov-Ogievskiy (9):
qemu-img: convert: don't use unallocated_blocks_are_zero
block: inline bdrv_unallocated_blocks_are_zero()
block/vdi: return ZERO block-status when appropriate
block/vpc: return ZERO block-status when appropriate
block/crypto: drop unallocated_blocks_are_zero
block/iscsi: drop unallocated_blocks_are_zero
block/file-posix: drop unallocated_blocks_are_zero
block/vhdx: drop unallocated_blocks_are_zero
block: drop unallocated_blocks_are_zero
include/block/block.h | 6 ------
include/block/block_int.h | 12 +++++++++++-
block.c | 15 ---------------
block/crypto.c | 1 -
block/file-posix.c | 3 ---
block/io.c | 8 ++++----
block/iscsi.c | 1 -
block/qcow2.c | 1 -
block/qed.c | 1 -
block/vdi.c | 3 +--
block/vhdx.c | 3 ---
block/vpc.c | 3 +--
qemu-img.c | 4 +---
13 files changed, 18 insertions(+), 43 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/9] qemu-img: convert: don't use unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero() Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
qemu-img convert wants to distinguish ZERO which comes from short
backing files. unallocated_blocks_are_zero field of bdi is unrelated:
space after EOF is always considered to be zero anyway. So, just make
post_backing_zero true in case of short backing file.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..4fe751878b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
BlockBackend *target;
bool has_zero_init;
bool compressed;
- bool unallocated_blocks_are_zero;
bool target_is_new;
bool target_has_backing;
int64_t target_backing_sectors; /* negative if unknown */
@@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
if (s->target_backing_sectors >= 0) {
if (sector_num >= s->target_backing_sectors) {
- post_backing_zero = s->unallocated_blocks_are_zero;
+ post_backing_zero = true;
} else if (sector_num + n > s->target_backing_sectors) {
/* Split requests around target_backing_sectors (because
* starting from there, zeros are handled differently) */
@@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
} else {
s.compressed = s.compressed || bdi.needs_compressed_writes;
s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
- s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
}
ret = convert_do_copy(&s);
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 1/9] qemu-img: convert: don't use unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 14:08 ` Eric Blake
2020-05-07 8:47 ` [PATCH v2 3/9] block/vdi: return ZERO block-status when appropriate Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
The function has the only user: bdrv_co_block_status(). Inline it to
simplify reviewing of the following patches, which will finally drop
unallocated_blocks_are_zero field too.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block.h | 1 -
block.c | 15 ---------------
block/io.c | 11 ++++++++---
3 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8b62429aa4..931003a476 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -431,7 +431,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);
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/block.c b/block.c
index cf5c19b1db..0283fdecbc 100644
--- a/block.c
+++ b/block.c
@@ -5305,21 +5305,6 @@ int bdrv_has_zero_init_truncate(BlockDriverState *bs)
return 0;
}
-bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
-{
- BlockDriverInfo bdi;
-
- if (bs->backing) {
- return false;
- }
-
- if (bdrv_get_info(bs, &bdi) == 0) {
- return bdi.unallocated_blocks_are_zero;
- }
-
- return false;
-}
-
bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
{
if (!(bs->open_flags & BDRV_O_UNMAP)) {
diff --git a/block/io.c b/block/io.c
index a4f9714230..00e7371d50 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2386,15 +2386,20 @@ 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) {
+ if (bs->backing) {
BlockDriverState *bs2 = bs->backing->bs;
int64_t size2 = bdrv_getlength(bs2);
if (size2 >= 0 && offset >= size2) {
ret |= BDRV_BLOCK_ZERO;
}
+ } else {
+ BlockDriverInfo bdi;
+ int ret2 = bdrv_get_info(bs, &bdi);
+
+ if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {
+ ret |= BDRV_BLOCK_ZERO;
+ }
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/9] block/vdi: return ZERO block-status when appropriate
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 1/9] qemu-img: convert: don't use unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero() Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 14:10 ` Eric Blake
2020-05-07 8:47 ` [PATCH v2 4/9] block/vpc: " Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
In case of !VDI_IS_ALLOCATED[], we do zero out the corresponding chunk
of qiov. So, this should be reported as ZERO.
Note that this changes visible output of "qemu-img map --output=json"
and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
we just mark as zero what is really zero. For qemu-io it's less
obvious: what was unallocated now is allocated.
There is an inconsistency in understanding of unallocated regions in
Qemu: backing-supporting format-drivers return 0 block-status to report
go-to-backing logic for this area. Some protocol-drivers (iscsi) return
0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
disk, read result is undefined).
BDRV_BLOCK_ALLOCATED is defined as something more close to
go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
iscsi is treated as unallocated. It doesn't influence backing-chain
behavior, as iscsi can't have backing file. But it does influence
"qemu-io -c map".
We should solve this inconsistency at some future point. Now, let's
just make backing-not-supporting format drivers (vdi at this patch and
vpc with the following) to behave more like backing-supporting drivers
and not report 0 block-status. More over, returning ZERO status is
absolutely valid thing, and again, corresponds to how the other
format-drivers (backing-supporting) work.
After block-status update, it never reports 0, so setting
unallocated_blocks_are_zero doesn't make sense (as the only user of it
is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
for unallocated areas). Drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/vdi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 0c7835ae70..83471528d2 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -334,7 +334,6 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
logout("\n");
bdi->cluster_size = s->block_size;
bdi->vm_state_offset = 0;
- bdi->unallocated_blocks_are_zero = true;
return 0;
}
@@ -536,7 +535,7 @@ static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
*pnum = MIN(s->block_size - index_in_block, bytes);
result = VDI_IS_ALLOCATED(bmap_entry);
if (!result) {
- return 0;
+ return BDRV_BLOCK_ZERO;
}
*map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 4/9] block/vpc: return ZERO block-status when appropriate
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-05-07 8:47 ` [PATCH v2 3/9] block/vdi: return ZERO block-status when appropriate Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 14:11 ` Eric Blake
2020-05-07 8:47 ` [PATCH v2 5/9] block/crypto: drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
In case when get_image_offset() returns -1, we do zero out the
corresponding chunk of qiov. So, this should be reported as ZERO.
Note that this changes visible output of "qemu-img map --output=json"
and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
we just mark as zero what is really zero. For qemu-io it's less
obvious: what was unallocated now is allocated.
There is an inconsistency in understanding of unallocated regions in
Qemu: backing-supporting format-drivers return 0 block-status to report
go-to-backing logic for this area. Some protocol-drivers (iscsi) return
0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
disk, read result is undefined).
BDRV_BLOCK_ALLOCATED is defined as something more close to
go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
iscsi is treated as unallocated. It doesn't influence backing-chain
behavior, as iscsi can't have backing file. But it does influence
"qemu-io -c map".
We should solve this inconsistency at some future point. Now, let's
just make backing-not-supporting format drivers (vdi in the previous
patch and vpc now) to behave more like backing-supporting drivers
and not report 0 block-status. More over, returning ZERO status is
absolutely valid thing, and again, corresponds to how the other
format-drivers (backing-supporting) work.
After block-status update, it never reports 0, so setting
unallocated_blocks_are_zero doesn't make sense (as the only user of it
is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
for unallocated areas). Drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/vpc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 2d1eade146..555f9d8587 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -606,7 +606,6 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
bdi->cluster_size = s->block_size;
}
- bdi->unallocated_blocks_are_zero = true;
return 0;
}
@@ -745,7 +744,7 @@ static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
image_offset = get_image_offset(bs, offset, false, NULL);
allocated = (image_offset != -1);
*pnum = 0;
- ret = 0;
+ ret = BDRV_BLOCK_ZERO;
do {
/* All sectors in a block are contiguous (without using the bitmap) */
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 5/9] block/crypto: drop unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-05-07 8:47 ` [PATCH v2 4/9] block/vpc: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 6/9] block/iscsi: " Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
It's false by default, no needs to set it. We are going to drop this
variable at all, so drop it now here, it doesn't hurt.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/crypto.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/crypto.c b/block/crypto.c
index e02f343590..7685e61844 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -694,7 +694,6 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
return ret;
}
- bdi->unallocated_blocks_are_zero = false;
bdi->cluster_size = subbdi.cluster_size;
return 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 6/9] block/iscsi: drop unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-05-07 8:47 ` [PATCH v2 5/9] block/crypto: drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 7/9] block/file-posix: " Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
We set bdi->unallocated_blocks_are_zero = iscsilun->lbprz, but
iscsi_co_block_status doesn't return 0 in case of iscsilun->lbprz, it
returns ZERO when appropriate. So actually unallocated_blocks_are_zero
is useless (it doesn't affect the only user of the field:
bdrv_co_block_status()). Drop it now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/iscsi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index a8b76979d8..767e3e75fd 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2163,7 +2163,6 @@ static int coroutine_fn iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
{
IscsiLun *iscsilun = bs->opaque;
- bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
bdi->cluster_size = iscsilun->cluster_size;
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 7/9] block/file-posix: drop unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2020-05-07 8:47 ` [PATCH v2 6/9] block/iscsi: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 8/9] block/vhdx: " Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
raw_co_block_status() in block/file-posix.c never returns 0, so
unallocated_blocks_are_zero is useless (it doesn't affect the only user
of the field: bdrv_co_block_status()). Drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/file-posix.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 05e094be29..5c01735108 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2878,9 +2878,6 @@ static int coroutine_fn raw_co_pwrite_zeroes(
static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
{
- BDRVRawState *s = bs->opaque;
-
- bdi->unallocated_blocks_are_zero = s->discard_zeroes;
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 8/9] block/vhdx: drop unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2020-05-07 8:47 ` [PATCH v2 7/9] block/file-posix: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:47 ` Vladimir Sementsov-Ogievskiy
2020-05-07 8:48 ` [PATCH v2 9/9] block: " Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:47 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
vhdx doesn't have .bdrv_co_block_status handler, so DATA|ALLOCATED is
always assumed for it in bdrv_co_block_status().
unallocated_blocks_are_zero is useless (it doesn't affect the only user
of the field: bdrv_co_block_status()), drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/vhdx.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index aedd782604..45963a3166 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1164,9 +1164,6 @@ static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
bdi->cluster_size = s->block_size;
- bdi->unallocated_blocks_are_zero =
- (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) == 0;
-
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 9/9] block: drop unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2020-05-07 8:47 ` [PATCH v2 8/9] block/vhdx: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 8:48 ` Vladimir Sementsov-Ogievskiy
2020-05-07 14:21 ` Eric Blake
2020-05-07 14:45 ` [PATCH v2 10/9] qed: Simplify backing reads Eric Blake
2020-05-19 17:40 ` [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
10 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 8:48 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, stefanha, codyprime, sw, pl, qemu-devel,
mreitz, ronniesahlberg, den, pbonzini
Currently this field only set by qed and qcow2. But in fact, all
backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
these semantics: on unallocated blocks, if there is no backing file they
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 <vsementsov@virtuozzo.com>
---
include/block/block.h | 5 -----
include/block/block_int.h | 12 +++++++++++-
block/io.c | 9 ++-------
block/qcow2.c | 1 -
block/qed.c | 1 -
5 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 931003a476..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;
/*
* True if this block driver only supports compressed writes
*/
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 92335f33c7..8fac6c3ce2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -115,7 +115,17 @@ 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:
+ * - 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
+ */
bool supports_backing;
/* For handling image reopen for split or non-split files */
diff --git a/block/io.c b/block/io.c
index 00e7371d50..484adec5a1 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2385,7 +2385,7 @@ 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) {
+ } else if (want_zero && bs->drv->supports_backing) {
if (bs->backing) {
BlockDriverState *bs2 = bs->backing->bs;
int64_t size2 = bdrv_getlength(bs2);
@@ -2394,12 +2394,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
ret |= BDRV_BLOCK_ZERO;
}
} else {
- BlockDriverInfo bdi;
- int ret2 = bdrv_get_info(bs, &bdi);
-
- if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {
- ret |= BDRV_BLOCK_ZERO;
- }
+ ret |= BDRV_BLOCK_ZERO;
}
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 2ba0b17c39..dc3c0aac2b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4858,7 +4858,6 @@ err:
static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
{
BDRVQcow2State *s = bs->opaque;
- bdi->unallocated_blocks_are_zero = true;
bdi->cluster_size = s->cluster_size;
bdi->vm_state_offset = qcow2_vm_state_offset(s);
return 0;
diff --git a/block/qed.c b/block/qed.c
index b0fdb8f565..fb7833dc8b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1514,7 +1514,6 @@ static int bdrv_qed_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
memset(bdi, 0, sizeof(*bdi));
bdi->cluster_size = s->header.cluster_size;
bdi->is_dirty = s->header.features & QED_F_NEED_CHECK;
- bdi->unallocated_blocks_are_zero = true;
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()
2020-05-07 8:47 ` [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero() Vladimir Sementsov-Ogievskiy
@ 2020-05-07 14:08 ` Eric Blake
2020-05-28 9:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 19+ messages in thread
From: Eric Blake @ 2020-05-07 14:08 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ronniesahlberg, codyprime, sw, pl, qemu-devel,
mreitz, stefanha, pbonzini, den
On 5/7/20 3:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> The function has the only user: bdrv_co_block_status(). Inline it to
s/the only/only one/
> simplify reviewing of the following patches, which will finally drop
> unallocated_blocks_are_zero field too.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> include/block/block.h | 1 -
> block.c | 15 ---------------
> block/io.c | 11 ++++++++---
> 3 files changed, 8 insertions(+), 19 deletions(-)
>
> +++ b/block/io.c
> @@ -2386,15 +2386,20 @@ 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) {
> + if (bs->backing) {
> BlockDriverState *bs2 = bs->backing->bs;
> int64_t size2 = bdrv_getlength(bs2);
>
> if (size2 >= 0 && offset >= size2) {
> ret |= BDRV_BLOCK_ZERO;
> }
> + } else {
> + BlockDriverInfo bdi;
> + int ret2 = bdrv_get_info(bs, &bdi);
> +
> + if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {
Could perhaps condense to:
else {
BlockDriverInfo bdi;
if (bdrv_get_info(bs, &bd) == 0 &&
bdi.unallocated_blocks_are_zero) {
but that's cosmetic.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/9] block/vdi: return ZERO block-status when appropriate
2020-05-07 8:47 ` [PATCH v2 3/9] block/vdi: return ZERO block-status when appropriate Vladimir Sementsov-Ogievskiy
@ 2020-05-07 14:10 ` Eric Blake
0 siblings, 0 replies; 19+ messages in thread
From: Eric Blake @ 2020-05-07 14:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ronniesahlberg, sw, pl, qemu-devel, mreitz, stefanha,
pbonzini, den
On 5/7/20 3:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> In case of !VDI_IS_ALLOCATED[], we do zero out the corresponding chunk
> of qiov. So, this should be reported as ZERO.
>
> Note that this changes visible output of "qemu-img map --output=json"
> and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
> we just mark as zero what is really zero. For qemu-io it's less
> obvious: what was unallocated now is allocated.
>
> There is an inconsistency in understanding of unallocated regions in
> Qemu: backing-supporting format-drivers return 0 block-status to report
> go-to-backing logic for this area. Some protocol-drivers (iscsi) return
> 0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
> disk, read result is undefined).
>
> BDRV_BLOCK_ALLOCATED is defined as something more close to
> go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
> iscsi is treated as unallocated. It doesn't influence backing-chain
> behavior, as iscsi can't have backing file. But it does influence
> "qemu-io -c map".
>
> We should solve this inconsistency at some future point. Now, let's
> just make backing-not-supporting format drivers (vdi at this patch and
> vpc with the following) to behave more like backing-supporting drivers
> and not report 0 block-status. More over, returning ZERO status is
> absolutely valid thing, and again, corresponds to how the other
> format-drivers (backing-supporting) work.
>
> After block-status update, it never reports 0, so setting
> unallocated_blocks_are_zero doesn't make sense (as the only user of it
> is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
> for unallocated areas). Drop it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/vdi.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Yes, much better commit message, showing why the code change is correct.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/9] block/vpc: return ZERO block-status when appropriate
2020-05-07 8:47 ` [PATCH v2 4/9] block/vpc: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 14:11 ` Eric Blake
0 siblings, 0 replies; 19+ messages in thread
From: Eric Blake @ 2020-05-07 14:11 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ronniesahlberg, sw, pl, qemu-devel, mreitz, stefanha,
pbonzini, den
On 5/7/20 3:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> In case when get_image_offset() returns -1, we do zero out the
> corresponding chunk of qiov. So, this should be reported as ZERO.
>
> Note that this changes visible output of "qemu-img map --output=json"
> and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
> we just mark as zero what is really zero. For qemu-io it's less
> obvious: what was unallocated now is allocated.
>
> There is an inconsistency in understanding of unallocated regions in
> Qemu: backing-supporting format-drivers return 0 block-status to report
> go-to-backing logic for this area. Some protocol-drivers (iscsi) return
> 0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
> disk, read result is undefined).
>
> BDRV_BLOCK_ALLOCATED is defined as something more close to
> go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
> iscsi is treated as unallocated. It doesn't influence backing-chain
> behavior, as iscsi can't have backing file. But it does influence
> "qemu-io -c map".
>
> We should solve this inconsistency at some future point. Now, let's
> just make backing-not-supporting format drivers (vdi in the previous
> patch and vpc now) to behave more like backing-supporting drivers
> and not report 0 block-status. More over, returning ZERO status is
> absolutely valid thing, and again, corresponds to how the other
> format-drivers (backing-supporting) work.
>
> After block-status update, it never reports 0, so setting
> unallocated_blocks_are_zero doesn't make sense (as the only user of it
> is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
> for unallocated areas). Drop it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/vpc.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 9/9] block: drop unallocated_blocks_are_zero
2020-05-07 8:48 ` [PATCH v2 9/9] block: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 14:21 ` Eric Blake
0 siblings, 0 replies; 19+ messages in thread
From: Eric Blake @ 2020-05-07 14:21 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ronniesahlberg, codyprime, sw, pl, qemu-devel,
mreitz, stefanha, pbonzini, den
On 5/7/20 3:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> Currently this field only set by qed and qcow2. But in fact, all
> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
> these semantics: on unallocated blocks, if there is no backing file they
> 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 <vsementsov@virtuozzo.com>
> ---
> include/block/block.h | 5 -----
> include/block/block_int.h | 12 +++++++++++-
> block/io.c | 9 ++-------
> block/qcow2.c | 1 -
> block/qed.c | 1 -
> 5 files changed, 13 insertions(+), 15 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 10/9] qed: Simplify backing reads
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2020-05-07 8:48 ` [PATCH v2 9/9] block: " Vladimir Sementsov-Ogievskiy
@ 2020-05-07 14:45 ` Eric Blake
2020-05-07 15:22 ` Eric Blake
2020-05-07 18:22 ` Vladimir Sementsov-Ogievskiy
2020-05-19 17:40 ` [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
10 siblings, 2 replies; 19+ messages in thread
From: Eric Blake @ 2020-05-07 14:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, vsementsov, open list:qed, stefanha, mreitz
The other four drivers that support backing files (qcow, qcow2,
parallels, vmdk) all rely on the block layer to populate zeroes when
reading beyond EOF of a short backing file. We can simplify the qed
code by doing likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
I noticed this during my audit that v1 of Vladimir's series was correct.
No change in iotests results (test 274 is currently failing for qed,
but for other reasons:
+Traceback (most recent call last):
+ File "274", line 24, in <module>
+ iotests.verify_image_format(supported_fmts=['qcow2'])
+AttributeError: module 'iotests' has no attribute 'verify_image_format'
)
block/qed.h | 1 -
block/qed.c | 64 +++++------------------------------------------------
2 files changed, 6 insertions(+), 59 deletions(-)
diff --git a/block/qed.h b/block/qed.h
index 42c115d8220c..3d12bf78d412 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -140,7 +140,6 @@ typedef struct QEDAIOCB {
/* Current cluster scatter-gather list */
QEMUIOVector cur_qiov;
- QEMUIOVector *backing_qiov;
uint64_t cur_pos; /* position on block device, in bytes */
uint64_t cur_cluster; /* cluster offset in image file */
unsigned int cur_nclusters; /* number of clusters being accessed */
diff --git a/block/qed.c b/block/qed.c
index 927382995a0c..bea4b9f6cc97 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -849,56 +849,18 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
* @s: QED state
* @pos: Byte position in device
* @qiov: Destination I/O vector
- * @backing_qiov: Possibly shortened copy of qiov, to be allocated here
- * @cb: Completion function
- * @opaque: User data for completion function
*
* This function reads qiov->size bytes starting at pos from the backing file.
* If there is no backing file then zeroes are read.
*/
static int coroutine_fn qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
- QEMUIOVector *qiov,
- QEMUIOVector **backing_qiov)
+ QEMUIOVector *qiov)
{
- uint64_t backing_length = 0;
- size_t size;
- int ret;
-
- /* If there is a backing file, get its length. Treat the absence of a
- * backing file like a zero length backing file.
- */
if (s->bs->backing) {
- int64_t l = bdrv_getlength(s->bs->backing->bs);
- if (l < 0) {
- return l;
- }
- backing_length = l;
- }
-
- /* Zero all sectors if reading beyond the end of the backing file */
- if (pos >= backing_length ||
- pos + qiov->size > backing_length) {
- qemu_iovec_memset(qiov, 0, 0, qiov->size);
- }
-
- /* Complete now if there are no backing file sectors to read */
- if (pos >= backing_length) {
- return 0;
- }
-
- /* If the read straddles the end of the backing file, shorten it */
- size = MIN((uint64_t)backing_length - pos, qiov->size);
-
- assert(*backing_qiov == NULL);
- *backing_qiov = g_new(QEMUIOVector, 1);
- qemu_iovec_init(*backing_qiov, qiov->niov);
- qemu_iovec_concat(*backing_qiov, qiov, 0, size);
-
- BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
- ret = bdrv_co_preadv(s->bs->backing, pos, size, *backing_qiov, 0);
- if (ret < 0) {
- return ret;
+ BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
+ return bdrv_co_preadv(s->bs->backing, pos, qiov->size, qiov, 0);
}
+ qemu_iovec_memset(qiov, 0, 0, qiov->size);
return 0;
}
@@ -915,7 +877,6 @@ static int coroutine_fn qed_copy_from_backing_file(BDRVQEDState *s,
uint64_t offset)
{
QEMUIOVector qiov;
- QEMUIOVector *backing_qiov = NULL;
int ret;
/* Skip copy entirely if there is no work to do */
@@ -925,13 +886,7 @@ static int coroutine_fn qed_copy_from_backing_file(BDRVQEDState *s,
qemu_iovec_init_buf(&qiov, qemu_blockalign(s->bs, len), len);
- ret = qed_read_backing_file(s, pos, &qiov, &backing_qiov);
-
- if (backing_qiov) {
- qemu_iovec_destroy(backing_qiov);
- g_free(backing_qiov);
- backing_qiov = NULL;
- }
+ ret = qed_read_backing_file(s, pos, &qiov);
if (ret) {
goto out;
@@ -1339,8 +1294,7 @@ static int coroutine_fn qed_aio_read_data(void *opaque, int ret,
qemu_iovec_memset(&acb->cur_qiov, 0, 0, acb->cur_qiov.size);
r = 0;
} else if (ret != QED_CLUSTER_FOUND) {
- r = qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
- &acb->backing_qiov);
+ r = qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov);
} else {
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
r = bdrv_co_preadv(bs->file, offset, acb->cur_qiov.size,
@@ -1365,12 +1319,6 @@ static int coroutine_fn qed_aio_next_io(QEDAIOCB *acb)
while (1) {
trace_qed_aio_next_io(s, acb, 0, acb->cur_pos + acb->cur_qiov.size);
- if (acb->backing_qiov) {
- qemu_iovec_destroy(acb->backing_qiov);
- g_free(acb->backing_qiov);
- acb->backing_qiov = NULL;
- }
-
acb->qiov_offset += acb->cur_qiov.size;
acb->cur_pos += acb->cur_qiov.size;
qemu_iovec_reset(&acb->cur_qiov);
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 10/9] qed: Simplify backing reads
2020-05-07 14:45 ` [PATCH v2 10/9] qed: Simplify backing reads Eric Blake
@ 2020-05-07 15:22 ` Eric Blake
2020-05-07 18:22 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 19+ messages in thread
From: Eric Blake @ 2020-05-07 15:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, vsementsov, stefanha, open list:qed, mreitz
On 5/7/20 9:45 AM, Eric Blake wrote:
> The other four drivers that support backing files (qcow, qcow2,
> parallels, vmdk) all rely on the block layer to populate zeroes when
> reading beyond EOF of a short backing file. We can simplify the qed
> code by doing likewise.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> I noticed this during my audit that v1 of Vladimir's series was correct.
>
> No change in iotests results (test 274 is currently failing for qed,
> but for other reasons:
> +Traceback (most recent call last):
> + File "274", line 24, in <module>
> + iotests.verify_image_format(supported_fmts=['qcow2'])
> +AttributeError: module 'iotests' has no attribute 'verify_image_format'
> )
That iotest failure was due to a stale branch on my end; after updating
to latest git master plus Kevin's latest 'block' branch, 274 is now
skipped on qed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 10/9] qed: Simplify backing reads
2020-05-07 14:45 ` [PATCH v2 10/9] qed: Simplify backing reads Eric Blake
2020-05-07 15:22 ` Eric Blake
@ 2020-05-07 18:22 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-07 18:22 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, open list:qed, stefanha, mreitz
07.05.2020 17:45, Eric Blake wrote:
> The other four drivers that support backing files (qcow, qcow2,
> parallels, vmdk) all rely on the block layer to populate zeroes when
> reading beyond EOF of a short backing file. We can simplify the qed
> code by doing likewise.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/9] drop unallocated_blocks_are_zero
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2020-05-07 14:45 ` [PATCH v2 10/9] qed: Simplify backing reads Eric Blake
@ 2020-05-19 17:40 ` Vladimir Sementsov-Ogievskiy
10 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-19 17:40 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, stefanha, codyprime, sw, pl, qemu-devel, mreitz,
ronniesahlberg, den, pbonzini
Ping
The whole series reviewed by Eric, with only one grammar fix needed in 02 commit message (and possible drop of ret2, up to maintainer).
07.05.2020 11:47, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> v2 (by Eric's review):
>
> 01: moved to the start of the series, add Eric's r-b
> 02: new
> 03-04: improve commit message
> 05: add Eric's r-b
> 06-08: improve commit message a bit, add Eric's r-b
> 09: typos and wording, rebase on 02
>
>
> This is first step to block-status refactoring, and solves most simple
> problem mentioned in my investigation of block-status described in
> the thread "backing chain & block status & filters":
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04706.html
>
>
> unallocated_blocks_are_zero doesn't simplify all the logic about
> block-status, and happily it's not needed, as shown by the following
> patches. So, let's get rid of it.
>
> Vladimir Sementsov-Ogievskiy (9):
> qemu-img: convert: don't use unallocated_blocks_are_zero
> block: inline bdrv_unallocated_blocks_are_zero()
> block/vdi: return ZERO block-status when appropriate
> block/vpc: return ZERO block-status when appropriate
> block/crypto: drop unallocated_blocks_are_zero
> block/iscsi: drop unallocated_blocks_are_zero
> block/file-posix: drop unallocated_blocks_are_zero
> block/vhdx: drop unallocated_blocks_are_zero
> block: drop unallocated_blocks_are_zero
>
> include/block/block.h | 6 ------
> include/block/block_int.h | 12 +++++++++++-
> block.c | 15 ---------------
> block/crypto.c | 1 -
> block/file-posix.c | 3 ---
> block/io.c | 8 ++++----
> block/iscsi.c | 1 -
> block/qcow2.c | 1 -
> block/qed.c | 1 -
> block/vdi.c | 3 +--
> block/vhdx.c | 3 ---
> block/vpc.c | 3 +--
> qemu-img.c | 4 +---
> 13 files changed, 18 insertions(+), 43 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()
2020-05-07 14:08 ` Eric Blake
@ 2020-05-28 9:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-28 9:31 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: fam, kwolf, ronniesahlberg, codyprime, sw, pl, qemu-devel,
mreitz, stefanha, pbonzini, den
07.05.2020 17:08, Eric Blake wrote:
> On 5/7/20 3:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The function has the only user: bdrv_co_block_status(). Inline it to
>
> s/the only/only one/
>
>> simplify reviewing of the following patches, which will finally drop
>> unallocated_blocks_are_zero field too.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> include/block/block.h | 1 -
>> block.c | 15 ---------------
>> block/io.c | 11 ++++++++---
>> 3 files changed, 8 insertions(+), 19 deletions(-)
>>
>
>> +++ b/block/io.c
>> @@ -2386,15 +2386,20 @@ 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) {
>> + if (bs->backing) {
>> BlockDriverState *bs2 = bs->backing->bs;
>> int64_t size2 = bdrv_getlength(bs2);
>> if (size2 >= 0 && offset >= size2) {
>> ret |= BDRV_BLOCK_ZERO;
>> }
>> + } else {
>> + BlockDriverInfo bdi;
>> + int ret2 = bdrv_get_info(bs, &bdi);
>> +
>> + if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {
>
> Could perhaps condense to:
>
> else {
> BlockDriverInfo bdi;
>
> if (bdrv_get_info(bs, &bd) == 0 &&
> bdi.unallocated_blocks_are_zero) {
>
> but that's cosmetic.
I'll keep it as is, as it is to be removed in 09 anyway.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-05-28 9:32 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07 8:47 [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 1/9] qemu-img: convert: don't use unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero() Vladimir Sementsov-Ogievskiy
2020-05-07 14:08 ` Eric Blake
2020-05-28 9:31 ` Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 3/9] block/vdi: return ZERO block-status when appropriate Vladimir Sementsov-Ogievskiy
2020-05-07 14:10 ` Eric Blake
2020-05-07 8:47 ` [PATCH v2 4/9] block/vpc: " Vladimir Sementsov-Ogievskiy
2020-05-07 14:11 ` Eric Blake
2020-05-07 8:47 ` [PATCH v2 5/9] block/crypto: drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 6/9] block/iscsi: " Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 7/9] block/file-posix: " Vladimir Sementsov-Ogievskiy
2020-05-07 8:47 ` [PATCH v2 8/9] block/vhdx: " Vladimir Sementsov-Ogievskiy
2020-05-07 8:48 ` [PATCH v2 9/9] block: " Vladimir Sementsov-Ogievskiy
2020-05-07 14:21 ` Eric Blake
2020-05-07 14:45 ` [PATCH v2 10/9] qed: Simplify backing reads Eric Blake
2020-05-07 15:22 ` Eric Blake
2020-05-07 18:22 ` Vladimir Sementsov-Ogievskiy
2020-05-19 17:40 ` [PATCH v2 0/9] drop unallocated_blocks_are_zero Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).