* [Qemu-devel] [PATCH v8 01/21] block: Add .bdrv_co_block_status() callback
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status() Eric Blake
` (20 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Now that the block layer exposes byte-based allocation,
it's time to tackle the drivers. Add a new callback that operates
on as small as byte boundaries. Subsequent patches will then update
individual drivers, then finally remove .bdrv_co_get_block_status().
The new code also passes through the 'want_zero' hint, which will
allow subsequent patches to further optimize callers that only care
about how much of the image is allocated (want_zero is false),
rather than full details about runs of zeroes and which offsets the
allocation actually maps to (want_zero is true). As part of this
effort, fix another part of the documentation: the claim in commit
4c41cb4 that BDRV_BLOCK_ALLOCATED is short for 'DATA || ZERO' is a
lie at the block layer (see commit e88ae2264), even though it is
how the bit is computed from the driver layer. After all, there
are intentionally cases where we return ZERO but not ALLOCATED at
the block layer, when we know that a read sees zero because the
backing file is too short. Note that the driver interface is thus
slightly different than the public interface with regards to which
bits will be set, and what guarantees are provided on input.
We also add an assertion that any driver using the new callback will
make progress (the only time pnum will be 0 is if the block layer
already handled an out-of-bounds request, or if there is an error);
the old driver interface did not provide this guarantee, which
could lead to some inf-loops in drastic corner-case failures.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v7: add R-b
v6: drop now-useless rounding of mid-sector end-of-file hole [Kevin],
better documentation of 'want_zero' [Kevin]
v5: rebase to master, typo fix, document more block layer guarantees
v4: rebase to master
v3: no change
v2: improve alignment handling, ensure all iotests still pass
---
include/block/block.h | 14 +++++++-------
include/block/block_int.h | 20 +++++++++++++++-----
block/io.c | 28 +++++++++++++++++++---------
3 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 19b3ab9cb5e..947e8876cdd 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -115,19 +115,19 @@ typedef struct HDGeometry {
* BDRV_BLOCK_ZERO: offset reads as zero
* BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
* BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
- * layer (short for DATA || ZERO), set by block layer
- * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this layer
+ * layer rather than any backing, set by block layer
+ * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this
+ * layer, set by block layer
*
* Internal flag:
* BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
* 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) 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:
+ * If BDRV_BLOCK_OFFSET_VALID is set, the map parameter represents the
+ * host offset within the returned BDS that is allocated for the
+ * corresponding raw guest 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
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5ea63f8fa8a..c93722b43a4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -202,15 +202,25 @@ struct BlockDriver {
/*
* 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 input aligned to request_alignment, as well as
- * non-NULL pnum and file.
+ * according to the current layer, and should only need to set
+ * BDRV_BLOCK_DATA, BDRV_BLOCK_ZERO, BDRV_BLOCK_OFFSET_VALID,
+ * and/or BDRV_BLOCK_RAW; if the current layer defers to a backing
+ * layer, the result should be 0 (and not BDRV_BLOCK_ZERO). See
+ * block.h for the overall meaning of the bits. As a hint, the
+ * flag want_zero is true if the caller cares more about precise
+ * mappings (favor accurate _OFFSET_VALID/_ZERO) or false for
+ * overall allocation (favor larger *pnum, perhaps by reporting
+ * _DATA instead of _ZERO). The block layer guarantees input
+ * clamped to bdrv_getlength() and aligned to request_alignment,
+ * as well as non-NULL pnum, map, and file; in turn, the driver
+ * must return an error or set pnum to an aligned non-zero value.
*/
int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *pnum,
BlockDriverState **file);
+ 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);
/*
* Invalidate any cached meta-data.
diff --git a/block/io.c b/block/io.c
index 89d0745e952..b00c7e2e2c0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1899,10 +1899,10 @@ 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 'want_zero' is true, the caller is querying for mapping
+ * purposes, with a focus on valid BDRV_BLOCK_OFFSET_VALID, _DATA, and
+ * _ZERO where possible; otherwise, the result favors larger 'pnum',
+ * with a focus on accurate BDRV_BLOCK_ALLOCATED.
*
* If 'offset' is beyond the end of the disk image the return value is
* BDRV_BLOCK_EOF and 'pnum' is set to 0.
@@ -1959,7 +1959,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
/* Must be non-NULL or bdrv_getlength() would have failed */
assert(bs->drv);
- if (!bs->drv->bdrv_co_get_block_status) {
+ if (!bs->drv->bdrv_co_get_block_status && !bs->drv->bdrv_co_block_status) {
*pnum = bytes;
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
if (offset + bytes == total_size) {
@@ -1976,13 +1976,14 @@ 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);
+ align = bs->bl.request_alignment;
+ if (bs->drv->bdrv_co_get_block_status && align < BDRV_SECTOR_SIZE) {
+ align = BDRV_SECTOR_SIZE;
+ }
aligned_offset = QEMU_ALIGN_DOWN(offset, align);
aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
- {
+ if (bs->drv->bdrv_co_get_block_status) {
int count; /* sectors */
int64_t longret;
@@ -2007,6 +2008,15 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
}
ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
*pnum = count * BDRV_SECTOR_SIZE;
+ } else {
+ ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
+ aligned_bytes, pnum, &local_map,
+ &local_file);
+ if (ret < 0) {
+ *pnum = 0;
+ goto out;
+ }
+ assert(*pnum); /* The block driver must make progress */
}
/*
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 01/21] block: Add .bdrv_co_block_status() callback Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-14 17:41 ` Philippe Mathieu-Daudé
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 03/21] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
` (19 subsequent siblings)
21 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
Commit bdd6a90 has a bug: drivers should never directly set
BDRV_BLOCK_ALLOCATED, but only io.c should do that (as needed).
Instead, drivers should report BDRV_BLOCK_DATA if it knows that
data comes from this BDS.
But let's look at the bigger picture: semantically, the nvme
driver is similar to the nbd, null, and raw drivers (no backing
file, all data comes from this BDS). But while two of those
other drivers have to supply the callback (null because it can
special-case BDRV_BLOCK_ZERO, raw because it can special-case
a different offset), in this case the block layer defaults are
good enough without the callback at all (similar to nbd).
So, fix the bug by deletion ;)
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: new patch
---
block/nvme.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 10bffbbf2f4..4e561b08df3 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1068,18 +1068,6 @@ static int nvme_reopen_prepare(BDRVReopenState *reopen_state,
return 0;
}
-static int64_t coroutine_fn nvme_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
-{
- *pnum = nb_sectors;
- *file = bs;
-
- return BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID |
- (sector_num << BDRV_SECTOR_BITS);
-}
-
static void nvme_refresh_filename(BlockDriverState *bs, QDict *opts)
{
QINCREF(opts);
@@ -1179,8 +1167,6 @@ static BlockDriver bdrv_nvme = {
.bdrv_co_flush_to_disk = nvme_co_flush,
.bdrv_reopen_prepare = nvme_reopen_prepare,
- .bdrv_co_get_block_status = nvme_co_get_block_status,
-
.bdrv_refresh_filename = nvme_refresh_filename,
.bdrv_refresh_limits = nvme_refresh_limits,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status()
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status() Eric Blake
@ 2018-02-14 17:41 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-14 17:41 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
On 02/13/2018 05:26 PM, Eric Blake wrote:
> Commit bdd6a90 has a bug: drivers should never directly set
> BDRV_BLOCK_ALLOCATED, but only io.c should do that (as needed).
Doesn't "pointless" in subject hide this is a bugfix?
> Instead, drivers should report BDRV_BLOCK_DATA if it knows that
> data comes from this BDS.
>
> But let's look at the bigger picture: semantically, the nvme
> driver is similar to the nbd, null, and raw drivers (no backing
> file, all data comes from this BDS). But while two of those
> other drivers have to supply the callback (null because it can
> special-case BDRV_BLOCK_ZERO, raw because it can special-case
> a different offset), in this case the block layer defaults are
> good enough without the callback at all (similar to nbd).
>
> So, fix the bug by deletion ;)
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: new patch
> ---
> block/nvme.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 10bffbbf2f4..4e561b08df3 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -1068,18 +1068,6 @@ static int nvme_reopen_prepare(BDRVReopenState *reopen_state,
> return 0;
> }
>
> -static int64_t coroutine_fn nvme_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num,
> - int nb_sectors, int *pnum,
> - BlockDriverState **file)
> -{
> - *pnum = nb_sectors;
> - *file = bs;
> -
> - return BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID |
> - (sector_num << BDRV_SECTOR_BITS);
> -}
> -
> static void nvme_refresh_filename(BlockDriverState *bs, QDict *opts)
> {
> QINCREF(opts);
> @@ -1179,8 +1167,6 @@ static BlockDriver bdrv_nvme = {
> .bdrv_co_flush_to_disk = nvme_co_flush,
> .bdrv_reopen_prepare = nvme_reopen_prepare,
>
> - .bdrv_co_get_block_status = nvme_co_get_block_status,
> -
> .bdrv_refresh_filename = nvme_refresh_filename,
> .bdrv_refresh_limits = nvme_refresh_limits,
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH v8 03/21] block: Switch passthrough drivers to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 01/21] block: Add .bdrv_co_block_status() callback Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status() Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 04/21] file-posix: Switch " Eric Blake
` (18 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz, Jeff Cody, Stefan Hajnoczi
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the generic helpers, and all passthrough clients
(blkdebug, commit, mirror, throttle) accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: rebase to master
v4: rebase to interface tweak
v3: rebase to addition of throttle driver
v2: rebase to master, retitle while merging blkdebug, commit, and mirror
---
include/block/block_int.h | 28 ++++++++++++++++------------
block/io.c | 36 ++++++++++++++++++++----------------
block/blkdebug.c | 20 +++++++++++---------
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/throttle.c | 2 +-
6 files changed, 50 insertions(+), 40 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c93722b43a4..bf2598856cf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1041,23 +1041,27 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
uint64_t *nperm, uint64_t *nshared);
/*
- * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * Default implementation for drivers to pass bdrv_co_block_status() to
* their file.
*/
-int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors,
- int *pnum,
- BlockDriverState **file);
+int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file);
/*
- * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * Default implementation for drivers to pass bdrv_co_block_status() to
* their backing file.
*/
-int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors,
- int *pnum,
- BlockDriverState **file);
+int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file);
const char *bdrv_get_parent_name(const BlockDriverState *bs);
void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
bool blk_dev_has_removable_media(BlockBackend *blk);
diff --git a/block/io.c b/block/io.c
index b00c7e2e2c0..5bae79f282e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1868,30 +1868,34 @@ typedef struct BdrvCoBlockStatusData {
bool done;
} BdrvCoBlockStatusData;
-int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors,
- int *pnum,
- BlockDriverState **file)
+int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
assert(bs->file && bs->file->bs);
- *pnum = nb_sectors;
+ *pnum = bytes;
+ *map = offset;
*file = bs->file->bs;
- return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
- (sector_num << BDRV_SECTOR_BITS);
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
}
-int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors,
- int *pnum,
- BlockDriverState **file)
+int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
assert(bs->backing && bs->backing->bs);
- *pnum = nb_sectors;
+ *pnum = bytes;
+ *map = offset;
*file = bs->backing->bs;
- return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
- (sector_num << BDRV_SECTOR_BITS);
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
}
/*
diff --git a/block/blkdebug.c b/block/blkdebug.c
index d83f23febd7..589712475ac 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -627,15 +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)
+static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ 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);
+ assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
+ return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes,
+ pnum, map, file);
}
static void blkdebug_close(BlockDriverState *bs)
@@ -907,7 +909,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 = blkdebug_co_get_block_status,
+ .bdrv_co_block_status = blkdebug_co_block_status,
.bdrv_debug_event = blkdebug_debug_event,
.bdrv_debug_breakpoint = blkdebug_debug_breakpoint,
diff --git a/block/commit.c b/block/commit.c
index bb6c904704d..1943c9c3e16 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -265,7 +265,7 @@ static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
static BlockDriver bdrv_commit_top = {
.format_name = "commit_top",
.bdrv_co_preadv = bdrv_commit_top_preadv,
- .bdrv_co_get_block_status = bdrv_co_get_block_status_from_backing,
+ .bdrv_co_block_status = bdrv_co_block_status_from_backing,
.bdrv_refresh_filename = bdrv_commit_top_refresh_filename,
.bdrv_close = bdrv_commit_top_close,
.bdrv_child_perm = bdrv_commit_top_child_perm,
diff --git a/block/mirror.c b/block/mirror.c
index c9badc1203b..f5bf620942f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1094,7 +1094,7 @@ static BlockDriver bdrv_mirror_top = {
.bdrv_co_pwrite_zeroes = bdrv_mirror_top_pwrite_zeroes,
.bdrv_co_pdiscard = bdrv_mirror_top_pdiscard,
.bdrv_co_flush = bdrv_mirror_top_flush,
- .bdrv_co_get_block_status = bdrv_co_get_block_status_from_backing,
+ .bdrv_co_block_status = bdrv_co_block_status_from_backing,
.bdrv_refresh_filename = bdrv_mirror_top_refresh_filename,
.bdrv_close = bdrv_mirror_top_close,
.bdrv_child_perm = bdrv_mirror_top_child_perm,
diff --git a/block/throttle.c b/block/throttle.c
index 495f88c7521..5f4d43d0fc5 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -240,7 +240,7 @@ static BlockDriver bdrv_throttle = {
.bdrv_reopen_prepare = throttle_reopen_prepare,
.bdrv_reopen_commit = throttle_reopen_commit,
.bdrv_reopen_abort = throttle_reopen_abort,
- .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
+ .bdrv_co_block_status = bdrv_co_block_status_from_file,
.bdrv_co_drain_begin = throttle_co_drain_begin,
.bdrv_co_drain_end = throttle_co_drain_end,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 04/21] file-posix: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (2 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 03/21] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 05/21] gluster: " Eric Blake
` (17 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the file protocol driver accordingly.
In want_zero mode, we continue to report fine-grained hole
information (the caller wants as much mapping detail as possible);
but when not in that mode, the caller prefers larger *pnum and
merely cares about what offsets are allocated at this layer, rather
than where the holes live. Since holes still read as zeroes at
this layer (rather than deferring to a backing layer), we can take
the shortcut of skipping lseek(), and merely state that all bytes
are allocated.
We can also drop redundant bounds checks that are already
guaranteed by the block layer.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: drop redundant code
v4: tweak commit message [Fam], rebase to interface tweak
v3: no change
v2: tweak comment, add mapping support
---
block/file-posix.c | 62 +++++++++++++++++++++++++-----------------------------
1 file changed, 29 insertions(+), 33 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index ca49c1a98ae..f1591c38490 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2131,25 +2131,24 @@ static int find_allocation(BlockDriverState *bs, off_t start,
}
/*
- * Returns the allocation status of the specified sectors.
+ * Returns the allocation status of the specified offset.
*
- * If 'sector_num' is beyond the end of the disk image the return value is 0
- * and 'pnum' is set to 0.
+ * The block layer guarantees 'offset' and 'bytes' are within bounds.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
+ * 'pnum' is set to the number of bytes (including and immediately following
+ * the specified offset) 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
- * beyond the end of the disk image it will be clamped.
+ * 'bytes' is the max value 'pnum' should be set to.
*/
-static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn raw_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
- off_t start, data = 0, hole = 0;
- int64_t total_size;
+ off_t data = 0, hole = 0;
int ret;
ret = fd_open(bs);
@@ -2157,39 +2156,36 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
return ret;
}
- start = sector_num * BDRV_SECTOR_SIZE;
- total_size = bdrv_getlength(bs);
- if (total_size < 0) {
- return total_size;
- } else if (start >= total_size) {
- *pnum = 0;
- return 0;
- } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
- nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
+ if (!want_zero) {
+ *pnum = bytes;
+ *map = offset;
+ *file = bs;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
- ret = find_allocation(bs, start, &data, &hole);
+ ret = find_allocation(bs, offset, &data, &hole);
if (ret == -ENXIO) {
/* Trailing hole */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_ZERO;
} else if (ret < 0) {
/* No info available, so pretend there are no holes */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_DATA;
- } else if (data == start) {
- /* On a data extent, compute sectors to the end of the extent,
+ } else if (data == offset) {
+ /* On a data extent, compute bytes to the end of the extent,
* possibly including a partial sector at EOF. */
- *pnum = MIN(nb_sectors, DIV_ROUND_UP(hole - start, BDRV_SECTOR_SIZE));
+ *pnum = MIN(bytes, hole - offset);
ret = BDRV_BLOCK_DATA;
} else {
- /* On a hole, compute sectors to the beginning of the next extent. */
- assert(hole == start);
- *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
+ /* On a hole, compute bytes to the beginning of the next extent. */
+ assert(hole == offset);
+ *pnum = MIN(bytes, data - offset);
ret = BDRV_BLOCK_ZERO;
}
+ *map = offset;
*file = bs;
- return ret | BDRV_BLOCK_OFFSET_VALID | start;
+ return ret | BDRV_BLOCK_OFFSET_VALID;
}
static coroutine_fn BlockAIOCB *raw_aio_pdiscard(BlockDriverState *bs,
@@ -2282,7 +2278,7 @@ BlockDriver bdrv_file = {
.bdrv_close = raw_close,
.bdrv_create = raw_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_co_get_block_status = raw_co_get_block_status,
+ .bdrv_co_block_status = raw_co_block_status,
.bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
.bdrv_co_preadv = raw_co_preadv,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 05/21] gluster: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (3 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 04/21] file-posix: Switch " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 06/21] iscsi: Switch cluster_sectors to byte-based Eric Blake
` (16 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Jeff Cody, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the gluster driver accordingly.
In want_zero mode, we continue to report fine-grained hole
information (the caller wants as much mapping detail as possible);
but when not in that mode, the caller prefers larger *pnum and
merely cares about what offsets are allocated at this layer, rather
than where the holes live. Since holes still read as zeroes at
this layer (rather than deferring to a backing layer), we can take
the shortcut of skipping find_allocation(), and merely state that
all bytes are allocated.
We can also drop redundant bounds checks that are already
guaranteed by the block layer.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: drop redundant code
v4: tweak commit message [Fam], rebase to interface tweak
v3: no change
v2: tweak comments [Prasanna], add mapping, drop R-b
---
block/gluster.c | 70 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 34 insertions(+), 36 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 3f17b7819d2..1a07d221d17 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1362,68 +1362,66 @@ exit:
}
/*
- * Returns the allocation status of the specified sectors.
+ * Returns the allocation status of the specified offset.
*
- * If 'sector_num' is beyond the end of the disk image the return value is 0
- * and 'pnum' is set to 0.
+ * The block layer guarantees 'offset' and 'bytes' are within bounds.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
+ * 'pnum' is set to the number of bytes (including and immediately following
+ * the specified offset) 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
- * beyond the end of the disk image it will be clamped.
+ * 'bytes' is the max value 'pnum' should be set to.
*
- * (Based on raw_co_get_block_status() from file-posix.c.)
+ * (Based on raw_co_block_status() from file-posix.c.)
*/
-static int64_t coroutine_fn qemu_gluster_co_get_block_status(
- BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BDRVGlusterState *s = bs->opaque;
- off_t start, data = 0, hole = 0;
- int64_t total_size;
+ off_t data = 0, hole = 0;
int ret = -EINVAL;
if (!s->fd) {
return ret;
}
- start = sector_num * BDRV_SECTOR_SIZE;
- total_size = bdrv_getlength(bs);
- if (total_size < 0) {
- return total_size;
- } else if (start >= total_size) {
- *pnum = 0;
- return 0;
- } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
- nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
+ if (!want_zero) {
+ *pnum = bytes;
+ *map = offset;
+ *file = bs;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
- ret = find_allocation(bs, start, &data, &hole);
+ ret = find_allocation(bs, offset, &data, &hole);
if (ret == -ENXIO) {
/* Trailing hole */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_ZERO;
} else if (ret < 0) {
/* No info available, so pretend there are no holes */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_DATA;
- } else if (data == start) {
- /* On a data extent, compute sectors to the end of the extent,
+ } else if (data == offset) {
+ /* On a data extent, compute bytes to the end of the extent,
* possibly including a partial sector at EOF. */
- *pnum = MIN(nb_sectors, DIV_ROUND_UP(hole - start, BDRV_SECTOR_SIZE));
+ *pnum = MIN(bytes, hole - offset);
ret = BDRV_BLOCK_DATA;
} else {
- /* On a hole, compute sectors to the beginning of the next extent. */
- assert(hole == start);
- *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
+ /* On a hole, compute bytes to the beginning of the next extent. */
+ assert(hole == offset);
+ *pnum = MIN(bytes, data - offset);
ret = BDRV_BLOCK_ZERO;
}
+ *map = offset;
*file = bs;
- return ret | BDRV_BLOCK_OFFSET_VALID | start;
+ return ret | BDRV_BLOCK_OFFSET_VALID;
}
@@ -1451,7 +1449,7 @@ static BlockDriver bdrv_gluster = {
#ifdef CONFIG_GLUSTERFS_ZEROFILL
.bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes,
#endif
- .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
+ .bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
};
@@ -1479,7 +1477,7 @@ static BlockDriver bdrv_gluster_tcp = {
#ifdef CONFIG_GLUSTERFS_ZEROFILL
.bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes,
#endif
- .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
+ .bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
};
@@ -1507,7 +1505,7 @@ static BlockDriver bdrv_gluster_unix = {
#ifdef CONFIG_GLUSTERFS_ZEROFILL
.bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes,
#endif
- .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
+ .bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
};
@@ -1541,7 +1539,7 @@ static BlockDriver bdrv_gluster_rdma = {
#ifdef CONFIG_GLUSTERFS_ZEROFILL
.bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes,
#endif
- .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
+ .bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
};
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 06/21] iscsi: Switch cluster_sectors to byte-based
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (4 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 05/21] gluster: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 07/21] iscsi: Switch iscsi_allocmap_update() " Eric Blake
` (15 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, Ronnie Sahlberg, Paolo Bonzini,
Peter Lieven, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert all uses of
the cluster size in sectors, along with adding assertions that we
are not dividing by zero.
Improve some comment grammar while in the area.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v8: rebase to master
v2-v7: no change
---
block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 421983dd6ff..3414c21c7f5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -86,7 +86,7 @@ typedef struct IscsiLun {
unsigned long *allocmap;
unsigned long *allocmap_valid;
long allocmap_size;
- int cluster_sectors;
+ int cluster_size;
bool use_16_for_rw;
bool write_protected;
bool lbpme;
@@ -430,9 +430,10 @@ static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
{
iscsi_allocmap_free(iscsilun);
+ assert(iscsilun->cluster_size);
iscsilun->allocmap_size =
- DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks, iscsilun),
- iscsilun->cluster_sectors);
+ DIV_ROUND_UP(iscsilun->num_blocks * iscsilun->block_size,
+ iscsilun->cluster_size);
iscsilun->allocmap = bitmap_try_new(iscsilun->allocmap_size);
if (!iscsilun->allocmap) {
@@ -440,7 +441,7 @@ static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
}
if (open_flags & BDRV_O_NOCACHE) {
- /* in case that cache.direct = on all allocmap entries are
+ /* when cache.direct = on all allocmap entries are
* treated as invalid to force a relookup of the block
* status on every read request */
return 0;
@@ -461,17 +462,19 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
int nb_sectors, bool allocated, bool valid)
{
int64_t cl_num_expanded, nb_cls_expanded, cl_num_shrunk, nb_cls_shrunk;
+ int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
if (iscsilun->allocmap == NULL) {
return;
}
/* expand to entirely contain all affected clusters */
- cl_num_expanded = sector_num / iscsilun->cluster_sectors;
+ assert(cluster_sectors);
+ cl_num_expanded = sector_num / cluster_sectors;
nb_cls_expanded = DIV_ROUND_UP(sector_num + nb_sectors,
- iscsilun->cluster_sectors) - cl_num_expanded;
+ cluster_sectors) - cl_num_expanded;
/* shrink to touch only completely contained clusters */
- cl_num_shrunk = DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors);
- nb_cls_shrunk = (sector_num + nb_sectors) / iscsilun->cluster_sectors
+ cl_num_shrunk = DIV_ROUND_UP(sector_num, cluster_sectors);
+ nb_cls_shrunk = (sector_num + nb_sectors) / cluster_sectors
- cl_num_shrunk;
if (allocated) {
bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
@@ -535,9 +538,12 @@ iscsi_allocmap_is_allocated(IscsiLun *iscsilun, int64_t sector_num,
if (iscsilun->allocmap == NULL) {
return true;
}
- size = DIV_ROUND_UP(sector_num + nb_sectors, iscsilun->cluster_sectors);
+ assert(iscsilun->cluster_size);
+ size = DIV_ROUND_UP(sector_num + nb_sectors,
+ iscsilun->cluster_size >> BDRV_SECTOR_BITS);
return !(find_next_bit(iscsilun->allocmap, size,
- sector_num / iscsilun->cluster_sectors) == size);
+ sector_num * BDRV_SECTOR_SIZE /
+ iscsilun->cluster_size) == size);
}
static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
@@ -547,9 +553,12 @@ static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
if (iscsilun->allocmap_valid == NULL) {
return false;
}
- size = DIV_ROUND_UP(sector_num + nb_sectors, iscsilun->cluster_sectors);
+ assert(iscsilun->cluster_size);
+ size = DIV_ROUND_UP(sector_num + nb_sectors,
+ iscsilun->cluster_size >> BDRV_SECTOR_BITS);
return (find_next_zero_bit(iscsilun->allocmap_valid, size,
- sector_num / iscsilun->cluster_sectors) == size);
+ sector_num * BDRV_SECTOR_SIZE /
+ iscsilun->cluster_size) == size);
}
static int coroutine_fn
@@ -793,16 +802,21 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
BlockDriverState *file;
/* check the block status from the beginning of the cluster
* containing the start sector */
- int64_t ret = iscsi_co_get_block_status(bs,
- sector_num - sector_num % iscsilun->cluster_sectors,
- BDRV_REQUEST_MAX_SECTORS, &pnum, &file);
+ int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
+ int head;
+ int64_t ret;
+
+ assert(cluster_sectors);
+ head = sector_num % cluster_sectors;
+ ret = iscsi_co_get_block_status(bs, sector_num - head,
+ BDRV_REQUEST_MAX_SECTORS, &pnum,
+ &file);
if (ret < 0) {
return ret;
}
/* if the whole request falls into an unallocated area we can avoid
- * to read and directly return zeroes instead */
- if (ret & BDRV_BLOCK_ZERO &&
- pnum >= nb_sectors + sector_num % iscsilun->cluster_sectors) {
+ * reading and directly return zeroes instead */
+ if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
qemu_iovec_memset(iov, 0, 0x00, iov->size);
return 0;
}
@@ -1953,8 +1967,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
* reasonable size */
if (iscsilun->bl.opt_unmap_gran * iscsilun->block_size >= 4 * 1024 &&
iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) {
- iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
- iscsilun->block_size) >> BDRV_SECTOR_BITS;
+ iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
+ iscsilun->block_size;
if (iscsilun->lbprz) {
ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
}
@@ -2163,7 +2177,7 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
{
IscsiLun *iscsilun = bs->opaque;
bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
- bdi->cluster_size = iscsilun->cluster_sectors * BDRV_SECTOR_SIZE;
+ bdi->cluster_size = iscsilun->cluster_size;
return 0;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 07/21] iscsi: Switch iscsi_allocmap_update() to byte-based
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (5 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 06/21] iscsi: Switch cluster_sectors to byte-based Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status() Eric Blake
` (14 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, Ronnie Sahlberg, Paolo Bonzini,
Peter Lieven, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert all uses of
the allocmap (no semantic change). Callers that already had bytes
available are simpler, and callers that now scale to bytes will be
easier to switch to byte-based in the future.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v7: rebase to master, simple enough to keep ack
v3-v6: no change
v2: rebase to count/bytes rename
---
block/iscsi.c | 90 +++++++++++++++++++++++++++++------------------------------
1 file changed, 44 insertions(+), 46 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 3414c21c7f5..d2b0466775c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -458,24 +458,22 @@ static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
}
static void
-iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
- int nb_sectors, bool allocated, bool valid)
+iscsi_allocmap_update(IscsiLun *iscsilun, int64_t offset,
+ int64_t bytes, bool allocated, bool valid)
{
int64_t cl_num_expanded, nb_cls_expanded, cl_num_shrunk, nb_cls_shrunk;
- int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
if (iscsilun->allocmap == NULL) {
return;
}
/* expand to entirely contain all affected clusters */
- assert(cluster_sectors);
- cl_num_expanded = sector_num / cluster_sectors;
- nb_cls_expanded = DIV_ROUND_UP(sector_num + nb_sectors,
- cluster_sectors) - cl_num_expanded;
+ assert(iscsilun->cluster_size);
+ cl_num_expanded = offset / iscsilun->cluster_size;
+ nb_cls_expanded = DIV_ROUND_UP(offset + bytes,
+ iscsilun->cluster_size) - cl_num_expanded;
/* shrink to touch only completely contained clusters */
- cl_num_shrunk = DIV_ROUND_UP(sector_num, cluster_sectors);
- nb_cls_shrunk = (sector_num + nb_sectors) / cluster_sectors
- - cl_num_shrunk;
+ cl_num_shrunk = DIV_ROUND_UP(offset, iscsilun->cluster_size);
+ nb_cls_shrunk = (offset + bytes) / iscsilun->cluster_size - cl_num_shrunk;
if (allocated) {
bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
} else {
@@ -498,26 +496,26 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
}
static void
-iscsi_allocmap_set_allocated(IscsiLun *iscsilun, int64_t sector_num,
- int nb_sectors)
+iscsi_allocmap_set_allocated(IscsiLun *iscsilun, int64_t offset,
+ int64_t bytes)
{
- iscsi_allocmap_update(iscsilun, sector_num, nb_sectors, true, true);
+ iscsi_allocmap_update(iscsilun, offset, bytes, true, true);
}
static void
-iscsi_allocmap_set_unallocated(IscsiLun *iscsilun, int64_t sector_num,
- int nb_sectors)
+iscsi_allocmap_set_unallocated(IscsiLun *iscsilun, int64_t offset,
+ int64_t bytes)
{
/* Note: if cache.direct=on the fifth argument to iscsi_allocmap_update
* is ignored, so this will in effect be an iscsi_allocmap_set_invalid.
*/
- iscsi_allocmap_update(iscsilun, sector_num, nb_sectors, false, true);
+ iscsi_allocmap_update(iscsilun, offset, bytes, false, true);
}
-static void iscsi_allocmap_set_invalid(IscsiLun *iscsilun, int64_t sector_num,
- int nb_sectors)
+static void iscsi_allocmap_set_invalid(IscsiLun *iscsilun, int64_t offset,
+ int64_t bytes)
{
- iscsi_allocmap_update(iscsilun, sector_num, nb_sectors, false, false);
+ iscsi_allocmap_update(iscsilun, offset, bytes, false, false);
}
static void iscsi_allocmap_invalidate(IscsiLun *iscsilun)
@@ -531,34 +529,30 @@ static void iscsi_allocmap_invalidate(IscsiLun *iscsilun)
}
static inline bool
-iscsi_allocmap_is_allocated(IscsiLun *iscsilun, int64_t sector_num,
- int nb_sectors)
+iscsi_allocmap_is_allocated(IscsiLun *iscsilun, int64_t offset,
+ int64_t bytes)
{
unsigned long size;
if (iscsilun->allocmap == NULL) {
return true;
}
assert(iscsilun->cluster_size);
- size = DIV_ROUND_UP(sector_num + nb_sectors,
- iscsilun->cluster_size >> BDRV_SECTOR_BITS);
+ size = DIV_ROUND_UP(offset + bytes, iscsilun->cluster_size);
return !(find_next_bit(iscsilun->allocmap, size,
- sector_num * BDRV_SECTOR_SIZE /
- iscsilun->cluster_size) == size);
+ offset / iscsilun->cluster_size) == size);
}
static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
- int64_t sector_num, int nb_sectors)
+ int64_t offset, int64_t bytes)
{
unsigned long size;
if (iscsilun->allocmap_valid == NULL) {
return false;
}
assert(iscsilun->cluster_size);
- size = DIV_ROUND_UP(sector_num + nb_sectors,
- iscsilun->cluster_size >> BDRV_SECTOR_BITS);
+ size = DIV_ROUND_UP(offset + bytes, iscsilun->cluster_size);
return (find_next_zero_bit(iscsilun->allocmap_valid, size,
- sector_num * BDRV_SECTOR_SIZE /
- iscsilun->cluster_size) == size);
+ offset / iscsilun->cluster_size) == size);
}
static int coroutine_fn
@@ -640,14 +634,16 @@ retry:
}
if (iTask.status != SCSI_STATUS_GOOD) {
- iscsi_allocmap_set_invalid(iscsilun, sector_num, nb_sectors);
+ iscsi_allocmap_set_invalid(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE);
error_report("iSCSI WRITE10/16 failed at lba %" PRIu64 ": %s", lba,
iTask.err_str);
r = iTask.err_code;
goto out_unlock;
}
- iscsi_allocmap_set_allocated(iscsilun, sector_num, nb_sectors);
+ iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE);
out_unlock:
qemu_mutex_unlock(&iscsilun->mutex);
@@ -747,9 +743,11 @@ retry:
}
if (ret & BDRV_BLOCK_ZERO) {
- iscsi_allocmap_set_unallocated(iscsilun, sector_num, *pnum);
+ iscsi_allocmap_set_unallocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ *pnum * BDRV_SECTOR_SIZE);
} else {
- iscsi_allocmap_set_allocated(iscsilun, sector_num, *pnum);
+ iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ *pnum * BDRV_SECTOR_SIZE);
}
if (*pnum > nb_sectors) {
@@ -789,15 +787,19 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
/* if cache.direct is off and we have a valid entry in our allocation map
* we can skip checking the block status and directly return zeroes if
* the request falls within an unallocated area */
- if (iscsi_allocmap_is_valid(iscsilun, sector_num, nb_sectors) &&
- !iscsi_allocmap_is_allocated(iscsilun, sector_num, nb_sectors)) {
+ if (iscsi_allocmap_is_valid(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE) &&
+ !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE)) {
qemu_iovec_memset(iov, 0, 0x00, iov->size);
return 0;
}
if (nb_sectors >= ISCSI_CHECKALLOC_THRES &&
- !iscsi_allocmap_is_valid(iscsilun, sector_num, nb_sectors) &&
- !iscsi_allocmap_is_allocated(iscsilun, sector_num, nb_sectors)) {
+ !iscsi_allocmap_is_valid(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE) &&
+ !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE)) {
int pnum;
BlockDriverState *file;
/* check the block status from the beginning of the cluster
@@ -1160,8 +1162,7 @@ retry:
goto retry;
}
- iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS);
+ iscsi_allocmap_set_invalid(iscsilun, offset, bytes);
if (iTask.status == SCSI_STATUS_CHECK_CONDITION) {
/* the target might fail with a check condition if it
@@ -1274,8 +1275,7 @@ retry:
}
if (iTask.status != SCSI_STATUS_GOOD) {
- iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS);
+ iscsi_allocmap_set_invalid(iscsilun, offset, bytes);
error_report("iSCSI WRITESAME10/16 failed at lba %" PRIu64 ": %s",
lba, iTask.err_str);
r = iTask.err_code;
@@ -1283,11 +1283,9 @@ retry:
}
if (flags & BDRV_REQ_MAY_UNMAP) {
- iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS);
+ iscsi_allocmap_set_invalid(iscsilun, offset, bytes);
} else {
- iscsi_allocmap_set_allocated(iscsilun, offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS);
+ iscsi_allocmap_set_allocated(iscsilun, offset, bytes);
}
out_unlock:
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (6 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 07/21] iscsi: Switch iscsi_allocmap_update() " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-14 11:53 ` Kevin Wolf
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 09/21] null: " Eric Blake
` (13 subsequent siblings)
21 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, Ronnie Sahlberg, Paolo Bonzini,
Peter Lieven, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the iscsi driver accordingly. In this case,
it is handy to teach iscsi_co_block_status() to handle a NULL map
and file parameter, even though the block layer passes non-NULL
values, because we also call the function directly. For now, there
are no optimizations done based on the want_zero flag.
We can also make the simplification of asserting that the block
layer passed in aligned values.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v8: rebase to master
v7: rebase to master
v6: no change
v5: assert rather than check for alignment
v4: rebase to interface tweaks
v3: no change
v2: rebase to mapping parameter
---
block/iscsi.c | 67 ++++++++++++++++++++++++++++-------------------------------
1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index d2b0466775c..4842519fdad 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -653,36 +653,36 @@ out_unlock:
-static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn iscsi_co_block_status(BlockDriverState *bs,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
IscsiLun *iscsilun = bs->opaque;
struct scsi_get_lba_status *lbas = NULL;
struct scsi_lba_status_descriptor *lbasd = NULL;
struct IscsiTask iTask;
uint64_t lba;
- int64_t ret;
+ int ret;
iscsi_co_init_iscsitask(iscsilun, &iTask);
- if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
- ret = -EINVAL;
- goto out;
- }
+ assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size));
/* default to all sectors allocated */
- ret = BDRV_BLOCK_DATA;
- ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
- *pnum = nb_sectors;
+ ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ if (map) {
+ *map = offset;
+ }
+ *pnum = bytes;
/* LUN does not support logical block provisioning */
if (!iscsilun->lbpme) {
goto out;
}
- lba = sector_qemu2lun(sector_num, iscsilun);
+ lba = offset / iscsilun->block_size;
qemu_mutex_lock(&iscsilun->mutex);
retry:
@@ -727,12 +727,12 @@ retry:
lbasd = &lbas->descriptors[0];
- if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
+ if (lba != lbasd->lba) {
ret = -EIO;
goto out_unlock;
}
- *pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun);
+ *pnum = lbasd->num_blocks * iscsilun->block_size;
if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
@@ -743,15 +743,13 @@ retry:
}
if (ret & BDRV_BLOCK_ZERO) {
- iscsi_allocmap_set_unallocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
- *pnum * BDRV_SECTOR_SIZE);
+ iscsi_allocmap_set_unallocated(iscsilun, offset, *pnum);
} else {
- iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
- *pnum * BDRV_SECTOR_SIZE);
+ iscsi_allocmap_set_allocated(iscsilun, offset, *pnum);
}
- if (*pnum > nb_sectors) {
- *pnum = nb_sectors;
+ if (*pnum > bytes) {
+ *pnum = bytes;
}
out_unlock:
qemu_mutex_unlock(&iscsilun->mutex);
@@ -760,7 +758,7 @@ out:
if (iTask.task != NULL) {
scsi_free_scsi_task(iTask.task);
}
- if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+ if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
*file = bs;
}
return ret;
@@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
nb_sectors * BDRV_SECTOR_SIZE) &&
!iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
nb_sectors * BDRV_SECTOR_SIZE)) {
- int pnum;
- BlockDriverState *file;
+ int64_t pnum;
/* check the block status from the beginning of the cluster
* containing the start sector */
- int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
- int head;
- int64_t ret;
+ int64_t head;
+ int ret;
- assert(cluster_sectors);
- head = sector_num % cluster_sectors;
- ret = iscsi_co_get_block_status(bs, sector_num - head,
- BDRV_REQUEST_MAX_SECTORS, &pnum,
- &file);
+ assert(iscsilun->cluster_size);
+ head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
+ ret = iscsi_co_block_status(bs, false,
+ sector_num * BDRV_SECTOR_SIZE - head,
+ BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);
if (ret < 0) {
return ret;
}
/* if the whole request falls into an unallocated area we can avoid
* reading and directly return zeroes instead */
- if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
+ if (ret & BDRV_BLOCK_ZERO &&
+ pnum >= nb_sectors * BDRV_SECTOR_SIZE + head) {
qemu_iovec_memset(iov, 0, 0x00, iov->size);
return 0;
}
@@ -2218,7 +2215,7 @@ static BlockDriver bdrv_iscsi = {
.bdrv_truncate = iscsi_truncate,
.bdrv_refresh_limits = iscsi_refresh_limits,
- .bdrv_co_get_block_status = iscsi_co_get_block_status,
+ .bdrv_co_block_status = iscsi_co_block_status,
.bdrv_co_pdiscard = iscsi_co_pdiscard,
.bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
.bdrv_co_readv = iscsi_co_readv,
@@ -2253,7 +2250,7 @@ static BlockDriver bdrv_iser = {
.bdrv_truncate = iscsi_truncate,
.bdrv_refresh_limits = iscsi_refresh_limits,
- .bdrv_co_get_block_status = iscsi_co_get_block_status,
+ .bdrv_co_block_status = iscsi_co_block_status,
.bdrv_co_pdiscard = iscsi_co_pdiscard,
.bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
.bdrv_co_readv = iscsi_co_readv,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status() Eric Blake
@ 2018-02-14 11:53 ` Kevin Wolf
2018-02-14 14:33 ` Eric Blake
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 11:53 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, famz, qemu-block, Ronnie Sahlberg, Paolo Bonzini,
Peter Lieven, Max Reitz
Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the iscsi driver accordingly. In this case,
> it is handy to teach iscsi_co_block_status() to handle a NULL map
> and file parameter, even though the block layer passes non-NULL
> values, because we also call the function directly. For now, there
> are no optimizations done based on the want_zero flag.
>
> We can also make the simplification of asserting that the block
> layer passed in aligned values.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
>
> ---
> v8: rebase to master
> v7: rebase to master
> v6: no change
> v5: assert rather than check for alignment
> v4: rebase to interface tweaks
> v3: no change
> v2: rebase to mapping parameter
> ---
> block/iscsi.c | 67 ++++++++++++++++++++++++++++-------------------------------
> 1 file changed, 32 insertions(+), 35 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index d2b0466775c..4842519fdad 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -653,36 +653,36 @@ out_unlock:
>
>
>
> -static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num,
> - int nb_sectors, int *pnum,
> - BlockDriverState **file)
> +static int coroutine_fn iscsi_co_block_status(BlockDriverState *bs,
> + bool want_zero, int64_t offset,
> + int64_t bytes, int64_t *pnum,
> + int64_t *map,
> + BlockDriverState **file)
> {
> IscsiLun *iscsilun = bs->opaque;
> struct scsi_get_lba_status *lbas = NULL;
> struct scsi_lba_status_descriptor *lbasd = NULL;
> struct IscsiTask iTask;
> uint64_t lba;
> - int64_t ret;
> + int ret;
>
> iscsi_co_init_iscsitask(iscsilun, &iTask);
>
> - if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
> - ret = -EINVAL;
> - goto out;
> - }
> + assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size));
>
> /* default to all sectors allocated */
> - ret = BDRV_BLOCK_DATA;
> - ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
> - *pnum = nb_sectors;
> + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> + if (map) {
> + *map = offset;
> + }
Can map ever be NULL? You didn't have that check in other drivers.
> + *pnum = bytes;
>
> /* LUN does not support logical block provisioning */
> if (!iscsilun->lbpme) {
> goto out;
> }
>
> - lba = sector_qemu2lun(sector_num, iscsilun);
> + lba = offset / iscsilun->block_size;
>
> qemu_mutex_lock(&iscsilun->mutex);
> retry:
> @@ -727,12 +727,12 @@ retry:
>
> lbasd = &lbas->descriptors[0];
>
> - if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
> + if (lba != lbasd->lba) {
> ret = -EIO;
> goto out_unlock;
> }
>
> - *pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun);
> + *pnum = lbasd->num_blocks * iscsilun->block_size;
>
> if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
> lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
> @@ -743,15 +743,13 @@ retry:
> }
>
> if (ret & BDRV_BLOCK_ZERO) {
> - iscsi_allocmap_set_unallocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
> - *pnum * BDRV_SECTOR_SIZE);
> + iscsi_allocmap_set_unallocated(iscsilun, offset, *pnum);
> } else {
> - iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
> - *pnum * BDRV_SECTOR_SIZE);
> + iscsi_allocmap_set_allocated(iscsilun, offset, *pnum);
> }
>
> - if (*pnum > nb_sectors) {
> - *pnum = nb_sectors;
> + if (*pnum > bytes) {
> + *pnum = bytes;
> }
> out_unlock:
> qemu_mutex_unlock(&iscsilun->mutex);
> @@ -760,7 +758,7 @@ out:
> if (iTask.task != NULL) {
> scsi_free_scsi_task(iTask.task);
> }
> - if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
> + if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
> *file = bs;
> }
Can file ever be NULL?
> return ret;
> @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
> nb_sectors * BDRV_SECTOR_SIZE) &&
No iscsi_co_preadv() yet... :-(
> !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
> nb_sectors * BDRV_SECTOR_SIZE)) {
> - int pnum;
> - BlockDriverState *file;
> + int64_t pnum;
> /* check the block status from the beginning of the cluster
> * containing the start sector */
> - int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
> - int head;
> - int64_t ret;
> + int64_t head;
> + int ret;
>
> - assert(cluster_sectors);
> - head = sector_num % cluster_sectors;
> - ret = iscsi_co_get_block_status(bs, sector_num - head,
> - BDRV_REQUEST_MAX_SECTORS, &pnum,
> - &file);
> + assert(iscsilun->cluster_size);
> + head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
> + ret = iscsi_co_block_status(bs, false,
> + sector_num * BDRV_SECTOR_SIZE - head,
> + BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);
It doesn't make a difference with your current implementation because it
ignores want_zero, but consistent with your approach that
want_zero=false returns just that everyhting is allocated for drivers
without support for backing files, I think you want want_zero=true here.
> if (ret < 0) {
> return ret;
> }
> /* if the whole request falls into an unallocated area we can avoid
> * reading and directly return zeroes instead */
> - if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
> + if (ret & BDRV_BLOCK_ZERO &&
> + pnum >= nb_sectors * BDRV_SECTOR_SIZE + head) {
> qemu_iovec_memset(iov, 0, 0x00, iov->size);
> return 0;
> }
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
2018-02-14 11:53 ` Kevin Wolf
@ 2018-02-14 14:33 ` Eric Blake
0 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-14 14:33 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, famz, qemu-block, Ronnie Sahlberg, Paolo Bonzini,
Peter Lieven, Max Reitz
On 02/14/2018 05:53 AM, Kevin Wolf wrote:
> Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the iscsi driver accordingly. In this case,
>> it is handy to teach iscsi_co_block_status() to handle a NULL map
>> and file parameter, even though the block layer passes non-NULL
>> values, because we also call the function directly. For now, there
>> are no optimizations done based on the want_zero flag.
[1]
>>
>> We can also make the simplification of asserting that the block
>> layer passed in aligned values.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>
>> /* default to all sectors allocated */
>> - ret = BDRV_BLOCK_DATA;
>> - ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
>> - *pnum = nb_sectors;
>> + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>> + if (map) {
>> + *map = offset;
>> + }
>
> Can map ever be NULL? You didn't have that check in other drivers.
The documentation in block_int.h states that io.c never passes NULL for
map. However, see my commit message [1], and the code below [2], for
why THIS driver has to check for NULL.
>> @@ -760,7 +758,7 @@ out:
>> if (iTask.task != NULL) {
>> scsi_free_scsi_task(iTask.task);
>> }
>> - if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
>> + if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
>> *file = bs;
>> }
>
> Can file ever be NULL?
Ditto.
>
>> return ret;
>> @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>> nb_sectors * BDRV_SECTOR_SIZE) &&
>
> No iscsi_co_preadv() yet... :-(
Yeah, but that's for another day.
>
>> !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
>> nb_sectors * BDRV_SECTOR_SIZE)) {
>> - int pnum;
>> - BlockDriverState *file;
>> + int64_t pnum;
>> /* check the block status from the beginning of the cluster
>> * containing the start sector */
>> - int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
>> - int head;
>> - int64_t ret;
>> + int64_t head;
>> + int ret;
>>
>> - assert(cluster_sectors);
>> - head = sector_num % cluster_sectors;
>> - ret = iscsi_co_get_block_status(bs, sector_num - head,
>> - BDRV_REQUEST_MAX_SECTORS, &pnum,
>> - &file);
>> + assert(iscsilun->cluster_size);
>> + head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
>> + ret = iscsi_co_block_status(bs, false,
>> + sector_num * BDRV_SECTOR_SIZE - head,
>> + BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);
>
[2] This is the reason that THIS driver has to check for NULL, even
though the block layer never passes NULL.
> It doesn't make a difference with your current implementation because it
> ignores want_zero, but consistent with your approach that
> want_zero=false returns just that everyhting is allocated for drivers
> without support for backing files, I think you want want_zero=true here.
Makes sense. If that's the only tweak, can you make it while taking the
series, or will I need to respin?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (7 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status() Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-14 12:05 ` Kevin Wolf
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 10/21] parallels: " Eric Blake
` (12 subsequent siblings)
21 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the null driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: minor fix to type of 'ret'
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping parameter
---
block/null.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/block/null.c b/block/null.c
index 214d394fff4..806a8631e4d 100644
--- a/block/null.c
+++ b/block/null.c
@@ -223,22 +223,23 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state,
return 0;
}
-static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn null_co_block_status(BlockDriverState *bs,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BDRVNullState *s = bs->opaque;
- off_t start = sector_num * BDRV_SECTOR_SIZE;
+ int ret = BDRV_BLOCK_OFFSET_VALID;
- *pnum = nb_sectors;
+ *pnum = bytes;
+ *map = offset;
*file = bs;
if (s->read_zeroes) {
- return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
- } else {
- return BDRV_BLOCK_OFFSET_VALID | start;
+ ret |= BDRV_BLOCK_ZERO;
}
+ return ret;
}
static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
@@ -270,7 +271,7 @@ static BlockDriver bdrv_null_co = {
.bdrv_co_flush_to_disk = null_co_flush,
.bdrv_reopen_prepare = null_reopen_prepare,
- .bdrv_co_get_block_status = null_co_get_block_status,
+ .bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
};
@@ -290,7 +291,7 @@ static BlockDriver bdrv_null_aio = {
.bdrv_aio_flush = null_aio_flush,
.bdrv_reopen_prepare = null_reopen_prepare,
- .bdrv_co_get_block_status = null_co_get_block_status,
+ .bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
};
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 09/21] null: " Eric Blake
@ 2018-02-14 12:05 ` Kevin Wolf
2018-02-14 14:44 ` Eric Blake
2018-02-23 16:43 ` Eric Blake
0 siblings, 2 replies; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 12:05 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the null driver accordingly.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
>
> ---
> v6-v7: no change
> v5: minor fix to type of 'ret'
> v4: rebase to interface tweak
> v3: no change
> v2: rebase to mapping parameter
> ---
> block/null.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/block/null.c b/block/null.c
> index 214d394fff4..806a8631e4d 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -223,22 +223,23 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state,
> return 0;
> }
>
> -static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num,
> - int nb_sectors, int *pnum,
> - BlockDriverState **file)
> +static int coroutine_fn null_co_block_status(BlockDriverState *bs,
> + bool want_zero, int64_t offset,
> + int64_t bytes, int64_t *pnum,
> + int64_t *map,
> + BlockDriverState **file)
> {
> BDRVNullState *s = bs->opaque;
> - off_t start = sector_num * BDRV_SECTOR_SIZE;
> + int ret = BDRV_BLOCK_OFFSET_VALID;
>
> - *pnum = nb_sectors;
> + *pnum = bytes;
> + *map = offset;
> *file = bs;
>
> if (s->read_zeroes) {
> - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
> - } else {
> - return BDRV_BLOCK_OFFSET_VALID | start;
> + ret |= BDRV_BLOCK_ZERO;
> }
> + return ret;
> }
Preexisting, but I think this return value is wrong. OFFSET_VALID
without DATA is to documented to have the following semantics:
* DATA ZERO OFFSET_VALID
* f t t sectors preallocated, read as zero, returned file not
* necessarily zero at offset
* f f t sectors preallocated but read from backing_hd,
* returned file contains garbage at offset
I'm not sure what OFFSET_VALID is even supposed to mean for null.
Or in fact, what it is supposed to mean for any protocol driver, because
normally it just means I can use this offset for accessing bs->file. But
protocol drivers don't have a bs->file, so it's interesting to see that
they still all set this flag.
OFFSET_VALID | DATA might be excusable because I can see that it's
convenient that a protocol driver refers to itself as *file instead of
returning NULL there and then the offset is valid (though it would be
pointless to actually follow the file pointer), but OFFSET_VALID without
DATA probably isn't.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-14 12:05 ` Kevin Wolf
@ 2018-02-14 14:44 ` Eric Blake
2018-02-14 14:55 ` Kevin Wolf
2018-02-23 16:43 ` Eric Blake
1 sibling, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-14 14:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, famz, qemu-block, Max Reitz
On 02/14/2018 06:05 AM, Kevin Wolf wrote:
> Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the null driver accordingly.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>
>> if (s->read_zeroes) {
>> - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
>> - } else {
>> - return BDRV_BLOCK_OFFSET_VALID | start;
>> + ret |= BDRV_BLOCK_ZERO;
>> }
>> + return ret;
>> }
>
> Preexisting, but I think this return value is wrong. OFFSET_VALID
> without DATA is to documented to have the following semantics:
>
> * DATA ZERO OFFSET_VALID
> * f t t sectors preallocated, read as zero, returned file not
> * necessarily zero at offset
> * f f t sectors preallocated but read from backing_hd,
> * returned file contains garbage at offset
>
> I'm not sure what OFFSET_VALID is even supposed to mean for null.
Yeah, and I was even thinking about that a bit yesterday when figuring
out what to do with nvme. It does highlight the fact that you get
garbage when reading from the null driver (unless the zero option was
enabled, then ZERO is set and you know you read zeros instead) - but
there no pointer that is preallocated (whether it contains garbage or
otherwise) that you can actually dereference to read what the guest
would see.
>
> Or in fact, what it is supposed to mean for any protocol driver, because
> normally it just means I can use this offset for accessing bs->file. But
> protocol drivers don't have a bs->file, so it's interesting to see that
> they still all set this flag.
>
> OFFSET_VALID | DATA might be excusable because I can see that it's
> convenient that a protocol driver refers to itself as *file instead of
> returning NULL there and then the offset is valid (though it would be
> pointless to actually follow the file pointer), but OFFSET_VALID without
> DATA probably isn't.
Hmm, you're probably right. Maybe that means I should tweak the
documentation to be more explicit: for a format driver, OFFSET_VALID can
always be used (and *file will be set to the underlying protocol
driver); but for a protocol driver, OFFSET_VALID only makes sense if
*file is the BDS itself and there is an actual buffer to read (that is,
the protocol driver must also be returning DATA and/or ZERO). Or maybe
we can indeed state that protocol drivers always set *file to NULL
(there is no further backing file to reference), and thus never need to
return OFFSET_VALID (but I'm not sure whether that will accidentally
propagate back up the call stack and negatively affect status queries of
format drivers).
Since it is pre-existing, should I respin to address the issue in a
separate patch, or should that be a followup after this series?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-14 14:44 ` Eric Blake
@ 2018-02-14 14:55 ` Kevin Wolf
0 siblings, 0 replies; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 14:55 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
Am 14.02.2018 um 15:44 hat Eric Blake geschrieben:
> On 02/14/2018 06:05 AM, Kevin Wolf wrote:
> > Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> > > We are gradually moving away from sector-based interfaces, towards
> > > byte-based. Update the null driver accordingly.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > Reviewed-by: Fam Zheng <famz@redhat.com>
> > >
>
> > > if (s->read_zeroes) {
> > > - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
> > > - } else {
> > > - return BDRV_BLOCK_OFFSET_VALID | start;
> > > + ret |= BDRV_BLOCK_ZERO;
> > > }
> > > + return ret;
> > > }
> >
> > Preexisting, but I think this return value is wrong. OFFSET_VALID
> > without DATA is to documented to have the following semantics:
> >
> > * DATA ZERO OFFSET_VALID
> > * f t t sectors preallocated, read as zero, returned file not
> > * necessarily zero at offset
> > * f f t sectors preallocated but read from backing_hd,
> > * returned file contains garbage at offset
> >
> > I'm not sure what OFFSET_VALID is even supposed to mean for null.
>
> Yeah, and I was even thinking about that a bit yesterday when figuring out
> what to do with nvme. It does highlight the fact that you get garbage when
> reading from the null driver (unless the zero option was enabled, then ZERO
> is set and you know you read zeros instead) - but there no pointer that is
> preallocated (whether it contains garbage or otherwise) that you can
> actually dereference to read what the guest would see.
>
> >
> > Or in fact, what it is supposed to mean for any protocol driver, because
> > normally it just means I can use this offset for accessing bs->file. But
> > protocol drivers don't have a bs->file, so it's interesting to see that
> > they still all set this flag.
> >
> > OFFSET_VALID | DATA might be excusable because I can see that it's
> > convenient that a protocol driver refers to itself as *file instead of
> > returning NULL there and then the offset is valid (though it would be
> > pointless to actually follow the file pointer), but OFFSET_VALID without
> > DATA probably isn't.
>
> Hmm, you're probably right. Maybe that means I should tweak the
> documentation to be more explicit: for a format driver, OFFSET_VALID can
> always be used (and *file will be set to the underlying protocol driver);
> but for a protocol driver, OFFSET_VALID only makes sense if *file is the BDS
> itself and there is an actual buffer to read (that is, the protocol driver
> must also be returning DATA and/or ZERO). Or maybe we can indeed state that
> protocol drivers always set *file to NULL (there is no further backing file
> to reference), and thus never need to return OFFSET_VALID (but I'm not sure
> whether that will accidentally propagate back up the call stack and
> negatively affect status queries of format drivers).
>
> Since it is pre-existing, should I respin to address the issue in a separate
> patch, or should that be a followup after this series?
It's a more fundamental question that shouldn't hold up this series. I
just wanted to raise it while I was looking at it. So yes, a followup is
fine.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-14 12:05 ` Kevin Wolf
2018-02-14 14:44 ` Eric Blake
@ 2018-02-23 16:43 ` Eric Blake
2018-02-23 17:05 ` Kevin Wolf
1 sibling, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-23 16:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, famz, qemu-block, Max Reitz
On 02/14/2018 06:05 AM, Kevin Wolf wrote:
>> +static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>> if (s->read_zeroes) {
>> - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
>> - } else {
>> - return BDRV_BLOCK_OFFSET_VALID | start;
>> + ret |= BDRV_BLOCK_ZERO;
>> }
>> + return ret;
>> }
>
> Preexisting, but I think this return value is wrong. OFFSET_VALID
> without DATA is to documented to have the following semantics:
>
> * DATA ZERO OFFSET_VALID
> * f t t sectors preallocated, read as zero, returned file not
> * necessarily zero at offset
> * f f t sectors preallocated but read from backing_hd,
> * returned file contains garbage at offset
>
> I'm not sure what OFFSET_VALID is even supposed to mean for null.
I'm finally getting around to playing with this.
>
> Or in fact, what it is supposed to mean for any protocol driver, because
> normally it just means I can use this offset for accessing bs->file. But > protocol drivers don't have a bs->file, so it's interesting to see that
> they still all set this flag.
More precisely, it means "I can use this offset for accessing the
returned *file". Format and filter drivers set *file = bs->file (ie.
their protocol layer), but protocol drivers set *file = bs (ie.
themselves). As long as you read it as "the offset is valid in the
returned *file", and are careful as to _which_ BDS gets returned in
*file*, it can still make sense.
So next I tried playing with a patch, to see how much returning
OFFSET_VALID with DATA matters; and it turns out is is easily observable
anywhere that the underlying protocol bleeds through to the format layer
(particularly the raw format driver):
$ echo abc > tmp
$ truncate --size=10M tmp
pre-patch:
$ ./qemu-img map --output=json tmp
[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true,
"offset": 0},
{ "start": 4096, "length": 10481664, "depth": 0, "zero": true, "data":
false, "offset": 4096}]
turn off OFFSET_VALID at the protocol layer:
diff --git i/block/file-posix.c w/block/file-posix.c
index f1591c38490..c05992c1121 100644
--- i/block/file-posix.c
+++ w/block/file-posix.c
@@ -2158,9 +2158,7 @@ static int coroutine_fn
raw_co_block_status(BlockDriverState *bs,
if (!want_zero) {
*pnum = bytes;
- *map = offset;
- *file = bs;
- return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ return BDRV_BLOCK_DATA;
}
ret = find_allocation(bs, offset, &data, &hole);
@@ -2183,9 +2181,7 @@ static int coroutine_fn
raw_co_block_status(BlockDriverState *bs,
*pnum = MIN(bytes, data - offset);
ret = BDRV_BLOCK_ZERO;
}
- *map = offset;
- *file = bs;
- return ret | BDRV_BLOCK_OFFSET_VALID;
+ return ret;
}
static coroutine_fn BlockAIOCB *raw_aio_pdiscard(BlockDriverState *bs,
post-patch:
$ ./qemu-img map --output=json tmp
[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true},
{ "start": 4096, "length": 10481664, "depth": 0, "zero": true, "data":
false}]
>
> OFFSET_VALID | DATA might be excusable because I can see that it's
> convenient that a protocol driver refers to itself as *file instead of
> returning NULL there and then the offset is valid (though it would be
> pointless to actually follow the file pointer), but OFFSET_VALID without
> DATA probably isn't.
So OFFSET_VALID | DATA for a protocol BDS is not just convenient, but
necessary to avoid breaking qemu-img map output. But you are also right
that OFFSET_VALID without data makes little sense at a protocol layer.
So with that in mind, I'm auditing all of the protocol layers to make
sure OFFSET_VALID ends up as something sane.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-23 16:43 ` Eric Blake
@ 2018-02-23 17:05 ` Kevin Wolf
2018-02-23 23:38 ` Eric Blake
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2018-02-23 17:05 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
Am 23.02.2018 um 17:43 hat Eric Blake geschrieben:
> > OFFSET_VALID | DATA might be excusable because I can see that it's
> > convenient that a protocol driver refers to itself as *file instead of
> > returning NULL there and then the offset is valid (though it would be
> > pointless to actually follow the file pointer), but OFFSET_VALID without
> > DATA probably isn't.
>
> So OFFSET_VALID | DATA for a protocol BDS is not just convenient, but
> necessary to avoid breaking qemu-img map output. But you are also right
> that OFFSET_VALID without data makes little sense at a protocol layer. So
> with that in mind, I'm auditing all of the protocol layers to make sure
> OFFSET_VALID ends up as something sane.
That's one way to look at it.
The other way is that qemu-img map shouldn't ask the protocol layer for
its offset because it already knows the offset (it is what it passes as
a parameter to bdrv_co_block_status).
Anyway, it's probably not worth changing the interface, we should just
make sure that the return values of the individual drivers are
consistent.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-23 17:05 ` Kevin Wolf
@ 2018-02-23 23:38 ` Eric Blake
2018-02-26 14:05 ` Kevin Wolf
0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-23 23:38 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, famz, qemu-block, Max Reitz, Vladimir Sementsov-Ogievskiy
On 02/23/2018 11:05 AM, Kevin Wolf wrote:
> Am 23.02.2018 um 17:43 hat Eric Blake geschrieben:
>>> OFFSET_VALID | DATA might be excusable because I can see that it's
>>> convenient that a protocol driver refers to itself as *file instead of
>>> returning NULL there and then the offset is valid (though it would be
>>> pointless to actually follow the file pointer), but OFFSET_VALID without
>>> DATA probably isn't.
>>
>> So OFFSET_VALID | DATA for a protocol BDS is not just convenient, but
>> necessary to avoid breaking qemu-img map output. But you are also right
>> that OFFSET_VALID without data makes little sense at a protocol layer. So
>> with that in mind, I'm auditing all of the protocol layers to make sure
>> OFFSET_VALID ends up as something sane.
>
> That's one way to look at it.
>
> The other way is that qemu-img map shouldn't ask the protocol layer for
> its offset because it already knows the offset (it is what it passes as
> a parameter to bdrv_co_block_status).
>
> Anyway, it's probably not worth changing the interface, we should just
> make sure that the return values of the individual drivers are
> consistent.
Yet another inconsistency, and it's making me scratch my head today.
By the way, in my byte-based stuff that is now pending on your tree, I
tried hard to NOT change semantics or the set of flags returned by a
given driver, and we agreed that's why you'd accept the series as-is and
make me do this followup exercise. But it's looking like my followups
may end up touching a lot of the same drivers again, now that I'm
looking at what the semantics SHOULD be (and whatever I do end up
tweaking, I will at least make sure that iotests is still happy with it).
First, let's read what states the NBD spec is proposing:
> It defines the following flags for the flags field:
>
> NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and future writes to that area may cause fragmentation or encounter an ENOSPC error); if clear, the block is allocated or the server could not otherwise determine its status. Note that the use of NBD_CMD_TRIM is related to this status, but that the server MAY report a hole even where NBD_CMD_TRIM has not been requested, and also that a server MAY report that the block is allocated even where NBD_CMD_TRIM has been requested.
> NBD_STATE_ZERO (bit 1): if set, the block contents read as all zeroes; if clear, the block contents are not known. Note that the use of NBD_CMD_WRITE_ZEROES is related to this status, but that the server MAY report zeroes even where NBD_CMD_WRITE_ZEROES has not been requested, and also that a server MAY report unknown content even where NBD_CMD_WRITE_ZEROES has been requested.
>
> It is not an error for a server to report that a region of the export has both NBD_STATE_HOLE set and NBD_STATE_ZERO clear. The contents of such an area are undefined, and a client reading such an area should make no assumption as to its contents or stability.
So here's how Vladimir proposed implementing it in his series (written
before my byte-based block status stuff went in to your tree):
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg04038.html
Server side (3/9):
+ int ret = bdrv_block_status_above(bs, NULL, offset, tail_bytes,
&num,
+ NULL, NULL);
+ if (ret < 0) {
+ return ret;
+ }
+
+ flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
+ (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
Client side (6/9):
+ *pnum = extent.length >> BDRV_SECTOR_BITS;
+ return (extent.flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_DATA) |
+ (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
Does anything there strike you as odd? In isolation, they seemed fine
to me, but side-by-side, I'm scratching my head: the server queries the
block layer, and turns BDRV_BLOCK_ALLOCATED into !NBD_STATE_HOLE; the
client side then takes the NBD protocol and tries to turn it back into
information to feed the block layer, where !NBD_STATE_HOLE now feeds
BDRV_BLOCK_DATA. Why the different choice of bits?
Part of the story is that right now, we document that ONLY the block
layer sets _ALLOCATED, in io.c, as a result of the driver layer
returning HOLE || ZERO (there are cases where the block layer can return
ZERO but not ALLOCATED, because the driver layer returned 0 but the
block layer still knows that area reads as zero). So Victor's patch
matches the fact that the driver shouldn't set ALLOCATED. Still, if we
are tying ALLOCATED to whether there is a hole, then that seems like
information we should be getting from the driver, not something
synthesized after we've left the driver!
Then there's the question of file-posix.c: what should it return for a
hole, ZERO|OFFSET_VALID or DATA|ZERO|OFFSET_VALID? The wording in
block.h implies that if DATA is not set, then the area reads as zero to
the guest, but may have indeterminate value on the underlying file - but
we KNOW that a hole in a POSIX file reads as 0 rather than having
indeterminate value, and returning DATA fits the current documentation
(but doing so bleeds through to at least 'qemu-img map --output=json'
for the raw format). I think we're overloading too many things into
DATA (which layer of the chain feeds what the guest sees, and do we have
a hole or is storage allocated for the data). The only uses of
BDRV_BLOCK_ALLOCATED are in the computation of bdrv_is_allocated(), in
qcow2 measure, and in qemu-img compare, which all really do care about
the semantics of "does THIS layer provide the guest image, or do I defer
to a backing layer". But the question NBD wants answered is "do I know
whether there is a hole in the storage" There are also relatively few
clients of BDRV_BLOCK_DATA (mirror.c, qemu-img,
bdrv_co_block_status_above), and I wonder if some of them are more
worried about BDRV_BLOCK_ALLOCATED instead.
I'm thinking of revamping things to still keep four bits, but with new
names and semantics as follows:
BDRV_BLOCK_LOCAL - the guest gets this portion of the file from this
BDS, rather than the backing chain - makes sense for format drivers,
pointless for protocol drivers
BDRV_BLOCK_ZERO - this portion of the file reads as zeroes
BDRV_BLOCK_ALLOC - this portion of the file has reserved disk space
BDRV_BLOCK_OFFSET_VALID - offset for accessing raw data
For format drivers:
L Z A O read as zero, returned file is zero at offset
L - A O read as valid from file at offset
L Z - O read as zero, but returned file has hole at offset
L - - O preallocated at offset but reads as garbage - bug?
L Z A - read as zero, but from unknown offset with storage
L - A - read as valid, but from unknown offset (including compressed,
encrypted)
L Z - - read as zero, but from unknown offset with hole
L - - - preallocated but no offset known - bug?
- Z A O read defers to backing layer, but protocol layer contains
allocated zeros at offset
- - A O read defers to backing layer, but preallocated at offset
- Z - O bug
- - - O bug
- Z A - bug
- - A - bug
- Z - - bug
- - - - read defers to backing layer
For protocol drivers:
- Z A O read as zero, offset is allocated
- - A O read as data, offset is allocated
- Z - O read as zero, offset is hole
- - - O bug?
- Z A - read as zero, but from unknown offset with storage
- - A - read as valid, but from unknown offset
- Z - - read as zero, but from unknown offset with hole
- - - - can't access this portion of file
With the new bit definitions, any driver that returns RAW (necessarily
with OFFSET_VALID) will have the block layer set LOCAL in addition to
whatever the next layer returns (turning the protocol driver's response
into the correct format layer response). Protocol drivers can omit the
callback and get the sane default of '- - A O' mapped in place (or would
that be better as '- - A -'?). file-posix.c would return either '- - A
O' (after SEEK_DATA) or '- Z - O' (after SEEK_HOLE). NBD would map ZERO
to NBD_STATE_ZERO, and ALLOC to !NBD_STATE_HOLE, in both server
(block-layer-to-NBD-protocol) and client (NBD-protocol-to-block-layer).
Format drivers would set LOCAL themselves (rather than the block layer
synthesizing it).
bdrv_is_allocated will still let clients learn which layers are local
without grabbing full mapping information, but is tied to the
BDRV_BLOCK_LOCAL bit. Optimizations made during mirror based on whether
and qemu-img compare previously based on BDRV_BLOCK_ALLOCATED are now
based on BDRV_BLOCK_LOCAL, those based on BDRV_BLOCK_DATA are now based
on BDRV_BLOCK_ALLOC.
Thoughts?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-23 23:38 ` Eric Blake
@ 2018-02-26 14:05 ` Kevin Wolf
2018-03-01 7:25 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2018-02-26 14:05 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, famz, qemu-block, Max Reitz, Vladimir Sementsov-Ogievskiy
Am 24.02.2018 um 00:38 hat Eric Blake geschrieben:
> On 02/23/2018 11:05 AM, Kevin Wolf wrote:
> > Am 23.02.2018 um 17:43 hat Eric Blake geschrieben:
> > > > OFFSET_VALID | DATA might be excusable because I can see that it's
> > > > convenient that a protocol driver refers to itself as *file instead of
> > > > returning NULL there and then the offset is valid (though it would be
> > > > pointless to actually follow the file pointer), but OFFSET_VALID without
> > > > DATA probably isn't.
> > >
> > > So OFFSET_VALID | DATA for a protocol BDS is not just convenient, but
> > > necessary to avoid breaking qemu-img map output. But you are also right
> > > that OFFSET_VALID without data makes little sense at a protocol layer. So
> > > with that in mind, I'm auditing all of the protocol layers to make sure
> > > OFFSET_VALID ends up as something sane.
> >
> > That's one way to look at it.
> >
> > The other way is that qemu-img map shouldn't ask the protocol layer for
> > its offset because it already knows the offset (it is what it passes as
> > a parameter to bdrv_co_block_status).
> >
> > Anyway, it's probably not worth changing the interface, we should just
> > make sure that the return values of the individual drivers are
> > consistent.
>
> Yet another inconsistency, and it's making me scratch my head today.
>
> By the way, in my byte-based stuff that is now pending on your tree, I tried
> hard to NOT change semantics or the set of flags returned by a given driver,
> and we agreed that's why you'd accept the series as-is and make me do this
> followup exercise. But it's looking like my followups may end up touching a
> lot of the same drivers again, now that I'm looking at what the semantics
> SHOULD be (and whatever I do end up tweaking, I will at least make sure that
> iotests is still happy with it).
Hm, that's unfortunate, but I don't think we should hold up your first
series just so we can touch the drivers only once.
> First, let's read what states the NBD spec is proposing:
>
> > It defines the following flags for the flags field:
> >
> > NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and future writes to that area may cause fragmentation or encounter an ENOSPC error); if clear, the block is allocated or the server could not otherwise determine its status. Note that the use of NBD_CMD_TRIM is related to this status, but that the server MAY report a hole even where NBD_CMD_TRIM has not been requested, and also that a server MAY report that the block is allocated even where NBD_CMD_TRIM has been requested.
> > NBD_STATE_ZERO (bit 1): if set, the block contents read as all zeroes; if clear, the block contents are not known. Note that the use of NBD_CMD_WRITE_ZEROES is related to this status, but that the server MAY report zeroes even where NBD_CMD_WRITE_ZEROES has not been requested, and also that a server MAY report unknown content even where NBD_CMD_WRITE_ZEROES has been requested.
> >
> > It is not an error for a server to report that a region of the export has both NBD_STATE_HOLE set and NBD_STATE_ZERO clear. The contents of such an area are undefined, and a client reading such an area should make no assumption as to its contents or stability.
>
> So here's how Vladimir proposed implementing it in his series (written
> before my byte-based block status stuff went in to your tree):
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg04038.html
>
> Server side (3/9):
>
> + int ret = bdrv_block_status_above(bs, NULL, offset, tail_bytes,
> &num,
> + NULL, NULL);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
> + (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
>
> Client side (6/9):
>
> + *pnum = extent.length >> BDRV_SECTOR_BITS;
> + return (extent.flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_DATA) |
> + (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
>
> Does anything there strike you as odd?
Two things I noticed while reading the above:
1. NBD doesn't consider backing files, so the definition of holes
becomes ambiguous. Is a hole any block that isn't allocated in the
top layer (may cause fragmentation or encounter an ENOSPC error) or
is it any block that isn't allocated anywhere in the whole backing
chain (may read as non-zero)?
Considering that there is a separate NBD_STATE_ZERO and nothing
forbids a state of NBD_STATE_HOLE without NBD_STATE_ZERO, maybe the
former is more useful. The code you quote implements the latter.
Maybe if we go with the former, we should add a note to the NBD spec
that explictly says that NBD_STATE_HOLE doesn't imply any specific
content that is returned on reads.
2. Using BDRV_BLOCK_ALLOCATED to determine NBD_STATE_HOLE seems wrong. A
(not preallocated) zero cluster in qcow2 returns BDRV_BLOCK_ALLOCATED
(because we don't fall through to the backing file) even though I
think it's a hole. BDRV_BLOCK_DATA should be used there (which makes
it consistent with the other direction).
> In isolation, they seemed fine to
> me, but side-by-side, I'm scratching my head: the server queries the block
> layer, and turns BDRV_BLOCK_ALLOCATED into !NBD_STATE_HOLE; the client side
> then takes the NBD protocol and tries to turn it back into information to
> feed the block layer, where !NBD_STATE_HOLE now feeds BDRV_BLOCK_DATA. Why
> the different choice of bits?
Which is actually consistent in the end, becaue BDRV_BLOCK_DATA implies
BDRV_BLOCK_ALLOCATED.
Essentially, assuming a simple backing chain 'base <- overlay', we got
these combinations to represent in NBD (with my suggestion of the flags
to use):
1. Cluster allocated in overlay
a. non-zero data 0
b. explicit zeroes 0 or ZERO
2. Cluster marked zero in overlay HOLE | ZERO
3. Cluster preallocated/zero in overlay ZERO
4. Cluster unallocated in overlay
a. Cluster allocated in base (non-zero) HOLE
b. Cluster allocated in base (zero) HOLE or HOLE | ZERO
c. Cluster marked zero in base HOLE | ZERO
d. Cluster preallocated/zero in base HOLE | ZERO
e. Cluster unallocated in base HOLE | ZERO
Instead of 'base' you can read 'anywhere in the backing chain' and the
flags should stay the same.
So !BDRV_BLOCK_ALLOCATED (i.e. falling through to the backing file) does
indeed imply NBD_STATE_HOLE, but so does case 2, which is just !DATA.
> Part of the story is that right now, we document that ONLY the block layer
> sets _ALLOCATED, in io.c, as a result of the driver layer returning HOLE ||
> ZERO (there are cases where the block layer can return ZERO but not
> ALLOCATED, because the driver layer returned 0 but the block layer still
> knows that area reads as zero). So Victor's patch matches the fact that the
> driver shouldn't set ALLOCATED. Still, if we are tying ALLOCATED to whether
> there is a hole, then that seems like information we should be getting from
> the driver, not something synthesized after we've left the driver!
Yes, I'm getting this impression, too. If your documentation says
something like "not allocated or unknown offset" (for !OFFSET_VALID),
you should probably be using one bit more to distinguish these cases.
> Then there's the question of file-posix.c: what should it return for a hole,
> ZERO|OFFSET_VALID or DATA|ZERO|OFFSET_VALID? The wording in block.h implies
> that if DATA is not set, then the area reads as zero to the guest, but may
> have indeterminate value on the underlying file - but we KNOW that a hole in
> a POSIX file reads as 0 rather than having indeterminate value, and
> returning DATA fits the current documentation (but doing so bleeds through
> to at least 'qemu-img map --output=json' for the raw format).
The "underlying file" for the file-posix layer (i.e. the filesystem) is
a block device. A hole in a file is defined by not mapping to anywhere
on the block device, so DATA should not be set.
DATA | ZERO would mean that the block is actually allocated on the block
device, but it still reads as zero.
The thing that is inconsistent here is OFFSET_VALID and the offset
returned because the protocol layer refers to itself there instead of
referring to the "underlying file". If done consistently, it would have
to return the offset on the block device (which is useless information
in QEMU, so I suggested not to set OFFSET_VALID there at all - but we
decided that that's too much hassle for no practical benefit).
> I think we're overloading too many things into DATA (which layer of
> the chain feeds what the guest sees, and do we have a hole or is
> storage allocated for the data).
As I understand it, DATA should only be about holes (in the sense of not
being mapped to anywhere in bs->file or any other child apart from the
backing file).
Documentation does define OFFSET_VALID without DATA, though, as
preallocation. Maybe preallocation would better be expressed as DATA,
but without ALLOCATED.
> The only uses of BDRV_BLOCK_ALLOCATED are in the computation of
> bdrv_is_allocated(), in qcow2 measure, and in qemu-img compare, which all
> really do care about the semantics of "does THIS layer provide the guest
> image, or do I defer to a backing layer". But the question NBD wants
> answered is "do I know whether there is a hole in the storage" There are
> also relatively few clients of BDRV_BLOCK_DATA (mirror.c, qemu-img,
> bdrv_co_block_status_above), and I wonder if some of them are more worried
> about BDRV_BLOCK_ALLOCATED instead.
>
> I'm thinking of revamping things to still keep four bits, but with new names
> and semantics as follows:
>
> BDRV_BLOCK_LOCAL - the guest gets this portion of the file from this BDS,
> rather than the backing chain - makes sense for format drivers, pointless
> for protocol drivers
This is the old BDRV_BLOCK_ALLOCATED.
Data almost never comes from the qcow2 layer, so what this really means
is that data doesn't come from bs->backing.
> BDRV_BLOCK_ZERO - this portion of the file reads as zeroes
Same as before.
> BDRV_BLOCK_ALLOC - this portion of the file has reserved disk space
I think this is essentially what I believe BDRV_BLOCK_DATA should have
been. "Disk space" isn't clearly defined, but "there is a mapping to a
child node (except bs->backing)" seems to be close enough to what you
have in mind.
> BDRV_BLOCK_OFFSET_VALID - offset for accessing raw data
Same as before.
As I understand it, you're just renaming the existing flags. I'm not
sure if this is a good idea, especially with ALLOC(ATED), which changes
the meaning. This is pretty confusing. I suggest MAPPED as an
alternative.
> For format drivers:
> L Z A O read as zero, returned file is zero at offset
This is not what your definition above said. ZERO is about reading from
the node itself rather than reading from bs->file.
I interpret this as: Read as zero, space is preallocated in the image
file, content in the image file is undefined.
> L - A O read as valid from file at offset
> L Z - O read as zero, but returned file has hole at offset
Read as zero, no mapping into bs->file, but the offset in bs->file is
valid. Doesn't make sense, OFFSET_VALID (O) should always imply
MAPPED (A).
> L - - O preallocated at offset but reads as garbage - bug?
Again O without A - yes, a bug.
> L Z A - read as zero, but from unknown offset with storage
And the space is preallocated in a non-backing child, though not
necessarily zeroed.
> L - A - read as valid, but from unknown offset (including compressed,
> encrypted)
> L Z - - read as zero, but from unknown offset with hole
> L - - - preallocated but no offset known - bug?
No, not preallocated, because MAPPED isn't set.
This is a block that isn't mapped to a block in any child node and
doesn't read as zero. It might be the appropriate response for the null
driver with read-zeroes=off.
> - Z A O read defers to backing layer, but protocol layer contains
> allocated zeros at offset
No. Space is preallocated for this block, but read defers to the backing
layer and we know that the backing layer provides zeros.
This is not something that a driver should return, but
it's a valid return value from bdrv_co_block_status(). One example for
this is reading from an offset that is higher than the length of the
backing file.
> - - A O read defers to backing layer, but preallocated at offset
Yes, this one is preallocation, finally.
> - Z - O bug
> - - - O bug
> - Z A - bug
Same as '- Z A O' except that the offset can't be directly accessed in
the child node (e.g. because this is an encrypted image).
> - - A - bug
Preallocated, but reads from backing file. Offset can't be directly
accessed in the child node.
> - Z - - bug
Read defers to backing layer and we know it will read zeros. Like for
'- Z A O', this isn't something that a driver should return, but makes
sense as a return value of bdrv_co_block_status().
> - - - - read defers to backing layer
>
> For protocol drivers:
> - Z A O read as zero, offset is allocated
> - - A O read as data, offset is allocated
> - Z - O read as zero, offset is hole
> - - - O bug?
> - Z A - read as zero, but from unknown offset with storage
> - - A - read as valid, but from unknown offset
> - Z - - read as zero, but from unknown offset with hole
> - - - - can't access this portion of file
Why don't you set LOCAL? It's usually true for protocol drivers that
they don't get their data from a backing file (though in theory you
could imagine a protocol driver with backing file support).
As discussed before, OFFSET_VALID doesn't really make sense here because
we don't return offsets of the image file on the block device, but we
only decided to keep it because of convenience. But if we change
everything, then this should be changed, too.
While the offset still refers to the same node for protocol drivers,
"unknown offset" doesn't make any sense. There is no mapping involved
that could be unknown. We really only have four cases for protocols.
ZERO and MAPPED make sense this way.
Not sure about 0 (or only OFFSET_VALID), could this ever be valid? You
say "can't access this portion of file", but where would this happen?
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-02-26 14:05 ` Kevin Wolf
@ 2018-03-01 7:25 ` Vladimir Sementsov-Ogievskiy
2018-03-01 9:48 ` Kevin Wolf
0 siblings, 1 reply; 42+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-01 7:25 UTC (permalink / raw)
To: Kevin Wolf, Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
26.02.2018 17:05, Kevin Wolf wrote:
> Am 24.02.2018 um 00:38 hat Eric Blake geschrieben:
>> On 02/23/2018 11:05 AM, Kevin Wolf wrote:
>>> Am 23.02.2018 um 17:43 hat Eric Blake geschrieben:
>>>>> OFFSET_VALID | DATA might be excusable because I can see that it's
>>>>> convenient that a protocol driver refers to itself as *file instead of
>>>>> returning NULL there and then the offset is valid (though it would be
>>>>> pointless to actually follow the file pointer), but OFFSET_VALID without
>>>>> DATA probably isn't.
>>>> So OFFSET_VALID | DATA for a protocol BDS is not just convenient, but
>>>> necessary to avoid breaking qemu-img map output. But you are also right
>>>> that OFFSET_VALID without data makes little sense at a protocol layer. So
>>>> with that in mind, I'm auditing all of the protocol layers to make sure
>>>> OFFSET_VALID ends up as something sane.
>>> That's one way to look at it.
>>>
>>> The other way is that qemu-img map shouldn't ask the protocol layer for
>>> its offset because it already knows the offset (it is what it passes as
>>> a parameter to bdrv_co_block_status).
>>>
>>> Anyway, it's probably not worth changing the interface, we should just
>>> make sure that the return values of the individual drivers are
>>> consistent.
>> Yet another inconsistency, and it's making me scratch my head today.
>>
>> By the way, in my byte-based stuff that is now pending on your tree, I tried
>> hard to NOT change semantics or the set of flags returned by a given driver,
>> and we agreed that's why you'd accept the series as-is and make me do this
>> followup exercise. But it's looking like my followups may end up touching a
>> lot of the same drivers again, now that I'm looking at what the semantics
>> SHOULD be (and whatever I do end up tweaking, I will at least make sure that
>> iotests is still happy with it).
> Hm, that's unfortunate, but I don't think we should hold up your first
> series just so we can touch the drivers only once.
>
>> First, let's read what states the NBD spec is proposing:
>>
>>> It defines the following flags for the flags field:
>>>
>>> NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and future writes to that area may cause fragmentation or encounter an ENOSPC error); if clear, the block is allocated or the server could not otherwise determine its status. Note that the use of NBD_CMD_TRIM is related to this status, but that the server MAY report a hole even where NBD_CMD_TRIM has not been requested, and also that a server MAY report that the block is allocated even where NBD_CMD_TRIM has been requested.
>>> NBD_STATE_ZERO (bit 1): if set, the block contents read as all zeroes; if clear, the block contents are not known. Note that the use of NBD_CMD_WRITE_ZEROES is related to this status, but that the server MAY report zeroes even where NBD_CMD_WRITE_ZEROES has not been requested, and also that a server MAY report unknown content even where NBD_CMD_WRITE_ZEROES has been requested.
>>>
>>> It is not an error for a server to report that a region of the export has both NBD_STATE_HOLE set and NBD_STATE_ZERO clear. The contents of such an area are undefined, and a client reading such an area should make no assumption as to its contents or stability.
>> So here's how Vladimir proposed implementing it in his series (written
>> before my byte-based block status stuff went in to your tree):
>> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg04038.html
>>
>> Server side (3/9):
>>
>> + int ret = bdrv_block_status_above(bs, NULL, offset, tail_bytes,
>> &num,
>> + NULL, NULL);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
>> + (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
>>
>> Client side (6/9):
>>
>> + *pnum = extent.length >> BDRV_SECTOR_BITS;
>> + return (extent.flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_DATA) |
>> + (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
>>
>> Does anything there strike you as odd?
> Two things I noticed while reading the above:
>
> 1. NBD doesn't consider backing files, so the definition of holes
> becomes ambiguous. Is a hole any block that isn't allocated in the
> top layer (may cause fragmentation or encounter an ENOSPC error) or
> is it any block that isn't allocated anywhere in the whole backing
> chain (may read as non-zero)?
>
> Considering that there is a separate NBD_STATE_ZERO and nothing
> forbids a state of NBD_STATE_HOLE without NBD_STATE_ZERO, maybe the
> former is more useful. The code you quote implements the latter.
>
> Maybe if we go with the former, we should add a note to the NBD spec
> that explictly says that NBD_STATE_HOLE doesn't imply any specific
> content that is returned on reads.
>
> 2. Using BDRV_BLOCK_ALLOCATED to determine NBD_STATE_HOLE seems wrong. A
> (not preallocated) zero cluster in qcow2 returns BDRV_BLOCK_ALLOCATED
> (because we don't fall through to the backing file) even though I
> think it's a hole. BDRV_BLOCK_DATA should be used there (which makes
> it consistent with the other direction).
>
>> In isolation, they seemed fine to
>> me, but side-by-side, I'm scratching my head: the server queries the block
>> layer, and turns BDRV_BLOCK_ALLOCATED into !NBD_STATE_HOLE; the client side
>> then takes the NBD protocol and tries to turn it back into information to
>> feed the block layer, where !NBD_STATE_HOLE now feeds BDRV_BLOCK_DATA. Why
>> the different choice of bits?
> Which is actually consistent in the end, becaue BDRV_BLOCK_DATA implies
> BDRV_BLOCK_ALLOCATED.
>
> Essentially, assuming a simple backing chain 'base <- overlay', we got
> these combinations to represent in NBD (with my suggestion of the flags
> to use):
>
> 1. Cluster allocated in overlay
> a. non-zero data 0
> b. explicit zeroes 0 or ZERO
> 2. Cluster marked zero in overlay HOLE | ZERO
> 3. Cluster preallocated/zero in overlay ZERO
> 4. Cluster unallocated in overlay
> a. Cluster allocated in base (non-zero) HOLE
> b. Cluster allocated in base (zero) HOLE or HOLE | ZERO
> c. Cluster marked zero in base HOLE | ZERO
> d. Cluster preallocated/zero in base HOLE | ZERO
> e. Cluster unallocated in base HOLE | ZERO
>
> Instead of 'base' you can read 'anywhere in the backing chain' and the
> flags should stay the same.
I think only "anywhere in the backing chain" is valid here. Otherwise,
semantics of bdrv_is_allocated would differ for NBD and for not-NBD. I
think, if bdrv_is_allocated returns false, it means that we can skip
this region in copying process, am I right?
>
> So !BDRV_BLOCK_ALLOCATED (i.e. falling through to the backing file) does
> indeed imply NBD_STATE_HOLE, but so does case 2, which is just !DATA.
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-03-01 7:25 ` Vladimir Sementsov-Ogievskiy
@ 2018-03-01 9:48 ` Kevin Wolf
2018-03-01 9:57 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2018-03-01 9:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Eric Blake, qemu-devel, famz, qemu-block, Max Reitz
Am 01.03.2018 um 08:25 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 26.02.2018 17:05, Kevin Wolf wrote:
> > Essentially, assuming a simple backing chain 'base <- overlay', we got
> > these combinations to represent in NBD (with my suggestion of the flags
> > to use):
> >
> > 1. Cluster allocated in overlay
> > a. non-zero data 0
> > b. explicit zeroes 0 or ZERO
> > 2. Cluster marked zero in overlay HOLE | ZERO
> > 3. Cluster preallocated/zero in overlay ZERO
> > 4. Cluster unallocated in overlay
> > a. Cluster allocated in base (non-zero) HOLE
> > b. Cluster allocated in base (zero) HOLE or HOLE | ZERO
> > c. Cluster marked zero in base HOLE | ZERO
> > d. Cluster preallocated/zero in base HOLE | ZERO
> > e. Cluster unallocated in base HOLE | ZERO
> >
> > Instead of 'base' you can read 'anywhere in the backing chain' and the
> > flags should stay the same.
>
> I think only "anywhere in the backing chain" is valid here. Otherwise,
> semantics of bdrv_is_allocated would differ for NBD and for not-NBD.
This was meant as a mapping from cases to flags, not the other way
round, so really doesn't say anything about the cases where the block is
allocated further down the chain.
But yes, it shouldn't make a difference where in the backing chain a
block is allocated, so these cases are the same as 4.
> I think, if bdrv_is_allocated returns false, it means that we can skip
> this region in copying process, am I right?
-ENOCONTEXT? Which copying process?
There are cases where you want to copy such regions, and other cases
where you want to skip them. It depends on the use case. For example,
'qemu-img convert' skips them with -B (because the backing file is
reused), but not without -B (which creates a full copy).
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-03-01 9:48 ` Kevin Wolf
@ 2018-03-01 9:57 ` Vladimir Sementsov-Ogievskiy
2018-03-01 10:13 ` Kevin Wolf
0 siblings, 1 reply; 42+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-01 9:57 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Eric Blake, qemu-devel, famz, qemu-block, Max Reitz
01.03.2018 12:48, Kevin Wolf wrote:
> Am 01.03.2018 um 08:25 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 26.02.2018 17:05, Kevin Wolf wrote:
>>> Essentially, assuming a simple backing chain 'base <- overlay', we got
>>> these combinations to represent in NBD (with my suggestion of the flags
>>> to use):
>>>
>>> 1. Cluster allocated in overlay
>>> a. non-zero data 0
>>> b. explicit zeroes 0 or ZERO
>>> 2. Cluster marked zero in overlay HOLE | ZERO
>>> 3. Cluster preallocated/zero in overlay ZERO
>>> 4. Cluster unallocated in overlay
>>> a. Cluster allocated in base (non-zero) HOLE
>>> b. Cluster allocated in base (zero) HOLE or HOLE | ZERO
>>> c. Cluster marked zero in base HOLE | ZERO
>>> d. Cluster preallocated/zero in base HOLE | ZERO
>>> e. Cluster unallocated in base HOLE | ZERO
>>>
>>> Instead of 'base' you can read 'anywhere in the backing chain' and the
>>> flags should stay the same.
>> I think only "anywhere in the backing chain" is valid here. Otherwise,
>> semantics of bdrv_is_allocated would differ for NBD and for not-NBD.
> This was meant as a mapping from cases to flags, not the other way
> round, so really doesn't say anything about the cases where the block is
> allocated further down the chain.
>
> But yes, it shouldn't make a difference where in the backing chain a
> block is allocated, so these cases are the same as 4.
>
>> I think, if bdrv_is_allocated returns false, it means that we can skip
>> this region in copying process, am I right?
> -ENOCONTEXT? Which copying process?
>
> There are cases where you want to copy such regions, and other cases
> where you want to skip them. It depends on the use case. For example,
> 'qemu-img convert' skips them with -B (because the backing file is
> reused), but not without -B (which creates a full copy).
>
> Kevin
Hm, I thought that bdrv_is_allocated loops through backings, but it
doesn't, sorry.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()
2018-03-01 9:57 ` Vladimir Sementsov-Ogievskiy
@ 2018-03-01 10:13 ` Kevin Wolf
0 siblings, 0 replies; 42+ messages in thread
From: Kevin Wolf @ 2018-03-01 10:13 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Eric Blake, qemu-devel, famz, qemu-block, Max Reitz
Am 01.03.2018 um 10:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 01.03.2018 12:48, Kevin Wolf wrote:
> > Am 01.03.2018 um 08:25 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 26.02.2018 17:05, Kevin Wolf wrote:
> > > > Essentially, assuming a simple backing chain 'base <- overlay', we got
> > > > these combinations to represent in NBD (with my suggestion of the flags
> > > > to use):
> > > >
> > > > 1. Cluster allocated in overlay
> > > > a. non-zero data 0
> > > > b. explicit zeroes 0 or ZERO
> > > > 2. Cluster marked zero in overlay HOLE | ZERO
> > > > 3. Cluster preallocated/zero in overlay ZERO
> > > > 4. Cluster unallocated in overlay
> > > > a. Cluster allocated in base (non-zero) HOLE
> > > > b. Cluster allocated in base (zero) HOLE or HOLE | ZERO
> > > > c. Cluster marked zero in base HOLE | ZERO
> > > > d. Cluster preallocated/zero in base HOLE | ZERO
> > > > e. Cluster unallocated in base HOLE | ZERO
> > > >
> > > > Instead of 'base' you can read 'anywhere in the backing chain' and the
> > > > flags should stay the same.
> > > I think only "anywhere in the backing chain" is valid here. Otherwise,
> > > semantics of bdrv_is_allocated would differ for NBD and for not-NBD.
> > This was meant as a mapping from cases to flags, not the other way
> > round, so really doesn't say anything about the cases where the block is
> > allocated further down the chain.
> >
> > But yes, it shouldn't make a difference where in the backing chain a
> > block is allocated, so these cases are the same as 4.
> >
> > > I think, if bdrv_is_allocated returns false, it means that we can skip
> > > this region in copying process, am I right?
> > -ENOCONTEXT? Which copying process?
> >
> > There are cases where you want to copy such regions, and other cases
> > where you want to skip them. It depends on the use case. For example,
> > 'qemu-img convert' skips them with -B (because the backing file is
> > reused), but not without -B (which creates a full copy).
> >
> > Kevin
>
> Hm, I thought that bdrv_is_allocated loops through backings, but it doesn't,
> sorry.
That would be bdrv_is_allocated_above() with a NULL base.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH v8 10/21] parallels: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (8 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 09/21] null: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 11/21] qcow: " Eric Blake
` (11 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the parallels driver accordingly. Note that
the internal function block_status() is still sector-based, because
it is still in use by other sector-based functions; but that's okay
because request_alignment is 512 as a result of those functions.
For now, no optimizations are added based on the mapping hint.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v7: fix bug in *map [Vladimir]
v6: no change
v5: fix pnum when return is 0
v4: rebase to interface tweak, R-b dropped
v3: no change
v2: rebase to mapping parameter; it is ignored, so R-b kept
---
block/parallels.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index e1e3d80c887..3e952a9c147 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -261,23 +261,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
}
-static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn parallels_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BDRVParallelsState *s = bs->opaque;
- int64_t offset;
+ int count;
+ assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
qemu_co_mutex_lock(&s->lock);
- offset = block_status(s, sector_num, nb_sectors, pnum);
+ offset = block_status(s, offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS, &count);
qemu_co_mutex_unlock(&s->lock);
+ *pnum = count * BDRV_SECTOR_SIZE;
if (offset < 0) {
return 0;
}
+ *map = offset * BDRV_SECTOR_SIZE;
*file = bs->file->bs;
- return (offset << BDRV_SECTOR_BITS) |
- BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
@@ -782,7 +790,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_open = parallels_open,
.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_format_default_perms,
- .bdrv_co_get_block_status = parallels_co_get_block_status,
+ .bdrv_co_block_status = parallels_co_block_status,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_flush_to_os = parallels_co_flush_to_os,
.bdrv_co_readv = parallels_co_readv,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 11/21] qcow: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (9 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 10/21] parallels: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 12/21] qcow2: " Eric Blake
` (10 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the qcow driver accordingly. There is no
intent to optimize based on the want_zero flag for this format.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v5-v7: no change
v4: rebase to interface tweak
v3: rebase to master
v2: rebase to mapping flag
---
block/qcow.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/block/qcow.c b/block/qcow.c
index 8631155ac81..dead5029c67 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -524,23 +524,28 @@ static int get_cluster_offset(BlockDriverState *bs,
return 1;
}
-static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn qcow_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVQcowState *s = bs->opaque;
- int index_in_cluster, n, ret;
+ int index_in_cluster, ret;
+ int64_t n;
uint64_t cluster_offset;
qemu_co_mutex_lock(&s->lock);
- ret = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0, &cluster_offset);
+ ret = get_cluster_offset(bs, offset, 0, 0, 0, 0, &cluster_offset);
qemu_co_mutex_unlock(&s->lock);
if (ret < 0) {
return ret;
}
- index_in_cluster = sector_num & (s->cluster_sectors - 1);
- n = s->cluster_sectors - index_in_cluster;
- if (n > nb_sectors)
- n = nb_sectors;
+ index_in_cluster = offset & (s->cluster_size - 1);
+ n = s->cluster_size - index_in_cluster;
+ if (n > bytes) {
+ n = bytes;
+ }
*pnum = n;
if (!cluster_offset) {
return 0;
@@ -548,9 +553,9 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypto) {
return BDRV_BLOCK_DATA;
}
- cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+ *map = cluster_offset | index_in_cluster;
*file = bs->file->bs;
- return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
@@ -1128,7 +1133,7 @@ static BlockDriver bdrv_qcow = {
.bdrv_co_readv = qcow_co_readv,
.bdrv_co_writev = qcow_co_writev,
- .bdrv_co_get_block_status = qcow_co_get_block_status,
+ .bdrv_co_block_status = qcow_co_block_status,
.bdrv_make_empty = qcow_make_empty,
.bdrv_co_pwritev_compressed = qcow_co_pwritev_compressed,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 12/21] qcow2: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (10 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 11/21] qcow: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 13/21] qed: " Eric Blake
` (9 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the qcow2 driver accordingly.
For now, we are ignoring the 'want_zero' hint. However, it should
be relatively straightforward to honor the hint as a way to return
larger *pnum values when we have consecutive clusters with the same
data/zero status but which differ only in having non-consecutive
mappings.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v5-v7: no change
v4: update to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/qcow2.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 57a517e2bdd..288b5299d80 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1670,32 +1670,34 @@ static void qcow2_join_options(QDict *options, QDict *old_options)
}
}
-static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t count,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVQcow2State *s = bs->opaque;
uint64_t cluster_offset;
int index_in_cluster, ret;
unsigned int bytes;
- int64_t status = 0;
+ int status = 0;
- bytes = MIN(INT_MAX, nb_sectors * BDRV_SECTOR_SIZE);
+ bytes = MIN(INT_MAX, count);
qemu_co_mutex_lock(&s->lock);
- ret = qcow2_get_cluster_offset(bs, sector_num << BDRV_SECTOR_BITS, &bytes,
- &cluster_offset);
+ ret = qcow2_get_cluster_offset(bs, offset, &bytes, &cluster_offset);
qemu_co_mutex_unlock(&s->lock);
if (ret < 0) {
return ret;
}
- *pnum = bytes >> BDRV_SECTOR_BITS;
+ *pnum = bytes;
if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED &&
!s->crypto) {
- index_in_cluster = sector_num & (s->cluster_sectors - 1);
- cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+ index_in_cluster = offset & (s->cluster_size - 1);
+ *map = cluster_offset | index_in_cluster;
*file = bs->file->bs;
- status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
+ status |= BDRV_BLOCK_OFFSET_VALID;
}
if (ret == QCOW2_CLUSTER_ZERO_PLAIN || ret == QCOW2_CLUSTER_ZERO_ALLOC) {
status |= BDRV_BLOCK_ZERO;
@@ -4352,7 +4354,7 @@ BlockDriver bdrv_qcow2 = {
.bdrv_child_perm = bdrv_format_default_perms,
.bdrv_create = qcow2_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_co_get_block_status = qcow2_co_get_block_status,
+ .bdrv_co_block_status = qcow2_co_block_status,
.bdrv_co_preadv = qcow2_co_preadv,
.bdrv_co_pwritev = qcow2_co_pwritev,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 13/21] qed: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (11 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 12/21] qcow2: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 14/21] raw: " Eric Blake
` (8 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the qed driver accordingly, taking the opportunity
to inline qed_is_allocated_cb() into its lone caller (the callback
used to be important, until we switched qed to coroutines). There is
no intent to optimize based on the want_zero flag for this format.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: initialize len before qed_find_cluster() [Vladimir]
v4: rebase to interface change, inline pointless callback
v3: no change
v2: rebase to mapping flag, fix mask in qed_is_allocated_cb
---
block/qed.c | 84 +++++++++++++++++++++----------------------------------------
1 file changed, 28 insertions(+), 56 deletions(-)
diff --git a/block/qed.c b/block/qed.c
index c6ff3ab015d..a5952209261 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -688,74 +688,46 @@ finish:
return ret;
}
-typedef struct {
- BlockDriverState *bs;
- Coroutine *co;
- uint64_t pos;
- int64_t status;
- int *pnum;
- BlockDriverState **file;
-} QEDIsAllocatedCB;
-
-/* Called with table_lock held. */
-static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
-{
- QEDIsAllocatedCB *cb = opaque;
- BDRVQEDState *s = cb->bs->opaque;
- *cb->pnum = len / BDRV_SECTOR_SIZE;
- switch (ret) {
- case QED_CLUSTER_FOUND:
- offset |= qed_offset_into_cluster(s, cb->pos);
- cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
- *cb->file = cb->bs->file->bs;
- break;
- case QED_CLUSTER_ZERO:
- cb->status = BDRV_BLOCK_ZERO;
- break;
- case QED_CLUSTER_L2:
- case QED_CLUSTER_L1:
- cb->status = 0;
- break;
- default:
- assert(ret < 0);
- cb->status = ret;
- break;
- }
-
- if (cb->co) {
- aio_co_wake(cb->co);
- }
-}
-
-static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
+static int coroutine_fn bdrv_qed_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t pos, int64_t bytes,
+ int64_t *pnum, int64_t *map,
BlockDriverState **file)
{
BDRVQEDState *s = bs->opaque;
- size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
- QEDIsAllocatedCB cb = {
- .bs = bs,
- .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
- .status = BDRV_BLOCK_OFFSET_MASK,
- .pnum = pnum,
- .file = file,
- };
+ size_t len = MIN(bytes, SIZE_MAX);
+ int status;
QEDRequest request = { .l2_table = NULL };
uint64_t offset;
int ret;
qemu_co_mutex_lock(&s->table_lock);
- ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
- qed_is_allocated_cb(&cb, ret, offset, len);
+ ret = qed_find_cluster(s, &request, pos, &len, &offset);
- /* The callback was invoked immediately */
- assert(cb.status != BDRV_BLOCK_OFFSET_MASK);
+ *pnum = len;
+ switch (ret) {
+ case QED_CLUSTER_FOUND:
+ *map = offset | qed_offset_into_cluster(s, pos);
+ status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ *file = bs->file->bs;
+ break;
+ case QED_CLUSTER_ZERO:
+ status = BDRV_BLOCK_ZERO;
+ break;
+ case QED_CLUSTER_L2:
+ case QED_CLUSTER_L1:
+ status = 0;
+ break;
+ default:
+ assert(ret < 0);
+ status = ret;
+ break;
+ }
qed_unref_l2_cache_entry(request.l2_table);
qemu_co_mutex_unlock(&s->table_lock);
- return cb.status;
+ return status;
}
static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
@@ -1594,7 +1566,7 @@ static BlockDriver bdrv_qed = {
.bdrv_child_perm = bdrv_format_default_perms,
.bdrv_create = bdrv_qed_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
+ .bdrv_co_block_status = bdrv_qed_co_block_status,
.bdrv_co_readv = bdrv_qed_co_readv,
.bdrv_co_writev = bdrv_qed_co_writev,
.bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 14/21] raw: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (12 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 13/21] qed: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 15/21] sheepdog: " Eric Blake
` (7 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the raw driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v5-v7: no change
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping
---
block/raw-format.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/block/raw-format.c b/block/raw-format.c
index ab552c09541..830243a8e48 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -250,17 +250,17 @@ fail:
return ret;
}
-static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
+static int coroutine_fn raw_co_block_status(BlockDriverState *bs,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
BlockDriverState **file)
{
BDRVRawState *s = bs->opaque;
- *pnum = nb_sectors;
+ *pnum = bytes;
*file = bs->file->bs;
- sector_num += s->offset / BDRV_SECTOR_SIZE;
- return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
- (sector_num << BDRV_SECTOR_BITS);
+ *map = offset + s->offset;
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
}
static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs,
@@ -496,7 +496,7 @@ BlockDriver bdrv_raw = {
.bdrv_co_pwritev = &raw_co_pwritev,
.bdrv_co_pwrite_zeroes = &raw_co_pwrite_zeroes,
.bdrv_co_pdiscard = &raw_co_pdiscard,
- .bdrv_co_get_block_status = &raw_co_get_block_status,
+ .bdrv_co_block_status = &raw_co_block_status,
.bdrv_truncate = &raw_truncate,
.bdrv_getlength = &raw_getlength,
.has_variable_length = true,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 15/21] sheepdog: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (13 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 14/21] raw: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 16/21] vdi: Avoid bitrot of debugging code Eric Blake
` (6 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, Hitoshi Mitake, Liu Yuan, Jeff Cody,
Max Reitz, open list:Sheepdog
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the sheepdog driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
v7: rebase to minor spacing changes in master
v5-v6: no change
v4: update to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/sheepdog.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index ac02b10fe03..3c3becf94df 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3004,19 +3004,19 @@ static coroutine_fn int sd_co_pdiscard(BlockDriverState *bs, int64_t offset,
return acb.ret;
}
-static coroutine_fn int64_t
-sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
- int *pnum, BlockDriverState **file)
+static coroutine_fn int
+sd_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVSheepdogState *s = bs->opaque;
SheepdogInode *inode = &s->inode;
uint32_t object_size = (UINT32_C(1) << inode->block_size_shift);
- uint64_t offset = sector_num * BDRV_SECTOR_SIZE;
unsigned long start = offset / object_size,
- end = DIV_ROUND_UP((sector_num + nb_sectors) *
- BDRV_SECTOR_SIZE, object_size);
+ end = DIV_ROUND_UP(offset + bytes, object_size);
unsigned long idx;
- int64_t ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+ *map = offset;
+ int ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
for (idx = start; idx < end; idx++) {
if (inode->data_vdi_id[idx] == 0) {
@@ -3033,9 +3033,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
}
}
- *pnum = (idx - start) * object_size / BDRV_SECTOR_SIZE;
- if (*pnum > nb_sectors) {
- *pnum = nb_sectors;
+ *pnum = (idx - start) * object_size;
+ if (*pnum > bytes) {
+ *pnum = bytes;
}
if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
*file = bs;
@@ -3113,7 +3113,7 @@ static BlockDriver bdrv_sheepdog = {
.bdrv_co_writev = sd_co_writev,
.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
.bdrv_co_pdiscard = sd_co_pdiscard,
- .bdrv_co_get_block_status = sd_co_get_block_status,
+ .bdrv_co_block_status = sd_co_block_status,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
@@ -3149,7 +3149,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
.bdrv_co_writev = sd_co_writev,
.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
.bdrv_co_pdiscard = sd_co_pdiscard,
- .bdrv_co_get_block_status = sd_co_get_block_status,
+ .bdrv_co_block_status = sd_co_block_status,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
@@ -3185,7 +3185,7 @@ static BlockDriver bdrv_sheepdog_unix = {
.bdrv_co_writev = sd_co_writev,
.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
.bdrv_co_pdiscard = sd_co_pdiscard,
- .bdrv_co_get_block_status = sd_co_get_block_status,
+ .bdrv_co_block_status = sd_co_block_status,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 16/21] vdi: Avoid bitrot of debugging code
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (14 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 15/21] sheepdog: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 17/21] vdi: Switch to .bdrv_co_block_status() Eric Blake
` (5 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Stefan Weil, Max Reitz
Rework the debug define so that we always get -Wformat checking,
even when debugging is disabled.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v2-v7: no change
---
block/vdi.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index fc1c614cb12..32b1763cde0 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -87,12 +87,18 @@
#define DEFAULT_CLUSTER_SIZE (1 * MiB)
#if defined(CONFIG_VDI_DEBUG)
-#define logout(fmt, ...) \
- fprintf(stderr, "vdi\t%-24s" fmt, __func__, ##__VA_ARGS__)
+#define VDI_DEBUG 1
#else
-#define logout(fmt, ...) ((void)0)
+#define VDI_DEBUG 0
#endif
+#define logout(fmt, ...) \
+ do { \
+ if (VDI_DEBUG) { \
+ fprintf(stderr, "vdi\t%-24s" fmt, __func__, ##__VA_ARGS__); \
+ } \
+ } while (0)
+
/* Image signature. */
#define VDI_SIGNATURE 0xbeda107f
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 17/21] vdi: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (15 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 16/21] vdi: Avoid bitrot of debugging code Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 18/21] vmdk: " Eric Blake
` (4 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Stefan Weil, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vdi driver accordingly. Note that the
TODO is already covered (the block layer guarantees bounds of its
requests), and that we can remove the now-unused s->block_sectors.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: fix pnum when offset rounded down to block_size [Vladimir]
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/vdi.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 32b1763cde0..0780c82d829 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -172,8 +172,6 @@ typedef struct {
uint32_t *bmap;
/* Size of block (bytes). */
uint32_t block_size;
- /* Size of block (sectors). */
- uint32_t block_sectors;
/* First sector of block map. */
uint32_t bmap_sector;
/* VDI header (converted to host endianness). */
@@ -463,7 +461,6 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
bs->total_sectors = header.disk_size / SECTOR_SIZE;
s->block_size = header.block_size;
- s->block_sectors = header.block_size / SECTOR_SIZE;
s->bmap_sector = header.offset_bmap / SECTOR_SIZE;
s->header = header;
@@ -509,33 +506,29 @@ static int vdi_reopen_prepare(BDRVReopenState *state,
return 0;
}
-static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
- /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */
BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
- size_t bmap_index = sector_num / s->block_sectors;
- size_t sector_in_block = sector_num % s->block_sectors;
- int n_sectors = s->block_sectors - sector_in_block;
+ size_t bmap_index = offset / s->block_size;
+ size_t index_in_block = offset % s->block_size;
uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]);
- uint64_t offset;
int result;
- logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, pnum);
- if (n_sectors > nb_sectors) {
- n_sectors = nb_sectors;
- }
- *pnum = n_sectors;
+ logout("%p, %" PRId64 ", %" PRId64 ", %p\n", bs, offset, bytes, pnum);
+ *pnum = MIN(s->block_size - index_in_block, bytes);
result = VDI_IS_ALLOCATED(bmap_entry);
if (!result) {
return 0;
}
- offset = s->header.offset_data +
- (uint64_t)bmap_entry * s->block_size +
- sector_in_block * SECTOR_SIZE;
+ *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
+ index_in_block;
*file = bs->file->bs;
- return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
static int coroutine_fn
@@ -903,7 +896,7 @@ static BlockDriver bdrv_vdi = {
.bdrv_child_perm = bdrv_format_default_perms,
.bdrv_create = vdi_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_co_get_block_status = vdi_co_get_block_status,
+ .bdrv_co_block_status = vdi_co_block_status,
.bdrv_make_empty = vdi_make_empty,
.bdrv_co_preadv = vdi_co_preadv,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 18/21] vmdk: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (16 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 17/21] vdi: Switch to .bdrv_co_block_status() Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 19/21] vpc: " Eric Blake
` (3 subsequent siblings)
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vmdk driver accordingly. Drop the
now-unused vmdk_find_index_in_cluster().
Also, fix a pre-existing bug: if find_extent() fails (unlikely,
since the block layer did a bounds check), then we must return a
failure, rather than 0.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v6-v7: no change
v5: drop dead code [Vladimir], return error on find_extent() failure
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/vmdk.c | 38 ++++++++++++++------------------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index ef15ddbfd3d..75f84213e6f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1304,33 +1304,27 @@ static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
return extent_relative_offset % cluster_size;
}
-static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
- int64_t sector_num)
-{
- uint64_t offset;
- offset = vmdk_find_offset_in_cluster(extent, sector_num * BDRV_SECTOR_SIZE);
- return offset / BDRV_SECTOR_SIZE;
-}
-
-static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn vmdk_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVVmdkState *s = bs->opaque;
int64_t index_in_cluster, n, ret;
- uint64_t offset;
+ uint64_t cluster_offset;
VmdkExtent *extent;
- extent = find_extent(s, sector_num, NULL);
+ extent = find_extent(s, offset >> BDRV_SECTOR_BITS, NULL);
if (!extent) {
- return 0;
+ return -EIO;
}
qemu_co_mutex_lock(&s->lock);
- ret = get_cluster_offset(bs, extent, NULL,
- sector_num * 512, false, &offset,
+ ret = get_cluster_offset(bs, extent, NULL, offset, false, &cluster_offset,
0, 0);
qemu_co_mutex_unlock(&s->lock);
- index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
+ index_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
switch (ret) {
case VMDK_ERROR:
ret = -EIO;
@@ -1345,18 +1339,14 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
ret = BDRV_BLOCK_DATA;
if (!extent->compressed) {
ret |= BDRV_BLOCK_OFFSET_VALID;
- ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
- & BDRV_BLOCK_OFFSET_MASK;
+ *map = cluster_offset + index_in_cluster;
}
*file = extent->file->bs;
break;
}
- n = extent->cluster_sectors - index_in_cluster;
- if (n > nb_sectors) {
- n = nb_sectors;
- }
- *pnum = n;
+ n = extent->cluster_sectors * BDRV_SECTOR_SIZE - index_in_cluster;
+ *pnum = MIN(n, bytes);
return ret;
}
@@ -2410,7 +2400,7 @@ static BlockDriver bdrv_vmdk = {
.bdrv_close = vmdk_close,
.bdrv_create = vmdk_create,
.bdrv_co_flush_to_disk = vmdk_co_flush,
- .bdrv_co_get_block_status = vmdk_co_get_block_status,
+ .bdrv_co_block_status = vmdk_co_block_status,
.bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
.bdrv_has_zero_init = vmdk_has_zero_init,
.bdrv_get_specific_info = vmdk_get_specific_info,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* [Qemu-devel] [PATCH v8 19/21] vpc: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (17 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 18/21] vmdk: " Eric Blake
@ 2018-02-13 20:26 ` Eric Blake
2018-02-14 13:08 ` Kevin Wolf
2018-02-13 20:27 ` [Qemu-devel] [PATCH v8 20/21] vvfat: " Eric Blake
` (2 subsequent siblings)
21 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:26 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vpc driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v7: tweak commit message and type of 'n' [Fam]
v6: no change
v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir]
v4: rebase to interface tweak
v3: rebase to master
v2: drop get_sector_offset() [Kevin], rebase to mapping flag
---
block/vpc.c | 45 +++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index cfa5144e867..fba4492fd7b 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -706,53 +706,54 @@ fail:
return ret;
}
-static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVVPCState *s = bs->opaque;
VHDFooter *footer = (VHDFooter*) s->footer_buf;
- int64_t start, offset;
+ int64_t image_offset;
bool allocated;
- int64_t ret;
- int n;
+ int ret;
+ int64_t n;
if (be32_to_cpu(footer->type) == VHD_FIXED) {
- *pnum = nb_sectors;
+ *pnum = bytes;
+ *map = offset;
*file = bs->file->bs;
- return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
- (sector_num << BDRV_SECTOR_BITS);
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
}
qemu_co_mutex_lock(&s->lock);
- offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, NULL);
- start = offset;
- allocated = (offset != -1);
+ image_offset = get_image_offset(bs, offset, false, NULL);
+ allocated = (image_offset != -1);
*pnum = 0;
ret = 0;
do {
/* All sectors in a block are contiguous (without using the bitmap) */
- n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
- - sector_num;
- n = MIN(n, nb_sectors);
+ n = ROUND_UP(offset + 1, s->block_size) - offset;
+ n = MIN(n, bytes);
*pnum += n;
- sector_num += n;
- nb_sectors -= n;
+ offset += n;
+ bytes -= n;
/* *pnum can't be greater than one block for allocated
* sectors since there is always a bitmap in between. */
if (allocated) {
*file = bs->file->bs;
- ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
+ *map = image_offset;
+ ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
break;
}
- if (nb_sectors == 0) {
+ if (bytes == 0) {
break;
}
- offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false,
- NULL);
- } while (offset == -1);
+ image_offset = get_image_offset(bs, offset, false, NULL);
+ } while (image_offset == -1);
qemu_co_mutex_unlock(&s->lock);
return ret;
@@ -1098,7 +1099,7 @@ static BlockDriver bdrv_vpc = {
.bdrv_co_preadv = vpc_co_preadv,
.bdrv_co_pwritev = vpc_co_pwritev,
- .bdrv_co_get_block_status = vpc_co_get_block_status,
+ .bdrv_co_block_status = vpc_co_block_status,
.bdrv_get_info = vpc_get_info,
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 19/21] vpc: Switch to .bdrv_co_block_status()
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 19/21] vpc: " Eric Blake
@ 2018-02-14 13:08 ` Kevin Wolf
2018-02-14 14:51 ` Eric Blake
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 13:08 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the vpc driver accordingly.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
>
> ---
> v7: tweak commit message and type of 'n' [Fam]
> v6: no change
> v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir]
> v4: rebase to interface tweak
> v3: rebase to master
> v2: drop get_sector_offset() [Kevin], rebase to mapping flag
> ---
> block/vpc.c | 45 +++++++++++++++++++++++----------------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index cfa5144e867..fba4492fd7b 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -706,53 +706,54 @@ fail:
> return ret;
> }
>
> -static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
> +static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
> + bool want_zero,
> + int64_t offset, int64_t bytes,
> + int64_t *pnum, int64_t *map,
> + BlockDriverState **file)
> {
> BDRVVPCState *s = bs->opaque;
> VHDFooter *footer = (VHDFooter*) s->footer_buf;
> - int64_t start, offset;
> + int64_t image_offset;
> bool allocated;
> - int64_t ret;
> - int n;
> + int ret;
> + int64_t n;
>
> if (be32_to_cpu(footer->type) == VHD_FIXED) {
> - *pnum = nb_sectors;
> + *pnum = bytes;
> + *map = offset;
> *file = bs->file->bs;
> - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> - (sector_num << BDRV_SECTOR_BITS);
> + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
> }
>
> qemu_co_mutex_lock(&s->lock);
>
> - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, NULL);
> - start = offset;
> - allocated = (offset != -1);
> + image_offset = get_image_offset(bs, offset, false, NULL);
> + allocated = (image_offset != -1);
> *pnum = 0;
> ret = 0;
>
> do {
> /* All sectors in a block are contiguous (without using the bitmap) */
> - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
> - - sector_num;
> - n = MIN(n, nb_sectors);
> + n = ROUND_UP(offset + 1, s->block_size) - offset;
> + n = MIN(n, bytes);
>
> *pnum += n;
> - sector_num += n;
> - nb_sectors -= n;
> + offset += n;
> + bytes -= n;
> /* *pnum can't be greater than one block for allocated
> * sectors since there is always a bitmap in between. */
> if (allocated) {
> *file = bs->file->bs;
> - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> + *map = image_offset;
This does work, but only because the loop isn't actually looping for
allocated == true. In the old code, it was obvious that start was
assigned only once and never changed during the loop, but image_offset
changes in each loop iteration.
It would probably be cleaner and more obviously correct to move the
assignment of *map to before the loop.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 19/21] vpc: Switch to .bdrv_co_block_status()
2018-02-14 13:08 ` Kevin Wolf
@ 2018-02-14 14:51 ` Eric Blake
0 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-14 14:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, famz, qemu-block, Max Reitz
On 02/14/2018 07:08 AM, Kevin Wolf wrote:
> Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the vpc driver accordingly.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>
>> ---
>> + allocated = (image_offset != -1);
>> *pnum = 0;
>> ret = 0;
>>
>> do {
>> /* All sectors in a block are contiguous (without using the bitmap) */
>> - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
>> - - sector_num;
>> - n = MIN(n, nb_sectors);
>> + n = ROUND_UP(offset + 1, s->block_size) - offset;
>> + n = MIN(n, bytes);
>>
>> *pnum += n;
>> - sector_num += n;
>> - nb_sectors -= n;
>> + offset += n;
>> + bytes -= n;
>> /* *pnum can't be greater than one block for allocated
>> * sectors since there is always a bitmap in between. */
>> if (allocated) {
>> *file = bs->file->bs;
>> - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
>> + *map = image_offset;
>
> This does work, but only because the loop isn't actually looping for
> allocated == true. In the old code, it was obvious that start was
> assigned only once and never changed during the loop, but image_offset
> changes in each loop iteration.
>
> It would probably be cleaner and more obviously correct to move the
> assignment of *map to before the loop.
Yes, that would be a bit nicer.
>
> Kevin
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH v8 20/21] vvfat: Switch to .bdrv_co_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (18 preceding siblings ...)
2018-02-13 20:26 ` [Qemu-devel] [PATCH v8 19/21] vpc: " Eric Blake
@ 2018-02-13 20:27 ` Eric Blake
2018-02-14 13:12 ` Kevin Wolf
2018-02-13 20:27 ` [Qemu-devel] [PATCH v8 21/21] block: Drop unused .bdrv_co_get_block_status() Eric Blake
2018-02-14 17:11 ` [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Kevin Wolf
21 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:27 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vvfat driver accordingly. Note that we
can rely on the block driver having already clamped limits to our
block size, and simplify accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v5-v7: no change
v4: rebase to interface tweak
v3: no change
v2: rebase to earlier changes, simplify
---
block/vvfat.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 7e06ebacf61..4a17a49e128 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3088,15 +3088,13 @@ vvfat_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
return ret;
}
-static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
+static int coroutine_fn vvfat_co_block_status(BlockDriverState *bs,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *n,
+ int64_t *map,
+ BlockDriverState **file)
{
- *n = bs->total_sectors - sector_num;
- if (*n > nb_sectors) {
- *n = nb_sectors;
- } else if (*n < 0) {
- return 0;
- }
+ *n = bytes;
return BDRV_BLOCK_DATA;
}
@@ -3257,7 +3255,7 @@ static BlockDriver bdrv_vvfat = {
.bdrv_co_preadv = vvfat_co_preadv,
.bdrv_co_pwritev = vvfat_co_pwritev,
- .bdrv_co_get_block_status = vvfat_co_get_block_status,
+ .bdrv_co_block_status = vvfat_co_block_status,
};
static void bdrv_vvfat_init(void)
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 20/21] vvfat: Switch to .bdrv_co_block_status()
2018-02-13 20:27 ` [Qemu-devel] [PATCH v8 20/21] vvfat: " Eric Blake
@ 2018-02-14 13:12 ` Kevin Wolf
2018-02-14 14:50 ` Eric Blake
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 13:12 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
Am 13.02.2018 um 21:27 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the vvfat driver accordingly. Note that we
> can rely on the block driver having already clamped limits to our
> block size, and simplify accordingly.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
>
> ---
> v5-v7: no change
> v4: rebase to interface tweak
> v3: no change
> v2: rebase to earlier changes, simplify
> ---
> block/vvfat.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 7e06ebacf61..4a17a49e128 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3088,15 +3088,13 @@ vvfat_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> return ret;
> }
>
> -static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
> +static int coroutine_fn vvfat_co_block_status(BlockDriverState *bs,
> + bool want_zero, int64_t offset,
> + int64_t bytes, int64_t *n,
> + int64_t *map,
> + BlockDriverState **file)
> {
> - *n = bs->total_sectors - sector_num;
> - if (*n > nb_sectors) {
> - *n = nb_sectors;
> - } else if (*n < 0) {
> - return 0;
> - }
> + *n = bytes;
> return BDRV_BLOCK_DATA;
> }
Preexisting, but this is inconsistent with other protocol drivers as far
as OFFSET_VALID is concerned (as I hinted in another mail, I like it
better this way, but it's still inconsistent).
Do we actually need any callback here or could the solution be to simply
remove it like with nvme?
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 20/21] vvfat: Switch to .bdrv_co_block_status()
2018-02-14 13:12 ` Kevin Wolf
@ 2018-02-14 14:50 ` Eric Blake
2018-02-14 15:00 ` Kevin Wolf
0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2018-02-14 14:50 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, famz, qemu-block, Max Reitz
On 02/14/2018 07:12 AM, Kevin Wolf wrote:
> Am 13.02.2018 um 21:27 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the vvfat driver accordingly. Note that we
>> can rely on the block driver having already clamped limits to our
>> block size, and simplify accordingly.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> {
>> - *n = bs->total_sectors - sector_num;
>> - if (*n > nb_sectors) {
>> - *n = nb_sectors;
>> - } else if (*n < 0) {
>> - return 0;
>> - }
>> + *n = bytes;
>> return BDRV_BLOCK_DATA;
>> }
>
> Preexisting, but this is inconsistent with other protocol drivers as far
> as OFFSET_VALID is concerned (as I hinted in another mail, I like it
> better this way, but it's still inconsistent).
>
> Do we actually need any callback here or could the solution be to simply
> remove it like with nvme?
Does that mean io.c's defaults for protocol drivers is wrong? It
defaults to setting OFFSET_VALID and *map on all protocol drivers
without a callback (at least nvme, nbd); I didn't delete this callback
because I noticed the difference in return value, and couldn't justify
whether it was intentional. Also, vvfat is weird - it is somewhat of a
format driver, rather than just a protocol; even though it sets
.protocol_name. It may be possible for vvfat to actually set
OFFSET_VALID to particular offsets within the host file that correspond
to what the guest would read, where it is not a simple 1:1 mapping,
precisely because it is imposing format on the host file. However,
vvfat is one of those things that I try to avoid as much as possible,
because it is just so weird.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 20/21] vvfat: Switch to .bdrv_co_block_status()
2018-02-14 14:50 ` Eric Blake
@ 2018-02-14 15:00 ` Kevin Wolf
0 siblings, 0 replies; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 15:00 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block, Max Reitz
Am 14.02.2018 um 15:50 hat Eric Blake geschrieben:
> On 02/14/2018 07:12 AM, Kevin Wolf wrote:
> > Am 13.02.2018 um 21:27 hat Eric Blake geschrieben:
> > > We are gradually moving away from sector-based interfaces, towards
> > > byte-based. Update the vvfat driver accordingly. Note that we
> > > can rely on the block driver having already clamped limits to our
> > > block size, and simplify accordingly.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > Reviewed-by: Fam Zheng <famz@redhat.com>
>
> > > {
> > > - *n = bs->total_sectors - sector_num;
> > > - if (*n > nb_sectors) {
> > > - *n = nb_sectors;
> > > - } else if (*n < 0) {
> > > - return 0;
> > > - }
> > > + *n = bytes;
> > > return BDRV_BLOCK_DATA;
> > > }
> >
> > Preexisting, but this is inconsistent with other protocol drivers as far
> > as OFFSET_VALID is concerned (as I hinted in another mail, I like it
> > better this way, but it's still inconsistent).
> >
> > Do we actually need any callback here or could the solution be to simply
> > remove it like with nvme?
>
> Does that mean io.c's defaults for protocol drivers is wrong? It defaults
> to setting OFFSET_VALID and *map on all protocol drivers without a callback
> (at least nvme, nbd); I didn't delete this callback because I noticed the
> difference in return value, and couldn't justify whether it was intentional.
> Also, vvfat is weird - it is somewhat of a format driver, rather than just a
> protocol; even though it sets .protocol_name. It may be possible for vvfat
> to actually set OFFSET_VALID to particular offsets within the host file that
> correspond to what the guest would read, where it is not a simple 1:1
> mapping, precisely because it is imposing format on the host file. However,
> vvfat is one of those things that I try to avoid as much as possible,
> because it is just so weird.
As I said in my reply to the null block driver, OFFSET_VALID doesn't
really make sense for protocol drivers anyway. Making use of it with
vvfat isn't any more practical than directly accessing the undefined
data of the null driver.
I think the unwritten rule at the moment is that protocols should always
set OFFSET_VALID and *file = bs (even though it doesn't make sense). So
with the current interface, I'd consider deleting the callback a vvfat
fix.
I also think that we should possibly look into changing the interface so
that protocols don't set OFFSET_VALID and *file, but then the default
handling would change too, and deleting the callback in vvfat would
still be right.
As this is preexisting, I'm okay with just merging the series as it is,
and then we can handle this while dealing with the more fundamental
question what protocol drivers should return in general.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH v8 21/21] block: Drop unused .bdrv_co_get_block_status()
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (19 preceding siblings ...)
2018-02-13 20:27 ` [Qemu-devel] [PATCH v8 20/21] vvfat: " Eric Blake
@ 2018-02-13 20:27 ` Eric Blake
2018-02-14 17:11 ` [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Kevin Wolf
21 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2018-02-13 20:27 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Now that all drivers have been updated to provide the
byte-based .bdrv_co_block_status(), we can delete the sector-based
interface.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v7: no change
v6: rebase to changes in patch 1, drop R-b
v5: rebase to master
v4: rebase to interface tweak
v3: no change
v2: rebase to earlier changes
---
include/block/block_int.h | 3 ---
block/io.c | 50 ++++++++++-------------------------------------
2 files changed, 10 insertions(+), 43 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index bf2598856cf..5ae7738cf8d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -215,9 +215,6 @@ struct BlockDriver {
* as well as non-NULL pnum, map, and file; in turn, the driver
* must return an error or set pnum to an aligned non-zero value.
*/
- int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum,
- BlockDriverState **file);
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);
diff --git a/block/io.c b/block/io.c
index 5bae79f282e..4c3dba09730 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1963,7 +1963,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
/* Must be non-NULL or bdrv_getlength() would have failed */
assert(bs->drv);
- if (!bs->drv->bdrv_co_get_block_status && !bs->drv->bdrv_co_block_status) {
+ if (!bs->drv->bdrv_co_block_status) {
*pnum = bytes;
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
if (offset + bytes == total_size) {
@@ -1981,53 +1981,23 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
/* Round out to request_alignment boundaries */
align = bs->bl.request_alignment;
- if (bs->drv->bdrv_co_get_block_status && align < BDRV_SECTOR_SIZE) {
- align = BDRV_SECTOR_SIZE;
- }
aligned_offset = QEMU_ALIGN_DOWN(offset, align);
aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
- if (bs->drv->bdrv_co_get_block_status) {
- 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;
- } else {
- ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
- aligned_bytes, pnum, &local_map,
- &local_file);
- if (ret < 0) {
- *pnum = 0;
- goto out;
- }
- assert(*pnum); /* The block driver must make progress */
+ ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
+ aligned_bytes, pnum, &local_map,
+ &local_file);
+ if (ret < 0) {
+ *pnum = 0;
+ goto out;
}
/*
- * The driver's result must be a multiple of request_alignment.
+ * The driver's result must be a non-zero multiple of request_alignment.
* Clamp pnum and adjust map to original request.
*/
- assert(QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset);
+ assert(*pnum && QEMU_IS_ALIGNED(*pnum, align) &&
+ align > offset - aligned_offset);
*pnum -= offset - aligned_offset;
if (*pnum > bytes) {
*pnum = bytes;
--
2.14.3
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks
2018-02-13 20:26 [Qemu-devel] [PATCH v8 00/21] add byte-based block_status driver callbacks Eric Blake
` (20 preceding siblings ...)
2018-02-13 20:27 ` [Qemu-devel] [PATCH v8 21/21] block: Drop unused .bdrv_co_get_block_status() Eric Blake
@ 2018-02-14 17:11 ` Kevin Wolf
21 siblings, 0 replies; 42+ messages in thread
From: Kevin Wolf @ 2018-02-14 17:11 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, famz, qemu-block
Am 13.02.2018 um 21:26 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.
>
> These patches have been around for a while, but it's time to
> finish it now that 2.12 is open for patches.
Thanks, touched up patch 8 and applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread