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
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
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.