[v4,02/20] block: Switch passthrough drivers to .bdrv_co_block_status()

Message ID 20171012185916.22776-3-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:58 p.m.
We are gradually moving away from sector-based interfaces, towards
byte-based.  Update the generic helpers, and all passthrough clients
(blkdebug, commit, mirror, throttle) accordingly.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: rebase to interface tweak
v3: rebase to addition of throttle driver
v2: rebase to master, retitle while merging blkdebug, commit, and mirror
---
 include/block/block_int.h | 28 ++++++++++++++++------------
 block/io.c                | 36 ++++++++++++++++++++----------------
 block/blkdebug.c          | 20 +++++++++++---------
 block/commit.c            |  2 +-
 block/mirror.c            |  2 +-
 block/throttle.c          |  2 +-
 6 files changed, 50 insertions(+), 40 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Nov. 28, 2017, 8:19 a.m. | #1
12.10.2017 21:58, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Update the generic helpers, and all passthrough clients
> (blkdebug, commit, mirror, throttle) accordingly.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to interface tweak
> v3: rebase to addition of throttle driver
> v2: rebase to master, retitle while merging blkdebug, commit, and mirror
> ---
>   include/block/block_int.h | 28 ++++++++++++++++------------
>   block/io.c                | 36 ++++++++++++++++++++----------------
>   block/blkdebug.c          | 20 +++++++++++---------
>   block/commit.c            |  2 +-
>   block/mirror.c            |  2 +-
>   block/throttle.c          |  2 +-
>   6 files changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 4153cd646d..7c8503f693 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1018,23 +1018,27 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>                                  uint64_t *nperm, uint64_t *nshared);
>
>   /*
> - * Default implementation for drivers to pass bdrv_co_get_block_status() to
> + * Default implementation for drivers to pass bdrv_co_block_status() to
>    * their file.
>    */
> -int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
> -                                                        int64_t sector_num,
> -                                                        int nb_sectors,
> -                                                        int *pnum,
> -                                                        BlockDriverState **file);
> +int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
> +                                                bool want_zero,
> +                                                int64_t offset,
> +                                                int64_t bytes,
> +                                                int64_t *pnum,
> +                                                int64_t *map,
> +                                                BlockDriverState **file);
>   /*
> - * Default implementation for drivers to pass bdrv_co_get_block_status() to
> + * Default implementation for drivers to pass bdrv_co_block_status() to
>    * their backing file.
>    */
> -int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
> -                                                           int64_t sector_num,
> -                                                           int nb_sectors,
> -                                                           int *pnum,
> -                                                           BlockDriverState **file);
> +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
> +                                                   bool want_zero,
> +                                                   int64_t offset,
> +                                                   int64_t bytes,
> +                                                   int64_t *pnum,
> +                                                   int64_t *map,
> +                                                   BlockDriverState **file);
>   const char *bdrv_get_parent_name(const BlockDriverState *bs);
>   void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
>   bool blk_dev_has_removable_media(BlockBackend *blk);
> diff --git a/block/io.c b/block/io.c
> index ef9ea44667..6a2a2e1484 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1754,30 +1754,34 @@ typedef struct BdrvCoBlockStatusData {
>       bool done;
>   } BdrvCoBlockStatusData;
>
> -int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
> -                                                        int64_t sector_num,
> -                                                        int nb_sectors,
> -                                                        int *pnum,
> -                                                        BlockDriverState **file)
> +int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
> +                                                bool want_zero,
> +                                                int64_t offset,
> +                                                int64_t bytes,
> +                                                int64_t *pnum,
> +                                                int64_t *map,
> +                                                BlockDriverState **file)
>   {
>       assert(bs->file && bs->file->bs);
> -    *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;
>   }
>
> -int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
> -                                                           int64_t sector_num,
> -                                                           int nb_sectors,
> -                                                           int *pnum,
> -                                                           BlockDriverState **file)
> +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
> +                                                   bool want_zero,
> +                                                   int64_t offset,
> +                                                   int64_t bytes,
> +                                                   int64_t *pnum,
> +                                                   int64_t *map,
> +                                                   BlockDriverState **file)
>   {
>       assert(bs->backing && bs->backing->bs);
> -    *pnum = nb_sectors;
> +    *pnum = bytes;
> +    *map = offset;
>       *file = bs->backing->bs;
> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> -           (sector_num << BDRV_SECTOR_BITS);
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
>   }

hmm. not related question, but can you please explain, why we use such stubs
instead of really call bdrv_co_block_status on bs->file->bs or 
bs->backing->bs ?

>
>   /*
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index e21669979d..82c30357f8 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -627,15 +627,17 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
>       return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
>   }
>
> -static int64_t coroutine_fn blkdebug_co_get_block_status(
> -    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
> -    BlockDriverState **file)
> +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
> +                                                 bool want_zero,
> +                                                 int64_t offset,
> +                                                 int64_t bytes,
> +                                                 int64_t *pnum,
> +                                                 int64_t *map,
> +                                                 BlockDriverState **file)
>   {
> -    assert(QEMU_IS_ALIGNED(sector_num | nb_sectors,
> -                           DIV_ROUND_UP(bs->bl.request_alignment,
> -                                        BDRV_SECTOR_SIZE)));
> -    return bdrv_co_get_block_status_from_file(bs, sector_num, nb_sectors,
> -                                              pnum, file);
> +    assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));

looks like this is obviously guaranteed by block layer, so no needs for 
special
function. and we can use

.bdrv_co_block_status       = bdrv_co_block_status_from_backing

like for other drivers.

> +    return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes,
> +                                          pnum, map, file);
>   }
>
>   static void blkdebug_close(BlockDriverState *bs)
> @@ -907,7 +909,7 @@ static BlockDriver bdrv_blkdebug = {
>       .bdrv_co_flush_to_disk  = blkdebug_co_flush,
>       .bdrv_co_pwrite_zeroes  = blkdebug_co_pwrite_zeroes,
>       .bdrv_co_pdiscard       = blkdebug_co_pdiscard,
> -    .bdrv_co_get_block_status = blkdebug_co_get_block_status,
> +    .bdrv_co_block_status   = blkdebug_co_block_status,
>
>       .bdrv_debug_event           = blkdebug_debug_event,
>       .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
> diff --git a/block/commit.c b/block/commit.c
> index 5036eec434..ec0511bac7 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -265,7 +265,7 @@ static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>   static BlockDriver bdrv_commit_top = {
>       .format_name                = "commit_top",
>       .bdrv_co_preadv             = bdrv_commit_top_preadv,
> -    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
> +    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
>       .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
>       .bdrv_close                 = bdrv_commit_top_close,
>       .bdrv_child_perm            = bdrv_commit_top_child_perm,
> diff --git a/block/mirror.c b/block/mirror.c
> index 307b6391a8..79739d063e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1094,7 +1094,7 @@ static BlockDriver bdrv_mirror_top = {
>       .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
>       .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
>       .bdrv_co_flush              = bdrv_mirror_top_flush,
> -    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
> +    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
>       .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
>       .bdrv_close                 = bdrv_mirror_top_close,
>       .bdrv_child_perm            = bdrv_mirror_top_child_perm,
> diff --git a/block/throttle.c b/block/throttle.c
> index 5bca76300f..76cd963a8c 100644
> --- a/block/throttle.c
> +++ b/block/throttle.c
> @@ -224,7 +224,7 @@ static BlockDriver bdrv_throttle = {
>       .bdrv_reopen_prepare                =   throttle_reopen_prepare,
>       .bdrv_reopen_commit                 =   throttle_reopen_commit,
>       .bdrv_reopen_abort                  =   throttle_reopen_abort,
> -    .bdrv_co_get_block_status           =   bdrv_co_get_block_status_from_file,
> +    .bdrv_co_block_status               =   bdrv_co_block_status_from_file,
>
>       .is_filter                          =   true,
>   };

conversion looks ok,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Eric Blake Nov. 28, 2017, 4:05 p.m. | #2
On 11/28/2017 02:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:58, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Update the generic helpers, and all passthrough clients
>> (blkdebug, commit, mirror, throttle) accordingly.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>> BlockDriverState **file)
>> +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
>> +                                                   bool want_zero,
>> +                                                   int64_t offset,
>> +                                                   int64_t bytes,
>> +                                                   int64_t *pnum,
>> +                                                   int64_t *map,
>> +                                                   BlockDriverState 
>> **file)
>>   {
>>       assert(bs->backing && bs->backing->bs);
>> -    *pnum = nb_sectors;
>> +    *pnum = bytes;
>> +    *map = offset;
>>       *file = bs->backing->bs;
>> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
>> -           (sector_num << BDRV_SECTOR_BITS);
>> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
>>   }
> 
> hmm. not related question, but can you please explain, why we use such 
> stubs
> instead of really call bdrv_co_block_status on bs->file->bs or 
> bs->backing->bs ?
> 

Returning BDRV_BLOCK_RAW is what tells io.c to call 
bdrv_co_block_status() on whatever got assigned into *file.  The two 
stubs make it so there are fewer conditionals in io.c; perhaps it could 
be reworked to avoid the stubs and have io.c have all the smarts when 
.bdrv_co_block_status is NULL, but as you say, that would be a separate 
patch (although it was Manos' work earlier this year that even created 
the common helper stubs rather than duplicating code in each callback in 
the first place).


>> +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,

>> +    assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
> 
> looks like this is obviously guaranteed by block layer, so no needs for 
> special
> function. and we can use
> 
> .bdrv_co_block_status       = bdrv_co_block_status_from_backing
> 
> like for other drivers.
> 

We could, but we don't want to.  The point of the blkdebug driver is to 
explicitly test that certain preconditions are being satisfied, so that 
even if the block layer in io.c changes, our iotests make sure that 
drivers are provided with certain guarantees.  In other words, the 
assert() added here is added value that we cannot stick in the common 
helper, but something that we want to keep associated with the blkdebug 
driver.

Patch

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4153cd646d..7c8503f693 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1018,23 +1018,27 @@  void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                uint64_t *nperm, uint64_t *nshared);

 /*
- * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * Default implementation for drivers to pass bdrv_co_block_status() to
  * their file.
  */
-int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
-                                                        int64_t sector_num,
-                                                        int nb_sectors,
-                                                        int *pnum,
-                                                        BlockDriverState **file);
+int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
+                                                bool want_zero,
+                                                int64_t offset,
+                                                int64_t bytes,
+                                                int64_t *pnum,
+                                                int64_t *map,
+                                                BlockDriverState **file);
 /*
- * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * Default implementation for drivers to pass bdrv_co_block_status() to
  * their backing file.
  */
-int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
-                                                           int64_t sector_num,
-                                                           int nb_sectors,
-                                                           int *pnum,
-                                                           BlockDriverState **file);
+int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
+                                                   bool want_zero,
+                                                   int64_t offset,
+                                                   int64_t bytes,
+                                                   int64_t *pnum,
+                                                   int64_t *map,
+                                                   BlockDriverState **file);
 const char *bdrv_get_parent_name(const BlockDriverState *bs);
 void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
 bool blk_dev_has_removable_media(BlockBackend *blk);
diff --git a/block/io.c b/block/io.c
index ef9ea44667..6a2a2e1484 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1754,30 +1754,34 @@  typedef struct BdrvCoBlockStatusData {
     bool done;
 } BdrvCoBlockStatusData;

-int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
-                                                        int64_t sector_num,
-                                                        int nb_sectors,
-                                                        int *pnum,
-                                                        BlockDriverState **file)
+int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
+                                                bool want_zero,
+                                                int64_t offset,
+                                                int64_t bytes,
+                                                int64_t *pnum,
+                                                int64_t *map,
+                                                BlockDriverState **file)
 {
     assert(bs->file && bs->file->bs);
-    *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;
 }

-int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
-                                                           int64_t sector_num,
-                                                           int nb_sectors,
-                                                           int *pnum,
-                                                           BlockDriverState **file)
+int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
+                                                   bool want_zero,
+                                                   int64_t offset,
+                                                   int64_t bytes,
+                                                   int64_t *pnum,
+                                                   int64_t *map,
+                                                   BlockDriverState **file)
 {
     assert(bs->backing && bs->backing->bs);
-    *pnum = nb_sectors;
+    *pnum = bytes;
+    *map = offset;
     *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-           (sector_num << BDRV_SECTOR_BITS);
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
 }

 /*
diff --git a/block/blkdebug.c b/block/blkdebug.c
index e21669979d..82c30357f8 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -627,15 +627,17 @@  static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
     return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
 }

-static int64_t coroutine_fn blkdebug_co_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
+static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
+                                                 bool want_zero,
+                                                 int64_t offset,
+                                                 int64_t bytes,
+                                                 int64_t *pnum,
+                                                 int64_t *map,
+                                                 BlockDriverState **file)
 {
-    assert(QEMU_IS_ALIGNED(sector_num | nb_sectors,
-                           DIV_ROUND_UP(bs->bl.request_alignment,
-                                        BDRV_SECTOR_SIZE)));
-    return bdrv_co_get_block_status_from_file(bs, sector_num, nb_sectors,
-                                              pnum, file);
+    assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
+    return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes,
+                                          pnum, map, file);
 }

 static void blkdebug_close(BlockDriverState *bs)
@@ -907,7 +909,7 @@  static BlockDriver bdrv_blkdebug = {
     .bdrv_co_flush_to_disk  = blkdebug_co_flush,
     .bdrv_co_pwrite_zeroes  = blkdebug_co_pwrite_zeroes,
     .bdrv_co_pdiscard       = blkdebug_co_pdiscard,
-    .bdrv_co_get_block_status = blkdebug_co_get_block_status,
+    .bdrv_co_block_status   = blkdebug_co_block_status,

     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
diff --git a/block/commit.c b/block/commit.c
index 5036eec434..ec0511bac7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -265,7 +265,7 @@  static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
 static BlockDriver bdrv_commit_top = {
     .format_name                = "commit_top",
     .bdrv_co_preadv             = bdrv_commit_top_preadv,
-    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
+    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
     .bdrv_close                 = bdrv_commit_top_close,
     .bdrv_child_perm            = bdrv_commit_top_child_perm,
diff --git a/block/mirror.c b/block/mirror.c
index 307b6391a8..79739d063e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1094,7 +1094,7 @@  static BlockDriver bdrv_mirror_top = {
     .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
     .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
     .bdrv_co_flush              = bdrv_mirror_top_flush,
-    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
+    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_close                 = bdrv_mirror_top_close,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..76cd963a8c 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -224,7 +224,7 @@  static BlockDriver bdrv_throttle = {
     .bdrv_reopen_prepare                =   throttle_reopen_prepare,
     .bdrv_reopen_commit                 =   throttle_reopen_commit,
     .bdrv_reopen_abort                  =   throttle_reopen_abort,
-    .bdrv_co_get_block_status           =   bdrv_co_get_block_status_from_file,
+    .bdrv_co_block_status               =   bdrv_co_block_status_from_file,

     .is_filter                          =   true,
 };