Re: [Qemu-devel] [PATCH 2/6] block: Check if block drivers can do copy offloading

[Stefan Hajnoczi]

On Fri, Jun 08, 2018 at 02:04:13PM +0800, Fam Zheng wrote:
> This avoids the wasteful cluster allocation in qcow2 before actually
> trying an unsupported copy range call, for example.

I don't understand how this function can work.  dst is never traversed
so does it always return false?

> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c                   | 12 ++++++++++++
>  block/file-posix.c        |  9 +++++++++
>  block/io.c                |  3 +++
>  block/iscsi.c             |  8 ++++++++
>  block/qcow2.c             | 11 +++++++++++
>  block/raw-format.c        |  6 ++++++
>  include/block/block_int.h |  4 ++++
>  7 files changed, 53 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 501b64c819..28aa8d8a65 100644
> --- a/block.c
> +++ b/block.c
> @@ -5320,3 +5320,15 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState 
> *bs, const char *name,
>  
>      return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
>  }
> +
> +bool bdrv_can_copy_range(BdrvChild *src, BdrvChild *dst)
> +{
> +    BlockDriverState *bs;
> +
> +    if (!src || !src->bs) {
> +        return false;
> +    }

src checked but not dst.  Does this mean src can be NULL but dst cannot
be NULL, and why?

> +    bs = src->bs;
> +    return bs && bs->drv && bs->drv->bdrv_can_copy_range &&

src->bs was already checked, so bs != NULL here and doesn't need a check.

> +static bool qcow2_can_copy_range(BlockDriverState *bs, BdrvChild *dst)
> +{
> +    bool r = bdrv_can_copy_range(bs->file, dst);
> +
> +    if (bs->backing) {
> +        r = r && bdrv_can_copy_range(bs->backing, dst);
> +    }
> +    return r;
> +}

This is too conservative.  It assumes every range includes clusters from
both bs->file and bs->backing, which is not true.

An || instead of && would return false positives in some cases, which
defeats the bdrv_can_copy_range() optimization, but at least allows
copy-offloading in all cases where it could be done.

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 888b7f7bff..2c51cd420f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -235,6 +235,9 @@ struct BlockDriver {
>                                                uint64_t bytes,
>                                                BdrvRequestFlags flags);
>  
> +    bool (*bdrv_can_copy_range)(BlockDriverState *bs,
> +                                BdrvChild *dst);
> +
>      /*
>       * Building block for bdrv_block_status[_above] and
>       * bdrv_is_allocated[_above].  The driver should answer only
> @@ -1139,5 +1142,6 @@ int coroutine_fn bdrv_co_copy_range_from(BdrvChild 
> *src, uint64_t src_offset,
>  int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
>                                         BdrvChild *dst, uint64_t dst_offset,
>                                         uint64_t bytes, BdrvRequestFlags 
> flags);
> +bool bdrv_can_copy_range(BdrvChild *src, BdrvChild *dst);

Please document this API and .bdrv_can_copy_range().

Please don't make me remind you.  Eventually I'll forget too.

An important point for the doc comments:

  This function is a lightweight check that avoids expensive operations
  performed by a full bdrv_co_copy_range() call.  This function may
  produce false positives.  It is still possible for
  bdrv_co_copy_range() to return -ENOTSUP after bdrv_can_copy_range()
  has returned true.

signature.asc
Description: PGP signature