Re: [Qemu-block] [PATCH v8 19/21] vpc: Switch to .bdrv_co_block_status()

[Eric Blake]

On 02/14/2018 07:08 AM, Kevin Wolf wrote:

Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:

We are gradually moving away from sector-based interfaces, towards
byte-based.  Update the vpc driver accordingly.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fam Zheng 

---



+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;


This does work, but only because the loop isn't actually looping for
allocated == true. In the old code, it was obvious that start was
assigned only once and never changed during the loop, but image_offset
changes in each loop iteration.

It would probably be cleaner and more obviously correct to move the
assignment of *map to before the loop.


Yes, that would be a bit nicer.



Kevin



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v8 19/21] vpc: Switch to .bdrv_co_block_status()

[Kevin Wolf]

Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Update the vpc driver accordingly.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Fam Zheng 
> 
> ---
> v7: tweak commit message and type of 'n' [Fam]
> v6: no change
> 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 | 45 +++--
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index cfa5144e867..fba4492fd7b 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -706,53 +706,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 n;
> +int ret;
> +int64_t 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(>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;

This does work, but only because the loop isn't actually looping for
allocated == true. In the old code, it was obvious that start was
assigned only once and never changed during the loop, but image_offset
changes in each loop iteration.

It would probably be cleaner and more obviously correct to move the
assignment of *map to before the loop.

Kevin

[Qemu-block] [PATCH v8 19/21] vpc: Switch to .bdrv_co_block_status()

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Update the vpc driver accordingly.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fam Zheng 

---
v7: tweak commit message and type of 'n' [Fam]
v6: no change
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 | 45 +++--
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index cfa5144e867..fba4492fd7b 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -706,53 +706,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 n;
+int ret;
+int64_t 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(>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(>lock);
 return ret;
@@ -1098,7 +1099,7 @@ static BlockDriver bdrv_vpc = {

 .bdrv_co_preadv = vpc_co_preadv,
 .bdrv_co_pwritev= vpc_co_pwritev,
-.bdrv_co_get_block_status   = vpc_co_get_block_status,
+.bdrv_co_block_status   = vpc_co_block_status,

 .bdrv_get_info  = vpc_get_info,

-- 
2.14.3