[v6,18/20] vpc: Switch to .bdrv_co_block_status()

Message ID 20171207203036.14993-19-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 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(-)

Comments

Vladimir Sementsov-Ogievskiy Dec. 9, 2017, 1:42 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 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>
Fam Zheng Dec. 29, 2017, 2:55 a.m. | #2
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;
Eric Blake Jan. 2, 2018, 8:30 p.m. | #3
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.

Patch

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,