[v6,09/20] parallels: Switch to .bdrv_co_block_status()

Message ID 20171207203036.14993-10-eblake@redhat.com
State New
Headers show
Series
  • add byte-based block_status driver callbacks
Related show

Commit Message

Eric Blake Dec. 7, 2017, 8:30 p.m.
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(-)

Comments

Vladimir Sementsov-Ogievskiy Dec. 9, 2017, 12:31 p.m. | #1
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,
Eric Blake Dec. 9, 2017, 4:39 p.m. | #2
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>
>
Vladimir Sementsov-Ogievskiy Dec. 11, 2017, 9:14 a.m. | #3
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>
>>
Vladimir Sementsov-Ogievskiy Dec. 11, 2017, 3:24 p.m. | #4
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>
>>
Eric Blake Dec. 12, 2017, 4:32 p.m. | #5
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.
Vladimir Sementsov-Ogievskiy Dec. 12, 2017, 4:49 p.m. | #6
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.

Patch

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,