diff mbox

[2/6] block: Check if block drivers can do copy offloading

Message ID 20180608060417.10170-3-famz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fam Zheng June 8, 2018, 6:04 a.m. UTC 
This avoids the wasteful cluster allocation in qcow2 before actually
trying an unsupported copy range call, for example.

Signed-off-by: Fam Zheng <famz@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(+)

Comments

Stefan Hajnoczi June 15, 2018, 3 p.m. UTC | #1 
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 <famz@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.
diff mbox

Patch

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;
+    }
+    bs = src->bs;
+    return bs && bs->drv && bs->drv->bdrv_can_copy_range &&
+           bs->drv->bdrv_can_copy_range(bs, dst);
+}
diff --git a/block/file-posix.c b/block/file-posix.c
index c6dae38f94..41c491c65b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2539,6 +2539,13 @@  static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
                                NULL, bytes, QEMU_AIO_COPY_RANGE);
 }
 
+static bool raw_can_copy_range(BlockDriverState *bs,
+                               BdrvChild *dst)
+{
+    return dst->bs && dst->bs->drv &&
+           dst->bs->drv->bdrv_can_copy_range == raw_can_copy_range;
+}
+
 BlockDriver bdrv_file = {
     .format_name = "file",
     .protocol_name = "file",
@@ -2564,6 +2571,7 @@  BlockDriver bdrv_file = {
     .bdrv_aio_pdiscard = raw_aio_pdiscard,
     .bdrv_co_copy_range_from = raw_co_copy_range_from,
     .bdrv_co_copy_range_to  = raw_co_copy_range_to,
+    .bdrv_can_copy_range = raw_can_copy_range,
     .bdrv_refresh_limits = raw_refresh_limits,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
@@ -3044,6 +3052,7 @@  static BlockDriver bdrv_host_device = {
     .bdrv_aio_pdiscard   = hdev_aio_pdiscard,
     .bdrv_co_copy_range_from = raw_co_copy_range_from,
     .bdrv_co_copy_range_to  = raw_co_copy_range_to,
+    .bdrv_can_copy_range = raw_can_copy_range,
     .bdrv_refresh_limits = raw_refresh_limits,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
diff --git a/block/io.c b/block/io.c
index b7beaeeb9f..d8039793c2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2913,6 +2913,9 @@  int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
     BlockDriverState *dst_bs = dst->bs;
     int ret;
 
+    if (!bdrv_can_copy_range(src, dst)) {
+        return -ENOTSUP;
+    }
     bdrv_inc_in_flight(src_bs);
     bdrv_inc_in_flight(dst_bs);
     tracked_request_begin(&src_req, src_bs, src_offset,
diff --git a/block/iscsi.c b/block/iscsi.c
index c2fbd8a8aa..6c465ebd46 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2420,6 +2420,12 @@  out_unlock:
     return r;
 }
 
+static bool iscsi_can_copy_range(BlockDriverState *bs, BdrvChild *dst)
+{
+    return dst->bs && dst->bs->drv &&
+           dst->bs->drv->bdrv_can_copy_range == iscsi_can_copy_range;
+}
+
 static QemuOptsList iscsi_create_opts = {
     .name = "iscsi-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(iscsi_create_opts.head),
@@ -2456,6 +2462,7 @@  static BlockDriver bdrv_iscsi = {
     .bdrv_co_pdiscard      = iscsi_co_pdiscard,
     .bdrv_co_copy_range_from = iscsi_co_copy_range_from,
     .bdrv_co_copy_range_to  = iscsi_co_copy_range_to,
+    .bdrv_can_copy_range   = iscsi_can_copy_range,
     .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
     .bdrv_co_readv         = iscsi_co_readv,
     .bdrv_co_writev        = iscsi_co_writev,
@@ -2493,6 +2500,7 @@  static BlockDriver bdrv_iser = {
     .bdrv_co_pdiscard      = iscsi_co_pdiscard,
     .bdrv_co_copy_range_from = iscsi_co_copy_range_from,
     .bdrv_co_copy_range_to  = iscsi_co_copy_range_to,
+    .bdrv_can_copy_range   = iscsi_can_copy_range,
     .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
     .bdrv_co_readv         = iscsi_co_readv,
     .bdrv_co_writev        = iscsi_co_writev,
diff --git a/block/qcow2.c b/block/qcow2.c
index 549fee9b69..1326410d1c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3440,6 +3440,16 @@  fail:
     return ret;
 }
 
+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;
+}
+
 static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
                           PreallocMode prealloc, Error **errp)
 {
@@ -4690,6 +4700,7 @@  BlockDriver bdrv_qcow2 = {
     .bdrv_co_pdiscard       = qcow2_co_pdiscard,
     .bdrv_co_copy_range_from = qcow2_co_copy_range_from,
     .bdrv_co_copy_range_to  = qcow2_co_copy_range_to,
+    .bdrv_can_copy_range    = qcow2_can_copy_range,
     .bdrv_truncate          = qcow2_truncate,
     .bdrv_co_pwritev_compressed = qcow2_co_pwritev_compressed,
     .bdrv_make_empty        = qcow2_make_empty,
diff --git a/block/raw-format.c b/block/raw-format.c
index f2e468df6f..707b25fc77 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -527,6 +527,11 @@  static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
                                  flags);
 }
 
+static bool raw_can_copy_range(BlockDriverState *bs, BdrvChild *dst)
+{
+    return bdrv_can_copy_range(bs->file, dst);
+}
+
 BlockDriver bdrv_raw = {
     .format_name          = "raw",
     .instance_size        = sizeof(BDRVRawState),
@@ -545,6 +550,7 @@  BlockDriver bdrv_raw = {
     .bdrv_co_block_status = &raw_co_block_status,
     .bdrv_co_copy_range_from = &raw_co_copy_range_from,
     .bdrv_co_copy_range_to  = &raw_co_copy_range_to,
+    .bdrv_can_copy_range  = &raw_can_copy_range,
     .bdrv_truncate        = &raw_truncate,
     .bdrv_getlength       = &raw_getlength,
     .has_variable_length  = true,
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);
 
 #endif /* BLOCK_INT_H */