[Qemu-devel] [PATCH 0/2] backup: Use copy offloading

[Qemu-devel] [PATCH 0/2] backup: Use copy offloading

[Fam Zheng]

Based-on: <20180529055959.32002-1-famz@redhat.com>
([PATCH v7 00/10] qemu-img convert with copy offloading)

This enhances the backup job to make use of the copy offloading API. It
eliminates the necessity to use the bounce buffer as well as speeding up the
copy operation when the backend supports it.

Fam Zheng (2):
  block: Honour BDRV_REQ_NO_SERIALISING in copy range
  backup: Use copy offloading

 block/backup.c        | 93 +++++++++++++++++++++++++++++++++------------------
 block/io.c            |  6 ++--
 block/trace-events    |  1 +
 include/block/block.h |  5 +--
 4 files changed, 69 insertions(+), 36 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] block: Honour BDRV_REQ_NO_SERIALISING in copy range

[Fam Zheng]

This semantics is needed by drive-backup so implement it before using
this API there.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c            | 6 ++++--
 include/block/block.h | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index b7beaeeb9f..f55e0c39fe 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2920,8 +2920,10 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
     tracked_request_begin(&dst_req, dst_bs, dst_offset,
                           bytes, BDRV_TRACKED_WRITE);
 
-    wait_serialising_requests(&src_req);
-    wait_serialising_requests(&dst_req);
+    if (!(flags & BDRV_REQ_NO_SERIALISING)) {
+        wait_serialising_requests(&src_req);
+        wait_serialising_requests(&dst_req);
+    }
     ret = bdrv_co_copy_range_from(src, src_offset,
                                   dst, dst_offset,
                                   bytes, flags);
diff --git a/include/block/block.h b/include/block/block.h
index 6cc6c7e699..6d3d156927 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -630,13 +630,14 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host);
  * @dst: Destination child to copy data to
  * @dst_offset: offset in @dst image to write data
  * @bytes: number of bytes to copy
- * @flags: request flags. Must be one of:
- *         0 - actually read data from src;
+ * @flags: request flags. Supported flags:
  *         BDRV_REQ_ZERO_WRITE - treat the @src range as zero data and do zero
  *                               write on @dst as if bdrv_co_pwrite_zeroes is
  *                               called. Used to simplify caller code, or
  *                               during BlockDriver.bdrv_co_copy_range_from()
  *                               recursion.
+ *         BDRV_REQ_NO_SERIALISING - do not serialize with other overlapping
+ *                                   requests currently in flight.
  *
  * Returns: 0 if succeeded; negative error code if failed.
  **/
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] backup: Use copy offloading

[Fam Zheng]

The implementation is similar to the 'qemu-img convert'. In the
beginning of the job, offloaded copy is attempted. If it fails, further
I/O will go through the existing bounce buffer code path.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c     | 93 +++++++++++++++++++++++++++++++++++-------------------
 block/trace-events |  1 +
 2 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 4e228e959b..ab189693f4 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -45,6 +45,8 @@ typedef struct BackupBlockJob {
     QLIST_HEAD(, CowRequest) inflight_reqs;
 
     HBitmap *copy_bitmap;
+    bool use_copy_range;
+    int64_t copy_range_size;
 } BackupBlockJob;
 
 static const BlockJobDriver backup_job_driver;
@@ -111,49 +113,70 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
     cow_request_begin(&cow_request, job, start, end);
 
     for (; start < end; start += job->cluster_size) {
+retry:
         if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
             trace_backup_do_cow_skip(job, start);
             continue; /* already copied */
         }
-        hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
 
         trace_backup_do_cow_process(job, start);
 
-        n = MIN(job->cluster_size, job->len - start);
-
-        if (!bounce_buffer) {
-            bounce_buffer = blk_blockalign(blk, job->cluster_size);
-        }
-        iov.iov_base = bounce_buffer;
-        iov.iov_len = n;
-        qemu_iovec_init_external(&bounce_qiov, &iov, 1);
-
-        ret = blk_co_preadv(blk, start, bounce_qiov.size, &bounce_qiov,
-                            is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
-        if (ret < 0) {
-            trace_backup_do_cow_read_fail(job, start, ret);
-            if (error_is_read) {
-                *error_is_read = true;
+        if (!job->use_copy_range) {
+            hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
+            n = MIN(job->cluster_size, job->len - start);
+            if (!bounce_buffer) {
+                bounce_buffer = blk_blockalign(blk, job->cluster_size);
             }
-            hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
-            goto out;
-        }
+            iov.iov_base = bounce_buffer;
+            iov.iov_len = n;
+            qemu_iovec_init_external(&bounce_qiov, &iov, 1);
 
-        if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
-            ret = blk_co_pwrite_zeroes(job->target, start,
-                                       bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
+            ret = blk_co_preadv(blk, start, bounce_qiov.size, &bounce_qiov,
+                                is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
+            if (ret < 0) {
+                trace_backup_do_cow_read_fail(job, start, ret);
+                if (error_is_read) {
+                    *error_is_read = true;
+                }
+                hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+                goto out;
+            }
+
+            if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
+                ret = blk_co_pwrite_zeroes(job->target, start,
+                                           bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
+            } else {
+                ret = blk_co_pwritev(job->target, start,
+                                     bounce_qiov.size, &bounce_qiov,
+                                     job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
+            }
+            if (ret < 0) {
+                trace_backup_do_cow_write_fail(job, start, ret);
+                if (error_is_read) {
+                    *error_is_read = false;
+                }
+                hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+                goto out;
+            }
         } else {
-            ret = blk_co_pwritev(job->target, start,
-                                 bounce_qiov.size, &bounce_qiov,
-                                 job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
-        }
-        if (ret < 0) {
-            trace_backup_do_cow_write_fail(job, start, ret);
-            if (error_is_read) {
-                *error_is_read = false;
+            int nr_clusters;
+
+            assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+            n = MIN(job->copy_range_size, job->len - start);
+            n = MIN(bytes, n);
+            nr_clusters = DIV_ROUND_UP(n, job->cluster_size);
+            hbitmap_reset(job->copy_bitmap, start / job->cluster_size,
+                          nr_clusters);
+            ret = blk_co_copy_range(blk, start, job->target, start, n,
+                                    is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
+            if (ret < 0) {
+                job->use_copy_range = false;
+                trace_backup_do_cow_copy_range_fail(job, start, ret);
+                hbitmap_set(job->copy_bitmap, start / job->cluster_size,
+                            nr_clusters);
+                /* Retry with bounce buffer. */
+                goto retry;
             }
-            hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
-            goto out;
         }
 
         /* Publish progress, guest I/O counts as progress too.  Note that the
@@ -665,6 +688,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     } else {
         job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
+    job->use_copy_range = true;
+    job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
+                                        blk_get_max_transfer(job->target));
+    job->copy_range_size = MAX(job->cluster_size,
+                               QEMU_ALIGN_UP(job->copy_range_size,
+                                             job->cluster_size));
 
     /* Required permissions are already taken with target's blk_new() */
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
diff --git a/block/trace-events b/block/trace-events
index 2d59b53fd3..c35287b48a 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -42,6 +42,7 @@ backup_do_cow_skip(void *job, int64_t start) "job %p start %"PRId64
 backup_do_cow_process(void *job, int64_t start) "job %p start %"PRId64
 backup_do_cow_read_fail(void *job, int64_t start, int ret) "job %p start %"PRId64" ret %d"
 backup_do_cow_write_fail(void *job, int64_t start, int ret) "job %p start %"PRId64" ret %d"
+backup_do_cow_copy_range_fail(void *job, int64_t start, int ret) "job %p start %"PRId64" ret %d"
 
 # blockdev.c
 qmp_block_job_cancel(void *job) "job %p"
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/2] backup: Use copy offloading

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

On Thu, May 31, 2018 at 10:34:45AM +0800, Fam Zheng wrote:
> The implementation is similar to the 'qemu-img convert'. In the
> beginning of the job, offloaded copy is attempted. If it fails, further
> I/O will go through the existing bounce buffer code path.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c     | 93 +++++++++++++++++++++++++++++++++++-------------------
>  block/trace-events |  1 +
>  2 files changed, 62 insertions(+), 32 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 4e228e959b..ab189693f4 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -45,6 +45,8 @@ typedef struct BackupBlockJob {
>      QLIST_HEAD(, CowRequest) inflight_reqs;
>  
>      HBitmap *copy_bitmap;
> +    bool use_copy_range;
> +    int64_t copy_range_size;
>  } BackupBlockJob;
>  
>  static const BlockJobDriver backup_job_driver;
> @@ -111,49 +113,70 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>      cow_request_begin(&cow_request, job, start, end);
>  
>      for (; start < end; start += job->cluster_size) {
> +retry:

This for loop is becoming complex.  Please introduce helper functions.
The loop body can be replaced with something like this:

  if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
      trace_backup_do_cow_skip(job, start);
      continue; /* already copied */
  }

  trace_backup_do_cow_process(job, start);

  ret = -ENOTSUPP;
  if (job->use_copy_range) {
      ret = cow_with_offload(...);
  }
  if (ret < 0) {
      job->use_copy_range = false;
      ret = cow_with_bounce_buffer(...);
  }
  if (ret < 0) {
      trace_backup_do_cow_write_fail(job, start, ret);
      goto out;
  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]