* [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based
@ 2017-10-12 3:46 Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 01/24] block: Allow NULL file for bdrv_get_block_status() Eric Blake
` (24 more replies)
0 siblings, 25 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:46 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow
There are patches floating around to add NBD_CMD_BLOCK_STATUS,
but NBD wants to report status on byte granularity (even if the
reporting will probably be naturally aligned to sectors or even
much higher levels). I've therefore started the task of
converting our block status code to report at a byte granularity
rather than sectors.
Now that 2.11 is open, I'm rebasing/reposting the remaining patches.
The overall conversion currently looks like:
part 1: bdrv_is_allocated (merged, commit 51b0a488)
part 2: dirty-bitmap (merged, commit ca759622)
part 3: bdrv_get_block_status (this series, v5 at [1])
part 4: .bdrv_co_block_status (v3 is posted [2], mostly reviewed, but needs rebase)
Available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-status-v6
Based-on: <20171009215533.12530-1-mreitz@redhat.com>
([PATCH 0/3] qcow2: Fix preallocated truncation)
(or more precisely, based on Max' block branch that includes that
and other iotest patches, as I was hitting iotest failures without
that branch; however, I suspect that the dependency is weak enough
that this will apply just fine on master at the moment)
[1] https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00535.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg03812.html
Since v5:
- rebase to master, since copy-on-read patches (commit 46174339) that
landed there conflict with v5
- tweak qcow2 is_zero() to not round unnecessarily [Kevin]
- major interface overhaul: instead of returning a sector-based
offset in a 64-bit return type (which then requires post-call
munging to get to byte offsets), return a byte-based offset via
a parameter. Lots of churn in the interdiff, but the end result
looks better [Kevin]
- other minor fixes pointed out during review, such as -O2 compilation,
improved comments, ...
001/24:[0031] [FC] 'block: Allow NULL file for bdrv_get_block_status()'
002/24:[0029] [FC] 'block: Add flag to avoid wasted work in bdrv_is_allocated()'
003/24:[----] [--] 'block: Make bdrv_round_to_clusters() signature more useful'
004/24:[0005] [FC] 'qcow2: Switch is_zero_sectors() to byte-based'
005/24:[----] [--] 'block: Switch bdrv_make_zero() to byte-based'
006/24:[----] [--] 'qemu-img: Switch get_block_status() to byte-based'
007/24:[0048] [FC] 'block: Convert bdrv_get_block_status() to bytes'
008/24:[0105] [FC] 'block: Switch bdrv_co_get_block_status() to byte-based'
009/24:[0037] [FC] 'block: Switch BdrvCoGetBlockStatusData to byte-based'
010/24:[0036] [FC] 'block: Switch bdrv_common_block_status_above() to byte-based'
011/24:[0056] [FC] 'block: Switch bdrv_co_get_block_status_above() to byte-based'
012/24:[0066] [FC] 'block: Convert bdrv_get_block_status_above() to bytes'
013/24:[----] [-C] 'qemu-img: Simplify logic in img_compare()'
014/24:[0002] [FC] 'qemu-img: Speed up compare on pre-allocated larger file'
015/24:[----] [--] 'qemu-img: Add find_nonzero()'
016/24:[----] [--] 'qemu-img: Drop redundant error message in compare'
017/24:[----] [--] 'qemu-img: Change check_empty_sectors() to byte-based'
018/24:[----] [--] 'qemu-img: Change compare_sectors() to be byte-based'
019/24:[----] [--] 'qemu-img: Change img_rebase() to be byte-based'
020/24:[0014] [FC] 'qemu-img: Change img_compare() to be byte-based'
021/24:[0055] [FC] 'block: Align block status requests'
022/24:[----] [--] 'block: Relax bdrv_aligned_preadv() assertion'
023/24:[down] 'qcow2: Relax is_zero() assertion'
024/24:[----] [--] 'qemu-io: Relax 'alloc' now that block-status doesn't assert'
Eric Blake (24):
block: Allow NULL file for bdrv_get_block_status()
block: Add flag to avoid wasted work in bdrv_is_allocated()
block: Make bdrv_round_to_clusters() signature more useful
qcow2: Switch is_zero_sectors() to byte-based
block: Switch bdrv_make_zero() to byte-based
qemu-img: Switch get_block_status() to byte-based
block: Convert bdrv_get_block_status() to bytes
block: Switch bdrv_co_get_block_status() to byte-based
block: Switch BdrvCoGetBlockStatusData to byte-based
block: Switch bdrv_common_block_status_above() to byte-based
block: Switch bdrv_co_get_block_status_above() to byte-based
block: Convert bdrv_get_block_status_above() to bytes
qemu-img: Simplify logic in img_compare()
qemu-img: Speed up compare on pre-allocated larger file
qemu-img: Add find_nonzero()
qemu-img: Drop redundant error message in compare
qemu-img: Change check_empty_sectors() to byte-based
qemu-img: Change compare_sectors() to be byte-based
qemu-img: Change img_rebase() to be byte-based
qemu-img: Change img_compare() to be byte-based
block: Align block status requests
block: Relax bdrv_aligned_preadv() assertion
qcow2: Relax is_zero() assertion
qemu-io: Relax 'alloc' now that block-status doesn't assert
include/block/block.h | 29 ++--
include/block/block_int.h | 11 +-
block/io.c | 332 +++++++++++++++++++++++----------------
block/blkdebug.c | 13 +-
block/mirror.c | 26 ++--
block/qcow2-cluster.c | 2 +-
block/qcow2.c | 53 +++----
qemu-img.c | 381 ++++++++++++++++++++-------------------------
qemu-io-cmds.c | 13 --
block/trace-events | 2 +-
tests/qemu-iotests/074.out | 2 -
tests/qemu-iotests/177 | 12 +-
tests/qemu-iotests/177.out | 19 ++-
13 files changed, 459 insertions(+), 436 deletions(-)
--
2.13.6
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 01/24] block: Allow NULL file for bdrv_get_block_status()
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
@ 2017-10-12 3:46 ` Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 02/24] block: Add flag to avoid wasted work in bdrv_is_allocated() Eric Blake
` (23 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:46 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz, Jeff Cody
Not all callers care about which BDS owns the mapping for a given
range of the file. This patch merely simplifies the callers by
consolidating the logic in the common call point, while guaranteeing
a non-NULL file to all the driver callbacks, for no semantic change.
The only caller that does not care about pnum is bdrv_is_allocated,
as invoked by vvfat; we can likewise add assertions that the rest
of the stack does not have to worry about a NULL pnum.
Furthermore, this will also set the stage for a future cleanup: when
a caller does not care about which BDS owns an offset, it would be
nice to allow the driver to optimize things to not have to return
BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented
allocation (for example, it's fairly easy to create a qcow2 image
where consecutive guest addresses are not at consecutive host
addresses), the current contract requires bdrv_get_block_status()
to clamp *pnum to the limit where host addresses are no longer
consecutive, but allowing a NULL file means that *pnum could be
set to the full length of known-allocated data.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: drop local_pnum experiment, tweak label indent [Kevin]
v5: use second label for cleaner exit logic [John], use local_pnum
v4: only context changes
v3: rebase to recent changes (qcow2_measure), dropped R-b
v2: use local variable and final transfer, rather than assignment
of parameter to local
[previously in different series]:
v2: new patch, https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05645.html
---
include/block/block_int.h | 10 ++++++----
block/io.c | 49 ++++++++++++++++++++++++++---------------------
block/mirror.c | 3 +--
block/qcow2.c | 8 ++------
qemu-img.c | 10 ++++------
5 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7e8a206239..489b390fd6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -202,10 +202,12 @@ struct BlockDriver {
int64_t offset, int bytes);
/*
- * Building block for bdrv_block_status[_above]. The driver should
- * answer only according to the current layer, and should not
- * set BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
- * for the meaning of _DATA, _ZERO, and _OFFSET_VALID.
+ * Building block for bdrv_block_status[_above] and
+ * bdrv_is_allocated[_above]. The driver should answer only
+ * according to the current layer, and should not set
+ * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
+ * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. The block
+ * layer guarantees non-NULL pnum and file.
*/
int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *pnum,
diff --git a/block/io.c b/block/io.c
index 8e419070b5..51d2b219b8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -698,7 +698,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
{
int64_t target_sectors, ret, nb_sectors, sector_num = 0;
BlockDriverState *bs = child->bs;
- BlockDriverState *file;
int n;
target_sectors = bdrv_nb_sectors(bs);
@@ -711,7 +710,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
if (nb_sectors <= 0) {
return 0;
}
- ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
+ ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
if (ret < 0) {
error_report("error getting block status at sector %" PRId64 ": %s",
sector_num, strerror(-ret));
@@ -1800,8 +1799,9 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
* beyond the end of the disk image it will be clamped; if 'pnum' is set to
* the end of the image, then the returned value will include BDRV_BLOCK_EOF.
*
- * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
- * points to the BDS which the sector range is allocated in.
+ * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
+ * 'file' is non-NULL, then '*file' points to the BDS which the sector range
+ * is allocated in.
*/
static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
int64_t sector_num,
@@ -1811,20 +1811,23 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
int64_t total_sectors;
int64_t n;
int64_t ret, ret2;
+ BlockDriverState *local_file = NULL;
- *file = NULL;
+ assert(pnum);
+ *pnum = 0;
total_sectors = bdrv_nb_sectors(bs);
if (total_sectors < 0) {
- return total_sectors;
+ ret = total_sectors;
+ goto early_out;
}
if (sector_num >= total_sectors) {
- *pnum = 0;
- return BDRV_BLOCK_EOF;
+ ret = BDRV_BLOCK_EOF;
+ goto early_out;
}
if (!nb_sectors) {
- *pnum = 0;
- return 0;
+ ret = 0;
+ goto early_out;
}
n = total_sectors - sector_num;
@@ -1840,23 +1843,23 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
}
if (bs->drv->protocol_name) {
ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
- *file = bs;
+ local_file = bs;
}
- return ret;
+ goto early_out;
}
bdrv_inc_in_flight(bs);
ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
- file);
+ &local_file);
if (ret < 0) {
*pnum = 0;
goto out;
}
if (ret & BDRV_BLOCK_RAW) {
- assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
- ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
- *pnum, pnum, file);
+ assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);
+ ret = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,
+ *pnum, pnum, &local_file);
goto out;
}
@@ -1874,14 +1877,13 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
}
}
- if (*file && *file != bs &&
+ if (local_file && local_file != bs &&
(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
(ret & BDRV_BLOCK_OFFSET_VALID)) {
- BlockDriverState *file2;
int file_pnum;
- ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
- *pnum, &file_pnum, &file2);
+ ret2 = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,
+ *pnum, &file_pnum, NULL);
if (ret2 >= 0) {
/* Ignore errors. This is just providing extra information, it
* is useful but not necessary.
@@ -1907,6 +1909,10 @@ out:
if (ret >= 0 && sector_num + *pnum == total_sectors) {
ret |= BDRV_BLOCK_EOF;
}
+early_out:
+ if (file) {
+ *file = local_file;
+ }
return ret;
}
@@ -2006,7 +2012,6 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum)
{
- BlockDriverState *file;
int64_t sector_num = offset >> BDRV_SECTOR_BITS;
int nb_sectors = bytes >> BDRV_SECTOR_BITS;
int64_t ret;
@@ -2015,7 +2020,7 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE) && bytes < INT_MAX);
ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &psectors,
- &file);
+ NULL);
if (ret < 0) {
return ret;
}
diff --git a/block/mirror.c b/block/mirror.c
index 153758ca9f..e76e754d26 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -390,7 +390,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
int io_sectors;
unsigned int io_bytes;
int64_t io_bytes_acct;
- BlockDriverState *file;
enum MirrorMethod {
MIRROR_METHOD_COPY,
MIRROR_METHOD_ZERO,
@@ -401,7 +400,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
ret = bdrv_get_block_status_above(source, NULL,
offset >> BDRV_SECTOR_BITS,
nb_chunks * sectors_per_chunk,
- &io_sectors, &file);
+ &io_sectors, NULL);
io_bytes = io_sectors * BDRV_SECTOR_SIZE;
if (ret < 0) {
io_bytes = MIN(nb_chunks * s->granularity, max_io_bytes);
diff --git a/block/qcow2.c b/block/qcow2.c
index d742b146da..990917096f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3003,7 +3003,6 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
uint32_t count)
{
int nr;
- BlockDriverState *file;
int64_t res;
if (start + count > bs->total_sectors) {
@@ -3013,8 +3012,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
if (!count) {
return true;
}
- res = bdrv_get_block_status_above(bs, NULL, start, count,
- &nr, &file);
+ res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL);
return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
}
@@ -3732,13 +3730,11 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
offset += pnum * BDRV_SECTOR_SIZE) {
int nb_sectors = MIN(ssize - offset,
BDRV_REQUEST_MAX_BYTES) / BDRV_SECTOR_SIZE;
- BlockDriverState *file;
int64_t ret;
ret = bdrv_get_block_status_above(in_bs, NULL,
offset >> BDRV_SECTOR_BITS,
- nb_sectors,
- &pnum, &file);
+ nb_sectors, &pnum, NULL);
if (ret < 0) {
error_setg_errno(&local_err, -ret,
"Unable to get block status");
diff --git a/qemu-img.c b/qemu-img.c
index d6007b2a6d..e9c7b30c91 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1375,7 +1375,6 @@ static int img_compare(int argc, char **argv)
for (;;) {
int64_t status1, status2;
- BlockDriverState *file;
nb_sectors = sectors_to_process(total_sectors, sector_num);
if (nb_sectors <= 0) {
@@ -1383,7 +1382,7 @@ static int img_compare(int argc, char **argv)
}
status1 = bdrv_get_block_status_above(bs1, NULL, sector_num,
total_sectors1 - sector_num,
- &pnum1, &file);
+ &pnum1, NULL);
if (status1 < 0) {
ret = 3;
error_report("Sector allocation test failed for %s", filename1);
@@ -1393,7 +1392,7 @@ static int img_compare(int argc, char **argv)
status2 = bdrv_get_block_status_above(bs2, NULL, sector_num,
total_sectors2 - sector_num,
- &pnum2, &file);
+ &pnum2, NULL);
if (status2 < 0) {
ret = 3;
error_report("Sector allocation test failed for %s", filename2);
@@ -1599,15 +1598,14 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
if (s->sector_next_status <= sector_num) {
- BlockDriverState *file;
if (s->target_has_backing) {
ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
sector_num - src_cur_offset,
- n, &n, &file);
+ n, &n, NULL);
} else {
ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
sector_num - src_cur_offset,
- n, &n, &file);
+ n, &n, NULL);
}
if (ret < 0) {
return ret;
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 02/24] block: Add flag to avoid wasted work in bdrv_is_allocated()
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 01/24] block: Allow NULL file for bdrv_get_block_status() Eric Blake
@ 2017-10-12 3:46 ` Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 03/24] block: Make bdrv_round_to_clusters() signature more useful Eric Blake
` (22 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:46 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
Not all callers care about which BDS owns the mapping for a given
range of the file, or where the zeroes lie within that mapping. In
particular, bdrv_is_allocated() cares more about finding the
largest run of allocated data from the guest perspective, whether
or not that data is consecutive from the host perspective, and
whether or not the data reads as zero. Therefore, doing subsequent
refinements such as checking how much of the format-layer
allocation also satisfies BDRV_BLOCK_ZERO at the protocol layer is
wasted work - in the best case, it just costs extra CPU cycles
during a single bdrv_is_allocated(), but in the worst case, it
results in a smaller *pnum, and forces callers to iterate through
more status probes when visiting the entire file for even more
extra CPU cycles.
This patch only optimizes the block layer (no behavior change when
want_zero is true, but skip unnecessary effort when it is false).
Then when subsequent patches tweak the driver callback to be
byte-based, we can also pass this hint through to the driver.
Tweak BdrvCoGetBlockStatusData to declare arguments in parameter
order, rather than mixing things up (minimizing padding is not
necessary here).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to previous changes, s/mapping/need_zero/, tweak struct
layout, drop R-b
v5: tweak commit message and one comment, rebase to previous changes,
minor enough to still add R-b
v4: only context changes
v3: s/allocation/mapping/ and flip sense of bool
v2: new patch
---
block/io.c | 57 +++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 16 deletions(-)
diff --git a/block/io.c b/block/io.c
index 51d2b219b8..0c87e9d8b3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1749,10 +1749,11 @@ int bdrv_flush_all(void)
typedef struct BdrvCoGetBlockStatusData {
BlockDriverState *bs;
BlockDriverState *base;
- BlockDriverState **file;
+ bool want_zero;
int64_t sector_num;
int nb_sectors;
int *pnum;
+ BlockDriverState **file;
int64_t ret;
bool done;
} BdrvCoGetBlockStatusData;
@@ -1788,6 +1789,11 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
* Drivers not implementing the functionality are assumed to not support
* backing files, hence all their sectors are reported as allocated.
*
+ * If 'want_zero' is true, the caller is querying for mapping purposes,
+ * and the result should include BDRV_BLOCK_OFFSET_VALID and
+ * BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those
+ * bits particularly if it allows for a larger value in 'pnum'.
+ *
* If 'sector_num' is beyond the end of the disk image the return value is
* BDRV_BLOCK_EOF and 'pnum' is set to 0.
*
@@ -1804,6 +1810,7 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
* is allocated in.
*/
static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
+ bool want_zero,
int64_t sector_num,
int nb_sectors, int *pnum,
BlockDriverState **file)
@@ -1858,31 +1865,34 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
if (ret & BDRV_BLOCK_RAW) {
assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);
- ret = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,
+ ret = bdrv_co_get_block_status(local_file, want_zero,
+ ret >> BDRV_SECTOR_BITS,
*pnum, pnum, &local_file);
goto out;
}
if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
ret |= BDRV_BLOCK_ALLOCATED;
- } else {
+ } else if (want_zero) {
if (bdrv_unallocated_blocks_are_zero(bs)) {
ret |= BDRV_BLOCK_ZERO;
} else if (bs->backing) {
BlockDriverState *bs2 = bs->backing->bs;
int64_t nb_sectors2 = bdrv_nb_sectors(bs2);
+
if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
ret |= BDRV_BLOCK_ZERO;
}
}
}
- if (local_file && local_file != bs &&
+ if (want_zero && local_file && local_file != bs &&
(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
(ret & BDRV_BLOCK_OFFSET_VALID)) {
int file_pnum;
- ret2 = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,
+ ret2 = bdrv_co_get_block_status(local_file, want_zero,
+ ret >> BDRV_SECTOR_BITS,
*pnum, &file_pnum, NULL);
if (ret2 >= 0) {
/* Ignore errors. This is just providing extra information, it
@@ -1918,6 +1928,7 @@ early_out:
static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
+ bool want_zero,
int64_t sector_num,
int nb_sectors,
int *pnum,
@@ -1929,7 +1940,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
assert(bs != base);
for (p = bs; p != base; p = backing_bs(p)) {
- ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
+ ret = bdrv_co_get_block_status(p, want_zero, sector_num, nb_sectors,
+ pnum, file);
if (ret < 0) {
break;
}
@@ -1959,6 +1971,7 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
BdrvCoGetBlockStatusData *data = opaque;
data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
+ data->want_zero,
data->sector_num,
data->nb_sectors,
data->pnum,
@@ -1971,20 +1984,22 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
*
* See bdrv_co_get_block_status_above() for details.
*/
-int64_t bdrv_get_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero,
+ int64_t sector_num,
+ int nb_sectors, int *pnum,
+ BlockDriverState **file)
{
Coroutine *co;
BdrvCoGetBlockStatusData data = {
.bs = bs,
.base = base,
- .file = file,
+ .want_zero = want_zero,
.sector_num = sector_num,
.nb_sectors = nb_sectors,
.pnum = pnum,
+ .file = file,
.done = false,
};
@@ -2000,6 +2015,16 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
return data.ret;
}
+int64_t bdrv_get_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ int64_t sector_num,
+ int nb_sectors, int *pnum,
+ BlockDriverState **file)
+{
+ return bdrv_common_block_status_above(bs, base, true, sector_num,
+ nb_sectors, pnum, file);
+}
+
int64_t bdrv_get_block_status(BlockDriverState *bs,
int64_t sector_num,
int nb_sectors, int *pnum,
@@ -2012,15 +2037,15 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum)
{
- int64_t sector_num = offset >> BDRV_SECTOR_BITS;
- int nb_sectors = bytes >> BDRV_SECTOR_BITS;
int64_t ret;
int psectors;
assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE) && bytes < INT_MAX);
- ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &psectors,
- NULL);
+ ret = bdrv_common_block_status_above(bs, backing_bs(bs), false,
+ offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS, &psectors,
+ NULL);
if (ret < 0) {
return ret;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 03/24] block: Make bdrv_round_to_clusters() signature more useful
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 01/24] block: Allow NULL file for bdrv_get_block_status() Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 02/24] block: Add flag to avoid wasted work in bdrv_is_allocated() Eric Blake
@ 2017-10-12 3:46 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 04/24] qcow2: Switch is_zero_sectors() to byte-based Eric Blake
` (21 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:46 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz, Jeff Cody
In the process of converting sector-based interfaces to bytes,
I'm finding it easier to represent a byte count as a 64-bit
integer at the block layer (even if we are internally capped
by SIZE_MAX or even INT_MAX for individual transactions, it's
still nicer to not have to worry about truncation/overflow
issues on as many variables). Update the signature of
bdrv_round_to_clusters() to uniformly use int64_t, matching
the signature already chosen for bdrv_is_allocated and the
fact that off_t is also a signed type, then adjust clients
according to the required fallout (even where the result could
now exceed 32 bits, no client is directly assigning the result
into a 32-bit value without breaking things into a loop first).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v5: depends on copy-on-read fixes [John], fix incorrect trace, update
commit message to document rounding considerations, drop R-b
v4: only context changes
v3: no change
v2: fix commit message [John], rebase to earlier changes, including
mirror_clip_bytes() signature update
---
include/block/block.h | 4 ++--
block/io.c | 6 +++---
block/mirror.c | 7 +++----
block/trace-events | 2 +-
4 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index d5c2731a03..440f3e9e39 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -474,9 +474,9 @@ int bdrv_get_flags(BlockDriverState *bs);
int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
void bdrv_round_to_clusters(BlockDriverState *bs,
- int64_t offset, unsigned int bytes,
+ int64_t offset, int64_t bytes,
int64_t *cluster_offset,
- unsigned int *cluster_bytes);
+ int64_t *cluster_bytes);
const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
void bdrv_get_backing_filename(BlockDriverState *bs,
diff --git a/block/io.c b/block/io.c
index 0c87e9d8b3..20b4cb456c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -449,9 +449,9 @@ static void mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
* Round a region to cluster boundaries
*/
void bdrv_round_to_clusters(BlockDriverState *bs,
- int64_t offset, unsigned int bytes,
+ int64_t offset, int64_t bytes,
int64_t *cluster_offset,
- unsigned int *cluster_bytes)
+ int64_t *cluster_bytes)
{
BlockDriverInfo bdi;
@@ -949,7 +949,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
struct iovec iov;
QEMUIOVector local_qiov;
int64_t cluster_offset;
- unsigned int cluster_bytes;
+ int64_t cluster_bytes;
size_t skip_bytes;
int ret;
int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
diff --git a/block/mirror.c b/block/mirror.c
index e76e754d26..d11706c566 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -190,10 +190,9 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
bool need_cow;
int ret = 0;
int64_t align_offset = *offset;
- unsigned int align_bytes = *bytes;
+ int64_t align_bytes = *bytes;
int max_bytes = s->granularity * s->max_iov;
- assert(*bytes < INT_MAX);
need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
s->cow_bitmap);
@@ -388,7 +387,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
while (nb_chunks > 0 && offset < s->bdev_length) {
int64_t ret;
int io_sectors;
- unsigned int io_bytes;
+ int64_t io_bytes;
int64_t io_bytes_acct;
enum MirrorMethod {
MIRROR_METHOD_COPY,
@@ -413,7 +412,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
io_bytes = s->granularity;
} else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
int64_t target_offset;
- unsigned int target_bytes;
+ int64_t target_bytes;
bdrv_round_to_clusters(blk_bs(s->target), offset, io_bytes,
&target_offset, &target_bytes);
if (target_offset == offset &&
diff --git a/block/trace-events b/block/trace-events
index 25dd5a3026..11c8d5f590 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -12,7 +12,7 @@ blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flag
bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
-bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, unsigned int cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %u"
+bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64
# block/stream.c
stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 04/24] qcow2: Switch is_zero_sectors() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (2 preceding siblings ...)
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 03/24] block: Make bdrv_round_to_clusters() signature more useful Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 05/24] block: Switch bdrv_make_zero() " Eric Blake
` (20 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert another internal
function (no semantic change), and rename it to is_zero() in the
process.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v6: tweak comments to add TODO for later in series [Kevin], minor enough
to keep R-b
v3-v5: no change
v2: rename function, rebase to upstream changes
---
block/qcow2.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 990917096f..df53535455 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2999,21 +2999,29 @@ finish:
}
-static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
- uint32_t count)
+static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
int nr;
int64_t res;
+ int64_t start;
- if (start + count > bs->total_sectors) {
- count = bs->total_sectors - start;
+ /* TODO: Widening to sector boundaries should only be needed as
+ * long as we can't query finer granularity. */
+ start = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
+ bytes = QEMU_ALIGN_UP(offset + bytes, BDRV_SECTOR_SIZE) - start;
+
+ /* Clamp to image length, before checking status of underlying sectors */
+ if (start + bytes > bs->total_sectors * BDRV_SECTOR_SIZE) {
+ bytes = bs->total_sectors * BDRV_SECTOR_SIZE - start;
}
- if (!count) {
+ if (!bytes) {
return true;
}
- res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL);
- return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
+ res = bdrv_get_block_status_above(bs, NULL, start >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS, &nr, NULL);
+ return res >= 0 && (res & BDRV_BLOCK_ZERO) &&
+ nr * BDRV_SECTOR_SIZE == bytes;
}
static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
@@ -3031,24 +3039,21 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
}
if (head || tail) {
- int64_t cl_start = (offset - head) >> BDRV_SECTOR_BITS;
uint64_t off;
unsigned int nr;
assert(head + bytes <= s->cluster_size);
/* check whether remainder of cluster already reads as zero */
- if (!(is_zero_sectors(bs, cl_start,
- DIV_ROUND_UP(head, BDRV_SECTOR_SIZE)) &&
- is_zero_sectors(bs, (offset + bytes) >> BDRV_SECTOR_BITS,
- DIV_ROUND_UP(-tail & (s->cluster_size - 1),
- BDRV_SECTOR_SIZE)))) {
+ if (!(is_zero(bs, offset - head, head) &&
+ is_zero(bs, offset + bytes,
+ tail ? s->cluster_size - tail : 0))) {
return -ENOTSUP;
}
qemu_co_mutex_lock(&s->lock);
/* We can have new write after previous check */
- offset = cl_start << BDRV_SECTOR_BITS;
+ offset = QEMU_ALIGN_DOWN(offset, s->cluster_size);
bytes = s->cluster_size;
nr = s->cluster_size;
ret = qcow2_get_cluster_offset(bs, offset, &nr, &off);
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 05/24] block: Switch bdrv_make_zero() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (3 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 04/24] qcow2: Switch is_zero_sectors() to byte-based Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 06/24] qemu-img: Switch get_block_status() " Eric Blake
` (19 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Change the internal
loop iteration of zeroing a device to track by bytes instead of
sectors (although we are still guaranteed that we iterate by steps
that are sector-aligned).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v3-v5: no change
v2: rebase to earlier changes
---
block/io.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/block/io.c b/block/io.c
index 20b4cb456c..e9604f0d00 100644
--- a/block/io.c
+++ b/block/io.c
@@ -696,38 +696,38 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
*/
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
{
- int64_t target_sectors, ret, nb_sectors, sector_num = 0;
+ int64_t target_size, ret, bytes, offset = 0;
BlockDriverState *bs = child->bs;
- int n;
+ int n; /* sectors */
- target_sectors = bdrv_nb_sectors(bs);
- if (target_sectors < 0) {
- return target_sectors;
+ target_size = bdrv_getlength(bs);
+ if (target_size < 0) {
+ return target_size;
}
for (;;) {
- nb_sectors = MIN(target_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
- if (nb_sectors <= 0) {
+ bytes = MIN(target_size - offset, BDRV_REQUEST_MAX_BYTES);
+ if (bytes <= 0) {
return 0;
}
- ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
+ ret = bdrv_get_block_status(bs, offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS, &n, NULL);
if (ret < 0) {
- error_report("error getting block status at sector %" PRId64 ": %s",
- sector_num, strerror(-ret));
+ error_report("error getting block status at offset %" PRId64 ": %s",
+ offset, strerror(-ret));
return ret;
}
if (ret & BDRV_BLOCK_ZERO) {
- sector_num += n;
+ offset += n * BDRV_SECTOR_BITS;
continue;
}
- ret = bdrv_pwrite_zeroes(child, sector_num << BDRV_SECTOR_BITS,
- n << BDRV_SECTOR_BITS, flags);
+ ret = bdrv_pwrite_zeroes(child, offset, n * BDRV_SECTOR_SIZE, flags);
if (ret < 0) {
- error_report("error writing zeroes at sector %" PRId64 ": %s",
- sector_num, strerror(-ret));
+ error_report("error writing zeroes at offset %" PRId64 ": %s",
+ offset, strerror(-ret));
return ret;
}
- sector_num += n;
+ offset += n * BDRV_SECTOR_SIZE;
}
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 06/24] qemu-img: Switch get_block_status() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (4 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 05/24] block: Switch bdrv_make_zero() " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes Eric Blake
` (18 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Continue by converting
an internal function (no semantic change), and simplifying its
caller accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v2-v5: no change
---
qemu-img.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index e9c7b30c91..af3effdec5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2671,14 +2671,16 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
}
}
-static int get_block_status(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, MapEntry *e)
+static int get_block_status(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, MapEntry *e)
{
int64_t ret;
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.
@@ -2686,8 +2688,8 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
depth = 0;
for (;;) {
- ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &nb_sectors,
- &file);
+ ret = bdrv_get_block_status(bs, offset >> BDRV_SECTOR_BITS, nb_sectors,
+ &nb_sectors, &file);
if (ret < 0) {
return ret;
}
@@ -2707,7 +2709,7 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
*e = (MapEntry) {
- .start = sector_num * BDRV_SECTOR_SIZE,
+ .start = offset,
.length = nb_sectors * BDRV_SECTOR_SIZE,
.data = !!(ret & BDRV_BLOCK_DATA),
.zero = !!(ret & BDRV_BLOCK_ZERO),
@@ -2837,16 +2839,12 @@ static int img_map(int argc, char **argv)
length = blk_getlength(blk);
while (curr.start + curr.length < length) {
- int64_t nsectors_left;
- int64_t sector_num;
- int n;
-
- sector_num = (curr.start + curr.length) >> BDRV_SECTOR_BITS;
+ int64_t offset = curr.start + curr.length;
+ int64_t n;
/* Probe up to 1 GiB at a time. */
- nsectors_left = DIV_ROUND_UP(length, BDRV_SECTOR_SIZE) - sector_num;
- n = MIN(1 << (30 - BDRV_SECTOR_BITS), nsectors_left);
- ret = get_block_status(bs, sector_num, n, &next);
+ n = QEMU_ALIGN_DOWN(MIN(1 << 30, length - offset), BDRV_SECTOR_SIZE);
+ ret = get_block_status(bs, offset, n, &next);
if (ret < 0) {
error_report("Could not read file metadata: %s", strerror(-ret));
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (5 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 06/24] qemu-img: Switch get_block_status() " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-20 14:31 ` Kevin Wolf
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 08/24] block: Switch bdrv_co_get_block_status() to byte-based Eric Blake
` (17 subsequent siblings)
24 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
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.
There was an inherent limitation in returning the offset via the
return value: we only have room for BDRV_BLOCK_OFFSET_MASK bits, which
means an offset can only be mapped for sector-aligned queries (or,
if we declare that non-aligned input is at the same relative position
modulo 512 of the answer), so the new interface also changes things to
return the offset via output through a parameter by reference rather
than mashed into the return value. We'll have some glue code that
munges between the two styles until we finish converting all uses.
For the most part this patch is just the addition of scaling at the
callers followed by inverse scaling at bdrv_block_status(), coupled
with the tweak in calling convention. 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 <eblake@redhat.com>
---
v6: redo interface to return byte-based offset by reference [Kevin]
v5: drop pointless 'if (pnum)' [John], add comment
v4: no change
v3: clamp bytes to 32-bits, rather than asserting
v2: rebase to earlier changes
---
include/block/block.h | 17 +++++++++--------
block/io.c | 47 ++++++++++++++++++++++++++++++++++-------------
block/qcow2-cluster.c | 2 +-
qemu-img.c | 25 ++++++++++++++-----------
4 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 440f3e9e39..7ac851f82f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -121,7 +121,7 @@ typedef struct HDGeometry {
#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
/*
- * Allocation status flags for bdrv_get_block_status() and friends.
+ * Allocation status flags for bdrv_block_status() and friends.
*
* Public flags:
* BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer
@@ -136,10 +136,11 @@ typedef struct HDGeometry {
* that the block layer recompute the answer from the returned
* BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
*
- * 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:
+ * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) of
+ * the return value (old interface) or the entire map parameter (new
+ * interface) 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, as follows:
*
* DATA ZERO OFFSET_VALID
* t t t sectors read as zero, returned file is zero at offset
@@ -421,9 +422,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);
+int bdrv_block_status(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, int64_t *pnum, int64_t *map,
+ 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 e9604f0d00..d543e1038f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -696,9 +696,9 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
*/
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
{
- int64_t target_size, ret, bytes, offset = 0;
+ int ret;
+ int64_t target_size, bytes, offset = 0;
BlockDriverState *bs = child->bs;
- int n; /* sectors */
target_size = bdrv_getlength(bs);
if (target_size < 0) {
@@ -710,24 +710,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, 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;
}
}
@@ -2025,13 +2024,35 @@ 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)
+int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map, 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(pnum);
+ /*
+ * The contract allows us to return pnum smaller than bytes, even
+ * if the next query would see the same status; we truncate the
+ * request to avoid overflowing the driver's 32-bit interface.
+ */
+ 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 (ret < 0) {
+ assert(INT_MIN <= ret);
+ *pnum = 0;
+ return ret;
+ }
+ *pnum = n * BDRV_SECTOR_SIZE;
+ if (map) {
+ *map = ret & BDRV_BLOCK_OFFSET_MASK;
+ } else {
+ ret &= ~BDRV_BLOCK_OFFSET_VALID;
+ }
+ return ret & ~BDRV_BLOCK_OFFSET_MASK;
}
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 0e5aec81cb..fb10e26068 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1632,7 +1632,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 af3effdec5..c81d6ce733 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1599,9 +1599,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, 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,
@@ -2674,13 +2679,12 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
static int get_block_status(BlockDriverState *bs, int64_t offset,
int64_t bytes, MapEntry *e)
{
- int64_t ret;
+ int ret;
int depth;
BlockDriverState *file;
bool has_offset;
- int nb_sectors = bytes >> BDRV_SECTOR_BITS;
+ int64_t map;
- 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.
@@ -2688,12 +2692,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, &map, &file);
if (ret < 0) {
return ret;
}
- assert(nb_sectors);
+ assert(bytes);
if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
break;
}
@@ -2710,10 +2713,10 @@ 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,
+ .offset = map,
.has_offset = has_offset,
.depth = depth,
.has_filename = file && has_offset,
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 08/24] block: Switch bdrv_co_get_block_status() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (6 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 09/24] block: Switch BdrvCoGetBlockStatusData " Eric Blake
` (16 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert another internal
function (no semantic change); and as with its public counterpart,
rename to bdrv_co_block_status() and split the offset return, to
make the compiler enforce that we catch all uses. For now, we
assert that callers and the return value still use aligned data,
but ultimately, this will be the function where we hand off to a
byte-based driver callback, and will eventually need to add logic
to ensure we round calls according to the driver's
request_alignment then touch up the result handed back to the
caller, to start permitting a caller to pass unaligned offsets.
Note that we are now prepared to accepts 'bytes' larger than INT_MAX;
this is okay as long as we clamp things internally before violating
any 32-bit limits, and makes no difference to how a client will
use the information (clients looping over the entire file must
already be prepared for consecutive calls to return the same status,
as drivers are already free to return shorter-than-maximal status
due to any other convenient split points, such as when the L2 table
crosses cluster boundaries in qcow2).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to return value interface change [Kevin]
v5: rebase to earlier changes in 1/23, add comment
v4: no change
v3: rebase to allocation/mapping sense change, clamp bytes to 32-bits
when needed, drop R-b
v2: rebase to earlier changes
---
block/io.c | 122 ++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 80 insertions(+), 42 deletions(-)
diff --git a/block/io.c b/block/io.c
index d543e1038f..5805929ef0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1793,80 +1793,102 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
* BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those
* bits particularly if it allows for a larger value in 'pnum'.
*
- * If 'sector_num' is beyond the end of the disk image the return value is
+ * If 'offset' is beyond the end of the disk image the return value is
* BDRV_BLOCK_EOF and 'pnum' is set to 0.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
- * allocated/unallocated state.
- *
- * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes
+ * 'bytes' is the max value 'pnum' should be set to. If bytes goes
* beyond the end of the disk image it will be clamped; if 'pnum' is set to
* the end of the image, then the returned value will include BDRV_BLOCK_EOF.
*
- * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
- * 'file' is non-NULL, then '*file' points to the BDS which the sector range
- * is allocated in.
+ * 'pnum' is set to the number of bytes (including and immediately
+ * following the specified offset) that are easily known to be in the
+ * same allocated/unallocated state. Note that a second call starting
+ * at the original offset plus returned pnum may have the same status.
+ * The returned value is non-zero on success except at end-of-file.
+ *
+ * Returns negative errno on failure. Otherwise, if the
+ * BDRV_BLOCK_OFFSET_VALID bit is set, 'map' and 'file' (if non-NULL) are
+ * set to the host mapping and BDS corresponding to the guest offset.
*/
-static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
- bool want_zero,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
- int64_t total_sectors;
- int64_t n;
- int64_t ret, ret2;
+ int64_t total_size;
+ int64_t n; /* bytes */
+ int64_t ret;
+ int64_t local_map = 0;
BlockDriverState *local_file = NULL;
+ int count; /* sectors */
assert(pnum);
*pnum = 0;
- total_sectors = bdrv_nb_sectors(bs);
- if (total_sectors < 0) {
- ret = total_sectors;
+ total_size = bdrv_getlength(bs);
+ if (total_size < 0) {
+ ret = total_size;
goto early_out;
}
- if (sector_num >= total_sectors) {
+ if (offset >= total_size) {
ret = BDRV_BLOCK_EOF;
goto early_out;
}
- if (!nb_sectors) {
+ if (!bytes) {
ret = 0;
goto early_out;
}
- n = total_sectors - sector_num;
- if (n < nb_sectors) {
- nb_sectors = n;
+ n = total_size - offset;
+ if (n < bytes) {
+ bytes = n;
}
if (!bs->drv->bdrv_co_get_block_status) {
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
- if (sector_num + nb_sectors == total_sectors) {
+ if (offset + bytes == total_size) {
ret |= BDRV_BLOCK_EOF;
}
if (bs->drv->protocol_name) {
- ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+ ret |= BDRV_BLOCK_OFFSET_VALID;
+ local_map = offset;
local_file = bs;
}
goto early_out;
}
bdrv_inc_in_flight(bs);
- ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
+ /*
+ * TODO: Rather than require aligned offsets, we could instead
+ * round to the driver's request_alignment here, then touch up
+ * count afterwards back to the caller's expectations.
+ */
+ assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
+ /*
+ * The contract allows us to return pnum smaller than bytes, even
+ * if the next query would see the same status; we truncate the
+ * request to avoid overflowing the driver's 32-bit interface.
+ */
+ bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
+ ret = bs->drv->bdrv_co_get_block_status(bs, offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS, &count,
&local_file);
if (ret < 0) {
- *pnum = 0;
goto out;
}
+ if (ret & BDRV_BLOCK_OFFSET_VALID) {
+ local_map = ret & BDRV_BLOCK_OFFSET_MASK;
+ }
+ *pnum = count * BDRV_SECTOR_SIZE;
if (ret & BDRV_BLOCK_RAW) {
assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);
- ret = bdrv_co_get_block_status(local_file, want_zero,
- ret >> BDRV_SECTOR_BITS,
- *pnum, pnum, &local_file);
+ ret = bdrv_co_block_status(local_file, want_zero, local_map,
+ *pnum, pnum, &local_map, &local_file);
+ assert(ret < 0 ||
+ QEMU_IS_ALIGNED(*pnum | local_map, BDRV_SECTOR_SIZE));
goto out;
}
@@ -1877,9 +1899,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
ret |= BDRV_BLOCK_ZERO;
} else if (bs->backing) {
BlockDriverState *bs2 = bs->backing->bs;
- int64_t nb_sectors2 = bdrv_nb_sectors(bs2);
+ int64_t size2 = bdrv_getlength(bs2);
- if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
+ if (size2 >= 0 && offset >= size2) {
ret |= BDRV_BLOCK_ZERO;
}
}
@@ -1888,11 +1910,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
if (want_zero && local_file && local_file != bs &&
(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
(ret & BDRV_BLOCK_OFFSET_VALID)) {
- int file_pnum;
+ int64_t file_pnum;
+ int ret2;
- ret2 = bdrv_co_get_block_status(local_file, want_zero,
- ret >> BDRV_SECTOR_BITS,
- *pnum, &file_pnum, NULL);
+ ret2 = bdrv_co_block_status(local_file, want_zero, local_map,
+ *pnum, &file_pnum, NULL, NULL);
if (ret2 >= 0) {
/* Ignore errors. This is just providing extra information, it
* is useful but not necessary.
@@ -1915,13 +1937,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
out:
bdrv_dec_in_flight(bs);
- if (ret >= 0 && sector_num + *pnum == total_sectors) {
+ if (ret >= 0 && offset + *pnum == total_size) {
ret |= BDRV_BLOCK_EOF;
}
early_out:
if (file) {
*file = local_file;
}
+ if (map) {
+ *map = local_map;
+ }
+ if (ret >= 0) {
+ ret &= ~BDRV_BLOCK_OFFSET_MASK;
+ } else {
+ assert(INT_MIN <= ret);
+ }
return ret;
}
@@ -1936,14 +1966,22 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
BlockDriverState *p;
int64_t ret = 0;
bool first = true;
+ int64_t map = 0;
assert(bs != base);
for (p = bs; p != base; p = backing_bs(p)) {
- ret = bdrv_co_get_block_status(p, want_zero, sector_num, nb_sectors,
- pnum, file);
+ int64_t count;
+
+ ret = bdrv_co_block_status(p, want_zero,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE, &count,
+ &map, file);
if (ret < 0) {
break;
}
+ assert(QEMU_IS_ALIGNED(count | map, BDRV_SECTOR_SIZE));
+ ret |= map;
+ *pnum = count >> BDRV_SECTOR_BITS;
if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) {
/*
* Reading beyond the end of the file continues to read
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 09/24] block: Switch BdrvCoGetBlockStatusData to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (7 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 08/24] block: Switch bdrv_co_get_block_status() to byte-based Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 10/24] block: Switch bdrv_common_block_status_above() " Eric Blake
` (15 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert another internal
type (no semantic change), and rename it to match the corresponding
public function rename.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: fix -O2 compilation [Jeff], rebase to split return interface
change [Kevin], drop R-b
v4-v5: no change
v3: rebase to context conflicts, simple enough to keep R-b
v2: rebase to earlier changes
---
block/io.c | 56 ++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 38 insertions(+), 18 deletions(-)
diff --git a/block/io.c b/block/io.c
index 5805929ef0..da600a8dc3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1745,17 +1745,18 @@ int bdrv_flush_all(void)
}
-typedef struct BdrvCoGetBlockStatusData {
+typedef struct BdrvCoBlockStatusData {
BlockDriverState *bs;
BlockDriverState *base;
bool want_zero;
- int64_t sector_num;
- int nb_sectors;
- int *pnum;
+ int64_t offset;
+ int64_t bytes;
+ int64_t *pnum;
+ int64_t *map;
BlockDriverState **file;
- int64_t ret;
+ int ret;
bool done;
-} BdrvCoGetBlockStatusData;
+} BdrvCoBlockStatusData;
int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
int64_t sector_num,
@@ -2005,14 +2006,24 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
/* Coroutine wrapper for bdrv_get_block_status_above() */
static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
{
- BdrvCoGetBlockStatusData *data = opaque;
+ BdrvCoBlockStatusData *data = opaque;
+ int n = 0;
+ int64_t ret;
- data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
- data->want_zero,
- data->sector_num,
- data->nb_sectors,
- data->pnum,
- data->file);
+ ret = bdrv_co_get_block_status_above(data->bs, data->base,
+ data->want_zero,
+ data->offset >> BDRV_SECTOR_BITS,
+ data->bytes >> BDRV_SECTOR_BITS,
+ &n,
+ data->file);
+ if (ret < 0) {
+ assert(INT_MIN <= ret);
+ data->ret = ret;
+ } else {
+ *data->pnum = n * BDRV_SECTOR_SIZE;
+ *data->map = ret & BDRV_BLOCK_OFFSET_MASK;
+ data->ret = ret & ~BDRV_BLOCK_OFFSET_MASK;
+ }
data->done = true;
}
@@ -2029,13 +2040,16 @@ static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
BlockDriverState **file)
{
Coroutine *co;
- BdrvCoGetBlockStatusData data = {
+ int64_t n;
+ int64_t map;
+ BdrvCoBlockStatusData data = {
.bs = bs,
.base = base,
.want_zero = want_zero,
- .sector_num = sector_num,
- .nb_sectors = nb_sectors,
- .pnum = pnum,
+ .offset = sector_num * BDRV_SECTOR_SIZE,
+ .bytes = nb_sectors * BDRV_SECTOR_SIZE,
+ .pnum = &n,
+ .map = &map,
.file = file,
.done = false,
};
@@ -2049,7 +2063,13 @@ static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
bdrv_coroutine_enter(bs, co);
BDRV_POLL_WHILE(bs, !data.done);
}
- return data.ret;
+ if (data.ret < 0) {
+ *pnum = 0;
+ return data.ret;
+ }
+ assert(QEMU_IS_ALIGNED(n | map, BDRV_SECTOR_SIZE));
+ *pnum = n >> BDRV_SECTOR_BITS;
+ return data.ret | map;
}
int64_t bdrv_get_block_status_above(BlockDriverState *bs,
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 10/24] block: Switch bdrv_common_block_status_above() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (8 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 09/24] block: Switch BdrvCoGetBlockStatusData " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 11/24] block: Switch bdrv_co_get_block_status_above() " Eric Blake
` (14 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert another internal
function (no semantic change).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to split return interface change [Kevin], R-b dropped
v4-v5: no change
v3: rebase to allocation/mapping sense change, simple enough to keep R-b
v2: new patch
---
block/io.c | 61 ++++++++++++++++++++++++++++++-------------------------------
1 file changed, 30 insertions(+), 31 deletions(-)
diff --git a/block/io.c b/block/io.c
index da600a8dc3..e8fda1a4b8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2032,24 +2032,22 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
*
* See bdrv_co_get_block_status_above() for details.
*/
-static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- bool want_zero,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int bdrv_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
Coroutine *co;
- int64_t n;
- int64_t map;
BdrvCoBlockStatusData data = {
.bs = bs,
.base = base,
.want_zero = want_zero,
- .offset = sector_num * BDRV_SECTOR_SIZE,
- .bytes = nb_sectors * BDRV_SECTOR_SIZE,
- .pnum = &n,
- .map = &map,
+ .offset = offset,
+ .bytes = bytes,
+ .pnum = pnum,
+ .map = map,
.file = file,
.done = false,
};
@@ -2063,13 +2061,7 @@ static int64_t bdrv_common_block_status_above(BlockDriverState *bs,
bdrv_coroutine_enter(bs, co);
BDRV_POLL_WHILE(bs, !data.done);
}
- if (data.ret < 0) {
- *pnum = 0;
- return data.ret;
- }
- assert(QEMU_IS_ALIGNED(n | map, BDRV_SECTOR_SIZE));
- *pnum = n >> BDRV_SECTOR_BITS;
- return data.ret | map;
+ return data.ret;
}
int64_t bdrv_get_block_status_above(BlockDriverState *bs,
@@ -2078,8 +2070,21 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
int nb_sectors, int *pnum,
BlockDriverState **file)
{
- return bdrv_common_block_status_above(bs, base, true, sector_num,
- nb_sectors, pnum, file);
+ int64_t ret;
+ int64_t n;
+ int64_t map;
+
+ ret = bdrv_common_block_status_above(bs, base, true,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ &n, &map, file);
+ if (ret < 0) {
+ *pnum = 0;
+ return ret;
+ }
+ assert(QEMU_IS_ALIGNED(n | map, BDRV_SECTOR_SIZE));
+ *pnum = n >> BDRV_SECTOR_BITS;
+ return ret | map;
}
int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
@@ -2116,21 +2121,15 @@ int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum)
{
- int64_t ret;
- int psectors;
+ int ret;
+ int64_t dummy;
- assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
- assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE) && bytes < INT_MAX);
- ret = bdrv_common_block_status_above(bs, backing_bs(bs), false,
- offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS, &psectors,
+ ret = bdrv_common_block_status_above(bs, backing_bs(bs), false, offset,
+ bytes, pnum ? pnum : &dummy, NULL,
NULL);
if (ret < 0) {
return ret;
}
- if (pnum) {
- *pnum = psectors * BDRV_SECTOR_SIZE;
- }
return !!(ret & BDRV_BLOCK_ALLOCATED);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 11/24] block: Switch bdrv_co_get_block_status_above() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (9 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 10/24] block: Switch bdrv_common_block_status_above() " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes Eric Blake
` (13 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert another internal
type (no semantic change), and rename it to match the corresponding
public function rename.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to split return interface change [Kevin], R-b dropped
v5: no change other than rebase to context
v4: no change
v3: rebase to allocation/mapping sense change, simple enough to keep R-b
v2: rebase to earlier changes
---
block/io.c | 68 ++++++++++++++++++++++----------------------------------------
1 file changed, 24 insertions(+), 44 deletions(-)
diff --git a/block/io.c b/block/io.c
index e8fda1a4b8..938325b756 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1956,33 +1956,26 @@ early_out:
return ret;
}
-static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- bool want_zero,
- int64_t sector_num,
- int nb_sectors,
- int *pnum,
- BlockDriverState **file)
+static int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BlockDriverState *p;
- int64_t ret = 0;
+ int ret = 0;
bool first = true;
- int64_t map = 0;
assert(bs != base);
for (p = bs; p != base; p = backing_bs(p)) {
- int64_t count;
-
- ret = bdrv_co_block_status(p, want_zero,
- sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE, &count,
- &map, file);
+ ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map,
+ file);
if (ret < 0) {
break;
}
- assert(QEMU_IS_ALIGNED(count | map, BDRV_SECTOR_SIZE));
- ret |= map;
- *pnum = count >> BDRV_SECTOR_BITS;
if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) {
/*
* Reading beyond the end of the file continues to read
@@ -1990,47 +1983,35 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
* unallocated length we learned from an earlier
* iteration.
*/
- *pnum = nb_sectors;
+ *pnum = bytes;
}
if (ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_DATA)) {
break;
}
- /* [sector_num, pnum] unallocated on this layer, which could be only
- * the first part of [sector_num, nb_sectors]. */
- nb_sectors = MIN(nb_sectors, *pnum);
+ /* [offset, pnum] unallocated on this layer, which could be only
+ * the first part of [offset, bytes]. */
+ bytes = MIN(bytes, *pnum);
first = false;
}
return ret;
}
/* Coroutine wrapper for bdrv_get_block_status_above() */
-static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
+static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
{
BdrvCoBlockStatusData *data = opaque;
- int n = 0;
- int64_t ret;
- ret = bdrv_co_get_block_status_above(data->bs, data->base,
- data->want_zero,
- data->offset >> BDRV_SECTOR_BITS,
- data->bytes >> BDRV_SECTOR_BITS,
- &n,
- data->file);
- if (ret < 0) {
- assert(INT_MIN <= ret);
- data->ret = ret;
- } else {
- *data->pnum = n * BDRV_SECTOR_SIZE;
- *data->map = ret & BDRV_BLOCK_OFFSET_MASK;
- data->ret = ret & ~BDRV_BLOCK_OFFSET_MASK;
- }
+ data->ret = bdrv_co_block_status_above(data->bs, data->base,
+ data->want_zero,
+ data->offset, data->bytes,
+ data->pnum, data->map, data->file);
data->done = true;
}
/*
- * Synchronous wrapper around bdrv_co_get_block_status_above().
+ * Synchronous wrapper around bdrv_co_block_status_above().
*
- * See bdrv_co_get_block_status_above() for details.
+ * See bdrv_co_block_status_above() for details.
*/
static int bdrv_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
@@ -2054,10 +2035,9 @@ static int bdrv_common_block_status_above(BlockDriverState *bs,
if (qemu_in_coroutine()) {
/* Fast-path if already in coroutine context */
- bdrv_get_block_status_above_co_entry(&data);
+ bdrv_block_status_above_co_entry(&data);
} else {
- co = qemu_coroutine_create(bdrv_get_block_status_above_co_entry,
- &data);
+ co = qemu_coroutine_create(bdrv_block_status_above_co_entry, &data);
bdrv_coroutine_enter(bs, co);
BDRV_POLL_WHILE(bs, !data.done);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (10 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 11/24] block: Switch bdrv_co_get_block_status_above() " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-20 15:03 ` Kevin Wolf
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 13/24] qemu-img: Simplify logic in img_compare() Eric Blake
` (12 subsequent siblings)
24 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz, Jeff Cody
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_above()
to bdrv_block_status_above() ensures that the compiler enforces that
all callers are updated. Likewise, since it a byte interface allows
an offset mapping that might not be sector aligned, split the mapping
out of the return value and into a pass-by-reference parameter. For
now, the io.c layer still assert()s that all uses are sector-aligned,
but that can be relaxed when a later patch implements byte-based
block status in the drivers.
For the most part this patch is just the addition of scaling at the
callers followed by inverse scaling at bdrv_block_status(), plus
updates for the new split return interface. But some code,
particularly bdrv_block_status(), gets a lot simpler because it no
longer has to mess with sectors. Likewise, mirror code no longer
computes s->granularity >> BDRV_SECTOR_BITS, and can therefore drop
an assertion about alignment because the loop no longer depends on
alignment (never mind that we don't really have a driver that
reports sub-sector alignments, so it's not really possible to test
the effect of sub-sector mirroring). Fix a neighboring assertion to
use is_power_of_2 while there.
For ease of review, bdrv_get_block_status() was tackled separately.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to split return interface change [Kevin]
v5: assert alignment rather than rounding up in img_compare [John],
rebase to earlier changes
v4: rebase to earlier changes
v3: rebase to allocation/mapping sense change and qcow2-measure, tweak
mirror assertions, drop R-b
v2: rebase to earlier changes
---
include/block/block.h | 8 +++-----
block/io.c | 55 ++++++++-------------------------------------------
block/mirror.c | 18 ++++++-----------
block/qcow2.c | 30 +++++++++++-----------------
qemu-img.c | 49 +++++++++++++++++++++++++--------------------
5 files changed, 57 insertions(+), 103 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 7ac851f82f..fbc21daf62 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -425,11 +425,9 @@ 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,
BlockDriverState **file);
-int64_t bdrv_get_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file);
+int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
+ int64_t offset, int64_t bytes, int64_t *pnum,
+ int64_t *map, BlockDriverState **file);
int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum);
int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
diff --git a/block/io.c b/block/io.c
index 938325b756..f0bd6aa63e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1996,7 +1996,7 @@ static int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
return ret;
}
-/* Coroutine wrapper for bdrv_get_block_status_above() */
+/* Coroutine wrapper for bdrv_block_status_above() */
static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
{
BdrvCoBlockStatusData *data = opaque;
@@ -2044,58 +2044,19 @@ static int bdrv_common_block_status_above(BlockDriverState *bs,
return data.ret;
}
-int64_t bdrv_get_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
+ int64_t offset, int64_t bytes, int64_t *pnum,
+ int64_t *map, BlockDriverState **file)
{
- int64_t ret;
- int64_t n;
- int64_t map;
-
- ret = bdrv_common_block_status_above(bs, base, true,
- sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE,
- &n, &map, file);
- if (ret < 0) {
- *pnum = 0;
- return ret;
- }
- assert(QEMU_IS_ALIGNED(n | map, BDRV_SECTOR_SIZE));
- *pnum = n >> BDRV_SECTOR_BITS;
- return ret | map;
+ return bdrv_common_block_status_above(bs, base, true, offset, bytes,
+ pnum, map, file);
}
int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum, int64_t *map, BlockDriverState **file)
{
- int64_t ret;
- int n;
-
- assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
- assert(pnum);
- /*
- * The contract allows us to return pnum smaller than bytes, even
- * if the next query would see the same status; we truncate the
- * request to avoid overflowing the driver's 32-bit interface.
- */
- 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 (ret < 0) {
- assert(INT_MIN <= ret);
- *pnum = 0;
- return ret;
- }
- *pnum = n * BDRV_SECTOR_SIZE;
- if (map) {
- *map = ret & BDRV_BLOCK_OFFSET_MASK;
- } else {
- ret &= ~BDRV_BLOCK_OFFSET_VALID;
- }
- return ret & ~BDRV_BLOCK_OFFSET_MASK;
+ return bdrv_block_status_above(bs, backing_bs(bs),
+ offset, bytes, pnum, map, file);
}
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
diff --git a/block/mirror.c b/block/mirror.c
index d11706c566..307b6391a8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -328,7 +328,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
uint64_t delay_ns = 0;
/* At least the first dirty chunk is mirrored in one iteration. */
int nb_chunks = 1;
- int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
@@ -376,7 +375,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
}
/* Clear dirty bits before querying the block status, because
- * calling bdrv_get_block_status_above could yield - if some blocks are
+ * calling bdrv_block_status_above could yield - if some blocks are
* marked dirty in this window, we need to know.
*/
bdrv_reset_dirty_bitmap_locked(s->dirty_bitmap, offset,
@@ -385,8 +384,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
bitmap_set(s->in_flight_bitmap, offset / s->granularity, nb_chunks);
while (nb_chunks > 0 && offset < s->bdev_length) {
- int64_t ret;
- int io_sectors;
+ int ret;
int64_t io_bytes;
int64_t io_bytes_acct;
enum MirrorMethod {
@@ -396,11 +394,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
} mirror_method = MIRROR_METHOD_COPY;
assert(!(offset % s->granularity));
- ret = bdrv_get_block_status_above(source, NULL,
- offset >> BDRV_SECTOR_BITS,
- nb_chunks * sectors_per_chunk,
- &io_sectors, NULL);
- io_bytes = io_sectors * BDRV_SECTOR_SIZE;
+ ret = bdrv_block_status_above(source, NULL, offset,
+ nb_chunks * s->granularity,
+ &io_bytes, NULL, NULL);
if (ret < 0) {
io_bytes = MIN(nb_chunks * s->granularity, max_io_bytes);
} else if (ret & BDRV_BLOCK_DATA) {
@@ -1131,9 +1127,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
granularity = bdrv_get_default_bitmap_granularity(target);
}
- assert ((granularity & (granularity - 1)) == 0);
- /* Granularity must be large enough for sector-based dirty bitmap */
- assert(granularity >= BDRV_SECTOR_SIZE);
+ assert(is_power_of_2(granularity));
if (buf_size < 0) {
error_setg(errp, "Invalid parameter 'buf-size'");
diff --git a/block/qcow2.c b/block/qcow2.c
index df53535455..092a3cabdb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3001,8 +3001,8 @@ finish:
static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
- int nr;
- int64_t res;
+ int64_t nr;
+ int res;
int64_t start;
/* TODO: Widening to sector boundaries should only be needed as
@@ -3018,10 +3018,8 @@ static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes)
if (!bytes) {
return true;
}
- res = bdrv_get_block_status_above(bs, NULL, start >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS, &nr, NULL);
- return res >= 0 && (res & BDRV_BLOCK_ZERO) &&
- nr * BDRV_SECTOR_SIZE == bytes;
+ res = bdrv_block_status_above(bs, NULL, start, bytes, &nr, NULL, NULL);
+ return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == bytes;
}
static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
@@ -3729,17 +3727,14 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
required = virtual_size;
} else {
int64_t offset;
- int pnum = 0;
+ int64_t pnum = 0;
- for (offset = 0; offset < ssize;
- offset += pnum * BDRV_SECTOR_SIZE) {
- int nb_sectors = MIN(ssize - offset,
- BDRV_REQUEST_MAX_BYTES) / BDRV_SECTOR_SIZE;
- int64_t ret;
+ for (offset = 0; offset < ssize; offset += pnum) {
+ int ret;
- ret = bdrv_get_block_status_above(in_bs, NULL,
- offset >> BDRV_SECTOR_BITS,
- nb_sectors, &pnum, NULL);
+ ret = bdrv_block_status_above(in_bs, NULL, offset,
+ ssize - offset, &pnum, NULL,
+ NULL);
if (ret < 0) {
error_setg_errno(&local_err, -ret,
"Unable to get block status");
@@ -3751,11 +3746,10 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
} else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
(BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
/* Extend pnum to end of cluster for next iteration */
- pnum = (ROUND_UP(offset + pnum * BDRV_SECTOR_SIZE,
- cluster_size) - offset) >> BDRV_SECTOR_BITS;
+ pnum = ROUND_UP(offset + pnum, cluster_size) - offset;
/* Count clusters we've seen */
- required += offset % cluster_size + pnum * BDRV_SECTOR_SIZE;
+ required += offset % cluster_size + pnum;
}
}
}
diff --git a/qemu-img.c b/qemu-img.c
index c81d6ce733..78c820e487 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1226,7 +1226,7 @@ static int img_compare(int argc, char **argv)
BlockDriverState *bs1, *bs2;
int64_t total_sectors1, total_sectors2;
uint8_t *buf1 = NULL, *buf2 = NULL;
- int pnum1, pnum2;
+ int64_t pnum1, pnum2;
int allocated1, allocated2;
int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
bool progress = false, quiet = false, strict = false;
@@ -1374,15 +1374,17 @@ static int img_compare(int argc, char **argv)
}
for (;;) {
- int64_t status1, status2;
+ int status1, status2;
nb_sectors = sectors_to_process(total_sectors, sector_num);
if (nb_sectors <= 0) {
break;
}
- status1 = bdrv_get_block_status_above(bs1, NULL, sector_num,
- total_sectors1 - sector_num,
- &pnum1, NULL);
+ status1 = bdrv_block_status_above(bs1, NULL,
+ sector_num * BDRV_SECTOR_SIZE,
+ (total_sectors1 - sector_num) *
+ BDRV_SECTOR_SIZE,
+ &pnum1, NULL, NULL);
if (status1 < 0) {
ret = 3;
error_report("Sector allocation test failed for %s", filename1);
@@ -1390,25 +1392,29 @@ static int img_compare(int argc, char **argv)
}
allocated1 = status1 & BDRV_BLOCK_ALLOCATED;
- status2 = bdrv_get_block_status_above(bs2, NULL, sector_num,
- total_sectors2 - sector_num,
- &pnum2, NULL);
+ status2 = bdrv_block_status_above(bs2, NULL,
+ sector_num * BDRV_SECTOR_SIZE,
+ (total_sectors2 - sector_num) *
+ BDRV_SECTOR_SIZE,
+ &pnum2, NULL, NULL);
if (status2 < 0) {
ret = 3;
error_report("Sector allocation test failed for %s", filename2);
goto out;
}
allocated2 = status2 & BDRV_BLOCK_ALLOCATED;
+ /* TODO: Relax this once comparison is byte-based, and we no longer
+ * have to worry about sector alignment */
+ assert(QEMU_IS_ALIGNED(pnum1 | pnum2, BDRV_SECTOR_SIZE));
if (pnum1) {
- nb_sectors = MIN(nb_sectors, pnum1);
+ nb_sectors = MIN(nb_sectors, pnum1 >> BDRV_SECTOR_BITS);
}
if (pnum2) {
- nb_sectors = MIN(nb_sectors, pnum2);
+ nb_sectors = MIN(nb_sectors, pnum2 >> BDRV_SECTOR_BITS);
}
if (strict) {
- if ((status1 & ~BDRV_BLOCK_OFFSET_MASK) !=
- (status2 & ~BDRV_BLOCK_OFFSET_MASK)) {
+ if (status1 != status2) {
ret = 1;
qprintf(quiet, "Strict mode: Offset %" PRId64
" block status mismatch!\n",
@@ -1417,7 +1423,7 @@ static int img_compare(int argc, char **argv)
}
}
if ((status1 & BDRV_BLOCK_ZERO) && (status2 & BDRV_BLOCK_ZERO)) {
- nb_sectors = MIN(pnum1, pnum2);
+ nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);
} else if (allocated1 == allocated2) {
if (allocated1) {
ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,
@@ -1589,8 +1595,8 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num,
static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
{
- int64_t ret, src_cur_offset;
- int n, src_cur;
+ int64_t src_cur_offset;
+ int ret, n, src_cur;
convert_select_part(s, sector_num, &src_cur, &src_cur_offset);
@@ -1598,23 +1604,24 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
if (s->sector_next_status <= sector_num) {
+ int64_t count = n * BDRV_SECTOR_SIZE;
+
if (s->target_has_backing) {
- 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, 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,
- n, &n, NULL);
+ ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
+ (sector_num - src_cur_offset) *
+ BDRV_SECTOR_SIZE,
+ count, &count, NULL, NULL);
}
if (ret < 0) {
return ret;
}
+ n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
if (ret & BDRV_BLOCK_ZERO) {
s->status = BLK_ZERO;
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 13/24] qemu-img: Simplify logic in img_compare()
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (11 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 14/24] qemu-img: Speed up compare on pre-allocated larger file Eric Blake
` (11 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
As long as we are querying the status for a chunk smaller than
the known image size, we are guaranteed that a successful return
will have set pnum to a non-zero size (pnum is zero only for
queries beyond the end of the file). Use that to slightly
simplify the calculation of the current chunk size being compared.
Likewise, we don't have to shrink the amount of data operated on
until we know we have to read the file, and therefore have to fit
in the bounds of our buffer. Also, note that 'total_sectors_over'
is equivalent to 'progress_base'.
With these changes in place, sectors_to_process() is now dead code,
and can be removed.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to context
v5: rebase to alignment assertion [John]
v4: no change
v3: new patch
---
qemu-img.c | 38 +++++++++++---------------------------
1 file changed, 11 insertions(+), 27 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 78c820e487..cfa28d41d5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1172,11 +1172,6 @@ static int64_t sectors_to_bytes(int64_t sectors)
return sectors << BDRV_SECTOR_BITS;
}
-static int64_t sectors_to_process(int64_t total, int64_t from)
-{
- return MIN(total - from, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
-}
-
/*
* Check if passed sectors are empty (not allocated or contain only 0 bytes)
*
@@ -1373,13 +1368,9 @@ static int img_compare(int argc, char **argv)
goto out;
}
- for (;;) {
+ while (sector_num < total_sectors) {
int status1, status2;
- nb_sectors = sectors_to_process(total_sectors, sector_num);
- if (nb_sectors <= 0) {
- break;
- }
status1 = bdrv_block_status_above(bs1, NULL,
sector_num * BDRV_SECTOR_SIZE,
(total_sectors1 - sector_num) *
@@ -1406,12 +1397,9 @@ static int img_compare(int argc, char **argv)
/* TODO: Relax this once comparison is byte-based, and we no longer
* have to worry about sector alignment */
assert(QEMU_IS_ALIGNED(pnum1 | pnum2, BDRV_SECTOR_SIZE));
- if (pnum1) {
- nb_sectors = MIN(nb_sectors, pnum1 >> BDRV_SECTOR_BITS);
- }
- if (pnum2) {
- nb_sectors = MIN(nb_sectors, pnum2 >> BDRV_SECTOR_BITS);
- }
+
+ assert(pnum1 && pnum2);
+ nb_sectors = MIN(pnum1, pnum2) >> BDRV_SECTOR_BITS;
if (strict) {
if (status1 != status2) {
@@ -1423,9 +1411,10 @@ static int img_compare(int argc, char **argv)
}
}
if ((status1 & BDRV_BLOCK_ZERO) && (status2 & BDRV_BLOCK_ZERO)) {
- nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);
+ /* nothing to do */
} else if (allocated1 == allocated2) {
if (allocated1) {
+ nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,
nb_sectors << BDRV_SECTOR_BITS);
if (ret < 0) {
@@ -1454,7 +1443,7 @@ static int img_compare(int argc, char **argv)
}
}
} else {
-
+ nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
if (allocated1) {
ret = check_empty_sectors(blk1, sector_num, nb_sectors,
filename1, buf1, quiet);
@@ -1477,30 +1466,24 @@ static int img_compare(int argc, char **argv)
if (total_sectors1 != total_sectors2) {
BlockBackend *blk_over;
- int64_t total_sectors_over;
const char *filename_over;
qprintf(quiet, "Warning: Image size mismatch!\n");
if (total_sectors1 > total_sectors2) {
- total_sectors_over = total_sectors1;
blk_over = blk1;
filename_over = filename1;
} else {
- total_sectors_over = total_sectors2;
blk_over = blk2;
filename_over = filename2;
}
- for (;;) {
+ while (sector_num < progress_base) {
int64_t count;
- nb_sectors = sectors_to_process(total_sectors_over, sector_num);
- if (nb_sectors <= 0) {
- break;
- }
ret = bdrv_is_allocated_above(blk_bs(blk_over), NULL,
sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE,
+ (progress_base - sector_num) *
+ BDRV_SECTOR_SIZE,
&count);
if (ret < 0) {
ret = 3;
@@ -1514,6 +1497,7 @@ static int img_compare(int argc, char **argv)
assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
nb_sectors = count >> BDRV_SECTOR_BITS;
if (ret) {
+ nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
ret = check_empty_sectors(blk_over, sector_num, nb_sectors,
filename_over, buf1, quiet);
if (ret) {
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 14/24] qemu-img: Speed up compare on pre-allocated larger file
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (12 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 13/24] qemu-img: Simplify logic in img_compare() Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 15/24] qemu-img: Add find_nonzero() Eric Blake
` (10 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
Compare the following images with all-zero contents:
$ truncate --size 1M A
$ qemu-img create -f qcow2 -o preallocation=off B 1G
$ qemu-img create -f qcow2 -o preallocation=metadata C 1G
On my machine, the difference is noticeable for pre-patch speeds,
with more than an order of magnitude in difference caused by the
choice of preallocation in the qcow2 file:
$ time ./qemu-img compare -f raw -F qcow2 A B
Warning: Image size mismatch!
Images are identical.
real 0m0.014s
user 0m0.007s
sys 0m0.007s
$ time ./qemu-img compare -f raw -F qcow2 A C
Warning: Image size mismatch!
Images are identical.
real 0m0.341s
user 0m0.144s
sys 0m0.188s
Why? Because bdrv_is_allocated() returns false for image B but
true for image C, throwing away the fact that both images know
via lseek(SEEK_HOLE) that the entire image still reads as zero.
>From there, qemu-img ends up calling bdrv_pread() for every byte
of the tail, instead of quickly looking for the next allocation.
The solution: use block_status instead of is_allocated, giving:
$ time ./qemu-img compare -f raw -F qcow2 A C
Warning: Image size mismatch!
Images are identical.
real 0m0.014s
user 0m0.011s
sys 0m0.003s
which is on par with the speeds for no pre-allocation.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v6: rebase to interface change [Kevin], minor enough to keep R-b
v4-v5: no change
v3: new patch
---
qemu-img.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index cfa28d41d5..e4b84c4f56 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1480,11 +1480,11 @@ static int img_compare(int argc, char **argv)
while (sector_num < progress_base) {
int64_t count;
- ret = bdrv_is_allocated_above(blk_bs(blk_over), NULL,
+ ret = bdrv_block_status_above(blk_bs(blk_over), NULL,
sector_num * BDRV_SECTOR_SIZE,
(progress_base - sector_num) *
BDRV_SECTOR_SIZE,
- &count);
+ &count, NULL, NULL);
if (ret < 0) {
ret = 3;
error_report("Sector allocation test failed for %s",
@@ -1492,11 +1492,11 @@ static int img_compare(int argc, char **argv)
goto out;
}
- /* TODO relax this once bdrv_is_allocated_above does not enforce
+ /* TODO relax this once bdrv_block_status_above does not enforce
* sector alignment */
assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
nb_sectors = count >> BDRV_SECTOR_BITS;
- if (ret) {
+ if (ret & BDRV_BLOCK_ALLOCATED && !(ret & BDRV_BLOCK_ZERO)) {
nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
ret = check_empty_sectors(blk_over, sector_num, nb_sectors,
filename_over, buf1, quiet);
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 15/24] qemu-img: Add find_nonzero()
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (13 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 14/24] qemu-img: Speed up compare on pre-allocated larger file Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 16/24] qemu-img: Drop redundant error message in compare Eric Blake
` (9 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
During 'qemu-img compare', when we are checking that an allocated
portion of one file is all zeros, we don't need to waste time
computing how many additional sectors after the first non-zero
byte are also non-zero. Create a new helper find_nonzero() to do
the check for a first non-zero sector, and rebase
check_empty_sectors() to use it.
The new interface intentionally uses bytes in its interface, even
though it still crawls the buffer a sector at a time; it is robust
to a partial sector at the end of the buffer.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v4-v5: no change
v3: new patch
---
qemu-img.c | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index e4b84c4f56..89f1093d81 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1065,6 +1065,28 @@ done:
}
/*
+ * Returns -1 if 'buf' contains only zeroes, otherwise the byte index
+ * of the first sector boundary within buf where the sector contains a
+ * non-zero byte. This function is robust to a buffer that is not
+ * sector-aligned.
+ */
+static int64_t find_nonzero(const uint8_t *buf, int64_t n)
+{
+ int64_t i;
+ int64_t end = QEMU_ALIGN_DOWN(n, BDRV_SECTOR_SIZE);
+
+ for (i = 0; i < end; i += BDRV_SECTOR_SIZE) {
+ if (!buffer_is_zero(buf + i, BDRV_SECTOR_SIZE)) {
+ return i;
+ }
+ }
+ if (i < n && !buffer_is_zero(buf + i, n - end)) {
+ return i;
+ }
+ return -1;
+}
+
+/*
* Returns true iff the first sector pointed to by 'buf' contains at least
* a non-NUL byte.
*
@@ -1189,7 +1211,9 @@ static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
int sect_count, const char *filename,
uint8_t *buffer, bool quiet)
{
- int pnum, ret = 0;
+ int ret = 0;
+ int64_t idx;
+
ret = blk_pread(blk, sect_num << BDRV_SECTOR_BITS, buffer,
sect_count << BDRV_SECTOR_BITS);
if (ret < 0) {
@@ -1197,10 +1221,10 @@ static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
sectors_to_bytes(sect_num), filename, strerror(-ret));
return ret;
}
- ret = is_allocated_sectors(buffer, sect_count, &pnum);
- if (ret || pnum != sect_count) {
+ idx = find_nonzero(buffer, sect_count * BDRV_SECTOR_SIZE);
+ if (idx >= 0) {
qprintf(quiet, "Content mismatch at offset %" PRId64 "!\n",
- sectors_to_bytes(ret ? sect_num : sect_num + pnum));
+ sectors_to_bytes(sect_num) + idx);
return 1;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 16/24] qemu-img: Drop redundant error message in compare
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (14 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 15/24] qemu-img: Add find_nonzero() Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 17/24] qemu-img: Change check_empty_sectors() to byte-based Eric Blake
` (8 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
If a read error is encountered during 'qemu-img compare', we
were printing the "Error while reading offset ..." message twice;
this was because our helper function was awkward, printing output
on some but not all paths. Fix it to consistently report errors
on all paths, so that the callers do not risk a redundant message,
and update the testsuite for the improved output.
Further simplify the code by hoisting the conversion from an error
message to an exit code into the helper function, rather than
repeating that logic at all callers (yes, the helper function is
now less generic, but it's a net win in lines of code).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v5: tweak commit message, but no code change
v4: no change
v3: new patch
---
qemu-img.c | 19 +++++--------------
tests/qemu-iotests/074.out | 2 --
2 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 89f1093d81..53cae6bb66 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1197,8 +1197,10 @@ static int64_t sectors_to_bytes(int64_t sectors)
/*
* Check if passed sectors are empty (not allocated or contain only 0 bytes)
*
- * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
- * data and negative value on error.
+ * Intended for use by 'qemu-img compare': Returns 0 in case sectors are
+ * filled with 0, 1 if sectors contain non-zero data (this is a comparison
+ * failure), and 4 on error (the exit status for read errors), after emitting
+ * an error message.
*
* @param blk: BlockBackend for the image
* @param sect_num: Number of first sector to check
@@ -1219,7 +1221,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
if (ret < 0) {
error_report("Error while reading offset %" PRId64 " of %s: %s",
sectors_to_bytes(sect_num), filename, strerror(-ret));
- return ret;
+ return 4;
}
idx = find_nonzero(buffer, sect_count * BDRV_SECTOR_SIZE);
if (idx >= 0) {
@@ -1476,11 +1478,6 @@ static int img_compare(int argc, char **argv)
filename2, buf1, quiet);
}
if (ret) {
- if (ret < 0) {
- error_report("Error while reading offset %" PRId64 ": %s",
- sectors_to_bytes(sector_num), strerror(-ret));
- ret = 4;
- }
goto out;
}
}
@@ -1525,12 +1522,6 @@ static int img_compare(int argc, char **argv)
ret = check_empty_sectors(blk_over, sector_num, nb_sectors,
filename_over, buf1, quiet);
if (ret) {
- if (ret < 0) {
- error_report("Error while reading offset %" PRId64
- " of %s: %s", sectors_to_bytes(sector_num),
- filename_over, strerror(-ret));
- ret = 4;
- }
goto out;
}
}
diff --git a/tests/qemu-iotests/074.out b/tests/qemu-iotests/074.out
index 8fba5aea9c..ede66c3f81 100644
--- a/tests/qemu-iotests/074.out
+++ b/tests/qemu-iotests/074.out
@@ -4,7 +4,6 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
wrote 512/512 bytes at offset 512
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
qemu-img: Error while reading offset 0 of blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
-qemu-img: Error while reading offset 0: Input/output error
4
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=0
@@ -12,7 +11,6 @@ Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=0
wrote 512/512 bytes at offset 512
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
qemu-img: Error while reading offset 0 of blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
-qemu-img: Error while reading offset 0 of blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
Warning: Image size mismatch!
4
Cleanup
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 17/24] qemu-img: Change check_empty_sectors() to byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (15 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 16/24] qemu-img: Drop redundant error message in compare Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 18/24] qemu-img: Change compare_sectors() to be byte-based Eric Blake
` (7 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
Continue on the quest to make more things byte-based instead of
sector-based.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v4-v5: no change
v3: new patch
---
qemu-img.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 53cae6bb66..66d595be3d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1203,30 +1203,29 @@ static int64_t sectors_to_bytes(int64_t sectors)
* an error message.
*
* @param blk: BlockBackend for the image
- * @param sect_num: Number of first sector to check
- * @param sect_count: Number of sectors to check
+ * @param offset: Starting offset to check
+ * @param bytes: Number of bytes to check
* @param filename: Name of disk file we are checking (logging purpose)
* @param buffer: Allocated buffer for storing read data
* @param quiet: Flag for quiet mode
*/
-static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
- int sect_count, const char *filename,
+static int check_empty_sectors(BlockBackend *blk, int64_t offset,
+ int64_t bytes, const char *filename,
uint8_t *buffer, bool quiet)
{
int ret = 0;
int64_t idx;
- ret = blk_pread(blk, sect_num << BDRV_SECTOR_BITS, buffer,
- sect_count << BDRV_SECTOR_BITS);
+ ret = blk_pread(blk, offset, buffer, bytes);
if (ret < 0) {
error_report("Error while reading offset %" PRId64 " of %s: %s",
- sectors_to_bytes(sect_num), filename, strerror(-ret));
+ offset, filename, strerror(-ret));
return 4;
}
- idx = find_nonzero(buffer, sect_count * BDRV_SECTOR_SIZE);
+ idx = find_nonzero(buffer, bytes);
if (idx >= 0) {
qprintf(quiet, "Content mismatch at offset %" PRId64 "!\n",
- sectors_to_bytes(sect_num) + idx);
+ offset + idx);
return 1;
}
@@ -1471,10 +1470,12 @@ static int img_compare(int argc, char **argv)
} else {
nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
if (allocated1) {
- ret = check_empty_sectors(blk1, sector_num, nb_sectors,
+ ret = check_empty_sectors(blk1, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
filename1, buf1, quiet);
} else {
- ret = check_empty_sectors(blk2, sector_num, nb_sectors,
+ ret = check_empty_sectors(blk2, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
filename2, buf1, quiet);
}
if (ret) {
@@ -1519,7 +1520,9 @@ static int img_compare(int argc, char **argv)
nb_sectors = count >> BDRV_SECTOR_BITS;
if (ret & BDRV_BLOCK_ALLOCATED && !(ret & BDRV_BLOCK_ZERO)) {
nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
- ret = check_empty_sectors(blk_over, sector_num, nb_sectors,
+ ret = check_empty_sectors(blk_over,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
filename_over, buf1, quiet);
if (ret) {
goto out;
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 18/24] qemu-img: Change compare_sectors() to be byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (16 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 17/24] qemu-img: Change check_empty_sectors() to byte-based Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 19/24] qemu-img: Change img_rebase() " Eric Blake
` (6 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
In the continuing quest to make more things byte-based, change
compare_sectors(), renaming it to compare_buffers() in the
process. Note that one caller (qemu-img compare) only cares
about the first difference, while the other (qemu-img rebase)
cares about how many consecutive sectors have the same
equal/different status; however, this patch does not bother to
micro-optimize the compare case to avoid the comparisons of
sectors beyond the first mismatch. Both callers are always
passing valid buffers in, so the initial check for buffer size
can be turned into an assertion.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v4-v5: no change
v3: new patch
---
qemu-img.c | 55 +++++++++++++++++++++++++++----------------------------
1 file changed, 27 insertions(+), 28 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 66d595be3d..c6b6263853 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1156,31 +1156,28 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
}
/*
- * Compares two buffers sector by sector. Returns 0 if the first sector of both
- * buffers matches, non-zero otherwise.
+ * Compares two buffers sector by sector. Returns 0 if the first
+ * sector of each buffer matches, non-zero otherwise.
*
- * pnum is set to the number of sectors (including and immediately following
- * the first one) that are known to have the same comparison result
+ * pnum is set to the sector-aligned size of the buffer prefix that
+ * has the same matching status as the first sector.
*/
-static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
- int *pnum)
+static int compare_buffers(const uint8_t *buf1, const uint8_t *buf2,
+ int64_t bytes, int64_t *pnum)
{
bool res;
- int i;
+ int64_t i = MIN(bytes, BDRV_SECTOR_SIZE);
- if (n <= 0) {
- *pnum = 0;
- return 0;
- }
+ assert(bytes > 0);
- res = !!memcmp(buf1, buf2, 512);
- for(i = 1; i < n; i++) {
- buf1 += 512;
- buf2 += 512;
+ res = !!memcmp(buf1, buf2, i);
+ while (i < bytes) {
+ int64_t len = MIN(bytes - i, BDRV_SECTOR_SIZE);
- if (!!memcmp(buf1, buf2, 512) != res) {
+ if (!!memcmp(buf1 + i, buf2 + i, len) != res) {
break;
}
+ i += len;
}
*pnum = i;
@@ -1255,7 +1252,7 @@ static int img_compare(int argc, char **argv)
int64_t total_sectors;
int64_t sector_num = 0;
int64_t nb_sectors;
- int c, pnum;
+ int c;
uint64_t progress_base;
bool image_opts = false;
bool force_share = false;
@@ -1439,6 +1436,8 @@ static int img_compare(int argc, char **argv)
/* nothing to do */
} else if (allocated1 == allocated2) {
if (allocated1) {
+ int64_t pnum;
+
nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,
nb_sectors << BDRV_SECTOR_BITS);
@@ -1458,11 +1457,11 @@ static int img_compare(int argc, char **argv)
ret = 4;
goto out;
}
- ret = compare_sectors(buf1, buf2, nb_sectors, &pnum);
- if (ret || pnum != nb_sectors) {
+ ret = compare_buffers(buf1, buf2,
+ nb_sectors * BDRV_SECTOR_SIZE, &pnum);
+ if (ret || pnum != nb_sectors * BDRV_SECTOR_SIZE) {
qprintf(quiet, "Content mismatch at offset %" PRId64 "!\n",
- sectors_to_bytes(
- ret ? sector_num : sector_num + pnum));
+ sectors_to_bytes(sector_num) + (ret ? 0 : pnum));
ret = 1;
goto out;
}
@@ -3354,16 +3353,16 @@ static int img_rebase(int argc, char **argv)
/* If they differ, we need to write to the COW file */
uint64_t written = 0;
- while (written < n) {
- int pnum;
+ while (written < n * BDRV_SECTOR_SIZE) {
+ int64_t pnum;
- if (compare_sectors(buf_old + written * 512,
- buf_new + written * 512, n - written, &pnum))
+ if (compare_buffers(buf_old + written,
+ buf_new + written,
+ n * BDRV_SECTOR_SIZE - written, &pnum))
{
ret = blk_pwrite(blk,
- (sector + written) << BDRV_SECTOR_BITS,
- buf_old + written * 512,
- pnum << BDRV_SECTOR_BITS, 0);
+ (sector << BDRV_SECTOR_BITS) + written,
+ buf_old + written, pnum, 0);
if (ret < 0) {
error_report("Error while writing to COW image: %s",
strerror(-ret));
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 19/24] qemu-img: Change img_rebase() to be byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (17 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 18/24] qemu-img: Change compare_sectors() to be byte-based Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 20/24] qemu-img: Change img_compare() " Eric Blake
` (5 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
In the continuing quest to make more things byte-based, change
the internal iteration of img_rebase(). We can finally drop the
TODO assertion added earlier, now that the entire algorithm is
byte-based and no longer has to shift from bytes to sectors.
Most of the change is mechanical ('num_sectors' becomes 'size',
'sector' becomes 'offset', 'n' goes from sectors to bytes); some
of it is also a cleanup (use of MIN() instead of open-coding,
loss of variable 'count' added earlier in commit d6a644bb).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v4-v5: no change
v3: new patch
---
qemu-img.c | 84 +++++++++++++++++++++++++-------------------------------------
1 file changed, 34 insertions(+), 50 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index c6b6263853..994365c6a4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3248,70 +3248,58 @@ static int img_rebase(int argc, char **argv)
* the image is the same as the original one at any time.
*/
if (!unsafe) {
- int64_t num_sectors;
- int64_t old_backing_num_sectors;
- int64_t new_backing_num_sectors = 0;
- uint64_t sector;
- int n;
- int64_t count;
+ int64_t size;
+ int64_t old_backing_size;
+ int64_t new_backing_size = 0;
+ uint64_t offset;
+ int64_t n;
float local_progress = 0;
buf_old = blk_blockalign(blk, IO_BUF_SIZE);
buf_new = blk_blockalign(blk, IO_BUF_SIZE);
- num_sectors = blk_nb_sectors(blk);
- if (num_sectors < 0) {
+ size = blk_getlength(blk);
+ if (size < 0) {
error_report("Could not get size of '%s': %s",
- filename, strerror(-num_sectors));
+ filename, strerror(-size));
ret = -1;
goto out;
}
- old_backing_num_sectors = blk_nb_sectors(blk_old_backing);
- if (old_backing_num_sectors < 0) {
+ old_backing_size = blk_getlength(blk_old_backing);
+ if (old_backing_size < 0) {
char backing_name[PATH_MAX];
bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
error_report("Could not get size of '%s': %s",
- backing_name, strerror(-old_backing_num_sectors));
+ backing_name, strerror(-old_backing_size));
ret = -1;
goto out;
}
if (blk_new_backing) {
- new_backing_num_sectors = blk_nb_sectors(blk_new_backing);
- if (new_backing_num_sectors < 0) {
+ new_backing_size = blk_getlength(blk_new_backing);
+ if (new_backing_size < 0) {
error_report("Could not get size of '%s': %s",
- out_baseimg, strerror(-new_backing_num_sectors));
+ out_baseimg, strerror(-new_backing_size));
ret = -1;
goto out;
}
}
- if (num_sectors != 0) {
- local_progress = (float)100 /
- (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512));
+ if (size != 0) {
+ local_progress = (float)100 / (size / MIN(size, IO_BUF_SIZE));
}
- for (sector = 0; sector < num_sectors; sector += n) {
-
- /* How many sectors can we handle with the next read? */
- if (sector + (IO_BUF_SIZE / 512) <= num_sectors) {
- n = (IO_BUF_SIZE / 512);
- } else {
- n = num_sectors - sector;
- }
+ for (offset = 0; offset < size; offset += n) {
+ /* How many bytes can we handle with the next read? */
+ n = MIN(IO_BUF_SIZE, size - offset);
/* If the cluster is allocated, we don't need to take action */
- ret = bdrv_is_allocated(bs, sector << BDRV_SECTOR_BITS,
- n << BDRV_SECTOR_BITS, &count);
+ ret = bdrv_is_allocated(bs, offset, n, &n);
if (ret < 0) {
error_report("error while reading image metadata: %s",
strerror(-ret));
goto out;
}
- /* TODO relax this once bdrv_is_allocated does not enforce
- * sector alignment */
- assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
- n = count >> BDRV_SECTOR_BITS;
if (ret) {
continue;
}
@@ -3320,30 +3308,28 @@ static int img_rebase(int argc, char **argv)
* Read old and new backing file and take into consideration that
* backing files may be smaller than the COW image.
*/
- if (sector >= old_backing_num_sectors) {
- memset(buf_old, 0, n * BDRV_SECTOR_SIZE);
+ if (offset >= old_backing_size) {
+ memset(buf_old, 0, n);
} else {
- if (sector + n > old_backing_num_sectors) {
- n = old_backing_num_sectors - sector;
+ if (offset + n > old_backing_size) {
+ n = old_backing_size - offset;
}
- ret = blk_pread(blk_old_backing, sector << BDRV_SECTOR_BITS,
- buf_old, n << BDRV_SECTOR_BITS);
+ ret = blk_pread(blk_old_backing, offset, buf_old, n);
if (ret < 0) {
error_report("error while reading from old backing file");
goto out;
}
}
- if (sector >= new_backing_num_sectors || !blk_new_backing) {
- memset(buf_new, 0, n * BDRV_SECTOR_SIZE);
+ if (offset >= new_backing_size || !blk_new_backing) {
+ memset(buf_new, 0, n);
} else {
- if (sector + n > new_backing_num_sectors) {
- n = new_backing_num_sectors - sector;
+ if (offset + n > new_backing_size) {
+ n = new_backing_size - offset;
}
- ret = blk_pread(blk_new_backing, sector << BDRV_SECTOR_BITS,
- buf_new, n << BDRV_SECTOR_BITS);
+ ret = blk_pread(blk_new_backing, offset, buf_new, n);
if (ret < 0) {
error_report("error while reading from new backing file");
goto out;
@@ -3353,15 +3339,13 @@ static int img_rebase(int argc, char **argv)
/* If they differ, we need to write to the COW file */
uint64_t written = 0;
- while (written < n * BDRV_SECTOR_SIZE) {
+ while (written < n) {
int64_t pnum;
- if (compare_buffers(buf_old + written,
- buf_new + written,
- n * BDRV_SECTOR_SIZE - written, &pnum))
+ if (compare_buffers(buf_old + written, buf_new + written,
+ n - written, &pnum))
{
- ret = blk_pwrite(blk,
- (sector << BDRV_SECTOR_BITS) + written,
+ ret = blk_pwrite(blk, offset + written,
buf_old + written, pnum, 0);
if (ret < 0) {
error_report("Error while writing to COW image: %s",
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 20/24] qemu-img: Change img_compare() to be byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (18 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 19/24] qemu-img: Change img_rebase() " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 21/24] block: Align block status requests Eric Blake
` (4 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
In the continuing quest to make more things byte-based, change
the internal iteration of img_compare(). We can finally drop the
TODO assertions added earlier, now that the entire algorithm is
byte-based and no longer has to shift from bytes to sectors.
Most of the change is mechanical ('total_sectors' becomes
'total_size', 'sector_num' becomes 'offset', 'nb_sectors' becomes
'chunk', 'progress_base' goes from sectors to bytes); some of it
is also a cleanup (sectors_to_bytes() is now unused, loss of
variable 'count' added earlier in commit 51b0a488).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v6: rebase to split return interface change [Kevin], minor enough to keep R-b
v5: rebase to earlier change, minor enough to keep R-b
v4: no change
v3: new patch
---
qemu-img.c | 124 ++++++++++++++++++++++++-------------------------------------
1 file changed, 48 insertions(+), 76 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 994365c6a4..02a6e27beb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1186,11 +1186,6 @@ static int compare_buffers(const uint8_t *buf1, const uint8_t *buf2,
#define IO_BUF_SIZE (2 * 1024 * 1024)
-static int64_t sectors_to_bytes(int64_t sectors)
-{
- return sectors << BDRV_SECTOR_BITS;
-}
-
/*
* Check if passed sectors are empty (not allocated or contain only 0 bytes)
*
@@ -1241,7 +1236,7 @@ static int img_compare(int argc, char **argv)
const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
BlockBackend *blk1, *blk2;
BlockDriverState *bs1, *bs2;
- int64_t total_sectors1, total_sectors2;
+ int64_t total_size1, total_size2;
uint8_t *buf1 = NULL, *buf2 = NULL;
int64_t pnum1, pnum2;
int allocated1, allocated2;
@@ -1249,9 +1244,9 @@ static int img_compare(int argc, char **argv)
bool progress = false, quiet = false, strict = false;
int flags;
bool writethrough;
- int64_t total_sectors;
- int64_t sector_num = 0;
- int64_t nb_sectors;
+ int64_t total_size;
+ int64_t offset = 0;
+ int64_t chunk;
int c;
uint64_t progress_base;
bool image_opts = false;
@@ -1365,39 +1360,37 @@ static int img_compare(int argc, char **argv)
buf1 = blk_blockalign(blk1, IO_BUF_SIZE);
buf2 = blk_blockalign(blk2, IO_BUF_SIZE);
- total_sectors1 = blk_nb_sectors(blk1);
- if (total_sectors1 < 0) {
+ total_size1 = blk_getlength(blk1);
+ if (total_size1 < 0) {
error_report("Can't get size of %s: %s",
- filename1, strerror(-total_sectors1));
+ filename1, strerror(-total_size1));
ret = 4;
goto out;
}
- total_sectors2 = blk_nb_sectors(blk2);
- if (total_sectors2 < 0) {
+ total_size2 = blk_getlength(blk2);
+ if (total_size2 < 0) {
error_report("Can't get size of %s: %s",
- filename2, strerror(-total_sectors2));
+ filename2, strerror(-total_size2));
ret = 4;
goto out;
}
- total_sectors = MIN(total_sectors1, total_sectors2);
- progress_base = MAX(total_sectors1, total_sectors2);
+ total_size = MIN(total_size1, total_size2);
+ progress_base = MAX(total_size1, total_size2);
qemu_progress_print(0, 100);
- if (strict && total_sectors1 != total_sectors2) {
+ if (strict && total_size1 != total_size2) {
ret = 1;
qprintf(quiet, "Strict mode: Image size mismatch!\n");
goto out;
}
- while (sector_num < total_sectors) {
+ while (offset < total_size) {
int status1, status2;
- status1 = bdrv_block_status_above(bs1, NULL,
- sector_num * BDRV_SECTOR_SIZE,
- (total_sectors1 - sector_num) *
- BDRV_SECTOR_SIZE,
- &pnum1, NULL, NULL);
+ status1 = bdrv_block_status_above(bs1, NULL, offset,
+ total_size1 - offset, &pnum1, NULL,
+ NULL);
if (status1 < 0) {
ret = 3;
error_report("Sector allocation test failed for %s", filename1);
@@ -1405,30 +1398,24 @@ static int img_compare(int argc, char **argv)
}
allocated1 = status1 & BDRV_BLOCK_ALLOCATED;
- status2 = bdrv_block_status_above(bs2, NULL,
- sector_num * BDRV_SECTOR_SIZE,
- (total_sectors2 - sector_num) *
- BDRV_SECTOR_SIZE,
- &pnum2, NULL, NULL);
+ status2 = bdrv_block_status_above(bs2, NULL, offset,
+ total_size2 - offset, &pnum2, NULL,
+ NULL);
if (status2 < 0) {
ret = 3;
error_report("Sector allocation test failed for %s", filename2);
goto out;
}
allocated2 = status2 & BDRV_BLOCK_ALLOCATED;
- /* TODO: Relax this once comparison is byte-based, and we no longer
- * have to worry about sector alignment */
- assert(QEMU_IS_ALIGNED(pnum1 | pnum2, BDRV_SECTOR_SIZE));
assert(pnum1 && pnum2);
- nb_sectors = MIN(pnum1, pnum2) >> BDRV_SECTOR_BITS;
+ chunk = MIN(pnum1, pnum2);
if (strict) {
if (status1 != status2) {
ret = 1;
qprintf(quiet, "Strict mode: Offset %" PRId64
- " block status mismatch!\n",
- sectors_to_bytes(sector_num));
+ " block status mismatch!\n", offset);
goto out;
}
}
@@ -1438,59 +1425,54 @@ static int img_compare(int argc, char **argv)
if (allocated1) {
int64_t pnum;
- nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
- ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,
- nb_sectors << BDRV_SECTOR_BITS);
+ chunk = MIN(chunk, IO_BUF_SIZE);
+ ret = blk_pread(blk1, offset, buf1, chunk);
if (ret < 0) {
- error_report("Error while reading offset %" PRId64 " of %s:"
- " %s", sectors_to_bytes(sector_num), filename1,
- strerror(-ret));
+ error_report("Error while reading offset %" PRId64
+ " of %s: %s",
+ offset, filename1, strerror(-ret));
ret = 4;
goto out;
}
- ret = blk_pread(blk2, sector_num << BDRV_SECTOR_BITS, buf2,
- nb_sectors << BDRV_SECTOR_BITS);
+ ret = blk_pread(blk2, offset, buf2, chunk);
if (ret < 0) {
error_report("Error while reading offset %" PRId64
- " of %s: %s", sectors_to_bytes(sector_num),
- filename2, strerror(-ret));
+ " of %s: %s",
+ offset, filename2, strerror(-ret));
ret = 4;
goto out;
}
- ret = compare_buffers(buf1, buf2,
- nb_sectors * BDRV_SECTOR_SIZE, &pnum);
- if (ret || pnum != nb_sectors * BDRV_SECTOR_SIZE) {
+ ret = compare_buffers(buf1, buf2, chunk, &pnum);
+ if (ret || pnum != chunk) {
qprintf(quiet, "Content mismatch at offset %" PRId64 "!\n",
- sectors_to_bytes(sector_num) + (ret ? 0 : pnum));
+ offset + (ret ? 0 : pnum));
ret = 1;
goto out;
}
}
} else {
- nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
+ chunk = MIN(chunk, IO_BUF_SIZE);
if (allocated1) {
- ret = check_empty_sectors(blk1, sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE,
+ ret = check_empty_sectors(blk1, offset, chunk,
filename1, buf1, quiet);
} else {
- ret = check_empty_sectors(blk2, sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE,
+ ret = check_empty_sectors(blk2, offset, chunk,
filename2, buf1, quiet);
}
if (ret) {
goto out;
}
}
- sector_num += nb_sectors;
- qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ offset += chunk;
+ qemu_progress_print(((float) chunk / progress_base) * 100, 100);
}
- if (total_sectors1 != total_sectors2) {
+ if (total_size1 != total_size2) {
BlockBackend *blk_over;
const char *filename_over;
qprintf(quiet, "Warning: Image size mismatch!\n");
- if (total_sectors1 > total_sectors2) {
+ if (total_size1 > total_size2) {
blk_over = blk1;
filename_over = filename1;
} else {
@@ -1498,14 +1480,10 @@ static int img_compare(int argc, char **argv)
filename_over = filename2;
}
- while (sector_num < progress_base) {
- int64_t count;
-
- ret = bdrv_block_status_above(blk_bs(blk_over), NULL,
- sector_num * BDRV_SECTOR_SIZE,
- (progress_base - sector_num) *
- BDRV_SECTOR_SIZE,
- &count, NULL, NULL);
+ while (offset < progress_base) {
+ ret = bdrv_block_status_above(blk_bs(blk_over), NULL, offset,
+ progress_base - offset, &chunk,
+ NULL, NULL);
if (ret < 0) {
ret = 3;
error_report("Sector allocation test failed for %s",
@@ -1513,22 +1491,16 @@ static int img_compare(int argc, char **argv)
goto out;
}
- /* TODO relax this once bdrv_block_status_above does not enforce
- * sector alignment */
- assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
- nb_sectors = count >> BDRV_SECTOR_BITS;
if (ret & BDRV_BLOCK_ALLOCATED && !(ret & BDRV_BLOCK_ZERO)) {
- nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);
- ret = check_empty_sectors(blk_over,
- sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE,
+ chunk = MIN(chunk, IO_BUF_SIZE);
+ ret = check_empty_sectors(blk_over, offset, chunk,
filename_over, buf1, quiet);
if (ret) {
goto out;
}
}
- sector_num += nb_sectors;
- qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+ offset += chunk;
+ qemu_progress_print(((float) chunk / progress_base) * 100, 100);
}
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 21/24] block: Align block status requests
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (19 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 20/24] qemu-img: Change img_compare() " Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 22/24] block: Relax bdrv_aligned_preadv() assertion Eric Blake
` (3 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz, Stefan Hajnoczi
Any device that has request_alignment greater than 512 should be
unable to report status at a finer granularity; it may also be
simpler for such devices to be guaranteed that the block layer
has rounded things out to the granularity boundary (the way the
block layer already rounds all other I/O out). Besides, getting
the code correct for super-sector alignment also benefits us
for the fact that our public interface now has byte granularity,
even though none of our drivers have byte-level callbacks.
Add an assertion in blkdebug that proves that the block layer
never requests status of unaligned sections, similar to what it
does on other requests (while still keeping the generic helper
in place for when future patches add a throttle driver). Note
that iotest 177 already covers this (it would fail if you use
just the blkdebug.c hunk without the io.c changes). Meanwhile,
we can drop assertions in callers that no longer have to pass
in sector-aligned addresses.
There is a mid-function scope added for 'count' and 'longret',
for a couple of reasons: first, an upcoming patch will add an
'if' statement that checks whether a driver has an old- or
new-style callback, and can conveniently use the same scope for
less indentation churn at that time. Second, since we are
trying to get rid of sector-based computations, wrapping things
in a scope makes it easier to group and see what will be
deleted in a final cleanup patch once all drivers have been
converted to the new-style callback.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to split return interface change [Kevin]
v5: rebase to earlier changes, add more comments
v4: no change
v3: tweak commit message [Fam], rebase to context conflicts, ensure
we don't exceed 32-bit limit, drop R-b
v2: new patch
---
include/block/block_int.h | 3 +-
block/io.c | 71 ++++++++++++++++++++++++++++++-----------------
block/blkdebug.c | 13 ++++++++-
3 files changed, 59 insertions(+), 28 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 489b390fd6..4b9b23a08d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -207,7 +207,8 @@ struct BlockDriver {
* according to the current layer, and should not set
* BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
* for the meaning of _DATA, _ZERO, and _OFFSET_VALID. The block
- * layer guarantees non-NULL pnum and file.
+ * layer guarantees input aligned to request_alignment, as well as
+ * non-NULL pnum and file.
*/
int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *pnum,
diff --git a/block/io.c b/block/io.c
index f0bd6aa63e..35431dc9dd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1819,10 +1819,11 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
{
int64_t total_size;
int64_t n; /* bytes */
- int64_t ret;
+ int ret;
int64_t local_map = 0;
BlockDriverState *local_file = NULL;
- int count; /* sectors */
+ int64_t aligned_offset, aligned_bytes;
+ uint32_t align;
assert(pnum);
*pnum = 0;
@@ -1861,35 +1862,58 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
}
bdrv_inc_in_flight(bs);
+
+ /* Round out to request_alignment boundaries */
+ /* TODO: until we have a byte-based driver callback, we also have to
+ * round out to sectors, even if that is bigger than request_alignment */
+ align = MAX(bs->bl.request_alignment, BDRV_SECTOR_SIZE);
+ aligned_offset = QEMU_ALIGN_DOWN(offset, align);
+ aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
+
+ {
+ int count; /* sectors */
+ int64_t longret;
+
+ assert(QEMU_IS_ALIGNED(aligned_offset | aligned_bytes,
+ BDRV_SECTOR_SIZE));
+ /*
+ * The contract allows us to return pnum smaller than bytes, even
+ * if the next query would see the same status; we truncate the
+ * request to avoid overflowing the driver's 32-bit interface.
+ */
+ longret = bs->drv->bdrv_co_get_block_status(
+ bs, aligned_offset >> BDRV_SECTOR_BITS,
+ MIN(INT_MAX, aligned_bytes) >> BDRV_SECTOR_BITS, &count,
+ &local_file);
+ if (longret < 0) {
+ assert(INT_MIN <= longret);
+ ret = longret;
+ goto out;
+ }
+ if (longret & BDRV_BLOCK_OFFSET_VALID) {
+ local_map = longret & BDRV_BLOCK_OFFSET_MASK;
+ }
+ ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
+ *pnum = count * BDRV_SECTOR_SIZE;
+ }
+
/*
- * TODO: Rather than require aligned offsets, we could instead
- * round to the driver's request_alignment here, then touch up
- * count afterwards back to the caller's expectations.
+ * The driver's result must be a multiple of request_alignment.
+ * Clamp pnum and adjust map to original request.
*/
- assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
- /*
- * The contract allows us to return pnum smaller than bytes, even
- * if the next query would see the same status; we truncate the
- * request to avoid overflowing the driver's 32-bit interface.
- */
- bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
- ret = bs->drv->bdrv_co_get_block_status(bs, offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS, &count,
- &local_file);
- if (ret < 0) {
- goto out;
+ assert(QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset);
+ *pnum -= offset - aligned_offset;
+ if (*pnum > bytes) {
+ *pnum = bytes;
}
if (ret & BDRV_BLOCK_OFFSET_VALID) {
- local_map = ret & BDRV_BLOCK_OFFSET_MASK;
+ local_map += offset - aligned_offset;
}
- *pnum = count * BDRV_SECTOR_SIZE;
if (ret & BDRV_BLOCK_RAW) {
assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);
ret = bdrv_co_block_status(local_file, want_zero, local_map,
*pnum, pnum, &local_map, &local_file);
- assert(ret < 0 ||
- QEMU_IS_ALIGNED(*pnum | local_map, BDRV_SECTOR_SIZE));
goto out;
}
@@ -1948,11 +1972,6 @@ early_out:
if (map) {
*map = local_map;
}
- if (ret >= 0) {
- ret &= ~BDRV_BLOCK_OFFSET_MASK;
- } else {
- assert(INT_MIN <= ret);
- }
return ret;
}
diff --git a/block/blkdebug.c b/block/blkdebug.c
index dfdf9b91aa..e21669979d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -627,6 +627,17 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
}
+static int64_t coroutine_fn blkdebug_co_get_block_status(
+ BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
+ BlockDriverState **file)
+{
+ assert(QEMU_IS_ALIGNED(sector_num | nb_sectors,
+ DIV_ROUND_UP(bs->bl.request_alignment,
+ BDRV_SECTOR_SIZE)));
+ return bdrv_co_get_block_status_from_file(bs, sector_num, nb_sectors,
+ pnum, file);
+}
+
static void blkdebug_close(BlockDriverState *bs)
{
BDRVBlkdebugState *s = bs->opaque;
@@ -896,7 +907,7 @@ static BlockDriver bdrv_blkdebug = {
.bdrv_co_flush_to_disk = blkdebug_co_flush,
.bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes,
.bdrv_co_pdiscard = blkdebug_co_pdiscard,
- .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
+ .bdrv_co_get_block_status = blkdebug_co_get_block_status,
.bdrv_debug_event = blkdebug_debug_event,
.bdrv_debug_breakpoint = blkdebug_debug_breakpoint,
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 22/24] block: Relax bdrv_aligned_preadv() assertion
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (20 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 21/24] block: Align block status requests Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion Eric Blake
` (2 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
Now that bdrv_is_allocated accepts non-aligned inputs, we can
remove the TODO added in commit d6a644bb.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
v4-v5: no change
v3: new patch [Kevin]
---
block/io.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/block/io.c b/block/io.c
index 35431dc9dd..e4caa4acf1 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1104,18 +1104,14 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
}
if (flags & BDRV_REQ_COPY_ON_READ) {
- /* TODO: Simplify further once bdrv_is_allocated no longer
- * requires sector alignment */
- int64_t start = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
- int64_t end = QEMU_ALIGN_UP(offset + bytes, BDRV_SECTOR_SIZE);
int64_t pnum;
- ret = bdrv_is_allocated(bs, start, end - start, &pnum);
+ ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
if (ret < 0) {
goto out;
}
- if (!ret || pnum != end - start) {
+ if (!ret || pnum != bytes) {
ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
goto out;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (21 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 22/24] block: Relax bdrv_aligned_preadv() assertion Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-12 13:20 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 24/24] qemu-io: Relax 'alloc' now that block-status doesn't assert Eric Blake
2017-10-20 16:45 ` [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Kevin Wolf
24 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
Now that bdrv_is_allocated accepts non-aligned inputs, we can
remove the TODO added in earlier refactoring.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch [Kevin]
---
block/qcow2.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 092a3cabdb..92cb9f9bfa 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3003,22 +3003,16 @@ static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
int64_t nr;
int res;
- int64_t start;
-
- /* TODO: Widening to sector boundaries should only be needed as
- * long as we can't query finer granularity. */
- start = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
- bytes = QEMU_ALIGN_UP(offset + bytes, BDRV_SECTOR_SIZE) - start;
/* Clamp to image length, before checking status of underlying sectors */
- if (start + bytes > bs->total_sectors * BDRV_SECTOR_SIZE) {
- bytes = bs->total_sectors * BDRV_SECTOR_SIZE - start;
+ if (offset + bytes > bs->total_sectors * BDRV_SECTOR_SIZE) {
+ bytes = bs->total_sectors * BDRV_SECTOR_SIZE - offset;
}
if (!bytes) {
return true;
}
- res = bdrv_block_status_above(bs, NULL, start, bytes, &nr, NULL, NULL);
+ res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL, NULL);
return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == bytes;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v6 24/24] qemu-io: Relax 'alloc' now that block-status doesn't assert
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (22 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion Eric Blake
@ 2017-10-12 3:47 ` Eric Blake
2017-10-20 16:45 ` [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Kevin Wolf
24 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-12 3:47 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
Previously, the alloc command required that input parameters be
sector-aligned and clamped to 32 bits, because the underlying
bdrv_is_allocated used a 32-bit parameter and asserted aligned
inputs. But now that we have fixed block status to report a
64-bit bytes value, and to properly round requests on behalf of
guests, we can pass any values, and can use qemu-io to add
coverage that our rounding is correct regardless of the guest
alignment constraints.
Update iotest 177 to intentionally probe block status at
unaligned boundaries as well as with a bytes value that does not
map to 32-bit sectors, which also required tweaking the image
prep to leave an unallocated portion to the image under test.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v3: also test huge bytes value, R-b dropped
v2: new patch
---
qemu-io-cmds.c | 13 -------------
tests/qemu-iotests/177 | 12 ++++++++++--
tests/qemu-iotests/177.out | 19 ++++++++++++++-----
3 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3727fb43f3..de8e3de726 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1769,10 +1769,6 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
if (offset < 0) {
print_cvtnum_err(offset, argv[1]);
return 0;
- } else if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
- printf("%" PRId64 " is not a sector-aligned value for 'offset'\n",
- offset);
- return 0;
}
if (argc == 3) {
@@ -1780,19 +1776,10 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
if (count < 0) {
print_cvtnum_err(count, argv[2]);
return 0;
- } else if (count > INT_MAX * BDRV_SECTOR_SIZE) {
- printf("length argument cannot exceed %llu, given %s\n",
- INT_MAX * BDRV_SECTOR_SIZE, argv[2]);
- return 0;
}
} else {
count = BDRV_SECTOR_SIZE;
}
- if (!QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE)) {
- printf("%" PRId64 " is not a sector-aligned value for 'count'\n",
- count);
- return 0;
- }
remaining = count;
sum_alloc = 0;
diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
index f8ed8fb86b..28990977f1 100755
--- a/tests/qemu-iotests/177
+++ b/tests/qemu-iotests/177
@@ -51,7 +51,7 @@ echo "== setting up files =="
TEST_IMG="$TEST_IMG.base" _make_test_img $size
$QEMU_IO -c "write -P 11 0 $size" "$TEST_IMG.base" | _filter_qemu_io
_make_test_img -b "$TEST_IMG.base"
-$QEMU_IO -c "write -P 22 0 $size" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "write -P 22 0 110M" "$TEST_IMG" | _filter_qemu_io
# Limited to 64k max-transfer
echo
@@ -82,6 +82,13 @@ $QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
-c "discard 80000001 30M" | _filter_qemu_io
echo
+echo "== block status smaller than alignment =="
+limits=align=4k
+$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
+ -c "alloc 1 1" -c "alloc 0x6dffff0 1000" -c "alloc 127m 5P" \
+ -c map | _filter_qemu_io
+
+echo
echo "== verify image content =="
function verify_io()
@@ -103,7 +110,8 @@ function verify_io()
echo read -P 0 32M 32M
echo read -P 22 64M 13M
echo read -P $discarded 77M 29M
- echo read -P 22 106M 22M
+ echo read -P 22 106M 4M
+ echo read -P 11 110M 18M
}
verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index 43a777836c..f788b55e20 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -5,8 +5,8 @@ Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
wrote 134217728/134217728 bytes at offset 0
128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
-wrote 134217728/134217728 bytes at offset 0
-128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 115343360/115343360 bytes at offset 0
+110 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== constrained alignment and max-transfer ==
wrote 131072/131072 bytes at offset 1000
@@ -26,6 +26,13 @@ wrote 33554432/33554432 bytes at offset 33554432
discard 31457280/31457280 bytes at offset 80000001
30 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== block status smaller than alignment ==
+1/1 bytes allocated at offset 1 bytes
+16/1000 bytes allocated at offset 110 MiB
+0/1048576 bytes allocated at offset 127 MiB
+110 MiB (0x6e00000) bytes allocated at offset 0 bytes (0x0)
+18 MiB (0x1200000) bytes not allocated at offset 110 MiB (0x6e00000)
+
== verify image content ==
read 1000/1000 bytes at offset 0
1000 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -43,12 +50,14 @@ read 13631488/13631488 bytes at offset 67108864
13 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 30408704/30408704 bytes at offset 80740352
29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 23068672/23068672 bytes at offset 111149056
-22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4194304/4194304 bytes at offset 111149056
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 18874368/18874368 bytes at offset 115343360
+18 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Offset Length File
0 0x800000 TEST_DIR/t.IMGFMT
0x900000 0x2400000 TEST_DIR/t.IMGFMT
0x3c00000 0x1100000 TEST_DIR/t.IMGFMT
-0x6a00000 0x1600000 TEST_DIR/t.IMGFMT
+0x6a00000 0x400000 TEST_DIR/t.IMGFMT
No errors were found on the image.
*** done
--
2.13.6
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion Eric Blake
@ 2017-10-12 13:20 ` Eric Blake
2017-10-20 15:11 ` Kevin Wolf
0 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2017-10-12 13:20 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 511 bytes --]
On 10/11/2017 10:47 PM, Eric Blake wrote:
> Now that bdrv_is_allocated accepts non-aligned inputs, we can
> remove the TODO added in earlier refactoring.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
I was a bit too hasty in the copy-and-paste from 22/24 - there is no
assertion in this patch, so the subject would be better as:
qcow2: Reduce is_zero() rounding
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes Eric Blake
@ 2017-10-20 14:31 ` Kevin Wolf
2017-10-20 15:12 ` Eric Blake
0 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2017-10-20 14:31 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, famz, jsnow, Stefan Hajnoczi, Max Reitz
Am 12.10.2017 um 05:47 hat Eric Blake geschrieben:
> 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.
>
> There was an inherent limitation in returning the offset via the
> return value: we only have room for BDRV_BLOCK_OFFSET_MASK bits, which
> means an offset can only be mapped for sector-aligned queries (or,
> if we declare that non-aligned input is at the same relative position
> modulo 512 of the answer), so the new interface also changes things to
> return the offset via output through a parameter by reference rather
> than mashed into the return value. We'll have some glue code that
> munges between the two styles until we finish converting all uses.
>
> For the most part this patch is just the addition of scaling at the
> callers followed by inverse scaling at bdrv_block_status(), coupled
> with the tweak in calling convention. 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 <eblake@redhat.com>
>
> ---
> v6: redo interface to return byte-based offset by reference [Kevin]
> v5: drop pointless 'if (pnum)' [John], add comment
> v4: no change
> v3: clamp bytes to 32-bits, rather than asserting
> v2: rebase to earlier changes
> ---
> include/block/block.h | 17 +++++++++--------
> block/io.c | 47 ++++++++++++++++++++++++++++++++++-------------
> block/qcow2-cluster.c | 2 +-
> qemu-img.c | 25 ++++++++++++++-----------
> 4 files changed, 58 insertions(+), 33 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 440f3e9e39..7ac851f82f 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -121,7 +121,7 @@ typedef struct HDGeometry {
> #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>
> /*
> - * Allocation status flags for bdrv_get_block_status() and friends.
> + * Allocation status flags for bdrv_block_status() and friends.
> *
> * Public flags:
> * BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer
> @@ -136,10 +136,11 @@ typedef struct HDGeometry {
> * that the block layer recompute the answer from the returned
> * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
> *
> - * 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:
> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) of
> + * the return value (old interface) or the entire map parameter (new
> + * interface) represent the offset in the returned BDS that is allocated for
> + * the corresponding raw data.
Are there any functions using the old interface left at the end of the
series or do we want a final patch that removes the old interface from
the description?
Kevin
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes Eric Blake
@ 2017-10-20 15:03 ` Kevin Wolf
2017-10-20 15:22 ` Eric Blake
0 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2017-10-20 15:03 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, famz, jsnow, Stefan Hajnoczi, Max Reitz,
Jeff Cody
Am 12.10.2017 um 05:47 hat Eric Blake geschrieben:
> 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_above()
> to bdrv_block_status_above() ensures that the compiler enforces that
> all callers are updated. Likewise, since it a byte interface allows
> an offset mapping that might not be sector aligned, split the mapping
> out of the return value and into a pass-by-reference parameter. For
> now, the io.c layer still assert()s that all uses are sector-aligned,
> but that can be relaxed when a later patch implements byte-based
> block status in the drivers.
>
> For the most part this patch is just the addition of scaling at the
> callers followed by inverse scaling at bdrv_block_status(), plus
> updates for the new split return interface. But some code,
> particularly bdrv_block_status(), gets a lot simpler because it no
> longer has to mess with sectors. Likewise, mirror code no longer
> computes s->granularity >> BDRV_SECTOR_BITS, and can therefore drop
> an assertion about alignment because the loop no longer depends on
> alignment (never mind that we don't really have a driver that
> reports sub-sector alignments, so it's not really possible to test
> the effect of sub-sector mirroring). Fix a neighboring assertion to
> use is_power_of_2 while there.
>
> For ease of review, bdrv_get_block_status() was tackled separately.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
> int64_t *pnum, int64_t *map, BlockDriverState **file)
> {
> - int64_t ret;
> - int n;
> -
> - assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> - assert(pnum);
> - /*
> - * The contract allows us to return pnum smaller than bytes, even
> - * if the next query would see the same status; we truncate the
> - * request to avoid overflowing the driver's 32-bit interface.
> - */
> - bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
Is the limitation to BDRV_REQUEST_MAX_BYTES going away without being
replaced by a new one in bdrv_co_block_status()? What protects us now
from 32-bit truncation?
Kevin
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion
2017-10-12 13:20 ` Eric Blake
@ 2017-10-20 15:11 ` Kevin Wolf
2017-10-20 15:32 ` Eric Blake
0 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2017-10-20 15:11 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, jsnow, famz, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 533 bytes --]
Am 12.10.2017 um 15:20 hat Eric Blake geschrieben:
> On 10/11/2017 10:47 PM, Eric Blake wrote:
> > Now that bdrv_is_allocated accepts non-aligned inputs, we can
> > remove the TODO added in earlier refactoring.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
>
> I was a bit too hasty in the copy-and-paste from 22/24 - there is no
> assertion in this patch, so the subject would be better as:
>
> qcow2: Reduce is_zero() rounding
In fact, I don't see any assertion being modified in patch 22 either.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes
2017-10-20 14:31 ` Kevin Wolf
@ 2017-10-20 15:12 ` Eric Blake
2017-10-20 15:31 ` Kevin Wolf
0 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2017-10-20 15:12 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, qemu-block, famz, jsnow, Stefan Hajnoczi, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]
On 10/20/2017 09:31 AM, Kevin Wolf wrote:
> Am 12.10.2017 um 05:47 hat Eric Blake geschrieben:
>> 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.
>>
>> *
>> - * 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:
>> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) of
>> + * the return value (old interface) or the entire map parameter (new
>> + * interface) represent the offset in the returned BDS that is allocated for
>> + * the corresponding raw data.
>
> Are there any functions using the old interface left at the end of the
> series or do we want a final patch that removes the old interface from
> the description?
Done here, in series 4, where we get rid of the old interface in the
drivers:
https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02941.html
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes
2017-10-20 15:03 ` Kevin Wolf
@ 2017-10-20 15:22 ` Eric Blake
2017-10-20 15:34 ` Kevin Wolf
0 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2017-10-20 15:22 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, qemu-block, famz, jsnow, Stefan Hajnoczi, Max Reitz,
Jeff Cody
[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]
On 10/20/2017 10:03 AM, Kevin Wolf wrote:
> Am 12.10.2017 um 05:47 hat Eric Blake geschrieben:
>> 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.
>>
>> int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
>> int64_t *pnum, int64_t *map, BlockDriverState **file)
>> {
>> - int64_t ret;
>> - int n;
>> -
>> - assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
>> - assert(pnum);
>> - /*
>> - * The contract allows us to return pnum smaller than bytes, even
>> - * if the next query would see the same status; we truncate the
>> - * request to avoid overflowing the driver's 32-bit interface.
>> - */
>> - bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
>
> Is the limitation to BDRV_REQUEST_MAX_BYTES going away without being
> replaced by a new one in bdrv_co_block_status()? What protects us now
> from 32-bit truncation?
As of this patch, we have this 64-bit-clean call chain:
bdrv_block_status()
bdrv_block_status_above()
bdrv_common_block_status_above()
bdrv_block_status_above_co_entry()
bdrv_co_block_status_above()
bdrv_co_block_status()
which in turn has:
/*
* The contract allows us to return pnum smaller than bytes, even
* if the next query would see the same status; we truncate the
* request to avoid overflowing the driver's 32-bit interface.
*/
bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
added earlier in 8/24.
At the end of series 3, the truncation prevention is still in place when
calling into the drivers, per the changes to bdrv_co_block_status() in
21/24.
Later, in series 4, all drivers are converted to 64-bit interfaces,
where the drivers are either made 64-bit clean, or do their own
truncation to 32 bits.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes
2017-10-20 15:12 ` Eric Blake
@ 2017-10-20 15:31 ` Kevin Wolf
0 siblings, 0 replies; 37+ messages in thread
From: Kevin Wolf @ 2017-10-20 15:31 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, famz, jsnow, Stefan Hajnoczi, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]
Am 20.10.2017 um 17:12 hat Eric Blake geschrieben:
> On 10/20/2017 09:31 AM, Kevin Wolf wrote:
> > Am 12.10.2017 um 05:47 hat Eric Blake geschrieben:
> >> 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.
> >>
>
> >> *
> >> - * 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:
> >> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) of
> >> + * the return value (old interface) or the entire map parameter (new
> >> + * interface) represent the offset in the returned BDS that is allocated for
> >> + * the corresponding raw data.
> >
> > Are there any functions using the old interface left at the end of the
> > series or do we want a final patch that removes the old interface from
> > the description?
>
> Done here, in series 4, where we get rid of the old interface in the
> drivers:
> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02941.html
Oh, right, I forgot about the driver callbacks. Makes sense.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion
2017-10-20 15:11 ` Kevin Wolf
@ 2017-10-20 15:32 ` Eric Blake
0 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-10-20 15:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, jsnow, famz, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]
On 10/20/2017 10:11 AM, Kevin Wolf wrote:
> Am 12.10.2017 um 15:20 hat Eric Blake geschrieben:
>> On 10/11/2017 10:47 PM, Eric Blake wrote:
>>> Now that bdrv_is_allocated accepts non-aligned inputs, we can
>>> remove the TODO added in earlier refactoring.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> I was a bit too hasty in the copy-and-paste from 22/24 - there is no
>> assertion in this patch, so the subject would be better as:
>>
>> qcow2: Reduce is_zero() rounding
>
> In fact, I don't see any assertion being modified in patch 22 either.
Oh, indeed. Not sure what I was thinking; I guess it's because I added
22 at the same time I added several other TODO assertions to series 1 a
couple months ago, with the plans to relax rounding/assertions as
appropriate later in the series, and then got confused on which things
were being relaxed where ;)
If it's just the subject lines for 22 and 23 that need fixing, I trust
you can do that as maintainer.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes
2017-10-20 15:22 ` Eric Blake
@ 2017-10-20 15:34 ` Kevin Wolf
0 siblings, 0 replies; 37+ messages in thread
From: Kevin Wolf @ 2017-10-20 15:34 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, famz, jsnow, Stefan Hajnoczi, Max Reitz,
Jeff Cody
[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]
Am 20.10.2017 um 17:22 hat Eric Blake geschrieben:
> On 10/20/2017 10:03 AM, Kevin Wolf wrote:
> > Am 12.10.2017 um 05:47 hat Eric Blake geschrieben:
> >> 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.
> >>
>
> >> int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
> >> int64_t *pnum, int64_t *map, BlockDriverState **file)
> >> {
> >> - int64_t ret;
> >> - int n;
> >> -
> >> - assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> >> - assert(pnum);
> >> - /*
> >> - * The contract allows us to return pnum smaller than bytes, even
> >> - * if the next query would see the same status; we truncate the
> >> - * request to avoid overflowing the driver's 32-bit interface.
> >> - */
> >> - bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
> >
> > Is the limitation to BDRV_REQUEST_MAX_BYTES going away without being
> > replaced by a new one in bdrv_co_block_status()? What protects us now
> > from 32-bit truncation?
>
> As of this patch, we have this 64-bit-clean call chain:
> bdrv_block_status()
> bdrv_block_status_above()
> bdrv_common_block_status_above()
> bdrv_block_status_above_co_entry()
> bdrv_co_block_status_above()
> bdrv_co_block_status()
>
> which in turn has:
> /*
> * The contract allows us to return pnum smaller than bytes, even
> * if the next query would see the same status; we truncate the
> * request to avoid overflowing the driver's 32-bit interface.
> */
> bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
>
> added earlier in 8/24.
>
> At the end of series 3, the truncation prevention is still in place when
> calling into the drivers, per the changes to bdrv_co_block_status() in
> 21/24.
I looked at the final state and missed it because now INT_MAX is used
rather than BDRV_REQUEST_MAX_BYTES. Anyway, seems to be okay then.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
` (23 preceding siblings ...)
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 24/24] qemu-io: Relax 'alloc' now that block-status doesn't assert Eric Blake
@ 2017-10-20 16:45 ` Kevin Wolf
2017-11-02 21:01 ` John Snow
24 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2017-10-20 16:45 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, qemu-block, famz, jsnow
Am 12.10.2017 um 05:46 hat Eric Blake geschrieben:
> There are patches floating around to add NBD_CMD_BLOCK_STATUS,
> but NBD wants to report status on byte granularity (even if the
> reporting will probably be naturally aligned to sectors or even
> much higher levels). I've therefore started the task of
> converting our block status code to report at a byte granularity
> rather than sectors.
>
> Now that 2.11 is open, I'm rebasing/reposting the remaining patches.
>
> The overall conversion currently looks like:
> part 1: bdrv_is_allocated (merged, commit 51b0a488)
> part 2: dirty-bitmap (merged, commit ca759622)
> part 3: bdrv_get_block_status (this series, v5 at [1])
> part 4: .bdrv_co_block_status (v3 is posted [2], mostly reviewed, but needs rebase)
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based
2017-10-20 16:45 ` [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Kevin Wolf
@ 2017-11-02 21:01 ` John Snow
2017-11-02 22:14 ` Eric Blake
0 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2017-11-02 21:01 UTC (permalink / raw)
To: Kevin Wolf, Eric Blake; +Cc: famz, qemu-devel, qemu-block
On 10/20/2017 12:45 PM, Kevin Wolf wrote:
> Am 12.10.2017 um 05:46 hat Eric Blake geschrieben:
>> There are patches floating around to add NBD_CMD_BLOCK_STATUS,
>> but NBD wants to report status on byte granularity (even if the
>> reporting will probably be naturally aligned to sectors or even
>> much higher levels). I've therefore started the task of
>> converting our block status code to report at a byte granularity
>> rather than sectors.
>>
>> Now that 2.11 is open, I'm rebasing/reposting the remaining patches.
>>
>> The overall conversion currently looks like:
>> part 1: bdrv_is_allocated (merged, commit 51b0a488)
>> part 2: dirty-bitmap (merged, commit ca759622)
>> part 3: bdrv_get_block_status (this series, v5 at [1])
>> part 4: .bdrv_co_block_status (v3 is posted [2], mostly reviewed, but needs rebase)
>
> Thanks, applied to the block branch.
>
> Kevin
>
Thanks;
Eric: does this just leave part four?
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based
2017-11-02 21:01 ` John Snow
@ 2017-11-02 22:14 ` Eric Blake
0 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2017-11-02 22:14 UTC (permalink / raw)
To: John Snow, Kevin Wolf; +Cc: famz, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 723 bytes --]
On 11/02/2017 04:01 PM, John Snow wrote:
>>> The overall conversion currently looks like:
>>> part 1: bdrv_is_allocated (merged, commit 51b0a488)
>>> part 2: dirty-bitmap (merged, commit ca759622)
>>> part 3: bdrv_get_block_status (this series, v5 at [1])
>>> part 4: .bdrv_co_block_status (v3 is posted [2], mostly reviewed, but needs rebase)
>>
>> Thanks, applied to the block branch.
>>
>> Kevin
>>
>
> Thanks;
>
> Eric: does this just leave part four?
Yes; the latest version of part four is v4:
https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02940.html
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2017-11-02 22:14 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-12 3:46 [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 01/24] block: Allow NULL file for bdrv_get_block_status() Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 02/24] block: Add flag to avoid wasted work in bdrv_is_allocated() Eric Blake
2017-10-12 3:46 ` [Qemu-devel] [PATCH v6 03/24] block: Make bdrv_round_to_clusters() signature more useful Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 04/24] qcow2: Switch is_zero_sectors() to byte-based Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 05/24] block: Switch bdrv_make_zero() " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 06/24] qemu-img: Switch get_block_status() " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 07/24] block: Convert bdrv_get_block_status() to bytes Eric Blake
2017-10-20 14:31 ` Kevin Wolf
2017-10-20 15:12 ` Eric Blake
2017-10-20 15:31 ` Kevin Wolf
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 08/24] block: Switch bdrv_co_get_block_status() to byte-based Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 09/24] block: Switch BdrvCoGetBlockStatusData " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 10/24] block: Switch bdrv_common_block_status_above() " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 11/24] block: Switch bdrv_co_get_block_status_above() " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 12/24] block: Convert bdrv_get_block_status_above() to bytes Eric Blake
2017-10-20 15:03 ` Kevin Wolf
2017-10-20 15:22 ` Eric Blake
2017-10-20 15:34 ` Kevin Wolf
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 13/24] qemu-img: Simplify logic in img_compare() Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 14/24] qemu-img: Speed up compare on pre-allocated larger file Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 15/24] qemu-img: Add find_nonzero() Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 16/24] qemu-img: Drop redundant error message in compare Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 17/24] qemu-img: Change check_empty_sectors() to byte-based Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 18/24] qemu-img: Change compare_sectors() to be byte-based Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 19/24] qemu-img: Change img_rebase() " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 20/24] qemu-img: Change img_compare() " Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 21/24] block: Align block status requests Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 22/24] block: Relax bdrv_aligned_preadv() assertion Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 23/24] qcow2: Relax is_zero() assertion Eric Blake
2017-10-12 13:20 ` Eric Blake
2017-10-20 15:11 ` Kevin Wolf
2017-10-20 15:32 ` Eric Blake
2017-10-12 3:47 ` [Qemu-devel] [PATCH v6 24/24] qemu-io: Relax 'alloc' now that block-status doesn't assert Eric Blake
2017-10-20 16:45 ` [Qemu-devel] [PATCH v6 00/24] make bdrv_get_block_status byte-based Kevin Wolf
2017-11-02 21:01 ` John Snow
2017-11-02 22:14 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.