Re: [Qemu-devel] [PATCH v4 04/11] block: Inline bdrv_co_block_status_from_*()

[Vladimir Sementsov-Ogievskiy]

10.04.2019 23:20, Max Reitz wrote:
> With bdrv_filtered_rw_bs(), we can easily handle this default filter
> behavior in bdrv_co_block_status().
> 
> blkdebug wants to have an additional assertion, so it keeps its own
> implementation, except bdrv_co_block_status_from_file() needs to be
> inlined there.
> 
> Suggested-by: Eric Blake <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   include/block/block_int.h | 22 -----------------
>   block/blkdebug.c          |  7 ++++--
>   block/blklogwrites.c      |  1 -
>   block/commit.c            |  1 -
>   block/copy-on-read.c      |  2 --
>   block/io.c                | 51 +++++++++++++--------------------------
>   block/mirror.c            |  1 -
>   block/throttle.c          |  1 -
>   8 files changed, 22 insertions(+), 64 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index d0309e6307..76c7c0a111 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1187,28 +1187,6 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>                                  uint64_t perm, uint64_t shared,
>                                  uint64_t *nperm, uint64_t *nshared);
>   
> -/*
> - * Default implementation for drivers to pass bdrv_co_block_status() to
> - * their 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_block_status() to
> - * their backing 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/blkdebug.c b/block/blkdebug.c
> index efd9441625..7950ae729c 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -637,8 +637,11 @@ static int coroutine_fn 
> blkdebug_co_block_status(BlockDriverState *bs,
>                                                    BlockDriverState **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);
> +    assert(bs->file && bs->file->bs);
> +    *pnum = bytes;
> +    *map = offset;
> +    *file = bs->file->bs;
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;

directly inlined, OK

>   }
>   
>   static void blkdebug_close(BlockDriverState *bs)
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index eb2b4901a5..1eb4a5c613 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -518,7 +518,6 @@ static BlockDriver bdrv_blk_log_writes = {
>       .bdrv_co_pwrite_zeroes  = blk_log_writes_co_pwrite_zeroes,
>       .bdrv_co_flush_to_disk  = blk_log_writes_co_flush_to_disk,
>       .bdrv_co_pdiscard       = blk_log_writes_co_pdiscard,
> -    .bdrv_co_block_status   = bdrv_co_block_status_from_file,
>   
>       .is_filter              = true,
>       .strong_runtime_opts    = blk_log_writes_strong_runtime_opts,
> diff --git a/block/commit.c b/block/commit.c
> index 252007fd57..c366ee9655 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -254,7 +254,6 @@ 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_block_status       = bdrv_co_block_status_from_backing,
>       .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
>       .bdrv_child_perm            = bdrv_commit_top_child_perm,
>   
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 53972b1da3..fe9260163c 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -150,8 +150,6 @@ static BlockDriver bdrv_copy_on_read = {
>       .bdrv_eject                         = cor_eject,
>       .bdrv_lock_medium                   = cor_lock_medium,
>   
> -    .bdrv_co_block_status               = bdrv_co_block_status_from_file,
> -
>       .bdrv_recurse_is_first_non_filter   = cor_recurse_is_first_non_filter,
>   
>       .has_variable_length                = true,
> diff --git a/block/io.c b/block/io.c
> index 5c33ecc080..8d124bae5c 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1993,36 +1993,6 @@ typedef struct BdrvCoBlockStatusData {
>       bool done;
>   } BdrvCoBlockStatusData;
>   
> -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 = bytes;
> -    *map = offset;
> -    *file = bs->file->bs;
> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
> -}
> -
> -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 = bytes;
> -    *map = offset;
> -    *file = bs->backing->bs;
> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
> -}
> -
>   /*
>    * Returns the allocation status of the specified sectors.
>    * Drivers not implementing the functionality are assumed to not support
> @@ -2063,6 +2033,7 @@ static int coroutine_fn 
> bdrv_co_block_status(BlockDriverState *bs,
>       BlockDriverState *local_file = NULL;
>       int64_t aligned_offset, aligned_bytes;
>       uint32_t align;
> +    bool has_filtered_child;
>   
>       assert(pnum);
>       *pnum = 0;
> @@ -2088,7 +2059,8 @@ static int coroutine_fn 
> bdrv_co_block_status(BlockDriverState *bs,
>   
>       /* Must be non-NULL or bdrv_getlength() would have failed */
>       assert(bs->drv);
> -    if (!bs->drv->bdrv_co_block_status) {
> +    has_filtered_child = bs->drv->is_filter && bdrv_filtered_rw_child(bs);
> +    if (!bs->drv->bdrv_co_block_status && !has_filtered_child) {
>           *pnum = bytes;
>           ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
>           if (offset + bytes == total_size) {
> @@ -2109,9 +2081,20 @@ static int coroutine_fn 
> bdrv_co_block_status(BlockDriverState *bs,
>       aligned_offset = QEMU_ALIGN_DOWN(offset, align);
>       aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
>   
> -    ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
> -                                        aligned_bytes, pnum, &local_map,
> -                                        &local_file);
> +    if (bs->drv->bdrv_co_block_status) {
> +        ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,
> +                                            aligned_bytes, pnum, &local_map,
> +                                            &local_file);
> +    } else {
> +        /* Default code for filters */
> +
> +        local_file = bdrv_filtered_rw_bs(bs);
> +        assert(local_file);
> +
> +        *pnum = aligned_bytes;
> +        local_map = aligned_offset;
> +        ret = BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
> +    }


preexistent, but why default for filters is aligned and for other nodes is not?

>       if (ret < 0) {
>           *pnum = 0;
>           goto out;
> diff --git a/block/mirror.c b/block/mirror.c
> index 80cef587f0..2e521c726a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1487,7 +1487,6 @@ 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_block_status       = bdrv_co_block_status_from_backing,
>       .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
>       .bdrv_child_perm            = bdrv_mirror_top_child_perm,
>   
> diff --git a/block/throttle.c b/block/throttle.c
> index f64dcc27b9..b6922e734f 100644
> --- a/block/throttle.c
> +++ b/block/throttle.c
> @@ -259,7 +259,6 @@ static BlockDriver bdrv_throttle = {
>       .bdrv_reopen_prepare                =   throttle_reopen_prepare,
>       .bdrv_reopen_commit                 =   throttle_reopen_commit,
>       .bdrv_reopen_abort                  =   throttle_reopen_abort,
> -    .bdrv_co_block_status               =   bdrv_co_block_status_from_file,
>   
>       .bdrv_co_drain_begin                =   throttle_co_drain_begin,
>       .bdrv_co_drain_end                  =   throttle_co_drain_end,
> 


-- 
Best regards,
Vladimir