[v4,16/20] vdi: Switch to .bdrv_co_block_status()

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

Commit Message

Eric Blake Oct. 12, 2017, 6:59 p.m.
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>

---
v4: rebase to interface tweak
v3: no change
v2: rebase to mapping flag
---
 block/vdi.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Nov. 30, 2017, 11:26 a.m. | #1
12.10.2017 21:59, 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>
>
> ---
> 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..b30886823d 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);

may be not bad to update message, as you changed numbers, like

logout("%p, offset %" PRId64 ", bytes %" PRId64 ", %p\n", bs, offset, bytes, pnum);

but for me it is ok as is too.

> +    *pnum = MIN(s->block_size, bytes);

looks like it should be MIN(s->block_size - index_in_block, bytes);

with that:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>       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,
Eric Blake Nov. 30, 2017, 11:11 p.m. | #2
On 11/30/2017 05:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, 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>
>>

>>       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);
> 
> may be not bad to update message, as you changed numbers, like
> 
> logout("%p, offset %" PRId64 ", bytes %" PRId64 ", %p\n", bs, offset, 
> bytes, pnum);
> 
> but for me it is ok as is too.

Debugging messages can be looked up in context when you enable 
debugging. I'm not bothering to change that.

> 
>> +    *pnum = MIN(s->block_size, bytes);
> 
> looks like it should be MIN(s->block_size - index_in_block, bytes);

Oh, good catch. You are right.

> 
> with that:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks again for a careful review throughout this series.

Patch

diff --git a/block/vdi.c b/block/vdi.c
index 6f83221ddc..b30886823d 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, 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,