* [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 10:15 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:35 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 02/20] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
` (18 subsequent siblings)
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Now that the block layer exposes byte-based allocation,
it's time to tackle the drivers. Add a new callback that operates
on as small as byte boundaries. Subsequent patches will then update
individual drivers, then finally remove .bdrv_co_get_block_status().
The new code also passes through the 'want_zero' hint, which will
allow subsequent patches to further optimize callers that only care
about how much of the image is allocated (want_zero is false),
rather than full details about runs of zeroes and which offsets the
allocation actually maps to (want_zero is true). As part of this
effort, fix another part of the documentation: the claim in commit
4c41cb4 that BDRV_BLOCK_ALLOCATED is short for 'DATA || ZERO' is a
lie at the block layer (see commit e88ae2264), even though it is
how the bit is computed from the driver layer. After all, there
are intentionally cases where we return ZERO but not ALLOCATED at
the block layer, when we know that a read sees zero because the
backing file is too short. Note that the driver interface is thus
slightly different than the public interface with regards to which
bits will be set, and what guarantees are provided on input.
We also add an assertion that any driver using the new callback will
make progress (the only time pnum will be 0 is if the block layer
already handled an out-of-bounds request, or if there is an error);
the old driver interface did not provide this guarantee, which
could lead to some inf-loops in drastic corner-case failures.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: drop now-useless rounding of mid-sector end-of-file hole [Kevin],
better documentation of 'want_zero' [Kevin]
v5: rebase to master, typo fix, document more block layer guarantees
v4: rebase to master
v3: no change
v2: improve alignment handling, ensure all iotests still pass
---
include/block/block.h | 14 +++++++-------
include/block/block_int.h | 20 +++++++++++++++-----
block/io.c | 28 +++++++++++++++++++---------
3 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index c05cac57e5..afc9ece4d6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -128,19 +128,19 @@ typedef struct HDGeometry {
* BDRV_BLOCK_ZERO: offset reads as zero
* BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
* BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
- * layer (short for DATA || ZERO), set by block layer
- * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this layer
+ * layer rather than any backing, set by block layer
+ * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this
+ * layer, set by block layer
*
* Internal flag:
* BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
* that the block layer recompute the answer from the returned
* BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
*
- * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) of
- * the return value (old interface) or the entire map parameter (new
- * interface) represent the offset in the returned BDS that is allocated for
- * the corresponding raw data. However, whether that offset actually
- * contains data also depends on BDRV_BLOCK_DATA, as follows:
+ * If BDRV_BLOCK_OFFSET_VALID is set, the map parameter represents the
+ * host offset within the returned BDS that is allocated for the
+ * corresponding raw guest data. However, whether that offset
+ * actually contains data also depends on BDRV_BLOCK_DATA, as follows:
*
* DATA ZERO OFFSET_VALID
* t t t sectors read as zero, returned file is zero at offset
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a5482775ec..614197f208 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -204,15 +204,25 @@ struct BlockDriver {
/*
* Building block for bdrv_block_status[_above] and
* bdrv_is_allocated[_above]. The driver should answer only
- * according to the current layer, and should not set
- * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
- * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. The block
- * layer guarantees input aligned to request_alignment, as well as
- * non-NULL pnum and file.
+ * according to the current layer, and should only need to set
+ * BDRV_BLOCK_DATA, BDRV_BLOCK_ZERO, BDRV_BLOCK_OFFSET_VALID,
+ * and/or BDRV_BLOCK_RAW; if the current layer defers to a backing
+ * layer, the result should be 0 (and not BDRV_BLOCK_ZERO). See
+ * block.h for the overall meaning of the bits. As a hint, the
+ * flag want_zero is true if the caller cares more about precise
+ * mappings (favor accurate _OFFSET_VALID/_ZERO) or false for
+ * overall allocation (favor larger *pnum, perhaps by reporting
+ * _DATA instead of _ZERO). The block layer guarantees input
+ * clamped to bdrv_getlength() and aligned to request_alignment,
+ * as well as non-NULL pnum, map, and file; in turn, the driver
+ * must return an error or set pnum to an aligned non-zero value.
*/
int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *pnum,
BlockDriverState **file);
+ int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,
+ bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
+ int64_t *map, BlockDriverState **file);
/*
* Invalidate any cached meta-data.
diff --git a/block/io.c b/block/io.c
index 6773926fc1..cbb6e6d073 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1829,10 +1829,10 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
* Drivers not implementing the functionality are assumed to not support
* backing files, hence all their sectors are reported as allocated.
*
- * If 'want_zero' is true, the caller is querying for mapping purposes,
- * and the result should include BDRV_BLOCK_OFFSET_VALID and
- * BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those
- * bits particularly if it allows for a larger value in 'pnum'.
+ * If 'want_zero' is true, the caller is querying for mapping
+ * purposes, with a focus on valid BDRV_BLOCK_OFFSET_VALID, _DATA, and
+ * _ZERO where possible; otherwise, the result favors larger 'pnum',
+ * with a focus on accurate BDRV_BLOCK_ALLOCATED.
*
* If 'offset' is beyond the end of the disk image the return value is
* BDRV_BLOCK_EOF and 'pnum' is set to 0.
@@ -1889,7 +1889,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
/* Must be non-NULL or bdrv_getlength() would have failed */
assert(bs->drv);
- if (!bs->drv->bdrv_co_get_block_status) {
+ if (!bs->drv->bdrv_co_get_block_status && !bs->drv->bdrv_co_block_status) {
*pnum = bytes;
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
if (offset + bytes == total_size) {
@@ -1906,13 +1906,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;
@@ -1937,6 +1938,15 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
}
ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
*pnum = count * BDRV_SECTOR_SIZE;
+ } else {
+ ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
+ aligned_bytes, pnum, &local_map,
+ &local_file);
+ if (ret < 0) {
+ *pnum = 0;
+ goto out;
+ }
+ assert(*pnum); /* The block driver must make progress */
}
/*
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
@ 2017-12-09 10:15 ` Vladimir Sementsov-Ogievskiy
2017-12-09 13:26 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:35 ` Fam Zheng
1 sibling, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 10:15 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Max Reitz
07.12.2017 23:30, 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 new code also passes through the 'want_zero' hint, which will
> allow subsequent patches to further optimize callers that only care
> about how much of the image is allocated (want_zero is false),
> rather than full details about runs of zeroes and which offsets the
> allocation actually maps to (want_zero is true). As part of this
> effort, fix another part of the documentation: the claim in commit
> 4c41cb4 that BDRV_BLOCK_ALLOCATED is short for 'DATA || ZERO' is a
> lie at the block layer (see commit e88ae2264), even though it is
> how the bit is computed from the driver layer. After all, there
> are intentionally cases where we return ZERO but not ALLOCATED at
> the block layer, when we know that a read sees zero because the
> backing file is too short. Note that the driver interface is thus
> slightly different than the public interface with regards to which
> bits will be set, and what guarantees are provided on input.
>
> We also add an assertion that any driver using the new callback will
> make progress (the only time pnum will be 0 is if the block layer
> already handled an out-of-bounds request, or if there is an error);
> the old driver interface did not provide this guarantee, which
> could lead to some inf-loops in drastic corner-case failures.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: drop now-useless rounding of mid-sector end-of-file hole [Kevin],
> better documentation of 'want_zero' [Kevin]
> v5: rebase to master, typo fix, document more block layer guarantees
> v4: rebase to master
> v3: no change
> v2: improve alignment handling, ensure all iotests still pass
> ---
> include/block/block.h | 14 +++++++-------
> include/block/block_int.h | 20 +++++++++++++++-----
> block/io.c | 28 +++++++++++++++++++---------
> 3 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index c05cac57e5..afc9ece4d6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -128,19 +128,19 @@ typedef struct HDGeometry {
> * BDRV_BLOCK_ZERO: offset reads as zero
> * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
> * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
> - * layer (short for DATA || ZERO), set by block layer
> - * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this layer
> + * layer rather than any backing, set by block layer
> + * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this
> + * layer, set by block layer
> *
> * Internal flag:
> * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
> * that the block layer recompute the answer from the returned
> * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
> *
> - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) of
> - * the return value (old interface) or the entire map parameter (new
> - * interface) represent the offset in the returned BDS that is allocated for
> - * the corresponding raw data. However, whether that offset actually
> - * contains data also depends on BDRV_BLOCK_DATA, as follows:
> + * If BDRV_BLOCK_OFFSET_VALID is set, the map parameter represents the
> + * host offset within the returned BDS that is allocated for the
> + * corresponding raw guest data. However, whether that offset
> + * actually contains data also depends on BDRV_BLOCK_DATA, as follows:
> *
> * DATA ZERO OFFSET_VALID
> * t t t sectors read as zero, returned file is zero at offset
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a5482775ec..614197f208 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -204,15 +204,25 @@ struct BlockDriver {
> /*
> * Building block for bdrv_block_status[_above] and
> * bdrv_is_allocated[_above]. The driver should answer only
> - * according to the current layer, and should not set
> - * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
> - * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. The block
> - * layer guarantees input aligned to request_alignment, as well as
> - * non-NULL pnum and file.
> + * according to the current layer, and should only need to set
> + * BDRV_BLOCK_DATA, BDRV_BLOCK_ZERO, BDRV_BLOCK_OFFSET_VALID,
> + * and/or BDRV_BLOCK_RAW; if the current layer defers to a backing
> + * layer, the result should be 0 (and not BDRV_BLOCK_ZERO). See
> + * block.h for the overall meaning of the bits. As a hint, the
> + * flag want_zero is true if the caller cares more about precise
> + * mappings (favor accurate _OFFSET_VALID/_ZERO) or false for
> + * overall allocation (favor larger *pnum, perhaps by reporting
> + * _DATA instead of _ZERO). The block layer guarantees input
> + * clamped to bdrv_getlength() and aligned to request_alignment,
> + * as well as non-NULL pnum, map, and file; in turn, the driver
> + * must return an error or set pnum to an aligned non-zero value.
> */
> int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
> int64_t sector_num, int nb_sectors, int *pnum,
> BlockDriverState **file);
> + int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,
> + bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
> + int64_t *map, BlockDriverState **file);
>
> /*
> * Invalidate any cached meta-data.
> diff --git a/block/io.c b/block/io.c
> index 6773926fc1..cbb6e6d073 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1829,10 +1829,10 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
> * Drivers not implementing the functionality are assumed to not support
> * backing files, hence all their sectors are reported as allocated.
> *
> - * If 'want_zero' is true, the caller is querying for mapping purposes,
> - * and the result should include BDRV_BLOCK_OFFSET_VALID and
> - * BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those
> - * bits particularly if it allows for a larger value in 'pnum'.
> + * If 'want_zero' is true, the caller is querying for mapping
> + * purposes, with a focus on valid BDRV_BLOCK_OFFSET_VALID, _DATA, and
> + * _ZERO where possible; otherwise, the result favors larger 'pnum',
> + * with a focus on accurate BDRV_BLOCK_ALLOCATED.
> *
> * If 'offset' is beyond the end of the disk image the return value is
> * BDRV_BLOCK_EOF and 'pnum' is set to 0.
> @@ -1889,7 +1889,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>
> /* Must be non-NULL or bdrv_getlength() would have failed */
> assert(bs->drv);
> - if (!bs->drv->bdrv_co_get_block_status) {
> + if (!bs->drv->bdrv_co_get_block_status && !bs->drv->bdrv_co_block_status) {
> *pnum = bytes;
> ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
> if (offset + bytes == total_size) {
> @@ -1906,13 +1906,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;
>
> @@ -1937,6 +1938,15 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
> }
> ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
> *pnum = count * BDRV_SECTOR_SIZE;
> + } else {
> + ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
> + aligned_bytes, pnum, &local_map,
> + &local_file);
> + if (ret < 0) {
> + *pnum = 0;
> + goto out;
> + }
> + assert(*pnum); /* The block driver must make progress */
don't you want to assert alignment here too, to address your "or set
pnum to an aligned non-zero value"
anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> }
>
> /*
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback
2017-12-09 10:15 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-09 13:26 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 13:26 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Max Reitz
09.12.2017 13:15, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2017 23:30, 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 new code also passes through the 'want_zero' hint, which will
>> allow subsequent patches to further optimize callers that only care
>> about how much of the image is allocated (want_zero is false),
>> rather than full details about runs of zeroes and which offsets the
>> allocation actually maps to (want_zero is true). As part of this
>> effort, fix another part of the documentation: the claim in commit
>> 4c41cb4 that BDRV_BLOCK_ALLOCATED is short for 'DATA || ZERO' is a
>> lie at the block layer (see commit e88ae2264), even though it is
>> how the bit is computed from the driver layer. After all, there
>> are intentionally cases where we return ZERO but not ALLOCATED at
>> the block layer, when we know that a read sees zero because the
>> backing file is too short. Note that the driver interface is thus
>> slightly different than the public interface with regards to which
>> bits will be set, and what guarantees are provided on input.
>>
>> We also add an assertion that any driver using the new callback will
>> make progress (the only time pnum will be 0 is if the block layer
>> already handled an out-of-bounds request, or if there is an error);
>> the old driver interface did not provide this guarantee, which
>> could lead to some inf-loops in drastic corner-case failures.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v6: drop now-useless rounding of mid-sector end-of-file hole [Kevin],
>> better documentation of 'want_zero' [Kevin]
>> v5: rebase to master, typo fix, document more block layer guarantees
>> v4: rebase to master
>> v3: no change
>> v2: improve alignment handling, ensure all iotests still pass
>> ---
>> include/block/block.h | 14 +++++++-------
>> include/block/block_int.h | 20 +++++++++++++++-----
>> block/io.c | 28 +++++++++++++++++++---------
>> 3 files changed, 41 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index c05cac57e5..afc9ece4d6 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -128,19 +128,19 @@ typedef struct HDGeometry {
>> * BDRV_BLOCK_ZERO: offset reads as zero
>> * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for
>> accessing raw data
>> * BDRV_BLOCK_ALLOCATED: the content of the block is determined by
>> this
>> - * layer (short for DATA || ZERO), set by
>> block layer
>> - * BDRV_BLOCK_EOF: the returned pnum covers through end of file for
>> this layer
>> + * layer rather than any backing, set by block
>> layer
>> + * BDRV_BLOCK_EOF: the returned pnum covers through end of file for
>> this
>> + * layer, set by block layer
>> *
>> * Internal flag:
>> * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to
>> request
>> * that the block layer recompute the answer from
>> the returned
>> * BDS; must be accompanied by just
>> BDRV_BLOCK_OFFSET_VALID.
>> *
>> - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62
>> (BDRV_BLOCK_OFFSET_MASK) of
>> - * the return value (old interface) or the entire map parameter (new
>> - * interface) represent the offset in the returned BDS that is
>> allocated for
>> - * the corresponding raw data. However, whether that offset actually
>> - * contains data also depends on BDRV_BLOCK_DATA, as follows:
>> + * If BDRV_BLOCK_OFFSET_VALID is set, the map parameter represents the
>> + * host offset within the returned BDS that is allocated for the
>> + * corresponding raw guest data. However, whether that offset
>> + * actually contains data also depends on BDRV_BLOCK_DATA, as follows:
>> *
>> * DATA ZERO OFFSET_VALID
>> * t t t sectors read as zero, returned file is
>> zero at offset
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index a5482775ec..614197f208 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -204,15 +204,25 @@ struct BlockDriver {
>> /*
>> * Building block for bdrv_block_status[_above] and
>> * bdrv_is_allocated[_above]. The driver should answer only
>> - * according to the current layer, and should not set
>> - * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
>> - * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. The block
>> - * layer guarantees input aligned to request_alignment, as well as
>> - * non-NULL pnum and file.
>> + * according to the current layer, and should only need to set
>> + * BDRV_BLOCK_DATA, BDRV_BLOCK_ZERO, BDRV_BLOCK_OFFSET_VALID,
>> + * and/or BDRV_BLOCK_RAW; if the current layer defers to a backing
>> + * layer, the result should be 0 (and not BDRV_BLOCK_ZERO). See
>> + * block.h for the overall meaning of the bits. As a hint, the
>> + * flag want_zero is true if the caller cares more about precise
>> + * mappings (favor accurate _OFFSET_VALID/_ZERO) or false for
>> + * overall allocation (favor larger *pnum, perhaps by reporting
>> + * _DATA instead of _ZERO). The block layer guarantees input
>> + * clamped to bdrv_getlength() and aligned to request_alignment,
>> + * as well as non-NULL pnum, map, and file; in turn, the driver
>> + * must return an error or set pnum to an aligned non-zero value.
>> */
>> int64_t coroutine_fn
>> (*bdrv_co_get_block_status)(BlockDriverState *bs,
>> int64_t sector_num, int nb_sectors, int *pnum,
>> BlockDriverState **file);
>> + int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,
>> + bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
>> + int64_t *map, BlockDriverState **file);
>>
>> /*
>> * Invalidate any cached meta-data.
>> diff --git a/block/io.c b/block/io.c
>> index 6773926fc1..cbb6e6d073 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1829,10 +1829,10 @@ int64_t coroutine_fn
>> bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
>> * Drivers not implementing the functionality are assumed to not
>> support
>> * backing files, hence all their sectors are reported as allocated.
>> *
>> - * If 'want_zero' is true, the caller is querying for mapping purposes,
>> - * and the result should include BDRV_BLOCK_OFFSET_VALID and
>> - * BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those
>> - * bits particularly if it allows for a larger value in 'pnum'.
>> + * If 'want_zero' is true, the caller is querying for mapping
>> + * purposes, with a focus on valid BDRV_BLOCK_OFFSET_VALID, _DATA, and
>> + * _ZERO where possible; otherwise, the result favors larger 'pnum',
>> + * with a focus on accurate BDRV_BLOCK_ALLOCATED.
>> *
>> * If 'offset' is beyond the end of the disk image the return value is
>> * BDRV_BLOCK_EOF and 'pnum' is set to 0.
>> @@ -1889,7 +1889,7 @@ static int coroutine_fn
>> bdrv_co_block_status(BlockDriverState *bs,
>>
>> /* Must be non-NULL or bdrv_getlength() would have failed */
>> assert(bs->drv);
>> - if (!bs->drv->bdrv_co_get_block_status) {
>> + if (!bs->drv->bdrv_co_get_block_status &&
>> !bs->drv->bdrv_co_block_status) {
>> *pnum = bytes;
>> ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
>> if (offset + bytes == total_size) {
>> @@ -1906,13 +1906,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;
>>
>> @@ -1937,6 +1938,15 @@ static int coroutine_fn
>> bdrv_co_block_status(BlockDriverState *bs,
>> }
>> ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
>> *pnum = count * BDRV_SECTOR_SIZE;
>> + } else {
>> + ret = bs->drv->bdrv_co_block_status(bs, want_zero,
>> aligned_offset,
>> + aligned_bytes, pnum,
>> &local_map,
>> + &local_file);
>> + if (ret < 0) {
>> + *pnum = 0;
>> + goto out;
>> + }
>> + assert(*pnum); /* The block driver must make progress */
>
> don't you want to assert alignment here too, to address your "or set
> pnum to an aligned non-zero value"
aha, which proves that I didn't look at the surrounding code, which has
corresponding assertion after else block.
>
> anyway,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> }
>>
>> /*
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
2017-12-09 10:15 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 2:35 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 2:35 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Stefan Hajnoczi,
Max Reitz
On Thu, 12/07 14:30, 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 new code also passes through the 'want_zero' hint, which will
> allow subsequent patches to further optimize callers that only care
> about how much of the image is allocated (want_zero is false),
> rather than full details about runs of zeroes and which offsets the
> allocation actually maps to (want_zero is true). As part of this
> effort, fix another part of the documentation: the claim in commit
> 4c41cb4 that BDRV_BLOCK_ALLOCATED is short for 'DATA || ZERO' is a
> lie at the block layer (see commit e88ae2264), even though it is
> how the bit is computed from the driver layer. After all, there
> are intentionally cases where we return ZERO but not ALLOCATED at
> the block layer, when we know that a read sees zero because the
> backing file is too short. Note that the driver interface is thus
> slightly different than the public interface with regards to which
> bits will be set, and what guarantees are provided on input.
>
> We also add an assertion that any driver using the new callback will
> make progress (the only time pnum will be 0 is if the block layer
> already handled an out-of-bounds request, or if there is an error);
> the old driver interface did not provide this guarantee, which
> could lead to some inf-loops in drastic corner-case failures.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: drop now-useless rounding of mid-sector end-of-file hole [Kevin],
> better documentation of 'want_zero' [Kevin]
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 02/20] block: Switch passthrough drivers to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 2:37 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 03/20] file-posix: Switch " Eric Blake
` (17 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz, Jeff Cody,
Stefan Hajnoczi
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the generic helpers, and all passthrough clients
(blkdebug, commit, mirror, throttle) accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: rebase to master
v4: rebase to interface tweak
v3: rebase to addition of throttle driver
v2: rebase to master, retitle while merging blkdebug, commit, and mirror
---
include/block/block_int.h | 28 ++++++++++++++++------------
block/io.c | 36 ++++++++++++++++++++----------------
block/blkdebug.c | 20 +++++++++++---------
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/throttle.c | 2 +-
6 files changed, 50 insertions(+), 40 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 614197f208..42e5c37a63 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1029,23 +1029,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 cbb6e6d073..109b3826cd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1798,30 +1798,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 c5327551ce..ae2a5ddba2 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 c9badc1203..f5bf620942 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 833175ac77..d1324e7a9f 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -239,7 +239,7 @@ static BlockDriver bdrv_throttle = {
.bdrv_reopen_prepare = throttle_reopen_prepare,
.bdrv_reopen_commit = throttle_reopen_commit,
.bdrv_reopen_abort = throttle_reopen_abort,
- .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
+ .bdrv_co_block_status = bdrv_co_block_status_from_file,
.bdrv_co_drain_begin = throttle_co_drain_begin,
.bdrv_co_drain_end = throttle_co_drain_end,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/20] block: Switch passthrough drivers to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 02/20] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
@ 2017-12-29 2:37 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 2:37 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Max Reitz, Jeff Cody,
Stefan Hajnoczi
On Thu, 12/07 14:30, 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>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> ---
> v5: rebase to master
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 03/20] file-posix: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 01/20] block: Add .bdrv_co_block_status() callback Eric Blake
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 02/20] block: Switch passthrough drivers to .bdrv_co_block_status() Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 11:52 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:41 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 04/20] gluster: " Eric Blake
` (16 subsequent siblings)
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the file protocol driver accordingly.
In want_zero mode, we continue to report fine-grained hole
information (the caller wants as much mapping detail as possible);
but when not in that mode, the caller prefers larger *pnum and
merely cares about what offsets are allocated at this layer, rather
than where the holes live. Since holes still read as zeroes at
this layer (rather than deferring to a backing layer), we can take
the shortcut of skipping lseek(), and merely state that all bytes
are allocated.
We can also drop redundant bounds checks that are already
guaranteed by the block layer.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v5: drop redundant code
v4: tweak commit message [Fam], rebase to interface tweak
v3: no change
v2: tweak comment, add mapping support
---
block/file-posix.c | 62 +++++++++++++++++++++++++-----------------------------
1 file changed, 29 insertions(+), 33 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e940..e847c7cdd9 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2128,25 +2128,24 @@ static int find_allocation(BlockDriverState *bs, off_t start,
}
/*
- * Returns the allocation status of the specified sectors.
+ * Returns the allocation status of the specified offset.
*
- * If 'sector_num' is beyond the end of the disk image the return value is 0
- * and 'pnum' is set to 0.
+ * The block layer guarantees 'offset' and 'bytes' are within bounds.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
+ * 'pnum' is set to the number of bytes (including and immediately following
+ * the specified offset) that are known to be in the same
* allocated/unallocated state.
*
- * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes
- * beyond the end of the disk image it will be clamped.
+ * 'bytes' is the max value 'pnum' should be set to.
*/
-static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn raw_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
- off_t start, data = 0, hole = 0;
- int64_t total_size;
+ off_t data = 0, hole = 0;
int ret;
ret = fd_open(bs);
@@ -2154,39 +2153,36 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
return ret;
}
- start = sector_num * BDRV_SECTOR_SIZE;
- total_size = bdrv_getlength(bs);
- if (total_size < 0) {
- return total_size;
- } else if (start >= total_size) {
- *pnum = 0;
- return 0;
- } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
- nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
+ if (!want_zero) {
+ *pnum = bytes;
+ *map = offset;
+ *file = bs;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
- ret = find_allocation(bs, start, &data, &hole);
+ ret = find_allocation(bs, offset, &data, &hole);
if (ret == -ENXIO) {
/* Trailing hole */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_ZERO;
} else if (ret < 0) {
/* No info available, so pretend there are no holes */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_DATA;
- } else if (data == start) {
- /* On a data extent, compute sectors to the end of the extent,
+ } else if (data == offset) {
+ /* On a data extent, compute bytes to the end of the extent,
* possibly including a partial sector at EOF. */
- *pnum = MIN(nb_sectors, DIV_ROUND_UP(hole - start, BDRV_SECTOR_SIZE));
+ *pnum = MIN(bytes, hole - offset);
ret = BDRV_BLOCK_DATA;
} else {
- /* On a hole, compute sectors to the beginning of the next extent. */
- assert(hole == start);
- *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
+ /* On a hole, compute bytes to the beginning of the next extent. */
+ assert(hole == offset);
+ *pnum = MIN(bytes, data - offset);
ret = BDRV_BLOCK_ZERO;
}
+ *map = offset;
*file = bs;
- return ret | BDRV_BLOCK_OFFSET_VALID | start;
+ return ret | BDRV_BLOCK_OFFSET_VALID;
}
static coroutine_fn BlockAIOCB *raw_aio_pdiscard(BlockDriverState *bs,
@@ -2280,7 +2276,7 @@ BlockDriver bdrv_file = {
.bdrv_close = raw_close,
.bdrv_create = raw_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_co_get_block_status = raw_co_get_block_status,
+ .bdrv_co_block_status = raw_co_block_status,
.bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
.bdrv_co_preadv = raw_co_preadv,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 03/20] file-posix: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 03/20] file-posix: Switch " Eric Blake
@ 2017-12-09 11:52 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:41 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 11:52 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
07.12.2017 23:30, 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.
>
> We can also drop redundant bounds checks that are already
> guaranteed by the block layer.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/20] file-posix: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 03/20] file-posix: Switch " Eric Blake
2017-12-09 11:52 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 2:41 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 2:41 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, kwolf, qemu-block, vsementsov, Max Reitz
On Thu, 12/07 14:30, 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.
>
> We can also drop redundant bounds checks that are already
> guaranteed by the block layer.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 04/20] gluster: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (2 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 03/20] file-posix: Switch " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 11:55 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:42 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 05/20] iscsi: Switch cluster_sectors to byte-based Eric Blake
` (15 subsequent siblings)
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Jeff Cody, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the gluster driver accordingly.
In want_zero mode, we continue to report fine-grained hole
information (the caller wants as much mapping detail as possible);
but when not in that mode, the caller prefers larger *pnum and
merely cares about what offsets are allocated at this layer, rather
than where the holes live. Since holes still read as zeroes at
this layer (rather than deferring to a backing layer), we can take
the shortcut of skipping find_allocation(), and merely state that
all bytes are allocated.
We can also drop redundant bounds checks that are already
guaranteed by the block layer.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v5: drop redundant code
v4: tweak commit message [Fam], rebase to interface tweak
v3: no change
v2: tweak comments [Prasanna], add mapping, drop R-b
---
block/gluster.c | 70 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 34 insertions(+), 36 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 0f4265a3a4..6388ced82e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1349,68 +1349,66 @@ exit:
}
/*
- * Returns the allocation status of the specified sectors.
+ * Returns the allocation status of the specified offset.
*
- * If 'sector_num' is beyond the end of the disk image the return value is 0
- * and 'pnum' is set to 0.
+ * The block layer guarantees 'offset' and 'bytes' are within bounds.
*
- * 'pnum' is set to the number of sectors (including and immediately following
- * the specified sector) that are known to be in the same
+ * 'pnum' is set to the number of bytes (including and immediately following
+ * the specified offset) that are known to be in the same
* allocated/unallocated state.
*
- * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes
- * beyond the end of the disk image it will be clamped.
+ * 'bytes' is the max value 'pnum' should be set to.
*
- * (Based on raw_co_get_block_status() from file-posix.c.)
+ * (Based on raw_co_block_status() from file-posix.c.)
*/
-static int64_t coroutine_fn qemu_gluster_co_get_block_status(
- BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BDRVGlusterState *s = bs->opaque;
- off_t start, data = 0, hole = 0;
- int64_t total_size;
+ off_t data = 0, hole = 0;
int ret = -EINVAL;
if (!s->fd) {
return ret;
}
- start = sector_num * BDRV_SECTOR_SIZE;
- total_size = bdrv_getlength(bs);
- if (total_size < 0) {
- return total_size;
- } else if (start >= total_size) {
- *pnum = 0;
- return 0;
- } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
- nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
+ if (!want_zero) {
+ *pnum = bytes;
+ *map = offset;
+ *file = bs;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
- ret = find_allocation(bs, start, &data, &hole);
+ ret = find_allocation(bs, offset, &data, &hole);
if (ret == -ENXIO) {
/* Trailing hole */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_ZERO;
} else if (ret < 0) {
/* No info available, so pretend there are no holes */
- *pnum = nb_sectors;
+ *pnum = bytes;
ret = BDRV_BLOCK_DATA;
- } else if (data == start) {
- /* On a data extent, compute sectors to the end of the extent,
+ } else if (data == offset) {
+ /* On a data extent, compute bytes to the end of the extent,
* possibly including a partial sector at EOF. */
- *pnum = MIN(nb_sectors, DIV_ROUND_UP(hole - start, BDRV_SECTOR_SIZE));
+ *pnum = MIN(bytes, hole - offset);
ret = BDRV_BLOCK_DATA;
} else {
- /* On a hole, compute sectors to the beginning of the next extent. */
- assert(hole == start);
- *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
+ /* On a hole, compute bytes to the beginning of the next extent. */
+ assert(hole == offset);
+ *pnum = MIN(bytes, data - offset);
ret = BDRV_BLOCK_ZERO;
}
+ *map = offset;
*file = bs;
- return ret | BDRV_BLOCK_OFFSET_VALID | start;
+ return ret | BDRV_BLOCK_OFFSET_VALID;
}
@@ -1438,7 +1436,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 +1464,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 +1492,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 +1526,7 @@ static BlockDriver bdrv_gluster_rdma = {
#ifdef CONFIG_GLUSTERFS_ZEROFILL
.bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes,
#endif
- .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
+ .bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
};
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 04/20] gluster: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 04/20] gluster: " Eric Blake
@ 2017-12-09 11:55 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:42 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 11:55 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, famz, qemu-block, Jeff Cody, Max Reitz
07.12.2017 23:30, 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.
>
> We can also drop redundant bounds checks that are already
> guaranteed by the block layer.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [Qemu-devel] [PATCH v6 04/20] gluster: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 04/20] gluster: " Eric Blake
2017-12-09 11:55 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 2:42 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 2:42 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Jeff Cody, Max Reitz
On Thu, 12/07 14:30, 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.
>
> We can also drop redundant bounds checks that are already
> guaranteed by the block layer.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 05/20] iscsi: Switch cluster_sectors to byte-based
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (3 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 04/20] gluster: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 2:49 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
` (14 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Ronnie Sahlberg,
Paolo Bonzini, Peter Lieven, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert all uses of
the cluster size in sectors, along with adding assertions that we
are not dividing by zero.
Improve some comment grammar while in the area.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
v2-v5: 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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 05/20] iscsi: Switch cluster_sectors to byte-based
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 05/20] iscsi: Switch cluster_sectors to byte-based Eric Blake
@ 2017-12-29 2:49 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 2:49 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Ronnie Sahlberg,
Paolo Bonzini, Peter Lieven, Max Reitz
On Thu, 12/07 14:30, 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.
>
> Improve some comment grammar while in the area.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (4 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 05/20] iscsi: Switch cluster_sectors to byte-based Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:25 ` Fam Zheng
2018-01-02 21:43 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 07/20] iscsi: Switch to .bdrv_co_block_status() Eric Blake
` (13 subsequent siblings)
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Ronnie Sahlberg,
Paolo Bonzini, Peter Lieven, Max Reitz
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Convert all uses of
the allocmap (no semantic change). Callers that already had bytes
available are simpler, and callers that now scale to bytes will be
easier to switch to byte-based in the future.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
v3-v5: no change
v2: rebase to count/bytes rename
---
block/iscsi.c | 90 +++++++++++++++++++++++++++++------------------------------
1 file changed, 44 insertions(+), 46 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
@ 2017-12-29 3:25 ` Fam Zheng
2018-01-02 21:43 ` [Qemu-devel] [Qemu-block] " Jeff Cody
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:25 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, vsementsov, qemu-block, Peter Lieven,
Max Reitz, Ronnie Sahlberg, Paolo Bonzini
On Thu, 12/07 14:30, 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>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
2017-12-29 3:25 ` Fam Zheng
@ 2018-01-02 21:43 ` Jeff Cody
2018-01-02 22:36 ` Eric Blake
1 sibling, 1 reply; 57+ messages in thread
From: Jeff Cody @ 2018-01-02 21:43 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, vsementsov, famz, qemu-block, Peter Lieven,
Max Reitz, Ronnie Sahlberg, Paolo Bonzini
On Thu, Dec 07, 2017 at 02:30:22PM -0600, 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>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> ---
> v3-v5: no change
> v2: rebase to count/bytes rename
Looks like this one will need a rebase, I get conflicts on master (and when
trying to naively rebase your repo's master on qemu master).
> ---
> 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.14.3
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() to byte-based
2018-01-02 21:43 ` [Qemu-devel] [Qemu-block] " Jeff Cody
@ 2018-01-02 22:36 ` Eric Blake
0 siblings, 0 replies; 57+ messages in thread
From: Eric Blake @ 2018-01-02 22:36 UTC (permalink / raw)
To: Jeff Cody
Cc: qemu-devel, kwolf, vsementsov, famz, qemu-block, Peter Lieven,
Max Reitz, Ronnie Sahlberg, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]
On 01/02/2018 03:43 PM, Jeff Cody wrote:
> On Thu, Dec 07, 2017 at 02:30:22PM -0600, 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>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> ---
>> v3-v5: no change
>> v2: rebase to count/bytes rename
>
> Looks like this one will need a rebase, I get conflicts on master (and when
> trying to naively rebase your repo's master on qemu master).
Yep, I'm seeing it to when rebasing on top of Kevin's block branch
(currently at 1a63a907 on top of master at 281f3274). I'll post a v7 soon.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 07/20] iscsi: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (5 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 06/20] iscsi: Switch iscsi_allocmap_update() " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:20 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 08/20] null: " Eric Blake
` (12 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Ronnie Sahlberg,
Paolo Bonzini, Peter Lieven, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the iscsi driver accordingly. In this case,
it is handy to teach iscsi_co_block_status() to handle a NULL map
and file parameter, even though the block layer passes non-NULL
values, because we also call the function directly. For now, there
are no optimizations done based on the want_zero flag.
We can also make the simplification of asserting that the block
layer passed in aligned values.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v5: assert rather than check for alignment
v4: rebase to interface tweaks
v3: no change
v2: rebase to mapping parameter
---
block/iscsi.c | 67 ++++++++++++++++++++++++++++-------------------------------
1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 4896d50d6e..4c35403e27 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -647,28 +647,28 @@ 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)) {
- ret = -EINVAL;
- goto out;
- }
+ assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size));
/* default to all sectors allocated */
- ret = BDRV_BLOCK_DATA;
- ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
- *pnum = nb_sectors;
+ ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ if (map) {
+ *map = offset;
+ }
+ *pnum = bytes;
/* LUN does not support logical block provisioning */
if (!iscsilun->lbpme) {
@@ -678,7 +678,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 +717,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 +733,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 +747,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 +786,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 +2189,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 +2224,7 @@ static BlockDriver bdrv_iser = {
.bdrv_truncate = iscsi_truncate,
.bdrv_refresh_limits = iscsi_refresh_limits,
- .bdrv_co_get_block_status = iscsi_co_get_block_status,
+ .bdrv_co_block_status = iscsi_co_block_status,
.bdrv_co_pdiscard = iscsi_co_pdiscard,
.bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
.bdrv_co_readv = iscsi_co_readv,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/20] iscsi: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 07/20] iscsi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-12-29 3:20 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:20 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Ronnie Sahlberg,
Paolo Bonzini, Peter Lieven, Max Reitz
On Thu, 12/07 14:30, 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.
>
> We can also make the simplification of asserting that the block
> layer passed in aligned values.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 08/20] null: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (6 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 07/20] iscsi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 2:56 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 09/20] parallels: " Eric Blake
` (11 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the null driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: minor fix to type of 'ret'
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping parameter
---
block/null.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/block/null.c b/block/null.c
index dd9c13f9ba..b762246860 100644
--- a/block/null.c
+++ b/block/null.c
@@ -223,22 +223,23 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state,
return 0;
}
-static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
- BlockDriverState **file)
+static int coroutine_fn null_co_block_status(BlockDriverState *bs,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BDRVNullState *s = bs->opaque;
- off_t start = sector_num * BDRV_SECTOR_SIZE;
+ int ret = BDRV_BLOCK_OFFSET_VALID;
- *pnum = nb_sectors;
+ *pnum = bytes;
+ *map = offset;
*file = bs;
if (s->read_zeroes) {
- return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
- } else {
- return BDRV_BLOCK_OFFSET_VALID | start;
+ ret |= BDRV_BLOCK_ZERO;
}
+ return ret;
}
static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
@@ -270,7 +271,7 @@ static BlockDriver bdrv_null_co = {
.bdrv_co_flush_to_disk = null_co_flush,
.bdrv_reopen_prepare = null_reopen_prepare,
- .bdrv_co_get_block_status = null_co_get_block_status,
+ .bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
};
@@ -290,7 +291,7 @@ static BlockDriver bdrv_null_aio = {
.bdrv_aio_flush = null_aio_flush,
.bdrv_reopen_prepare = null_reopen_prepare,
- .bdrv_co_get_block_status = null_co_get_block_status,
+ .bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
};
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (7 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 08/20] null: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 12:31 ` Vladimir Sementsov-Ogievskiy
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 10/20] qcow: " Eric Blake
` (10 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, 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>
---
v5: fix pnum when return is 0
v4: rebase to interface tweak, R-b dropped
v3: no change
v2: rebase to mapping parameter; it is ignored, so R-b kept
---
block/parallels.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 9545761f49..9a3d3748ae 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -280,23 +280,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
}
-static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn parallels_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BDRVParallelsState *s = bs->opaque;
- int64_t offset;
+ int count;
+ assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
qemu_co_mutex_lock(&s->lock);
- offset = block_status(s, sector_num, nb_sectors, pnum);
+ offset = block_status(s, offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS, &count);
qemu_co_mutex_unlock(&s->lock);
+ *pnum = count * BDRV_SECTOR_SIZE;
if (offset < 0) {
return 0;
}
+ *map = offset;
*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,
@@ -793,7 +801,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_open = parallels_open,
.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_format_default_perms,
- .bdrv_co_get_block_status = parallels_co_get_block_status,
+ .bdrv_co_block_status = parallels_co_block_status,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_flush_to_os = parallels_co_flush_to_os,
.bdrv_co_readv = parallels_co_readv,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 09/20] parallels: " Eric Blake
@ 2017-12-09 12:31 ` Vladimir Sementsov-Ogievskiy
2017-12-09 16:39 ` Eric Blake
0 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 12:31 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev,
Max Reitz
07.12.2017 23:30, 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>
>
> ---
> v5: fix pnum when return is 0
> v4: rebase to interface tweak, R-b dropped
> v3: no change
> v2: rebase to mapping parameter; it is ignored, so R-b kept
> ---
> block/parallels.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 9545761f49..9a3d3748ae 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -280,23 +280,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
> }
>
>
> -static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
> +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs,
> + bool want_zero,
> + int64_t offset,
> + int64_t bytes,
> + int64_t *pnum,
> + int64_t *map,
> + BlockDriverState **file)
> {
> BDRVParallelsState *s = bs->opaque;
> - int64_t offset;
> + int count;
>
> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> qemu_co_mutex_lock(&s->lock);
> - offset = block_status(s, sector_num, nb_sectors, pnum);
> + offset = block_status(s, offset >> BDRV_SECTOR_BITS,
> + bytes >> BDRV_SECTOR_BITS, &count);
> qemu_co_mutex_unlock(&s->lock);
>
> + *pnum = count * BDRV_SECTOR_SIZE;
> if (offset < 0) {
> return 0;
> }
>
> + *map = offset;
oh, didn't notice previous time :(, should be
*map = offset * BDRV_SECTOR_SIZE
(also, if you already use ">> BDRV_SECTOR_BITS" in this function,
would not it be more consistent to use "<< BDRV_SECTOR_BITS"
for sector-to-byte conversion?)
with that fixed (with "<<" or "*", up to you),
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> *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,
> @@ -793,7 +801,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] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-09 12:31 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-09 16:39 ` Eric Blake
2017-12-11 9:14 ` Vladimir Sementsov-Ogievskiy
2017-12-11 15:24 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-09 16:39 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev,
Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]
On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2017 23:30, 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>
>>
>> {
>> BDRVParallelsState *s = bs->opaque;
>> - int64_t offset;
>> + int count;
>>
>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
>> qemu_co_mutex_lock(&s->lock);
>> - offset = block_status(s, sector_num, nb_sectors, pnum);
>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS,
>> + bytes >> BDRV_SECTOR_BITS, &count);
>> qemu_co_mutex_unlock(&s->lock);
>>
>> + *pnum = count * BDRV_SECTOR_SIZE;
>> if (offset < 0) {
>> return 0;
>> }
>>
>> + *map = offset;
>
> oh, didn't notice previous time :(, should be
>
> *map = offset * BDRV_SECTOR_SIZE
>
Oh, right.
> (also, if you already use ">> BDRV_SECTOR_BITS" in this function,
> would not it be more consistent to use "<< BDRV_SECTOR_BITS"
> for sector-to-byte conversion?)
>
> with that fixed (with "<<" or "*", up to you),
'>> BDRV_SECTOR_BITS' always works. You could also write '/
BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing
by a power of 2, and use the shift instruction under the hood), but I
find that a bit harder to reason about.
On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
output as the input; if the left side is 32 bits, it risks overflowing.
But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've
learned (from past mistakes in other byte-conversion series) that the
multiply form is less likely to introduce unintended truncation bugs.
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-09 16:39 ` Eric Blake
@ 2017-12-11 9:14 ` Vladimir Sementsov-Ogievskiy
2017-12-11 15:24 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-11 9:14 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev,
Max Reitz
09.12.2017 19:39, Eric Blake wrote:
> On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.12.2017 23:30, 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>
>>>
>>> {
>>> BDRVParallelsState *s = bs->opaque;
>>> - int64_t offset;
>>> + int count;
>>>
>>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
>>> qemu_co_mutex_lock(&s->lock);
>>> - offset = block_status(s, sector_num, nb_sectors, pnum);
>>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS,
>>> + bytes >> BDRV_SECTOR_BITS, &count);
>>> qemu_co_mutex_unlock(&s->lock);
>>>
>>> + *pnum = count * BDRV_SECTOR_SIZE;
>>> if (offset < 0) {
>>> return 0;
>>> }
>>>
>>> + *map = offset;
>> oh, didn't notice previous time :(, should be
>>
>> *map = offset * BDRV_SECTOR_SIZE
>>
> Oh, right.
>
>> (also, if you already use ">> BDRV_SECTOR_BITS" in this function,
>> would not it be more consistent to use "<< BDRV_SECTOR_BITS"
>> for sector-to-byte conversion?)
>>
>> with that fixed (with "<<" or "*", up to you),
> '>> BDRV_SECTOR_BITS' always works. You could also write '/
> BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing
> by a power of 2, and use the shift instruction under the hood), but I
> find that a bit harder to reason about.
>
> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
> output as the input; if the left side is 32 bits, it risks overflowing.
> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've
> learned (from past mistakes in other byte-conversion series) that the
> multiply form is less likely to introduce unintended truncation bugs.
hmm, interesting observation, thank you
>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-09 16:39 ` Eric Blake
2017-12-11 9:14 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-11 15:24 ` Vladimir Sementsov-Ogievskiy
2017-12-12 16:32 ` Eric Blake
1 sibling, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-11 15:24 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev,
Max Reitz
09.12.2017 19:39, Eric Blake wrote:
> On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.12.2017 23:30, 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>
>>>
>>> {
>>> BDRVParallelsState *s = bs->opaque;
>>> - int64_t offset;
>>> + int count;
>>>
>>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
>>> qemu_co_mutex_lock(&s->lock);
>>> - offset = block_status(s, sector_num, nb_sectors, pnum);
>>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS,
>>> + bytes >> BDRV_SECTOR_BITS, &count);
>>> qemu_co_mutex_unlock(&s->lock);
>>>
>>> + *pnum = count * BDRV_SECTOR_SIZE;
>>> if (offset < 0) {
>>> return 0;
>>> }
>>>
>>> + *map = offset;
>> oh, didn't notice previous time :(, should be
>>
>> *map = offset * BDRV_SECTOR_SIZE
>>
> Oh, right.
>
>> (also, if you already use ">> BDRV_SECTOR_BITS" in this function,
>> would not it be more consistent to use "<< BDRV_SECTOR_BITS"
>> for sector-to-byte conversion?)
>>
>> with that fixed (with "<<" or "*", up to you),
> '>> BDRV_SECTOR_BITS' always works. You could also write '/
> BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing
> by a power of 2, and use the shift instruction under the hood), but I
> find that a bit harder to reason about.
>
> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
> output as the input; if the left side is 32 bits, it risks overflowing.
> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've
> learned (from past mistakes in other byte-conversion series) that the
> multiply form is less likely to introduce unintended truncation bugs.
hm, now I doubt. I've wrote tiny program:
#include <stdint.h>
#include <stdio.h>
int main() {
uint32_t blocks = 1 << 20;
int block_bits = 15;
uint32_t block_size = 1 << block_bits;
uint64_t a = blocks * block_size;
uint64_t b = blocks << block_bits;
uint64_t c = (uint64_t)blocks * block_size;
uint64_t d = (uint64_t)blocks << block_bits;
printf("%lx %lx %lx %lx\n", a, b, c, d);
return 0;
}
and it prints
0 0 800000000 800000000
for me. So, no difference between * and <<...
>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-11 15:24 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-12 16:32 ` Eric Blake
2017-12-12 16:49 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-12 16:32 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev,
Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]
On 12/11/2017 09:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
>> output as the input; if the left side is 32 bits, it risks overflowing.
>> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've
>> learned (from past mistakes in other byte-conversion series) that the
>> multiply form is less likely to introduce unintended truncation bugs.
>
> hm, now I doubt. I've wrote tiny program:
>
> #include <stdint.h>
> #include <stdio.h>
>
> int main() {
> uint32_t blocks = 1 << 20;
> int block_bits = 15;
> uint32_t block_size = 1 << block_bits;
> uint64_t a = blocks * block_size;
> uint64_t b = blocks << block_bits;
> uint64_t c = (uint64_t)blocks * block_size;
Not the same. 'block_size' in your code is 'uint32_t', so your
multiplication is still 32-bit if you don't cast blocks; while qemu has::
include/block/block.h:#define BDRV_SECTOR_BITS 9
include/block/block.h:#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS)
and the 1ULL in the qemu definition forces 'unsigned long long' results
whether or not you cast.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()
2017-12-12 16:32 ` Eric Blake
@ 2017-12-12 16:49 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-12 16:49 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Denis V. Lunev,
Max Reitz
12.12.2017 19:32, Eric Blake wrote:
> On 12/11/2017 09:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
>>> output as the input; if the left side is 32 bits, it risks overflowing.
>>> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've
>>> learned (from past mistakes in other byte-conversion series) that the
>>> multiply form is less likely to introduce unintended truncation bugs.
>> hm, now I doubt. I've wrote tiny program:
>>
>> #include <stdint.h>
>> #include <stdio.h>
>>
>> int main() {
>> uint32_t blocks = 1 << 20;
>> int block_bits = 15;
>> uint32_t block_size = 1 << block_bits;
>> uint64_t a = blocks * block_size;
>> uint64_t b = blocks << block_bits;
>> uint64_t c = (uint64_t)blocks * block_size;
> Not the same. 'block_size' in your code is 'uint32_t', so your
> multiplication is still 32-bit if you don't cast blocks; while qemu has::
>
> include/block/block.h:#define BDRV_SECTOR_BITS 9
> include/block/block.h:#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS)
>
> and the 1ULL in the qemu definition forces 'unsigned long long' results
> whether or not you cast.
>
Ah, cool, missed this. And we can't do such thing for BDRV_SECTOR_BITS
because it will be unspecified behavior.
Thank you for explanation.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 10/20] qcow: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (8 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 09/20] parallels: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:00 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 11/20] qcow2: " Eric Blake
` (9 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the qcow driver accordingly. There is no
intent to optimize based on the want_zero flag for this format.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: no change
v4: rebase to interface tweak
v3: rebase to master
v2: rebase to mapping flag
---
block/qcow.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/block/qcow.c b/block/qcow.c
index 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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 10/20] qcow: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 10/20] qcow: " Eric Blake
@ 2017-12-29 3:00 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:00 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, kwolf, qemu-block, vsementsov, Max Reitz
On Thu, 12/07 14:30, 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>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 11/20] qcow2: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (9 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 10/20] qcow: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:01 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 12/20] qed: " Eric Blake
` (8 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the qcow2 driver accordingly.
For now, we are ignoring the 'want_zero' hint. However, it should
be relatively straightforward to honor the hint as a way to return
larger *pnum values when we have consecutive clusters with the same
data/zero status but which differ only in having non-consecutive
mappings.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: no change
v4: update to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/qcow2.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 1914a940e5..642fa3bc8f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1637,32 +1637,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;
@@ -4355,7 +4357,7 @@ BlockDriver bdrv_qcow2 = {
.bdrv_child_perm = bdrv_format_default_perms,
.bdrv_create = qcow2_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
- .bdrv_co_get_block_status = qcow2_co_get_block_status,
+ .bdrv_co_block_status = qcow2_co_block_status,
.bdrv_co_preadv = qcow2_co_preadv,
.bdrv_co_pwritev = qcow2_co_pwritev,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/20] qcow2: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 11/20] qcow2: " Eric Blake
@ 2017-12-29 3:01 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:01 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, kwolf, qemu-block, vsementsov, Max Reitz
On Thu, 12/07 14:30, 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 12/20] qed: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (10 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 11/20] qcow2: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:06 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 13/20] raw: " Eric Blake
` (7 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Stefan Hajnoczi, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the qed driver accordingly, taking the opportunity
to inline qed_is_allocated_cb() into its lone caller (the callback
used to be important, until we switched qed to coroutines). There is
no intent to optimize based on the want_zero flag for this format.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: initialize len before qed_find_cluster() [Vladimir]
v4: rebase to interface change, inline pointless callback
v3: no change
v2: rebase to mapping flag, fix mask in qed_is_allocated_cb
---
block/qed.c | 84 +++++++++++++++++++++----------------------------------------
1 file changed, 28 insertions(+), 56 deletions(-)
diff --git a/block/qed.c b/block/qed.c
index 821dcaa055..13fbc3e2f1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -688,74 +688,46 @@ finish:
return ret;
}
-typedef struct {
- BlockDriverState *bs;
- Coroutine *co;
- uint64_t pos;
- int64_t status;
- int *pnum;
- BlockDriverState **file;
-} QEDIsAllocatedCB;
-
-/* Called with table_lock held. */
-static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
-{
- QEDIsAllocatedCB *cb = opaque;
- BDRVQEDState *s = cb->bs->opaque;
- *cb->pnum = len / BDRV_SECTOR_SIZE;
- switch (ret) {
- case QED_CLUSTER_FOUND:
- offset |= qed_offset_into_cluster(s, cb->pos);
- cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
- *cb->file = cb->bs->file->bs;
- break;
- case QED_CLUSTER_ZERO:
- cb->status = BDRV_BLOCK_ZERO;
- break;
- case QED_CLUSTER_L2:
- case QED_CLUSTER_L1:
- cb->status = 0;
- break;
- default:
- assert(ret < 0);
- cb->status = ret;
- break;
- }
-
- if (cb->co) {
- aio_co_wake(cb->co);
- }
-}
-
-static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num,
- int nb_sectors, int *pnum,
+static int coroutine_fn bdrv_qed_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t pos, int64_t bytes,
+ int64_t *pnum, int64_t *map,
BlockDriverState **file)
{
BDRVQEDState *s = bs->opaque;
- size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
- QEDIsAllocatedCB cb = {
- .bs = bs,
- .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
- .status = BDRV_BLOCK_OFFSET_MASK,
- .pnum = pnum,
- .file = file,
- };
+ size_t len = MIN(bytes, SIZE_MAX);
+ int status;
QEDRequest request = { .l2_table = NULL };
uint64_t offset;
int ret;
qemu_co_mutex_lock(&s->table_lock);
- ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
- qed_is_allocated_cb(&cb, ret, offset, len);
+ ret = qed_find_cluster(s, &request, pos, &len, &offset);
- /* The callback was invoked immediately */
- assert(cb.status != BDRV_BLOCK_OFFSET_MASK);
+ *pnum = len;
+ switch (ret) {
+ case QED_CLUSTER_FOUND:
+ *map = offset | qed_offset_into_cluster(s, pos);
+ status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ *file = bs->file->bs;
+ break;
+ case QED_CLUSTER_ZERO:
+ status = BDRV_BLOCK_ZERO;
+ break;
+ case QED_CLUSTER_L2:
+ case QED_CLUSTER_L1:
+ status = 0;
+ break;
+ default:
+ assert(ret < 0);
+ status = ret;
+ break;
+ }
qed_unref_l2_cache_entry(request.l2_table);
qemu_co_mutex_unlock(&s->table_lock);
- return cb.status;
+ return status;
}
static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
@@ -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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 12/20] qed: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 12/20] qed: " Eric Blake
@ 2017-12-29 3:06 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:06 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Stefan Hajnoczi,
Max Reitz
On Thu, 12/07 14:30, 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>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 13/20] raw: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (11 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 12/20] qed: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:07 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 14/20] sheepdog: " Eric Blake
` (6 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the raw driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: no change
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping
---
block/raw-format.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/block/raw-format.c b/block/raw-format.c
index 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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* [Qemu-devel] [PATCH v6 14/20] sheepdog: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (12 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 13/20] raw: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:08 ` Fam Zheng
2018-01-02 21:52 ` Jeff Cody
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 15/20] vdi: Avoid bitrot of debugging code Eric Blake
` (5 subsequent siblings)
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, Hitoshi Mitake, Liu Yuan,
Jeff Cody, Max Reitz, open list:Sheepdog
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the sheepdog driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: no change
v4: update to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/sheepdog.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 14/20] sheepdog: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 14/20] sheepdog: " Eric Blake
@ 2017-12-29 3:08 ` Fam Zheng
2018-01-02 21:52 ` Jeff Cody
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:08 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Hitoshi Mitake,
Liu Yuan, Jeff Cody, Max Reitz, open list:Sheepdog
On Thu, 12/07 14:30, 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>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [Qemu-devel] [PATCH v6 14/20] sheepdog: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 14/20] sheepdog: " Eric Blake
2017-12-29 3:08 ` Fam Zheng
@ 2018-01-02 21:52 ` Jeff Cody
1 sibling, 0 replies; 57+ messages in thread
From: Jeff Cody @ 2018-01-02 21:52 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, vsementsov, famz, qemu-block, Hitoshi Mitake,
Max Reitz, open list:Sheepdog, Liu Yuan
On Thu, Dec 07, 2017 at 02:30:30PM -0600, 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>
>
> ---
> v5: no change
> v4: update to interface tweak
> v3: no change
> v2: rebase to mapping flag
> ---
> block/sheepdog.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 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.14.3
>
>
Minor conflicts here with head, but straightforward to fix.
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 15/20] vdi: Avoid bitrot of debugging code
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (13 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 14/20] sheepdog: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:09 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 16/20] vdi: Switch to .bdrv_co_block_status() Eric Blake
` (4 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Stefan Weil, Max Reitz
Rework the debug define so that we always get -Wformat checking,
even when debugging is disabled.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v2-v5: 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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* [Qemu-devel] [PATCH v6 16/20] vdi: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (14 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 15/20] vdi: Avoid bitrot of debugging code Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:10 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 17/20] vmdk: " Eric Blake
` (3 subsequent siblings)
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Stefan Weil, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vdi driver accordingly. Note that the
TODO is already covered (the block layer guarantees bounds of its
requests), and that we can remove the now-unused s->block_sectors.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: fix pnum when offset rounded down to block_size [Vladimir]
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/vdi.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 6f83221ddc..f309562d36 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 - index_in_block, bytes);
result = VDI_IS_ALLOCATED(bmap_entry);
if (!result) {
return 0;
}
- offset = s->header.offset_data +
- (uint64_t)bmap_entry * s->block_size +
- sector_in_block * SECTOR_SIZE;
+ *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
+ index_in_block;
*file = bs->file->bs;
- return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
}
static int coroutine_fn
@@ -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.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 16/20] vdi: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 16/20] vdi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-12-29 3:10 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:10 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, vsementsov, qemu-block, Stefan Weil, Max Reitz
On Thu, 12/07 14:30, 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>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 17/20] vmdk: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (15 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 16/20] vdi: Switch to .bdrv_co_block_status() Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 12:36 ` Vladimir Sementsov-Ogievskiy
2017-12-29 3:16 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 18/20] vpc: " Eric Blake
` (2 subsequent siblings)
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vmdk driver accordingly. Drop the
now-unused vmdk_find_index_in_cluster().
Also, fix a pre-existing bug: if find_extent() fails (unlikely,
since the block layer did a bounds check), then we must return a
failure, rather than 0.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v5: drop dead code [Vladimir], return error on find_extent() failure
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping flag
---
block/vmdk.c | 38 ++++++++++++++------------------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index c665bcc977..8ae36e7a45 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1295,33 +1295,27 @@ static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
return extent_relative_offset % cluster_size;
}
-static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
- int64_t sector_num)
-{
- uint64_t offset;
- offset = vmdk_find_offset_in_cluster(extent, sector_num * BDRV_SECTOR_SIZE);
- return offset / BDRV_SECTOR_SIZE;
-}
-
-static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
+static int coroutine_fn vmdk_co_block_status(BlockDriverState *bs,
+ bool want_zero,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVVmdkState *s = bs->opaque;
int64_t index_in_cluster, n, ret;
- uint64_t offset;
+ uint64_t cluster_offset;
VmdkExtent *extent;
- extent = find_extent(s, sector_num, NULL);
+ extent = find_extent(s, offset >> BDRV_SECTOR_BITS, NULL);
if (!extent) {
- return 0;
+ return -EIO;
}
qemu_co_mutex_lock(&s->lock);
- ret = get_cluster_offset(bs, extent, NULL,
- sector_num * 512, false, &offset,
+ ret = get_cluster_offset(bs, extent, NULL, offset, false, &cluster_offset,
0, 0);
qemu_co_mutex_unlock(&s->lock);
- index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
+ index_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
switch (ret) {
case VMDK_ERROR:
ret = -EIO;
@@ -1336,18 +1330,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 +2383,7 @@ static BlockDriver bdrv_vmdk = {
.bdrv_close = vmdk_close,
.bdrv_create = vmdk_create,
.bdrv_co_flush_to_disk = vmdk_co_flush,
- .bdrv_co_get_block_status = vmdk_co_get_block_status,
+ .bdrv_co_block_status = vmdk_co_block_status,
.bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
.bdrv_has_zero_init = vmdk_has_zero_init,
.bdrv_get_specific_info = vmdk_get_specific_info,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 17/20] vmdk: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 17/20] vmdk: " Eric Blake
@ 2017-12-09 12:36 ` Vladimir Sementsov-Ogievskiy
2017-12-29 3:16 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 12:36 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
07.12.2017 23:30, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the vmdk driver accordingly. Drop the
> now-unused vmdk_find_index_in_cluster().
>
> Also, fix a pre-existing bug: if find_extent() fails (unlikely,
> since the block layer did a bounds check), then we must return a
> failure, rather than 0.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [Qemu-devel] [PATCH v6 17/20] vmdk: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 17/20] vmdk: " Eric Blake
2017-12-09 12:36 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 3:16 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:16 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, kwolf, qemu-block, vsementsov, Max Reitz
On Thu, 12/07 14:30, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the vmdk driver accordingly. Drop the
> now-unused vmdk_find_index_in_cluster().
>
> Also, fix a pre-existing bug: if find_extent() fails (unlikely,
> since the block layer did a bounds check), then we must return a
> failure, rather than 0.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 18/20] vpc: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (16 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 17/20] vmdk: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 13:42 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:55 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 19/20] vvfat: " Eric Blake
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, 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>
---
v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir]
v4: rebase to interface tweak
v3: rebase to master
v2: drop get_sector_offset() [Kevin], rebase to mapping flag
---
block/vpc.c | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 1576d7b595..0000b5b9b4 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -705,53 +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 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);
+ allocated = (image_offset != -1);
*pnum = 0;
ret = 0;
do {
/* All sectors in a block are contiguous (without using the bitmap) */
- n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
- - sector_num;
- n = MIN(n, nb_sectors);
+ n = ROUND_UP(offset + 1, s->block_size) - offset;
+ n = MIN(n, bytes);
*pnum += n;
- sector_num += n;
- nb_sectors -= n;
+ offset += n;
+ bytes -= n;
/* *pnum can't be greater than one block for allocated
* sectors since there is always a bitmap in between. */
if (allocated) {
*file = bs->file->bs;
- ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
+ *map = image_offset;
+ ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
break;
}
- if (nb_sectors == 0) {
+ if (bytes == 0) {
break;
}
- offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false,
- NULL);
- } while (offset == -1);
+ image_offset = get_image_offset(bs, offset, false, NULL);
+ } while (image_offset == -1);
qemu_co_mutex_unlock(&s->lock);
return ret;
@@ -1097,7 +1098,7 @@ static BlockDriver bdrv_vpc = {
.bdrv_co_preadv = vpc_co_preadv,
.bdrv_co_pwritev = vpc_co_pwritev,
- .bdrv_co_get_block_status = vpc_co_get_block_status,
+ .bdrv_co_block_status = vpc_co_block_status,
.bdrv_get_info = vpc_get_info,
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 18/20] vpc: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 18/20] vpc: " Eric Blake
@ 2017-12-09 13:42 ` Vladimir Sementsov-Ogievskiy
2017-12-29 2:55 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 13:42 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, famz, qemu-block, Max Reitz
07.12.2017 23:30, 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>
>
> ---
> v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir]
> v4: rebase to interface tweak
> v3: rebase to master
> v2: drop get_sector_offset() [Kevin], rebase to mapping flag
> ---
> block/vpc.c | 43 ++++++++++++++++++++++---------------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 1576d7b595..0000b5b9b4 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -705,53 +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 image_offset;
image_offset is used instead of both old start and offset vars
> 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;
... instead of both..
> - allocated = (offset != -1);
> + image_offset = get_image_offset(bs, offset, false, NULL);
> + allocated = (image_offset != -1);
> *pnum = 0;
> ret = 0;
>
> do {
> /* All sectors in a block are contiguous (without using the bitmap) */
> - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
> - - sector_num;
> - n = MIN(n, nb_sectors);
> + n = ROUND_UP(offset + 1, s->block_size) - offset;
> + n = MIN(n, bytes);
>
> *pnum += n;
> - sector_num += n;
> - nb_sectors -= n;
> + offset += n;
> + bytes -= n;
> /* *pnum can't be greater than one block for allocated
> * sectors since there is always a bitmap in between. */
> if (allocated) {
> *file = bs->file->bs;
> - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> + *map = image_offset;
image_offset still unchanged as we can go here only on first loop iteration,
so it's ok to use it instead of 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);
> - } while (offset == -1);
> + image_offset = get_image_offset(bs, offset, false, NULL);
> + } while (image_offset == -1);
>
> qemu_co_mutex_unlock(&s->lock);
> return ret;
> @@ -1097,7 +1098,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,
>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 18/20] vpc: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 18/20] vpc: " Eric Blake
2017-12-09 13:42 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 2:55 ` Fam Zheng
2018-01-02 20:30 ` Eric Blake
1 sibling, 1 reply; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 2:55 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, kwolf, vsementsov, qemu-block, Max Reitz
On Thu, 12/07 14:30, 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().
No get_sector_offset change in the patch any more, it was removed by
778b087e513ea6fdc525c5a194ff7c9b8d3f53cb.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir]
> v4: rebase to interface tweak
> v3: rebase to master
> v2: drop get_sector_offset() [Kevin], rebase to mapping flag
> ---
> block/vpc.c | 43 ++++++++++++++++++++++---------------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 1576d7b595..0000b5b9b4 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -705,53 +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 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);
> + 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);
Should 'n' be updated to int64_t to match the types of offset and bytes?
>
> *pnum += n;
> - sector_num += n;
> - nb_sectors -= n;
> + offset += n;
> + bytes -= n;
> /* *pnum can't be greater than one block for allocated
> * sectors since there is always a bitmap in between. */
> if (allocated) {
> *file = bs->file->bs;
> - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> + *map = image_offset;
> + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> break;
> }
> - if (nb_sectors == 0) {
> + if (bytes == 0) {
> break;
> }
> - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false,
> - NULL);
> - } while (offset == -1);
> + image_offset = get_image_offset(bs, offset, false, NULL);
> + } while (image_offset == -1);
>
> qemu_co_mutex_unlock(&s->lock);
> return ret;
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 18/20] vpc: Switch to .bdrv_co_block_status()
2017-12-29 2:55 ` Fam Zheng
@ 2018-01-02 20:30 ` Eric Blake
0 siblings, 0 replies; 57+ messages in thread
From: Eric Blake @ 2018-01-02 20:30 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, kwolf, vsementsov, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]
On 12/28/2017 08:55 PM, Fam Zheng wrote:
> On Thu, 12/07 14:30, 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().
>
> No get_sector_offset change in the patch any more, it was removed by
> 778b087e513ea6fdc525c5a194ff7c9b8d3f53cb.
Will tweak the commit message if I have reason to spin v7 (at least 9/20
had a real bug, which is probably easier for me to respin than to ask
the maintainer to adjust).
>
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> - int64_t start, offset;
>> + int64_t image_offset;
>> bool allocated;
>> - int64_t ret;
>> + int ret;
>> int n;
>>
>> 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);
>
> Should 'n' be updated to int64_t to match the types of offset and bytes?
s->block_size is uint32_t, and a power of 2; therefore, rounding 'offset
+ 1' up to block size, then subtracting offset, can't exceed 32 bits.
But that's tricky to audit for; I'm not opposed to changing the type of
'n' to 64-bits if you think that is easier to read.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 19/20] vvfat: Switch to .bdrv_co_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (17 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 18/20] vpc: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-29 3:26 ` Fam Zheng
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
19 siblings, 1 reply; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, qemu-block, vsementsov, Max Reitz
We are gradually moving away from sector-based interfaces, towards
byte-based. Update the vvfat driver accordingly. Note that we
can rely on the block driver having already clamped limits to our
block size, and simplify accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v5: no change
v4: rebase to interface tweak
v3: no change
v2: rebase to earlier changes, simplify
---
block/vvfat.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index a690595f2c..37d85a389f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3086,15 +3086,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;
}
@@ -3255,7 +3253,7 @@ static BlockDriver bdrv_vvfat = {
.bdrv_co_preadv = vvfat_co_preadv,
.bdrv_co_pwritev = vvfat_co_pwritev,
- .bdrv_co_get_block_status = vvfat_co_get_block_status,
+ .bdrv_co_block_status = vvfat_co_block_status,
};
static void bdrv_vvfat_init(void)
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 19/20] vvfat: Switch to .bdrv_co_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 19/20] vvfat: " Eric Blake
@ 2017-12-29 3:26 ` Fam Zheng
0 siblings, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:26 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, kwolf, qemu-block, vsementsov, Max Reitz
On Thu, 12/07 14:30, 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>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status()
2017-12-07 20:30 [Qemu-devel] [PATCH v6 00/20] add byte-based block_status driver callbacks Eric Blake
` (18 preceding siblings ...)
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 19/20] vvfat: " Eric Blake
@ 2017-12-07 20:30 ` Eric Blake
2017-12-09 13:23 ` Vladimir Sementsov-Ogievskiy
2017-12-29 3:27 ` Fam Zheng
19 siblings, 2 replies; 57+ messages in thread
From: Eric Blake @ 2017-12-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, qemu-block, vsementsov, 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>
---
v6: rebase to changes in patch 1, drop R-b
v5: rebase to master
v4: rebase to interface tweak
v3: no change
v2: rebase to earlier changes
---
include/block/block_int.h | 3 ---
block/io.c | 50 ++++++++++-------------------------------------
2 files changed, 10 insertions(+), 43 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 42e5c37a63..6f8df6a847 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -217,9 +217,6 @@ struct BlockDriver {
* as well as non-NULL pnum, map, and file; in turn, the driver
* must return an error or set pnum to an aligned non-zero value.
*/
- int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum,
- BlockDriverState **file);
int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,
bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file);
diff --git a/block/io.c b/block/io.c
index 109b3826cd..e0376c1d7c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1893,7 +1893,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
/* Must be non-NULL or bdrv_getlength() would have failed */
assert(bs->drv);
- if (!bs->drv->bdrv_co_get_block_status && !bs->drv->bdrv_co_block_status) {
+ if (!bs->drv->bdrv_co_block_status) {
*pnum = bytes;
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
if (offset + bytes == total_size) {
@@ -1911,53 +1911,23 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
/* Round out to request_alignment boundaries */
align = bs->bl.request_alignment;
- if (bs->drv->bdrv_co_get_block_status && align < BDRV_SECTOR_SIZE) {
- align = BDRV_SECTOR_SIZE;
- }
aligned_offset = QEMU_ALIGN_DOWN(offset, align);
aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
- if (bs->drv->bdrv_co_get_block_status) {
- int count; /* sectors */
- int64_t longret;
-
- assert(QEMU_IS_ALIGNED(aligned_offset | aligned_bytes,
- BDRV_SECTOR_SIZE));
- /*
- * The contract allows us to return pnum smaller than bytes, even
- * if the next query would see the same status; we truncate the
- * request to avoid overflowing the driver's 32-bit interface.
- */
- longret = bs->drv->bdrv_co_get_block_status(
- bs, aligned_offset >> BDRV_SECTOR_BITS,
- MIN(INT_MAX, aligned_bytes) >> BDRV_SECTOR_BITS, &count,
- &local_file);
- if (longret < 0) {
- assert(INT_MIN <= longret);
- ret = longret;
- goto out;
- }
- if (longret & BDRV_BLOCK_OFFSET_VALID) {
- local_map = longret & BDRV_BLOCK_OFFSET_MASK;
- }
- ret = longret & ~BDRV_BLOCK_OFFSET_MASK;
- *pnum = count * BDRV_SECTOR_SIZE;
- } else {
- ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
- aligned_bytes, pnum, &local_map,
- &local_file);
- if (ret < 0) {
- *pnum = 0;
- goto out;
- }
- assert(*pnum); /* The block driver must make progress */
+ ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
+ aligned_bytes, pnum, &local_map,
+ &local_file);
+ if (ret < 0) {
+ *pnum = 0;
+ goto out;
}
/*
- * The driver's result must be a multiple of request_alignment.
+ * The driver's result must be a non-zero multiple of request_alignment.
* Clamp pnum and adjust map to original request.
*/
- assert(QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset);
+ assert(*pnum && QEMU_IS_ALIGNED(*pnum, align) &&
+ align > offset - aligned_offset);
*pnum -= offset - aligned_offset;
if (*pnum > bytes) {
*pnum = bytes;
--
2.14.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
@ 2017-12-09 13:23 ` Vladimir Sementsov-Ogievskiy
2017-12-29 3:27 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-09 13:23 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, famz, qemu-block, Stefan Hajnoczi, Max Reitz
07.12.2017 23:30, 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] 57+ messages in thread
* Re: [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status()
2017-12-07 20:30 ` [Qemu-devel] [PATCH v6 20/20] block: Drop unused .bdrv_co_get_block_status() Eric Blake
2017-12-09 13:23 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 3:27 ` Fam Zheng
1 sibling, 0 replies; 57+ messages in thread
From: Fam Zheng @ 2017-12-29 3:27 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, kwolf, qemu-block, vsementsov, Stefan Hajnoczi,
Max Reitz
On Thu, 12/07 14:30, 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: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 57+ messages in thread