* [Qemu-devel] [PATCH v4 01/20] block: Add .bdrv_co_block_status() callback
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
@ 2017-10-12 18:58 ` Eric Blake
2017-11-28 8:02 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
` (20 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:58 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. 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 old code now uses a goto in order to minimize churn at that later
removal. Update documentation in this patch so that the later
removal can be a straight delete.
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 (mapping is false), rather
than full details about runs of zeroes and which offsets the
allocation actually maps to (mapping is true).
Note that most drivers give sector-aligned answers, except at
end-of-file, even when request_alignment is smaller than a sector.
However, bdrv_getlength() is sector-aligned (even though it gives a
byte answer), often by exceeding the actual file size. If we were to
give back strict results, at least file-posix.c would report a
transition from DATA to HOLE at the end of a file even in the middle
of a sector, which can throw off callers; so we intentionally lie and
state that any partial sector at the end of a file has the same
status for the entire sector. Maybe at some future day we can
report actual file size instead of rounding up, but not for this
series.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v4: rebase to master
v3: no change
v2: improve alignment handling, ensure all iotests still pass
---
include/block/block.h | 9 ++++-----
include/block/block_int.h | 12 +++++++++---
block/io.c | 30 +++++++++++++++++++++++++-----
3 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index fbc21daf62..c5d6b2c933 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -136,11 +136,10 @@ typedef struct HDGeometry {
* that the block layer recompute the answer from the returned
* BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
*
- * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) 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 4b9b23a08d..4153cd646d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -206,13 +206,19 @@ struct BlockDriver {
* 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.
+ * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. As a hint,
+ * the flag want_zero is true if the caller cares more about
+ * precise mappings (favor _OFFSET_VALID/_ZERO) or false for
+ * overall allocation (favor larger *pnum). The block layer
+ * guarantees input aligned to request_alignment, as well as
+ * non-NULL pnum, map, and file.
*/
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 *bd,
+ 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 e4caa4acf1..ef9ea44667 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1843,7 +1843,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
bytes = n;
}
- 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) {
@@ -1860,13 +1860,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;
@@ -1891,8 +1892,27 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
}
ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
*pnum = count * BDRV_SECTOR_SIZE;
+ goto refine;
}
+ 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;
+ }
+
+ /*
+ * total_size is always sector-aligned, by sometimes exceeding actual
+ * file size. Expand pnum if it lands mid-sector due to end-of-file.
+ */
+ if (QEMU_ALIGN_UP(*pnum + aligned_offset,
+ BDRV_SECTOR_SIZE) == total_size) {
+ *pnum = total_size - aligned_offset;
+ }
+
+refine:
/*
* The driver's result must be a multiple of request_alignment.
* Clamp pnum and adjust map to original request.
--
2.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 01/20] block: Add .bdrv_co_block_status() callback
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
@ 2017-11-28 8:02 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-28 8:02 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi, jsnow
12.10.2017 21:58, Eric Blake wrote:
> 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 old code now uses a goto in order to minimize churn at that later
> removal. Update documentation in this patch so that the later
> removal can be a straight delete.
>
> 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 (mapping is false), rather
> than full details about runs of zeroes and which offsets the
> allocation actually maps to (mapping is true).
>
> Note that most drivers give sector-aligned answers, except at
> end-of-file, even when request_alignment is smaller than a sector.
> However, bdrv_getlength() is sector-aligned (even though it gives a
> byte answer), often by exceeding the actual file size. If we were to
> give back strict results, at least file-posix.c would report a
> transition from DATA to HOLE at the end of a file even in the middle
> of a sector, which can throw off callers; so we intentionally lie and
> state that any partial sector at the end of a file has the same
> status for the entire sector. Maybe at some future day we can
> report actual file size instead of rounding up, but not for this
> series.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to master
> v3: no change
> v2: improve alignment handling, ensure all iotests still pass
> ---
> include/block/block.h | 9 ++++-----
> include/block/block_int.h | 12 +++++++++---
> block/io.c | 30 +++++++++++++++++++++++++-----
> 3 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index fbc21daf62..c5d6b2c933 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -136,11 +136,10 @@ typedef struct HDGeometry {
> * that the block layer recompute the answer from the returned
> * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
> *
> - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) 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 4b9b23a08d..4153cd646d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -206,13 +206,19 @@ struct BlockDriver {
> * 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.
> + * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. As a hint,
> + * the flag want_zero is true if the caller cares more about
> + * precise mappings (favor _OFFSET_VALID/_ZERO) or false for
> + * overall allocation (favor larger *pnum). The block layer
> + * guarantees input aligned to request_alignment, as well as
> + * non-NULL pnum, map, and file.
> */
> 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 *bd,
s/bd/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 e4caa4acf1..ef9ea44667 100644
[...]
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
@ 2017-10-12 18:58 ` Eric Blake
2017-11-28 8:19 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 03/20] file-posix: Switch " Eric Blake
` (19 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:58 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 4153cd646d..7c8503f693 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1018,23 +1018,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 ef9ea44667..6a2a2e1484 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1754,30 +1754,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 e21669979d..82c30357f8 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 5036eec434..ec0511bac7 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 307b6391a8..79739d063e 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 5bca76300f..76cd963a8c 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -224,7 +224,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,
.is_filter = true,
};
--
2.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status()
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
@ 2017-11-28 8:19 ` Vladimir Sementsov-Ogievskiy
2017-11-28 16:05 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-28 8:19 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Jeff Cody, Max Reitz, Stefan Hajnoczi,
jsnow
12.10.2017 21:58, Eric Blake wrote:
> 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>
>
> ---
> 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 4153cd646d..7c8503f693 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1018,23 +1018,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 ef9ea44667..6a2a2e1484 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1754,30 +1754,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;
> }
hmm. not related question, but can you please explain, why we use such stubs
instead of really call bdrv_co_block_status on bs->file->bs or
bs->backing->bs ?
>
> /*
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index e21669979d..82c30357f8 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));
looks like this is obviously guaranteed by block layer, so no needs for
special
function. and we can use
.bdrv_co_block_status = bdrv_co_block_status_from_backing
like for other drivers.
> + 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 5036eec434..ec0511bac7 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 307b6391a8..79739d063e 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 5bca76300f..76cd963a8c 100644
> --- a/block/throttle.c
> +++ b/block/throttle.c
> @@ -224,7 +224,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,
>
> .is_filter = true,
> };
conversion looks ok,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status()
2017-11-28 8:19 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-28 16:05 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-28 16:05 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Jeff Cody, Max Reitz, Stefan Hajnoczi,
jsnow
On 11/28/2017 02:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:58, Eric Blake wrote:
>> 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>
>>
>> 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;
>> }
>
> hmm. not related question, but can you please explain, why we use such
> stubs
> instead of really call bdrv_co_block_status on bs->file->bs or
> bs->backing->bs ?
>
Returning BDRV_BLOCK_RAW is what tells io.c to call
bdrv_co_block_status() on whatever got assigned into *file. The two
stubs make it so there are fewer conditionals in io.c; perhaps it could
be reworked to avoid the stubs and have io.c have all the smarts when
.bdrv_co_block_status is NULL, but as you say, that would be a separate
patch (although it was Manos' work earlier this year that even created
the common helper stubs rather than duplicating code in each callback in
the first place).
>> +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
>> + assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
>
> looks like this is obviously guaranteed by block layer, so no needs for
> special
> function. and we can use
>
> .bdrv_co_block_status = bdrv_co_block_status_from_backing
>
> like for other drivers.
>
We could, but we don't want to. The point of the blkdebug driver is to
explicitly test that certain preconditions are being satisfied, so that
even if the block layer in io.c changes, our iotests make sure that
drivers are provided with certain guarantees. In other words, the
assert() added here is added value that we cannot stick in the common
helper, but something that we want to keep associated with the blkdebug
driver.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 03/20] file-posix: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
@ 2017-10-12 18:58 ` Eric Blake
2017-11-30 8:06 ` Vladimir Sementsov-Ogievskiy
2017-11-30 20:40 ` Eric Blake
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 04/20] gluster: " Eric Blake
` (18 subsequent siblings)
21 siblings, 2 replies; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:58 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v4: tweak commit message [Fam], rebase to interface tweak
v3: no change
v2: tweak comment, add mapping support
---
block/file-posix.c | 59 +++++++++++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 25 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e940..536cd1db03 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2128,24 +2128,26 @@ 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
+ * If 'offset' is beyond the end of the disk image the return value is 0
* and 'pnum' is set to 0.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
+ * '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
+ * 'bytes' is the max value 'pnum' should be set to. If bytes goes
* beyond the end of the disk image it will be clamped.
*/
-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;
+ off_t data = 0, hole = 0;
int64_t total_size;
int ret;
@@ -2154,39 +2156,46 @@ 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) {
+ } else if (offset >= 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);
+ } else if (offset + bytes > total_size) {
+ bytes = total_size - offset;
}
- ret = find_allocation(bs, start, &data, &hole);
+ if (!want_zero) {
+ *pnum = bytes;
+ *map = offset;
+ *file = bs;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ }
+
+ 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,
@@ -2280,7 +2289,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 03/20] file-posix: Switch to .bdrv_co_block_status()
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 03/20] file-posix: Switch " Eric Blake
@ 2017-11-30 8:06 ` Vladimir Sementsov-Ogievskiy
2017-11-30 20:40 ` Eric Blake
1 sibling, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 8:06 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:58, Eric Blake wrote:
> 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH v4 03/20] file-posix: Switch to .bdrv_co_block_status()
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 03/20] file-posix: Switch " Eric Blake
2017-11-30 8:06 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 20:40 ` Eric Blake
1 sibling, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 20:40 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
On 10/12/2017 01:58 PM, Eric Blake wrote:
> 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> @@ -2154,39 +2156,46 @@ 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) {
> + } else if (offset >= 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);
> + } else if (offset + bytes > total_size) {
> + bytes = total_size - offset;
> }
I just realized: this is dead code. The block layer already checked
bdrv_getlength() and clamped values. So, v5 will trim this and similar
redundant driver code.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 04/20] gluster: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (2 preceding siblings ...)
2017-10-12 18:58 ` [Qemu-devel] [PATCH v4 03/20] file-posix: Switch " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 8:44 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 05/20] iscsi: Switch cluster_sectors to byte-based Eric Blake
` (17 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v4: tweak commit message [Fam], rebase to interface tweak
v3: no change
v2: tweak comments [Prasanna], add mapping, drop R-b
---
block/gluster.c | 67 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 0f4265a3a4..518f1984de 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1349,26 +1349,30 @@ 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
+ * If 'offset' is beyond the end of the disk image the return value is 0
* and 'pnum' is set to 0.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
+ * '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
+ * 'bytes' is the max value 'pnum' should be set to. If bytes goes
* beyond the end of the disk image it will be clamped.
*
- * (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;
+ off_t data = 0, hole = 0;
int64_t total_size;
int ret = -EINVAL;
@@ -1376,41 +1380,48 @@ static int64_t coroutine_fn qemu_gluster_co_get_block_status(
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) {
+ } else if (offset >= 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);
+ } else if (offset + bytes > total_size) {
+ bytes = total_size - offset;
}
- ret = find_allocation(bs, start, &data, &hole);
+ if (!want_zero) {
+ *pnum = bytes;
+ *map = offset;
+ *file = bs;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ }
+
+ 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;
}
@@ -1438,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,
};
@@ -1466,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,
};
@@ -1494,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,
};
@@ -1528,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 04/20] gluster: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 04/20] gluster: " Eric Blake
@ 2017-11-30 8:44 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 8:44 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Jeff Cody, Max Reitz, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 05/20] iscsi: Switch cluster_sectors to byte-based
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (3 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 04/20] gluster: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-10-18 9:20 ` Paolo Bonzini
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
` (16 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, 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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v2: no change
---
block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 4683f3b244..8f903d8370 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -84,7 +84,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;
@@ -427,9 +427,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) {
@@ -437,7 +438,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;
@@ -458,17 +459,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);
@@ -532,9 +535,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,
@@ -544,9 +550,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
@@ -781,16 +790,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;
}
@@ -1930,8 +1944,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);
}
@@ -2137,7 +2151,7 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
IscsiLun *iscsilun = bs->opaque;
bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws;
- bdi->cluster_size = iscsilun->cluster_sectors * BDRV_SECTOR_SIZE;
+ bdi->cluster_size = iscsilun->cluster_size;
return 0;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 05/20] iscsi: Switch cluster_sectors to byte-based
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 05/20] iscsi: Switch cluster_sectors to byte-based Eric Blake
@ 2017-10-18 9:20 ` Paolo Bonzini
0 siblings, 0 replies; 56+ messages in thread
From: Paolo Bonzini @ 2017-10-18 9:20 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, Ronnie Sahlberg, Peter Lieven,
Max Reitz
On 12/10/2017 20:59, Eric Blake wrote:
> 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v2: no change
> ---
> block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 4683f3b244..8f903d8370 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -84,7 +84,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;
> @@ -427,9 +427,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) {
> @@ -437,7 +438,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;
> @@ -458,17 +459,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);
> @@ -532,9 +535,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,
> @@ -544,9 +550,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
> @@ -781,16 +790,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;
> }
> @@ -1930,8 +1944,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);
> }
> @@ -2137,7 +2151,7 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> IscsiLun *iscsilun = bs->opaque;
> bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
> bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws;
> - bdi->cluster_size = iscsilun->cluster_sectors * BDRV_SECTOR_SIZE;
> + bdi->cluster_size = iscsilun->cluster_size;
> return 0;
> }
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (4 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 05/20] iscsi: Switch cluster_sectors to byte-based Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-10-18 9:20 ` Paolo Bonzini
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 07/20] iscsi: Switch to .bdrv_co_block_status() Eric Blake
` (15 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 8f903d8370..4896d50d6e 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -455,24 +455,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 {
@@ -495,26 +493,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)
@@ -528,34 +526,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
@@ -637,12 +631,14 @@ 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);
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);
@@ -737,9 +733,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) {
@@ -777,15 +775,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
@@ -1154,8 +1156,7 @@ retry:
goto out_unlock;
}
- iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
- bytes >> BDRV_SECTOR_BITS);
+ iscsi_allocmap_set_invalid(iscsilun, offset, bytes);
out_unlock:
qemu_mutex_unlock(&iscsilun->mutex);
@@ -1253,18 +1254,15 @@ 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);
r = iTask.err_code;
goto out_unlock;
}
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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
@ 2017-10-18 9:20 ` Paolo Bonzini
0 siblings, 0 replies; 56+ messages in thread
From: Paolo Bonzini @ 2017-10-18 9:20 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, Ronnie Sahlberg, Peter Lieven,
Max Reitz
On 12/10/2017 20:59, Eric Blake wrote:
> 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>
>
> ---
> 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 8f903d8370..4896d50d6e 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -455,24 +455,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 {
> @@ -495,26 +493,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)
> @@ -528,34 +526,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
> @@ -637,12 +631,14 @@ 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);
> 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);
> @@ -737,9 +733,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) {
> @@ -777,15 +775,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
> @@ -1154,8 +1156,7 @@ retry:
> goto out_unlock;
> }
>
> - iscsi_allocmap_set_invalid(iscsilun, offset >> BDRV_SECTOR_BITS,
> - bytes >> BDRV_SECTOR_BITS);
> + iscsi_allocmap_set_invalid(iscsilun, offset, bytes);
>
> out_unlock:
> qemu_mutex_unlock(&iscsilun->mutex);
> @@ -1253,18 +1254,15 @@ 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);
> r = iTask.err_code;
> goto out_unlock;
> }
>
> 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:
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 07/20] iscsi: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (5 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-10-18 9:20 ` Paolo Bonzini
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 08/20] null: " Eric Blake
` (14 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, 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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v4: rebase to interface tweaks
v3: no change
v2: rebase to mapping parameter
---
block/iscsi.c | 64 +++++++++++++++++++++++++++++------------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 4896d50d6e..cb30e9652c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -647,28 +647,31 @@ 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;
- int64_t ret;
+ int ret;
iscsi_co_init_iscsitask(iscsilun, &iTask);
- if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
+ if (!is_byte_request_lun_aligned(offset, bytes, iscsilun)) {
ret = -EINVAL;
goto out;
}
/* 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) {
@@ -678,7 +681,7 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
qemu_mutex_lock(&iscsilun->mutex);
retry:
if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
- sector_qemu2lun(sector_num, iscsilun),
+ offset / iscsilun->block_size,
8 + 16, iscsi_co_generic_cb,
&iTask) == NULL) {
ret = -ENOMEM;
@@ -717,12 +720,12 @@ retry:
lbasd = &lbas->descriptors[0];
- if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
+ if (offset / iscsilun->block_size != 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) {
@@ -733,15 +736,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);
@@ -749,7 +750,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;
@@ -788,25 +789,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;
}
@@ -2192,7 +2192,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,
@@ -2227,7 +2227,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 07/20] iscsi: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 07/20] iscsi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-10-18 9:20 ` Paolo Bonzini
0 siblings, 0 replies; 56+ messages in thread
From: Paolo Bonzini @ 2017-10-18 9:20 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, Ronnie Sahlberg, Peter Lieven,
Max Reitz
On 12/10/2017 20:59, Eric Blake wrote:
> 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to interface tweaks
> v3: no change
> v2: rebase to mapping parameter
> ---
> block/iscsi.c | 64 +++++++++++++++++++++++++++++------------------------------
> 1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 4896d50d6e..cb30e9652c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -647,28 +647,31 @@ 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;
> - int64_t ret;
> + int ret;
>
> iscsi_co_init_iscsitask(iscsilun, &iTask);
>
> - if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
> + if (!is_byte_request_lun_aligned(offset, bytes, iscsilun)) {
> ret = -EINVAL;
> goto out;
> }
>
> /* 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) {
> @@ -678,7 +681,7 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
> qemu_mutex_lock(&iscsilun->mutex);
> retry:
> if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
> - sector_qemu2lun(sector_num, iscsilun),
> + offset / iscsilun->block_size,
> 8 + 16, iscsi_co_generic_cb,
> &iTask) == NULL) {
> ret = -ENOMEM;
> @@ -717,12 +720,12 @@ retry:
>
> lbasd = &lbas->descriptors[0];
>
> - if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
> + if (offset / iscsilun->block_size != 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) {
> @@ -733,15 +736,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);
> @@ -749,7 +750,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;
> @@ -788,25 +789,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;
> }
> @@ -2192,7 +2192,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,
> @@ -2227,7 +2227,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,
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 08/20] null: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (6 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 07/20] iscsi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 8:47 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 09/20] parallels: " Eric Blake
` (13 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 dd9c13f9ba..20c7eb0ee2 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;
+ int64_t 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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 08/20] null: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 08/20] null: " Eric Blake
@ 2017-11-30 8:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 8:47 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:59, Eric Blake wrote:
> 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>
>
> ---
> 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 dd9c13f9ba..20c7eb0ee2 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;
> + int64_t ret = BDRV_BLOCK_OFFSET_VALID;
int ret =
with that:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> - *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,
> };
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 09/20] parallels: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (7 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 08/20] null: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 9:03 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 10/20] qcow: " Eric Blake
` (12 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 2b6c6e5709..4a086f29e9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -278,23 +278,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);
if (offset < 0) {
return 0;
}
+ *pnum = count * BDRV_SECTOR_SIZE;
+ *map = offset;
*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,
@@ -781,7 +789,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 09/20] parallels: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 09/20] parallels: " Eric Blake
@ 2017-11-30 9:03 ` Vladimir Sementsov-Ogievskiy
2017-11-30 22:12 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 9:03 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi,
Denis V. Lunev, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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>
>
> ---
> 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 2b6c6e5709..4a086f29e9 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -278,23 +278,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);
>
> if (offset < 0) {
pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here.
> return 0;
> }
>
> + *pnum = count * BDRV_SECTOR_SIZE;
> + *map = offset;
> *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,
> @@ -781,7 +789,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,
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 09/20] parallels: Switch to .bdrv_co_block_status()
2017-11-30 9:03 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 22:12 ` Eric Blake
2017-11-30 22:18 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-11-30 22:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi,
Denis V. Lunev, jsnow
On 11/30/2017 03:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> 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>
>>
>> ---
>> +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);
>>
>> if (offset < 0) {
>
> pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here.
>
>> return 0;
>> }
Setting *map is only required when return value includes
BDRV_BLOCK_OFFSET_VALID, so that one was not necessary. But you do
raise an interesting point about a pre-existing bug with pnum not being
set. Commit 298a1665a guarantees that *pnum is 0 (and not uninitialized
garbage) - but that still violates the contract that we (want to) have
that all drivers either make progress or return an error (setting pnum
to 0 should only be done at EOF or on error).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 09/20] parallels: Switch to .bdrv_co_block_status()
2017-11-30 22:12 ` Eric Blake
@ 2017-11-30 22:18 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 22:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi,
Denis V. Lunev, jsnow
On 11/30/2017 04:12 PM, Eric Blake wrote:
>>> 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);
>>>
>>> if (offset < 0) {
>>
>> pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here.
>>
>>> return 0;
>>> }
>
> Setting *map is only required when return value includes
> BDRV_BLOCK_OFFSET_VALID, so that one was not necessary. But you do
> raise an interesting point about a pre-existing bug with pnum not being
> set. Commit 298a1665a guarantees that *pnum is 0 (and not uninitialized
> garbage) - but that still violates the contract that we (want to) have
> that all drivers either make progress or return an error (setting pnum
> to 0 should only be done at EOF or on error).
Oh. The pre-existing code DID set pnum to a non-zero value, as a side
effect of block_status(); the new code fails to do so. So it is not
pre-existing; good catch!
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 10/20] qcow: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (8 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 09/20] parallels: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 9:26 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 11/20] qcow2: " Eric Blake
` (11 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 9569deeaf0..f09661dc0c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -513,23 +513,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;
@@ -537,9 +542,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,
@@ -1111,7 +1116,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 10/20] qcow: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 10/20] qcow: " Eric Blake
@ 2017-11-30 9:26 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 9:26 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:59, Eric Blake wrote:
> 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>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 11/20] qcow2: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (9 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 10/20] qcow: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 9:51 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 12/20] qed: " Eric Blake
` (10 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 92cb9f9bfa..a84e50a6e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1625,32 +1625,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;
@@ -4333,7 +4335,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 11/20] qcow2: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 11/20] qcow2: " Eric Blake
@ 2017-11-30 9:51 ` Vladimir Sementsov-Ogievskiy
2017-11-30 22:38 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 9:51 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:59, Eric Blake wrote:
> 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.
useful thing (for example, to get several compressed clusters in one chunk,
but do not include following zero clusters). but should not the hint be
called want_mapping for it?
or may be we will move to "int hint_flags", which will include status flags
we a interested in?
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> ---
> 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 92cb9f9bfa..a84e50a6e6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1625,32 +1625,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,
'count' is used instead of 'bytes' because of qcow2_get_cluster_offset
which wants int*..
> + 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;
> @@ -4333,7 +4335,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,
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 11/20] qcow2: Switch to .bdrv_co_block_status()
2017-11-30 9:51 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 22:38 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 22:38 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
On 11/30/2017 03:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> 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.
>
> useful thing (for example, to get several compressed clusters in one chunk,
> but do not include following zero clusters). but should not the hint be
> called want_mapping for it?
> or may be we will move to "int hint_flags", which will include status flags
> we a interested in?
Right now, there are only two public interfaces: bdrv_is_allocated
(want_zero is false), and bdrv_block_status (want_zero is true). You
may have a point that there could be further optimizations possible if
the caller has more control over what flags it is interested in, but
that should be a patch series for a later day; I'm already cramming
enough into this series, and it's already stretched out (first patches
went into 2.10), so I want it done.
>
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> -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,
>
> 'count' is used instead of 'bytes' because of qcow2_get_cluster_offset
> which wants int*..
Yes,...
>
>> + int64_t *pnum, int64_t
>> *map,
>> + BlockDriverState **file)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> uint64_t cluster_offset;
>> int index_in_cluster, ret;
>> unsigned int bytes;
...because of that.
>> - 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);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 12/20] qed: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (10 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 11/20] qcow2: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 10:27 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 13/20] raw: " Eric Blake
` (9 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. 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>
---
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 28e2ec89e8..cbab5b6f83 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;
+ 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)
@@ -1595,7 +1567,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 12/20] qed: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 12/20] qed: " Eric Blake
@ 2017-11-30 10:27 ` Vladimir Sementsov-Ogievskiy
2017-11-30 23:17 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 10:27 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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>
>
> ---
> 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 28e2ec89e8..cbab5b6f83 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;
size_t len = bytes;
with that:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + 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);
len is in-out parameter, you can't use it uninitialized.
>
> - /* 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)
> @@ -1595,7 +1567,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,
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 12/20] qed: Switch to .bdrv_co_block_status()
2017-11-30 10:27 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 23:17 ` Eric Blake
2017-12-04 8:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-11-30 23:17 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi, jsnow
On 11/30/2017 04:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> 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>
>>
>> {
>> 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;
>
> size_t len = bytes;
>
Or rather, size_t len = MIN(bytes, SIZE_MAX); thanks to 32-bit platforms.
> with that:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> + 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);
>
> len is in-out parameter, you can't use it uninitialized.
Good catch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 12/20] qed: Switch to .bdrv_co_block_status()
2017-11-30 23:17 ` Eric Blake
@ 2017-12-04 8:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-04 8:59 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi, jsnow
01.12.2017 02:17, Eric Blake wrote:
> On 11/30/2017 04:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.10.2017 21:59, Eric Blake wrote:
>>> 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>
>>>
>
>>> {
>>> 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;
>>
>> size_t len = bytes;
>>
>
> Or rather, size_t len = MIN(bytes, SIZE_MAX); thanks to 32-bit platforms.
ok, this is even better
>
>> with that:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>> + 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);
>>
>> len is in-out parameter, you can't use it uninitialized.
>
> Good catch.
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 13/20] raw: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (11 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 12/20] qed: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 11:10 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 14/20] sheepdog: " Eric Blake
` (8 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 ab552c0954..830243a8e4 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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* [Qemu-devel] [PATCH v4 14/20] sheepdog: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (12 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 13/20] raw: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 11:13 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 15/20] vdi: Avoid bitrot of debugging code Eric Blake
` (7 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 696a71442a..0af8b07892 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2988,19 +2988,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) {
@@ -3017,9 +3017,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;
@@ -3097,7 +3097,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,
@@ -3133,7 +3133,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,
@@ -3169,7 +3169,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 14/20] sheepdog: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 14/20] sheepdog: " Eric Blake
@ 2017-11-30 11:13 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 11:13 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Hitoshi Mitake, Jeff Cody, Max Reitz,
open list:Sheepdog, Liu Yuan, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 15/20] vdi: Avoid bitrot of debugging code
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (13 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 14/20] sheepdog: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 11:17 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 16/20] vdi: Switch to .bdrv_co_block_status() Eric Blake
` (6 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
v2: no change
---
block/vdi.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 8da5dfc897..6f83221ddc 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -86,12 +86,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 15/20] vdi: Avoid bitrot of debugging code
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 15/20] vdi: Avoid bitrot of debugging code Eric Blake
@ 2017-11-30 11:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 11:17 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Weil, Max Reitz, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 16/20] vdi: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (14 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 15/20] vdi: Avoid bitrot of debugging code Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 11:26 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 17/20] vmdk: " Eric Blake
` (5 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 6f83221ddc..b30886823d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -171,8 +171,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). */
@@ -462,7 +460,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;
@@ -508,33 +505,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, 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
@@ -902,7 +895,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 16/20] vdi: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 16/20] vdi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-11-30 11:26 ` Vladimir Sementsov-Ogievskiy
2017-11-30 23:11 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 11:26 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Weil, Max Reitz, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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>
>
> ---
> 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 6f83221ddc..b30886823d 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -171,8 +171,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). */
> @@ -462,7 +460,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;
>
> @@ -508,33 +505,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);
may be not bad to update message, as you changed numbers, like
logout("%p, offset %" PRId64 ", bytes %" PRId64 ", %p\n", bs, offset, bytes, pnum);
but for me it is ok as is too.
> + *pnum = MIN(s->block_size, bytes);
looks like it should be MIN(s->block_size - index_in_block, bytes);
with that:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 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
> @@ -902,7 +895,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,
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 16/20] vdi: Switch to .bdrv_co_block_status()
2017-11-30 11:26 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 23:11 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 23:11 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Weil, Max Reitz, jsnow
On 11/30/2017 05:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> 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>
>>
>> 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);
>
> may be not bad to update message, as you changed numbers, like
>
> logout("%p, offset %" PRId64 ", bytes %" PRId64 ", %p\n", bs, offset,
> bytes, pnum);
>
> but for me it is ok as is too.
Debugging messages can be looked up in context when you enable
debugging. I'm not bothering to change that.
>
>> + *pnum = MIN(s->block_size, bytes);
>
> looks like it should be MIN(s->block_size - index_in_block, bytes);
Oh, good catch. You are right.
>
> with that:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks again for a careful review throughout this series.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 17/20] vmdk: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (15 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 16/20] vdi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 11:39 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 18/20] vpc: " Eric Blake
` (4 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vmdk driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/vmdk.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index c665bcc977..624b5c296a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1303,25 +1303,27 @@ static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
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;
}
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;
@@ -1336,18 +1338,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;
}
@@ -2393,7 +2391,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 17/20] vmdk: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 17/20] vmdk: " Eric Blake
@ 2017-11-30 11:39 ` Vladimir Sementsov-Ogievskiy
2017-11-30 23:18 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 11:39 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:59, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the vmdk driver accordingly.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to interface tweak
> v3: no change
> v2: rebase to mapping flag
> ---
> block/vmdk.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index c665bcc977..624b5c296a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1303,25 +1303,27 @@ static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
> 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;
> }
> 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);
vmdk_find_index_in_cluster becomes unused. with it dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> switch (ret) {
> case VMDK_ERROR:
> ret = -EIO;
> @@ -1336,18 +1338,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;
> }
>
> @@ -2393,7 +2391,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,
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 17/20] vmdk: Switch to .bdrv_co_block_status()
2017-11-30 11:39 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 23:18 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 23:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
On 11/30/2017 05:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the vmdk driver accordingly.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v4: rebase to interface tweak
>> v3: no change
>> v2: rebase to mapping flag
>> ---
>> block/vmdk.c | 28 +++++++++++++---------------
>> 1 file changed, 13 insertions(+), 15 deletions(-)
>> +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;
Pre-existing bug - this returns 0, but did not set pnum.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 18/20] vpc: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (16 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 17/20] vmdk: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 12:22 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 19/20] vvfat: " Eric Blake
` (3 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vpc driver accordingly. Drop the now-unused
get_sector_offset().
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v4: rebase to interface tweak
v3: rebase to master
v2: drop get_sector_offset() [Kevin], rebase to mapping flag
---
block/vpc.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 1576d7b595..4100ce1ed3 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -705,52 +705,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 start, image_offset;
bool allocated;
- int64_t ret;
+ int ret;
int 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);
+ start = image_offset & BDRV_BLOCK_OFFSET_MASK;
+ 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 = start;
+ 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);
+ image_offset = get_image_offset(bs, offset, false, NULL);
} while (offset == -1);
qemu_co_mutex_unlock(&s->lock);
@@ -1097,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 18/20] vpc: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 18/20] vpc: " Eric Blake
@ 2017-11-30 12:22 ` Vladimir Sementsov-Ogievskiy
2017-11-30 23:28 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 12:22 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:59, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the vpc driver accordingly. Drop the now-unused
> get_sector_offset().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to interface tweak
> v3: rebase to master
> v2: drop get_sector_offset() [Kevin], rebase to mapping flag
> ---
> block/vpc.c | 42 ++++++++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 1576d7b595..4100ce1ed3 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -705,52 +705,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 start, image_offset;
> bool allocated;
> - int64_t ret;
> + int ret;
> int 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);
> + start = image_offset & BDRV_BLOCK_OFFSET_MASK;
why do you do so? round down to sector boundary? does it mean,
that *map will corresponds not to offset, but to some previous byte?
> + 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 = start;
> + 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);
> + image_offset = get_image_offset(bs, offset, false, NULL);
> } while (offset == -1);
image_offset here?
>
> qemu_co_mutex_unlock(&s->lock);
> @@ -1097,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,
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 18/20] vpc: Switch to .bdrv_co_block_status()
2017-11-30 12:22 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 23:28 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 23:28 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
On 11/30/2017 06:22 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Update the vpc driver accordingly. Drop the now-unused
>> get_sector_offset().
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>>
>> - 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);
>> + start = image_offset & BDRV_BLOCK_OFFSET_MASK;
>
> why do you do so? round down to sector boundary? does it mean,
> that *map will corresponds not to offset, but to some previous byte?
Ouch, leftovers from before the interface change in v4. Will fix.
>> }
>> - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS,
>> false,
>> - NULL);
>> + image_offset = get_image_offset(bs, offset, false, NULL);
>> } while (offset == -1);
>
> image_offset here?
Yes. (Does it show that for non-typical formats, I only compile-tested
instead of actually running iotests on the format?)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 19/20] vvfat: Switch to .bdrv_co_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (17 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 18/20] vpc: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 12:25 ` Vladimir Sementsov-Ogievskiy
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
` (2 subsequent siblings)
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, 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>
---
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 a0f2335894..9142117fc6 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3082,15 +3082,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;
}
@@ -3251,7 +3249,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.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 19/20] vvfat: Switch to .bdrv_co_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 19/20] vvfat: " Eric Blake
@ 2017-11-30 12:25 ` Vladimir Sementsov-Ogievskiy
2017-11-30 23:31 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 12:25 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
12.10.2017 21:59, Eric Blake wrote:
> 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>
>
> ---
> 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 a0f2335894..9142117fc6 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3082,15 +3082,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,
may be rename to *pnum ?
> + 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;
> }
>
> @@ -3251,7 +3249,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)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 19/20] vvfat: Switch to .bdrv_co_block_status()
2017-11-30 12:25 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 23:31 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 23:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, jsnow, famz, qemu-block, Max Reitz
On 11/30/2017 06:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> 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>
>
>> 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,
>
> may be rename to *pnum ?
I don't see it making a difference; it would match the callback
definition more closely, but the code is short enough that it's not that
confusing with keeping the naming as-is.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH v4 20/20] block: Drop unused .bdrv_co_get_block_status()
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (18 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 19/20] vvfat: " Eric Blake
@ 2017-10-12 18:59 ` Eric Blake
2017-11-30 12:29 ` Vladimir Sementsov-Ogievskiy
2017-11-21 11:27 ` [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Vladimir Sementsov-Ogievskiy
2017-11-30 13:04 ` Vladimir Sementsov-Ogievskiy
21 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2017-10-12 18:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, famz, jsnow, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. 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>
---
v4: rebase to interface tweak
v3: no change
v2: rebase to earlier changes
---
include/block/block_int.h | 3 ---
block/io.c | 34 +---------------------------------
2 files changed, 1 insertion(+), 36 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7c8503f693..6e7ae5fc8d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -213,9 +213,6 @@ struct BlockDriver {
* guarantees input aligned to request_alignment, as well as
* non-NULL pnum, map, and file.
*/
- 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 *bd,
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 6a2a2e1484..7ae4cdadf5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1847,7 +1847,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
bytes = n;
}
- 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) {
@@ -1865,40 +1865,9 @@ 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;
- goto refine;
- }
-
ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
aligned_bytes, pnum, &local_map,
&local_file);
@@ -1916,7 +1885,6 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
*pnum = total_size - aligned_offset;
}
-refine:
/*
* The driver's result must be a multiple of request_alignment.
* Clamp pnum and adjust map to original request.
--
2.13.6
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 20/20] block: Drop unused .bdrv_co_get_block_status()
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
@ 2017-11-30 12:29 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 12:29 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Max Reitz, Stefan Hajnoczi, jsnow
12.10.2017 21:59, Eric Blake wrote:
> 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>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (19 preceding siblings ...)
2017-10-12 18:59 ` [Qemu-devel] [PATCH v4 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
@ 2017-11-21 11:27 ` Vladimir Sementsov-Ogievskiy
2017-11-21 12:28 ` Eric Blake
2017-11-30 13:04 ` Vladimir Sementsov-Ogievskiy
21 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-21 11:27 UTC (permalink / raw)
To: qemu-devel, Eric Blake
Hi!
Is it a latest portion of moving to byte-based? All other parts are
already merged? Are you going to update it? (or this version if ok for
review?)
12.10.2017 21:58, Eric Blake wrote:
> There are patches floating around to add NBD_CMD_BLOCK_STATUS,
> but NBD wants to report status on byte granularity (even if the
> reporting will probably be naturally aligned to sectors or even
> much higher levels). I've therefore started the task of
> converting our block status code to report at a byte granularity
> rather than sectors.
>
> Now that 2.11 is open, I'm rebasing/reposting the remaining patches.
>
> The overall conversion currently looks like:
> part 1: bdrv_is_allocated (merged, commit 51b0a488)
> part 2: dirty-bitmap (merged, commit ca759622)
> part 3: bdrv_get_block_status (v6 is posted [1], partially reviewed)
> part 4: .bdrv_co_block_status (this series, v3 was here [2])
>
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-callback-v4
>
> Based-on: <20171012034720.11947-1-eblake@redhat.com>
> ([PATCH v6 00/24] make bdrv_get_block_status byte-based)
>
> Since v3:
> - rebase to series 3 tweak in get_status interface
> - further simplify qed code
> - better documentation of optimization flag (s/mapping/want_zero/)
>
> 001/20:[0033] [FC] 'block: Add .bdrv_co_block_status() callback'
> 002/20:[0077] [FC] 'block: Switch passthrough drivers to .bdrv_co_block_status()'
> 003/20:[0020] [FC] 'file-posix: Switch to .bdrv_co_block_status()'
> 004/20:[0019] [FC] 'gluster: Switch to .bdrv_co_block_status()'
> 005/20:[----] [--] 'iscsi: Switch cluster_sectors to byte-based'
> 006/20:[----] [--] 'iscsi: Switch iscsi_allocmap_update() to byte-based'
> 007/20:[0022] [FC] 'iscsi: Switch to .bdrv_co_block_status()'
> 008/20:[0013] [FC] 'null: Switch to .bdrv_co_block_status()'
> 009/20:[0017] [FC] 'parallels: Switch to .bdrv_co_block_status()'
> 010/20:[0014] [FC] 'qcow: Switch to .bdrv_co_block_status()'
> 011/20:[0019] [FC] 'qcow2: Switch to .bdrv_co_block_status()'
> 012/20:[0086] [FC] 'qed: Switch to .bdrv_co_block_status()'
> 013/20:[0015] [FC] 'raw: Switch to .bdrv_co_block_status()'
> 014/20:[0011] [FC] 'sheepdog: Switch to .bdrv_co_block_status()'
> 015/20:[----] [--] 'vdi: Avoid bitrot of debugging code'
> 016/20:[0017] [FC] 'vdi: Switch to .bdrv_co_block_status()'
> 017/20:[0013] [FC] 'vmdk: Switch to .bdrv_co_block_status()'
> 018/20:[0019] [FC] 'vpc: Switch to .bdrv_co_block_status()'
> 019/20:[0010] [FC] 'vvfat: Switch to .bdrv_co_block_status()'
> 020/20:[0019] [FC] 'block: Drop unused .bdrv_co_get_block_status()'
>
> Eric Blake (20):
> block: Add .bdrv_co_block_status() callback
> block: Switch passthrough drivers to .bdrv_co_block_status()
> file-posix: Switch to .bdrv_co_block_status()
> gluster: Switch to .bdrv_co_block_status()
> iscsi: Switch cluster_sectors to byte-based
> iscsi: Switch iscsi_allocmap_update() to byte-based
> iscsi: Switch to .bdrv_co_block_status()
> null: Switch to .bdrv_co_block_status()
> parallels: Switch to .bdrv_co_block_status()
> qcow: Switch to .bdrv_co_block_status()
> qcow2: Switch to .bdrv_co_block_status()
> qed: Switch to .bdrv_co_block_status()
> raw: Switch to .bdrv_co_block_status()
> sheepdog: Switch to .bdrv_co_block_status()
> vdi: Avoid bitrot of debugging code
> vdi: Switch to .bdrv_co_block_status()
> vmdk: Switch to .bdrv_co_block_status()
> vpc: Switch to .bdrv_co_block_status()
> vvfat: Switch to .bdrv_co_block_status()
> block: Drop unused .bdrv_co_get_block_status()
>
> include/block/block.h | 9 ++-
> include/block/block_int.h | 43 +++++++------
> block/io.c | 80 +++++++++++-------------
> block/blkdebug.c | 20 +++---
> block/commit.c | 2 +-
> block/file-posix.c | 59 ++++++++++--------
> block/gluster.c | 67 +++++++++++---------
> block/iscsi.c | 154 +++++++++++++++++++++++++---------------------
> block/mirror.c | 2 +-
> block/null.c | 23 +++----
> block/parallels.c | 22 ++++---
> block/qcow.c | 27 ++++----
> block/qcow2.c | 24 ++++----
> block/qed.c | 84 +++++++++----------------
> block/raw-format.c | 16 ++---
> block/sheepdog.c | 26 ++++----
> block/throttle.c | 2 +-
> block/vdi.c | 45 +++++++-------
> block/vmdk.c | 28 ++++-----
> block/vpc.c | 42 +++++++------
> block/vvfat.c | 16 +++--
> 21 files changed, 404 insertions(+), 387 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks
2017-10-12 18:58 [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Eric Blake
` (20 preceding siblings ...)
2017-11-21 11:27 ` [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks Vladimir Sementsov-Ogievskiy
@ 2017-11-30 13:04 ` Vladimir Sementsov-Ogievskiy
2017-11-30 15:36 ` Eric Blake
21 siblings, 1 reply; 56+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-30 13:04 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block
Most of conversions looks fine, but it is not simple to prove the
correctness, because we start to use internal driver logic on offsets
and lengths, not aligned to sectors. And we can't imagine the
consequences (at least, I can't and my r-b doesn't give the guarantee)
of such change. It is like take some function and expand its scope of
parameters.
May be I'm too paranoiac. May be it would be good to align requests
in bdrv_co_block_status to sectors at least for drivers which do not
provide request_alignment. May be each patch should be reviewed by
person, knowing that particular driver.
12.10.2017 21:58, Eric Blake wrote:
> There are patches floating around to add NBD_CMD_BLOCK_STATUS,
> but NBD wants to report status on byte granularity (even if the
> reporting will probably be naturally aligned to sectors or even
> much higher levels). I've therefore started the task of
> converting our block status code to report at a byte granularity
> rather than sectors.
>
> Now that 2.11 is open, I'm rebasing/reposting the remaining patches.
>
> The overall conversion currently looks like:
> part 1: bdrv_is_allocated (merged, commit 51b0a488)
> part 2: dirty-bitmap (merged, commit ca759622)
> part 3: bdrv_get_block_status (v6 is posted [1], partially reviewed)
> part 4: .bdrv_co_block_status (this series, v3 was here [2])
>
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-callback-v4
>
> Based-on: <20171012034720.11947-1-eblake@redhat.com>
> ([PATCH v6 00/24] make bdrv_get_block_status byte-based)
>
> Since v3:
> - rebase to series 3 tweak in get_status interface
> - further simplify qed code
> - better documentation of optimization flag (s/mapping/want_zero/)
>
> 001/20:[0033] [FC] 'block: Add .bdrv_co_block_status() callback'
> 002/20:[0077] [FC] 'block: Switch passthrough drivers to .bdrv_co_block_status()'
> 003/20:[0020] [FC] 'file-posix: Switch to .bdrv_co_block_status()'
> 004/20:[0019] [FC] 'gluster: Switch to .bdrv_co_block_status()'
> 005/20:[----] [--] 'iscsi: Switch cluster_sectors to byte-based'
> 006/20:[----] [--] 'iscsi: Switch iscsi_allocmap_update() to byte-based'
> 007/20:[0022] [FC] 'iscsi: Switch to .bdrv_co_block_status()'
> 008/20:[0013] [FC] 'null: Switch to .bdrv_co_block_status()'
> 009/20:[0017] [FC] 'parallels: Switch to .bdrv_co_block_status()'
> 010/20:[0014] [FC] 'qcow: Switch to .bdrv_co_block_status()'
> 011/20:[0019] [FC] 'qcow2: Switch to .bdrv_co_block_status()'
> 012/20:[0086] [FC] 'qed: Switch to .bdrv_co_block_status()'
> 013/20:[0015] [FC] 'raw: Switch to .bdrv_co_block_status()'
> 014/20:[0011] [FC] 'sheepdog: Switch to .bdrv_co_block_status()'
> 015/20:[----] [--] 'vdi: Avoid bitrot of debugging code'
> 016/20:[0017] [FC] 'vdi: Switch to .bdrv_co_block_status()'
> 017/20:[0013] [FC] 'vmdk: Switch to .bdrv_co_block_status()'
> 018/20:[0019] [FC] 'vpc: Switch to .bdrv_co_block_status()'
> 019/20:[0010] [FC] 'vvfat: Switch to .bdrv_co_block_status()'
> 020/20:[0019] [FC] 'block: Drop unused .bdrv_co_get_block_status()'
>
> Eric Blake (20):
> block: Add .bdrv_co_block_status() callback
> block: Switch passthrough drivers to .bdrv_co_block_status()
> file-posix: Switch to .bdrv_co_block_status()
> gluster: Switch to .bdrv_co_block_status()
> iscsi: Switch cluster_sectors to byte-based
> iscsi: Switch iscsi_allocmap_update() to byte-based
> iscsi: Switch to .bdrv_co_block_status()
> null: Switch to .bdrv_co_block_status()
> parallels: Switch to .bdrv_co_block_status()
> qcow: Switch to .bdrv_co_block_status()
> qcow2: Switch to .bdrv_co_block_status()
> qed: Switch to .bdrv_co_block_status()
> raw: Switch to .bdrv_co_block_status()
> sheepdog: Switch to .bdrv_co_block_status()
> vdi: Avoid bitrot of debugging code
> vdi: Switch to .bdrv_co_block_status()
> vmdk: Switch to .bdrv_co_block_status()
> vpc: Switch to .bdrv_co_block_status()
> vvfat: Switch to .bdrv_co_block_status()
> block: Drop unused .bdrv_co_get_block_status()
>
> include/block/block.h | 9 ++-
> include/block/block_int.h | 43 +++++++------
> block/io.c | 80 +++++++++++-------------
> block/blkdebug.c | 20 +++---
> block/commit.c | 2 +-
> block/file-posix.c | 59 ++++++++++--------
> block/gluster.c | 67 +++++++++++---------
> block/iscsi.c | 154 +++++++++++++++++++++++++---------------------
> block/mirror.c | 2 +-
> block/null.c | 23 +++----
> block/parallels.c | 22 ++++---
> block/qcow.c | 27 ++++----
> block/qcow2.c | 24 ++++----
> block/qed.c | 84 +++++++++----------------
> block/raw-format.c | 16 ++---
> block/sheepdog.c | 26 ++++----
> block/throttle.c | 2 +-
> block/vdi.c | 45 +++++++-------
> block/vmdk.c | 28 ++++-----
> block/vpc.c | 42 +++++++------
> block/vvfat.c | 16 +++--
> 21 files changed, 404 insertions(+), 387 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/20] add byte-based block_status driver callbacks
2017-11-30 13:04 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-30 15:36 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2017-11-30 15:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel; +Cc: kwolf, jsnow, famz, qemu-block
On 11/30/2017 07:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> Most of conversions looks fine, but it is not simple to prove the
> correctness, because we start to use internal driver logic on offsets
> and lengths, not aligned to sectors.
The block layer guarantees that it will not pass unaligned data to the
drivers, given the driver's definition of request_alignment. For
drivers that have a request_alignment of 1, you are correct that the
driver now sees requests at a smaller alignment than before, so it needs
to be carefully reviewed per-driver. But for drivers that are still
sector-based (where request_alignment is 512 for other reasons), there
is no change in behavior.
> And we can't imagine the
> consequences (at least, I can't and my r-b doesn't give the guarantee)
> of such change. It is like take some function and expand its scope of
> parameters.
>
> May be I'm too paranoiac. May be it would be good to align requests
> in bdrv_co_block_status to sectors at least for drivers which do not
> provide request_alignment. May be each patch should be reviewed by
> person, knowing that particular driver.
ALL drivers have request_alignment set to a non-zero value; it defaults
to 1 if the driver provides modern interfaces and does not override the
value, and defaults to 512 if the driver still uses older sector-based
interfaces. But I do welcome additional review; I have to post a v5 anyway.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 56+ messages in thread