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 <f...@redhat.com>
> ---
> 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
